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.

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.

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

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 51a258c24ff5..26519c134578 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:
@@ -75,6 +76,7 @@ struct klp_func {
 	bool nop;
 	bool patched;
 	bool transition;
+	bool using;
 };
 
 struct klp_object;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 52426665eecc..b938667f96e3 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)
@@ -903,6 +921,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..561bfb3f59f7 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -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,9 @@ static void klp_unpatch_func(struct klp_func *func)
 		kfree(ops);
 	} else {
 		list_del_rcu(&func->stack_node);
+		stack_top_func = list_first_entry(&ops->func_stack, struct klp_func,
+							stack_node);
+		stack_top_func->using = true;
 	}
 
 	func->patched = false;
@@ -160,6 +164,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 +175,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,9 +217,15 @@ static int klp_patch_func(struct klp_func *func)
 			goto err;
 		}
 
-
+		func->using = true;
 	} else {
+		// find the function that enabling at this time and mark it disabled.
+		stack_top_func = list_first_entry(&ops->func_stack, struct klp_func,
+							stack_node);
+		stack_top_func->using = false;
+		// now, this new patched function is the one enabling
 		list_add_rcu(&func->stack_node, &ops->func_stack);
+		func->using = true;
 	}
 
 	func->patched = true;
-- 
2.18.2

Hi,

On Thu, 18 Jul 2024, 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.
> 
> 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.

is this always correct though? See the logic in klp_ftrace_handler(). If 
there is a transition running, it is a little bit more complicated.

Miroslav

> is this always correct though? See the logic in klp_ftrace_handler(). If 
> there is a transition running, it is a little bit more complicated.
> 
> Miroslav

Hi! Miroslav.

In reality, we often encounter such situation that serval patch in one system, some patch make changes to one same function A. This make us confused that we don't know which version of function A is now using. 

In my view, this "using" state show the function version that is now enabling. Even if there is a transition running, the end state of one task will finally use patchN's version.

If we see the attribute "using" is 1, it mean that livepatch will use this function to work but seem to be not all task running this version because the "transition" flag of this patch is "1". We can be told from "transition" that if all threads have completed synchronization.

So, the main function of this attribute is to enable user space find out which version of the patched function is running now (or will use after transition completed)in the system.

Please see if I have explain my opinion clearly.

Thanks!
Wardenjohn.

On Sat 2024-07-20 13:56:56, zhang warden wrote:
> 
> > is this always correct though? See the logic in klp_ftrace_handler(). If 
> > there is a transition running, it is a little bit more complicated.
> > 
> > Miroslav
> 
> Hi! Miroslav.
> 
> In reality, we often encounter such situation that serval patch in one system, some patch make changes to one same function A. This make us confused that we don't know which version of function A is now using. 
> 
> In my view, this "using" state show the function version that is now enabling. Even if there is a transition running, the end state of one task will finally use patchN's version.
> 
> If we see the attribute "using" is 1, it mean that livepatch will use this function to work but seem to be not all task running this version because the "transition" flag of this patch is "1". We can be told from "transition" that if all threads have completed synchronization.
>
> So, the main function of this attribute is to enable user space find out which version of the patched function is running now (or will use after transition completed)in the system.

The value is useless when the transition is in progress.
You simply do not know which variant is used in this case.

Which brings the question how exactly you use the value.
Could you please provide an example of decision which you make based
on the value?

If we agree that it makes sense then we should make it 3-state
where the meaning of values would be:

   -1: unknown (transition in progress)
   0: unused
   1: used

Best Regards,
Petr

Hi Petr!

> The value is useless when the transition is in progress.
> You simply do not know which variant is used in this case.
> 
Yes, I agree that if the patch is in transition, we can not know which version of this function is running by one task.

As my previous explanation, each patch have a state "transition" to show if this patch is under transition state. If this function "using" is 1, it shows that this function is going to become the version to be use, but not all the task use this newest version because some task is under transition (this is the "unknown" state from your opinion).

> Which brings the question how exactly you use the value.
> Could you please provide an example of decision which you make based
> on the value?
> 

Here I can give you an example.
We are going to fix a problem of io_uring.
Our team made a livepatch of io_sq_offload_create.
This livepatch module is deployed to some running servers.

Then, another team make some change to the same function and deployed it to the same cluster.

Finally, they found that there are some livepatch module modifying the same function io_sq_offload_create. But none of them can tell which version of io_sq_offload_create is now exactly running in the system.

We can only use crash to debug /proc/kcore to see if we can get more information from the kcore.

If livepatch can tell which version of the function is now running or going to run, it will be very useful.
> If we agree that it makes sense then we should make it 3-state
> where the meaning of values would be:
> 
>   -1: unknown (transition in progress)
>   0: unused
>   1: used
> 

Yeah, I agree with this state. I combine "transition" and "using" to tell the unknown state. It can be better if this state can be shown in using flag.

Thanks!
Wardenjohn