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.
