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.
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@xxxxxxxxx>
Missing "Suggested-by: Petr Mladek <pmladek@xxxxxxxx> perhaps?
>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.
>+ 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?
> /*
> * 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.
Regards,
Miroslav
