Hi,
On Thu, 5 Sep 2024, zhang warden wrote:
>
> Hi, Miroslav
> > On Sep 5, 2024, at 18:10, Miroslav Benes <mbenes@xxxxxxx> 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
