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

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

[zhangyongde.zyd]

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.

Changes v1 => v2:
1. reset using type from bool to int
2. the value of "using" contains 3 types, 0(not used),
 1(using), -1(unknown). (As suggested by Petr).
3. add the process of transition state (-1 state). (suggested by Petr and Miroslav)

v1: https://lore.kernel.org/live-patching/1A0E6D0A-4245-4F44-8667-CDD86A925347@gmail.com/T/#t

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

[zhangyongde.zyd]

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.

cat /sys/kernel/livepatch/<patch1>/<object1>/<function1,sympos>/using -> 0
means that the function1 of patch1 is disabled.

cat /sys/kernel/livepatch/<patchN>/<object1>/<function1,sympos>/using -> 1
means that the function1 of patchN is enabled.

cat /sys/kernel/livepatch/<patchN>/<object1>/<function1,sympos>/using -> -1
means that the function1 of patchN is under transition and unknown.

Signed-off-by: Wardenjohn <zhangwarden@gmail.com>

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 51a258c24ff5..fd8224969c5c 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -37,6 +37,7 @@
  * @nop:        temporary patch to use the original code again; dyn. allocated
  * @patched:	the func has been added to the klp_ops list
  * @transition:	the func is currently being applied or reverted
+ * @using:    the func is on top of the function stack that is using
  *
  * The patched and transition variables define the func's patching state.  When
  * patching, a func is always in one of the following states:
@@ -52,6 +53,12 @@
  *   patched=1 transition=1: patched, may be visible to some tasks
  *   patched=0 transition=1: unpatched, temporary ending state
  *   patched=0 transition=0: unpatched
