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

[Miroslav Benes <mbenes@suse.cz>]

Hi,

On Thu, 5 Sep 2024, zhang warden wrote:

> 
> 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?

Yes, but keep them in one set for now, please.
 
> >> 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?

node member. You removed the global list, hence this member is not needed 
anymore.

> > 
> >> +       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' ?

No, I am asking if there is a reason why you added 'func->ops = NULL;' 
here and not right after the rest of func initializations

        INIT_LIST_HEAD(&func->stack_node);
        func->patched = false;
        func->transition = false;

> > 
> >> /*
> >> * 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?

If you have no other plans with it, then it can be removed since there is 
no user after the patch.

Miroslav