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

[zhang warden]

Hi, Petr

> On Sep 10, 2024, at 16:01, Petr Mladek <pmladek@xxxxxxxx> wrote:
> 
> On Sun 2024-09-08 10:51:14, zhang warden wrote:
>> 
>> 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()
> 
> This would require adding "struct rcu_head" into "struct klp_ops",
> like:
> 
> struct klp_ops {
> struct list_head func_stack;
> struct ftrace_ops fops;
> struct rcu_head rcu;
> };
> 
> and then freeing the structure using kfree_rcu():
> 
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 90408500e5a3..f096dd9390d2 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -149,7 +149,7 @@ static void klp_unpatch_func(struct klp_func *func)
> 
> list_del_rcu(&func->stack_node);
> list_del(&ops->node);
> - kfree(ops);
> + kfree_rcu(ops, rcu);
> } else {
> list_del_rcu(&func->stack_node);
> }
> @@ -223,7 +223,7 @@ static int klp_patch_func(struct klp_func *func)
> err:
> list_del_rcu(&func->stack_node);
> list_del(&ops->node);
> - kfree(ops);
> + kfree_rcu(ops, rcu);
> return ret;
> }
> 
> With this the function should be safe against accessing an invalid
> pointer.
> 
>>> 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);
>>> }
> 
> But the function is still not correct according the order of reading.
> A more correct solution would be something like:
> 
> 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();
> 
> /* This livepatch is used when the function is on top of the stack. */
> ops = func->ops;
> if (ops) {
> using_func = list_first_or_null_rcu(&ops->func_stack,
> struct klp_func, stack_node);
> if (func == using_func)
> using = 1;
> else
> using = 0;
> }
> 
> /*
>  * The function stack gives the right information only when there
>  * is no transition in progress.
>  *
>  * Make sure that we see the updated ops->func_stack when
>  * func->transition is cleared. This matches with:
>  *
>  * The write barrier in  __klp_enable_patch() between
>  * klp_init_transition() and klp_patch_object().
>  *
>  * The write barrier in  __klp_disable_patch() between
>  * klp_init_transition() and klp_start_transition().
>  *
>  * The write barrier in klp_complete_transition()
>  * between klp_unpatch_objects() and func->transition = false.
>  */
> smp_rmb();
> 
> if (func->transition)
> using = -1;
> 
> rcu_read_unlock();
> 
> return sysfs_emit(buf, "%d\n", func->using);
> }
> 
> Now, the question is whether we want to maintain such a barrier. Any
> lockless access and barrier adds a maintenance burden.
> 
> You might try to put the above into a patch see what others tell
> about it.
> 
> Best Regards,
> Petr

After the previous discussion, it seems that patch-level "order" feature
 is the more acceptable way to fulfill the patch order information.

Therefore, I am trying to go that way instead of adding "using" into klp_func.

Maybe patch-level interface will bring less maintenance burden.

Regards,
Wardenjohn.