+ * 
+ *  'using' flag is used to show if this function is now using
+ * 
+ *   using=-1 (unknown): the function is now under transition
+ *   using=1  (using):   the function is now running
+ *   using=0  (not used): the function is not used
  */
 struct klp_func {
 	/* external */
@@ -75,6 +82,7 @@ struct klp_func {
 	bool nop;
 	bool patched;
 	bool transition;
+	int using;
 };
 
 struct klp_object;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 52426665eecc..67630f9f1a21 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -349,6 +349,7 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
  * /sys/kernel/livepatch/<patch>/<object>
  * /sys/kernel/livepatch/<patch>/<object>/patched
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
+ * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>/using
  */
 static int __klp_disable_patch(struct klp_patch *patch);
 
@@ -470,6 +471,22 @@ static struct attribute *klp_object_attrs[] = {
 };
 ATTRIBUTE_GROUPS(klp_object);
 
+static ssize_t using_show(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	struct klp_func *func;
+
+	func = container_of(kobj, struct klp_func, kobj);
+	return sysfs_emit(buf, "%d\n", func->using);
+}
+
+static struct kobj_attribute using_kobj_attr = __ATTR_RO(using);
+static struct attribute *klp_func_attrs[] = {
+	&using_kobj_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(klp_func);
+
 static void klp_free_object_dynamic(struct klp_object *obj)
 {
 	kfree(obj->name);
@@ -631,6 +648,7 @@ static void klp_kobj_release_func(struct kobject *kobj)
 static const struct kobj_type klp_ktype_func = {
 	.release = klp_kobj_release_func,
 	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = klp_func_groups,
 };
 
 static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
@@ -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;
 	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);
 
 			if (&func->stack_node == &ops->func_stack)
 				goto unlock;
@@ -127,6 +127,7 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 static void klp_unpatch_func(struct klp_func *func)
 {
 	struct klp_ops *ops;
+	struct klp_func *stack_top_func;
 
 	if (WARN_ON(!func->patched))
 		return;
@@ -152,6 +153,10 @@ static void klp_unpatch_func(struct klp_func *func)
 		kfree(ops);
 	} else {
 		list_del_rcu(&func->stack_node);
+		// the previous function is deleted, the stack top is under transition
+		stack_top_func = list_first_entry(&ops->func_stack, struct klp_func,
+							stack_node);
+		stack_top_func->using = -1;
 	}
 
 	func->patched = false;
@@ -160,6 +165,7 @@ static void klp_unpatch_func(struct klp_func *func)
 static int klp_patch_func(struct klp_func *func)
 {
 	struct klp_ops *ops;
+	struct klp_func *stack_top_func;
 	int ret;
 
 	if (WARN_ON(!func->old_func))
@@ -170,6 +176,7 @@ static int klp_patch_func(struct klp_func *func)
 
 	ops = klp_find_ops(func->old_func);
 	if (!ops) {
+		// this function is the first time to be patched
 		unsigned long ftrace_loc;
 
 		ftrace_loc = ftrace_location((unsigned long)func->old_func);
@@ -211,11 +218,16 @@ static int klp_patch_func(struct klp_func *func)
 			goto err;
 		}
 
-
 	} else {
+		// stack_top_func is going to be in transition
+		stack_top_func = list_first_entry(&ops->func_stack, struct klp_func,
+							stack_node);
+		stack_top_func->using = -1;
+		// The new patched function is the one enabling
 		list_add_rcu(&func->stack_node, &ops->func_stack);
-	}
+	}  
 
+	func->using = -1;
 	func->patched = true;
 
 	return 0;
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
@@ -90,8 +90,9 @@ static void klp_synchronize_transition(void)
 static void klp_complete_transition(void)
 {
 	struct klp_object *obj;
-	struct klp_func *func;
+	struct klp_func *func, *next_func, *stack_top_func;
 	struct task_struct *g, *task;
+	struct klp_ops *ops;
 	unsigned int cpu;
 
 	pr_debug("'%s': completing %s transition\n",
@@ -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){
+				func->using = 1;
+				func->transition = false;
+				next_func = list_entry_rcu(func->stack_node.next,
+								struct klp_func, stack_node);
+				next_func->using = 0;
+			}
+	else
+		// 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");
 
+
 	/*
 	 * 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
 	 */
 	klp_for_each_object(patch, obj)
 		klp_for_each_func(obj, func)
-- 
2.18.2

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

[zhang warden]

> On Aug 5, 2024, at 14:46, zhangyongde.zyd <zhangwarden@gmail.com> wrote:
> 
> From: Wardenjohn <zhangwarden@gmail.com>
> 
> 
> static void klp_init_func_early(struct klp_object *obj,
> struct klp_func *func)
> {
> + func->using = false;
> kobject_init(&func->kobj, &klp_ktype_func);
> list_add_tail(&func->node, &obj->func_list);
> }

I reviewed my mail again and found some typos and point them out first.

func->using = false
is a remaining mistake by the previous patch which will remove in the next patch.

> 
> + * 
> + * When this patch is in transition, all functions of this patch will
> + * set to be unknown
> */
This comment is left and become useless, it will be removed	in the next patch.

Thanks!
Wardenjohn.

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

[zhang warden]

> On Aug 5, 2024, at 14:46, zhangyongde.zyd <zhangwarden@gmail.com> 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.
> 
> cat /sys/kernel/livepatch/<patch1>/<object1>/<function1,sympos>/using -> 0
> means that the function1 of patch1 is disabled.
> 
> cat /sys/kernel/livepatch/<patchN>/<object1>/<function1,sympos>/using -> 1
> means that the function1 of patchN is enabled.
> 
> cat /sys/kernel/livepatch/<patchN>/<object1>/<function1,sympos>/using -> -1
> means that the function1 of patchN is under transition and unknown.
> 
> Signed-off-by: Wardenjohn <zhangwarden@gmail.com>
> 

Hi maintainers. How about your suggestions to patch V2?

According to your suggestion, I made some new changes to the V1 patch.

I am waiting for your suggestions. 

Thanks.
Wardenjohn.

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

[Petr Mladek]

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

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

[zhang warden]

> 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

Hi Petr!

Thank you for taking the time to review my patch.

I will update this patch to next version with your suggestions!

Regards,
Wardenjohn.

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

[zhang warden]

> On Aug 14, 2024, at 00:05, Petr Mladek <pmladek@suse.com> wrote:
> 
> 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.

Hi Peter!

With your suggestions, it seems that you suggest move the klp_ops pinter into struct klp_func.

I may do this operation:

struct klp_func {

/* internal */
void *old_func;
struct kobject kobj;
struct list_head node;
struct list_head stack_node;
+ struct klp_ops *ops;
unsigned long old_size, new_size;
bool nop;
bool patched;
bool transition;
};

With this operation, klp_ops global list will no longer needed. And if we want the ftrace_ops of a function, we just need to get the ops member of klp_func eg, func->ops. 

And klp_find_ops() will be replaced by `ops = func->ops`, which is more easy.

Is it right?

Best Regards.
Wardenjohn.

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

[Petr Mladek]

On Wed 2024-08-14 22:23:21, zhang warden wrote:
> 
> 
> > On Aug 14, 2024, at 00:05, Petr Mladek <pmladek@suse.com> wrote:
> > 
> > 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.
> 
> Hi Peter!
> 
> With your suggestions, it seems that you suggest move the klp_ops pinter into struct klp_func.
> 
> I may do this operation:
> 
> struct klp_func {
> 
> /* internal */
> void *old_func;
> struct kobject kobj;
> struct list_head node;
> struct list_head stack_node;
> + struct klp_ops *ops;
> unsigned long old_size, new_size;
> bool nop;
> bool patched;
> bool transition;
> };

Yes.

> With this operation, klp_ops global list will no longer needed. And if we want the ftrace_ops of a function, we just need to get the ops member of klp_func eg, func->ops. 
> 
> And klp_find_ops() will be replaced by `ops = func->ops`, which is more easy.

func->ops will work only when it is already assigned, for example, in

   + klp_check_stack_func()
   + klp_unpatch_func()
   + using_show()	/* the new sysfs callback */

But we will still need klp_find_ops() in klp_patch_func() to find
whether an already registered livepatch has already attached
the ftrace handled for the same function (func->old_func).

The new version would need to go through all registred patches,
something like:

struct klp_ops *klp_find_ops(void *old_func)
{
	struct klp_patch *patch;
	struct klp_object *obj;
	struct klp_func *func;

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

	return NULL;
}

BTW: It really looks useful. klp_check_stack_func() is called for_each_func()
     also during task transition, even from the scheduler:

       + klp_cond_resched()
	 + __klp_sched_try_switch()
	   + klp_try_switch_task()
	     + klp_check_and_switch_task()
	       + klp_check_stack()
		 + klp_for_each_object()
		   + klp_for_each_func()
		     + klp_find_ops()

      It would newly just use func->ops. It might be even noticeable
      speedup.

Please, implement this in a separate patch:

  + 1st patch adds func->ops
  + 2nd patch adds "using" sysfs interface.

Best Regards,
Petr