Re: [PATCH v2 1/1] livepatch: Add using attribute to klp_func for using function show

[Petr Mladek <pmladek@suse.com>]

On Mon 2024-08-05 14:46:56, zhangyongde.zyd wrote:
> From: Wardenjohn <zhangwarden@gmail.com>
> 
> One system may contains more than one livepatch module. We can see
> which patch is enabled. If some patches applied to one system
> modifing the same function, livepatch will use the function enabled
> on top of the function stack. However, we can not excatly know
> which function of which patch is now enabling.
> 
> This patch introduce one sysfs attribute of "using" to klp_func.
> For example, if there are serval patches  make changes to function
> "meminfo_proc_show", the attribute "enabled" of all the patch is 1.
> With this attribute, we can easily know the version enabling belongs
> to which patch.
> 
> The "using" is set as three state. 0 is disabled, it means that this
> version of function is not used. 1 is running, it means that this
> version of function is now running. -1 is unknown, it means that
> this version of function is under transition, some task is still
> chaning their running version of this function.
> 
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -773,6 +791,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  	INIT_LIST_HEAD(&func->stack_node);
>  	func->patched = false;
>  	func->transition = false;
> +	func->using = 0;
>  
>  	/* The format for the sysfs directory is <function,sympos> where sympos
>  	 * is the nth occurrence of this symbol in kallsyms for the patched
> @@ -903,6 +922,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  static void klp_init_func_early(struct klp_object *obj,
>  				struct klp_func *func)
>  {
> +	func->using = false;

It should be enough to initialize the value only one.
klp_init_func() is the right place.
klp_init_func_early() does only the bare minimum to allow freeing.

>  	kobject_init(&func->kobj, &klp_ktype_func);
>  	list_add_tail(&func->node, &obj->func_list);
>  }
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 90408500e5a3..bf4a8edbd888 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -104,7 +104,7 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>  			 * original function.
>  			 */
>  			func = list_entry_rcu(func->stack_node.next,
> -					      struct klp_func, stack_node);
> +						struct klp_func, stack_node);

Looks like an unwanted change.

>  
>  			if (&func->stack_node == &ops->func_stack)
>  				goto unlock;
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index ba069459c101..12241dabce6f 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -119,9 +120,35 @@ static void klp_complete_transition(void)
>  		klp_synchronize_transition();
>  	}
>  
> -	klp_for_each_object(klp_transition_patch, obj)
> -		klp_for_each_func(obj, func)
> -			func->transition = false;
> +	/*
> +	* The transition patch is finished. The stack top function is now truly
> +	* running. The previous function should be set as 0 as none task is 
> +	* using this function anymore.
> +	* 
> +	* If this is a patching patch, all function is using.
> +	* if this patch is unpatching, all function of the func stack top is using
> +	*/
> +	if (klp_target_state == KLP_TRANSITION_PATCHED)
> +		klp_for_each_object(klp_transition_patch, obj)
> +			klp_for_each_func(obj, func){

Missing space between "){'"

You should check your patch with ./scripts/checkpatch.pl before sending.

> +				func->using = 1;
> +				func->transition = false;
> +				next_func = list_entry_rcu(func->stack_node.next,
> +								struct klp_func, stack_node);

What if there is only one function on the stack?
You could take inspiration in klp_ftrace_handler.

> +				next_func->using = 0;
> +			}

Wrong indentation, see Documentation/process/coding-style.rst
./scripts/checkpatch.pl would likely caught this.

> +	else

Please, always put multi-line code in { }. It helps to avoid mistakes
and read the code.

> +		// for the unpatch func, if ops exist, the top of this func is using
> +		klp_for_each_object(klp_transition_patch, obj)
> +			klp_for_each_func(obj, func){
> +				func->transition = false;
> +				ops = klp_find_ops(func->old_func);
> +				if (ops){
> +					stack_top_func = list_first_entry(&ops->func_stack, struct klp_func,
> +							stack_node);
> +					stack_top_func->using = 1;
> +				}
> +			}
>  
>  	/* Prevent klp_ftrace_handler() from seeing KLP_TRANSITION_IDLE state */
>  	if (klp_target_state == KLP_TRANSITION_PATCHED)
> @@ -538,6 +565,7 @@ void klp_start_transition(void)
>  		  klp_transition_patch->mod->name,
>  		  klp_target_state == KLP_TRANSITION_PATCHED ? "patching" : "unpatching");
>  
> +

Extra line?

>  	/*
>  	 * Mark all normal tasks as needing a patch state update.  They'll
>  	 * switch either in klp_try_complete_transition() or as they exit the
> @@ -633,6 +661,9 @@ void klp_init_transition(struct klp_patch *patch, int state)
>  	 *
>  	 * When unpatching, the funcs are already in the func_stack and so are
>  	 * already visible to the ftrace handler.
> +	 * 
> +	 * When this patch is in transition, all functions of this patch will
> +	 * set to be unknown

The sentence is not complete. It does not say what exactly is set to unknown.

>  	 */
>  	klp_for_each_object(patch, obj)
>  		klp_for_each_func(obj, func)


Alternative solution:

The patch adds a lot of extra complexity to maintain the information.

Alternative solution would be to store the pointer of struct klp_ops
*ops into struct klp_func. Then using_show() could just check if
the related struct klp_func in on top of the stack.

It would allow to remove the global list klp_ops and all the related
code. klp_find_ops() would instead do:

   for_each_patch
     for_each_object
       for_each_func

The search would need more code. But it would be simple and
straightforward. We do this many times all over the code.

IMHO, it would actually remove some complexity and be a win-win solution.

Best Regards,
Petr