linux-kernel - Re: [PATCH v4 1/2] Introduce klp_ops into klp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LSU.2.21.2409051139540.8559@pobox.suse.cz>
Date: Thu, 5 Sep 2024 12:10:14 +0200 (CEST)
From: Miroslav Benes <mbenes@...e.cz>
To: Wardenjohn <zhangwarden@...il.com>
cc: jpoimboe@...nel.org, jikos@...nel.org, pmladek@...e.com, 
    joe.lawrence@...hat.com, live-patching@...r.kernel.org, 
    linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/2] Introduce klp_ops into klp_func structure

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@...il.com>

Missing "Suggested-by: Petr Mladek <pmladek@...e.com> 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