Re: [PATCH v4 1/2] Introduce klp_ops into klp_func structure

[zhang warden <zhangwarden@gmail.com>]

Hi, Miroslav
> On Sep 5, 2024, at 18:10, Miroslav Benes <mbenes@suse.cz> wrote:
> 
> Hi,
> 
> the subject is missing "livepatch: " prefix and I would prefer something 
> like
> 
> "livepatch: Move struct klp_ops to struct klp_func"
> 
> On Wed, 28 Aug 2024, Wardenjohn wrote:
> 
>> Before introduce feature "using". Klp transition will need an
>> easier way to get klp_ops from klp_func.
> 
> I think that the patch might make sense on its own as a 
> cleanup/optimization as Petr suggested. If fixed. See below. Then the 
> changelog can be restructured and the above can be removed.
> 

After the previous discuss, maybe this patch of "livepatch: Move struct klp_ops to struct klp_func" is useful, while the second patch need to be considered later, right?

> Btw if it comes to it, I would much rather have something like "active" 
> instead of "using". 
> 
>> This patch make changes as follows:
>> 1. Move klp_ops into klp_func structure.
>> Rewrite the logic of klp_find_ops and
>> other logic to get klp_ops of a function.
>> 
>> 2. Move definition of struct klp_ops into
>> include/linux/livepatch.h
>> 
>> With this changes, we can get klp_ops of a klp_func easily. 
>> klp_find_ops can also be simple and straightforward. And we 
>> do not need to maintain one global list for now.
>> 
>> Signed-off-by: Wardenjohn <zhangwarden@gmail.com>
> 
> Missing "Suggested-by: Petr Mladek <pmladek@suse.com> perhaps?
> 

Oops, I do miss this message here.

>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 51a258c24ff5..d874aecc817b 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -22,6 +22,25 @@
>> #define KLP_TRANSITION_UNPATCHED        0
>> #define KLP_TRANSITION_PATCHED          1
>> 
>> +/**
>> + * struct klp_ops - structure for tracking registered ftrace ops structs
>> + *
>> + * A single ftrace_ops is shared between all enabled replacement functions
>> + * (klp_func structs) which have the same old_func.  This allows the switch
>> + * between function versions to happen instantaneously by updating the klp_ops
>> + * struct's func_stack list.  The winner is the klp_func at the top of the
>> + * func_stack (front of the list).
>> + *
>> + * @node:      node for the global klp_ops list
>> + * @func_stack:        list head for the stack of klp_func's (active func is on top)
>> + * @fops:      registered ftrace ops struct
>> + */
>> +struct klp_ops {
>> +       struct list_head node;
> 
> Not needed anymore.

What is not needed any more, do you means the comment?
> 
>> +       struct list_head func_stack;
>> +       struct ftrace_ops fops;
>> +};
>> +
>> 
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index 52426665eecc..e4572bf34316 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -760,6 +760,8 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>> if (!func->old_name)
>> return -EINVAL;
>> 
>> + func->ops = NULL;
>> +
> 
> Any reason why it is not added a couple of lines later alongside the rest 
> of the initialization?

Do you mean I should add couple of lines after 'return -EINVAL' ?

> 
>> /*
>> * NOPs get the address later. The patched module must be loaded,
>> * see klp_init_object_loaded().
>> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
>> index 90408500e5a3..8ab9c35570f4 100644
>> --- a/kernel/livepatch/patch.c
>> +++ b/kernel/livepatch/patch.c
>> @@ -20,18 +20,25 @@
>> #include "patch.h"
>> #include "transition.h"
>> 
>> -static LIST_HEAD(klp_ops);
>> 
>> struct klp_ops *klp_find_ops(void *old_func)
>> {
>> - struct klp_ops *ops;
>> + struct klp_patch *patch;
>> + struct klp_object *obj;
>> struct klp_func *func;
>> 
>> - list_for_each_entry(ops, &klp_ops, node) {
>> - func = list_first_entry(&ops->func_stack, struct klp_func,
>> - stack_node);
>> - if (func->old_func == old_func)
>> - return ops;
>> + klp_for_each_patch(patch) {
>> + klp_for_each_object(patch, obj) {
>> + klp_for_each_func(obj, func) {
>> + /*
>> + * Ignore entry where func->ops has not been
>> + * assigned yet. It is most likely the one
>> + * which is about to be created/added.
>> + */
>> + if (func->old_func == old_func && func->ops)
>> + return func->ops;
>> + }
>> + }
>> }
> 
> The function is not used anywhere after this patch.
> 

Maybe there still other places will call this klp_find_ops? Is it safe to delete it?

Regards,
Wardenjohn