Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden <zhangwarden@gmail.com>]

Hi, Petr
> 
> The 1st patch adds the pointer to struct klp_ops into struct
> klp_func. We might check the state a similar way as klp_ftrace_handler().
> 
> I had something like this in mind when I suggested to move the pointer:
> 
> static ssize_t using_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> struct klp_func *func, *using_func;
> struct klp_ops *ops;
> int using;
> 
> func = container_of(kobj, struct klp_func, kobj);
> 
> rcu_read_lock();
> 
> if (func->transition) {
> using = -1;
> goto out;
> }
> 
> # FIXME: This requires releasing struct klp_ops via call_rcu()
> ops = func->ops;
> if (!ops) {
> using = 0;
> goto out;
> }
> 
> using_func = list_first_or_null_rcu(&ops->func_stack,
> struct klp_func, stack_node);
> if (func == using_func)
> using = 1;
> else
> using = 0;
> 
> out:
> rcu_read_unlock();
> 
> return sysfs_emit(buf, "%d\n", func->using);
> }

Bravo, I also have something like this in mind when Miroslav suggested to implement the logic in using_show. 

> 
> It is racy and tricky. We probably should add some memory barriers.
> And maybe even the ordering of reads should be different.
> 
> We could not take klp_mutex because it might cause a deadlock when
> the sysfs file gets removed. kobject_put(&func->kobj) is called
> by __klp_free_funcs() under klp_mutex.
> 
> It would be easier if we could take klp_mutex. But it would require
> decrementing the kobject refcout without of klp_mutex. It might
> be complicated.
> 
> I am afraid that this approach is not worth the effort and
> is is not the way to go.
> 

But I don't think I've thought as deeply as you do and may not have considered the possible risks. Therefore, I do need your help to make my patch perfect.  ^_^

Thank you.
Wardenjohn.