[PATCH v4 0/2] livepatch: Add using attribute to klp_func for using function

[PATCH v4 0/2] livepatch: Add using attribute to klp_func for using function

[Wardenjohn]

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

Changes v2 => v3:
1. Move klp_ops defintion into linux/livepatch.h
2. Move klp_ops pointer into klp_func (Suggested by Petr)
3. Rewrite function of klp_find_ops
4. Adjust the newer logic of "using" feature

In klp_complete_transition, if patch state is KLP_TRANSITION_PATCHED, we can get the 
function's next node. If the next node is itself, that means there is only one function.
If next node is not equal to the current function node, the next node "using" state
can be changed into "0".

Changes v3 => v4:

Improve the commit log of patch1. Add the reason of klp_find_ops change into commit log.
(Suggested by Jiri Kosina)

BTW, I remember to use ./script/checkpatch.pl to check my patches..Hah.. :)

[PATCH v4 1/2] Introduce klp_ops into klp_func structure

[Wardenjohn]

Before introduce feature "using". Klp transition will need an
easier way to get klp_ops from klp_func.

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

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;
+	struct list_head func_stack;
+	struct ftrace_ops fops;
+};
+
 /**
  * struct klp_func - function structure for live patching
  * @old_name:	name of the function to be patched
@@ -32,6 +51,7 @@
  * @kobj:	kobject for sysfs resources
  * @node:	list node for klp_object func_list
  * @stack_node:	list node for klp_ops func_stack list
+ * @ops:	pointer to klp_ops struct for this function
  * @old_size:	size of the old function
  * @new_size:	size of the new function
  * @nop:        temporary patch to use the original code again; dyn. allocated
@@ -71,6 +91,7 @@ struct klp_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;
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;
+
 	/*
 	 * 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;
+			}
+		}
 	}
 
 	return NULL;
@@ -133,7 +140,7 @@ static void klp_unpatch_func(struct klp_func *func)
 	if (WARN_ON(!func->old_func))
 		return;
 
-	ops = klp_find_ops(func->old_func);
+	ops = func->ops;
 	if (WARN_ON(!ops))
 		return;
 
@@ -149,6 +156,7 @@ static void klp_unpatch_func(struct klp_func *func)
 
 		list_del_rcu(&func->stack_node);
 		list_del(&ops->node);
+		func->ops = NULL;
 		kfree(ops);
 	} else {
 		list_del_rcu(&func->stack_node);
@@ -168,7 +176,7 @@ static int klp_patch_func(struct klp_func *func)
 	if (WARN_ON(func->patched))
 		return -EINVAL;
 
-	ops = klp_find_ops(func->old_func);
+	ops = func->ops;
 	if (!ops) {
 		unsigned long ftrace_loc;
 
@@ -191,8 +199,6 @@ static int klp_patch_func(struct klp_func *func)
 				  FTRACE_OPS_FL_IPMODIFY |
 				  FTRACE_OPS_FL_PERMANENT;
 
-		list_add(&ops->node, &klp_ops);
-
 		INIT_LIST_HEAD(&ops->func_stack);
 		list_add_rcu(&func->stack_node, &ops->func_stack);
 
@@ -211,7 +217,7 @@ static int klp_patch_func(struct klp_func *func)
 			goto err;
 		}
 
-
+		func->ops = ops;
 	} else {
 		list_add_rcu(&func->stack_node, &ops->func_stack);
 	}
@@ -224,6 +230,7 @@ static int klp_patch_func(struct klp_func *func)
 	list_del_rcu(&func->stack_node);
 	list_del(&ops->node);
 	kfree(ops);
+	func->ops = NULL;
 	return ret;
 }
 
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index d5f2fbe373e0..21d0d20b7189 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -6,25 +6,6 @@
 #include <linux/list.h>
 #include <linux/ftrace.h>
 
-/**
- * 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;
-	struct list_head func_stack;
-	struct ftrace_ops fops;
-};
-
 struct klp_ops *klp_find_ops(void *old_func);
 
 int klp_patch_object(struct klp_object *obj);
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index ba069459c101..d9a3f9c7a93b 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -230,7 +230,7 @@ static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
 		 * Check for the to-be-patched function
 		 * (the previous func).
 		 */
-		ops = klp_find_ops(func->old_func);
+		ops = func->ops;
 
 		if (list_is_singular(&ops->func_stack)) {
 			/* original function */
-- 
2.18.2

Re: [PATCH v4 1/2] Introduce klp_ops into klp_func structure

[Miroslav Benes]

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

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

Re: [PATCH v4 1/2] Introduce klp_ops into klp_func structure

[zhang warden]

Hi, Miroslav
> On Sep 5, 2024, at 18:10, Miroslav Benes <mbenes@suse.cz> wrote:
> 
> 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.
> 

After the previous discuss, maybe this patch of "livepatch: Move struct klp_ops to struct klp_func" is useful, while the second patch need to be considered later, right?

> 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@gmail.com>
> 
> Missing "Suggested-by: Petr Mladek <pmladek@suse.com> perhaps?
> 

Oops, I do miss this message here.

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

What is not needed any more, do you means the comment?
> 
>> +       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?

Do you mean I should add couple of lines after 'return -EINVAL' ?

> 
>> /*
>> * 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.
> 

Maybe there still other places will call this klp_find_ops? Is it safe to delete it?

Regards,
Wardenjohn

Re: [PATCH v4 1/2] Introduce klp_ops into klp_func structure

[Miroslav Benes]

Hi,

On Thu, 5 Sep 2024, zhang warden wrote:

> 
> Hi, Miroslav
> > On Sep 5, 2024, at 18:10, Miroslav Benes <mbenes@suse.cz> wrote:
> > 
> > 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.
> > 
> 
> After the previous discuss, maybe this patch of "livepatch: Move struct 
> klp_ops to struct klp_func" is useful, while the second patch need to be 
> considered later, right?

Yes, but keep them in one set for now, please.
 
> >> 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.
> 
> What is not needed any more, do you means the comment?

node member. You removed the global list, hence this member is 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?
> 
> Do you mean I should add couple of lines after 'return -EINVAL' ?

No, I am asking if there is a reason why you added 'func->ops = NULL;' 
here and not right after the rest of func initializations

        INIT_LIST_HEAD(&func->stack_node);
        func->patched = false;
        func->transition = false;

> > 
> >> /*
> >> * 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.
> > 
> 
> Maybe there still other places will call this klp_find_ops? Is it safe to delete it?

If you have no other plans with it, then it can be removed since there is 
no user after the patch.

Miroslav

Re: [PATCH v4 1/2] Introduce klp_ops into klp_func structure

[zhang warden]

Hi Miroslav

> 
> node member. You removed the global list, hence this member is not needed 
> anymore.

OK, I got it.

> 
>>> 
>>>> +       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?
>> 
>> Do you mean I should add couple of lines after 'return -EINVAL' ?
> 
> No, I am asking if there is a reason why you added 'func->ops = NULL;' 
> here and not right after the rest of func initializations
> 
>        INIT_LIST_HEAD(&func->stack_node);
>        func->patched = false;
>        func->transition = false;
> 

Hah... it just my habit to do so. Will fix it later.

>> 
>> Maybe there still other places will call this klp_find_ops? Is it safe to delete it?
> 
> If you have no other plans with it, then it can be removed since there is 
> no user after the patch.
> 

> Wardenjohn, you should then get all the information that you need. Also, 
> please test your patches with livepatch kselftests before a submission 
> next time. New sysfs attributes need to be documented in 
> Documentation/ABI/testing/sysfs-kernel-livepatch and there should be a new 
> kselftest for them.

OK, will do it.

Regards.
Wardenjohn.

Re: [PATCH v4 1/2] Introduce klp_ops into klp_func structure

[zhang warden]

Hi, Miroslav & Petr

> On Sep 6, 2024, at 17:44, zhang warden <zhangwarden@gmail.com> wrote:
> 
>> 
>>>> 
>>>>> +       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?
>>> 
>>> Do you mean I should add couple of lines after 'return -EINVAL' ?
>> 
>> No, I am asking if there is a reason why you added 'func->ops = NULL;' 
>> here and not right after the rest of func initializations
>> 
>>       INIT_LIST_HEAD(&func->stack_node);
>>       func->patched = false;
>>       func->transition = false;
>> 
> 

I think I found a bug in my patch.

I move struct klp_ops to klp_func. But every time, klp_func
will init klp_func->ops to NULL. Which will make the test branch
 `if(! ops)` always true in function klp_patch_func.

An alternative solution should be something like
 (as Peter suggested before) 

https://lore.kernel.org/live-patching/20240805064656.40017-1-zhangyongde.zyd@alibaba-inc.com/T/#t


 static struct klp_ops *klp_find_ops(void *old_func)
 {
       struct klp_patch *patch;
       struct klp_object *obj;
       struct klp_func *func;
       struct klp_ops *ops;
 
        klp_for_each_patch(patch)
                klp_for_each_object(patch, obj)
                        klp_for_each_func(obj, func)
                               if(func->old_func == old_func)
                                        return func->ops;
        return NULL;
 }

and  klp_ops should be initialize in klp_init_func:

        func->patched = false;
        func->transition = false;
+       func->ops = klp_find_ops(func->old_func);

Here, func->ops should not init as NULL, it should be initialize with 
the existed ops (if klp_find_ops returns NULL, this patch is the first
time to be patched).

Regards.
Wardenjohn.

[PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Wardenjohn]

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 d874aecc817b..5a6bacebd66f 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -57,6 +57,7 @@ struct klp_ops {
  * @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:
@@ -72,6 +73,12 @@ struct klp_ops {
  *   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 */
@@ -96,6 +103,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 e4572bf34316..bc1b2085e3c5 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)
@@ -775,6 +793,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
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 8ab9c35570f4..5138cedfcfaa 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -134,6 +134,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;
@@ -160,6 +161,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;
@@ -168,6 +173,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))
@@ -219,10 +225,16 @@ static int klp_patch_func(struct klp_func *func)
 
 		func->ops = ops;
 	} 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->patched = true;
+	func->using = -1;
 
 	return 0;
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index d9a3f9c7a93b..365dac635efe 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,39 @@ 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);
+				if (&func->stack_node != &func->ops->func_stack)
+					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 = func->ops;
+				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)
-- 
2.18.2

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden]

> On Aug 28, 2024, at 10:23, Wardenjohn <zhangwarden@gmail.com> wrote:
> 
> 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 this patch? I am looking for your suggestions.

Regards,
Wardenjohn.

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Josh Poimboeuf]

On Wed, Aug 28, 2024 at 10:23:50AM +0800, Wardenjohn wrote:
> 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.

I'm missing how this is actually useful in the real world.  It feels
like a solution in search of a problem.  And it adds significant
maintenance burden.  Why?

Do you not have any control over what order your patches are applied?
If not, that sounds dangerous and you have much bigger problems.

This "problem" needs to be managed in user space.

-- 
Josh

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden]

> On Sep 4, 2024, at 12:48, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> 
> On Wed, Aug 28, 2024 at 10:23:50AM +0800, Wardenjohn wrote:
>> 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.
> 
> I'm missing how this is actually useful in the real world.  It feels
> like a solution in search of a problem.  And it adds significant
> maintenance burden.  Why?
> 
> Do you not have any control over what order your patches are applied?
> If not, that sounds dangerous and you have much bigger problems.
> 
> This "problem" needs to be managed in user space.
> 
> -- 
> Josh

Hi, Josh.

First, introducing feature "using" is just a state of one klp_function is using or not in the system.
I think this state will not bring significant maintenance burden, right?

And then, this feature can tell user that which function is now running in this system. As we know, livepatch enable many patches for one function, and the stack top function of target function is the "using" function. Here will bring us some questions: 
1. Since we patch some patches to one function, which version of this function A is my system exactly now using? 
2. One patch may contains many function fixes, the "using" version of target function belongs to which patch now? 
Livepatch now cannot tell us this information, although livepatch can do it.

In the scenario where multiple people work together to maintain the same system, or a long time running system, the patch sets will inevitably be cumulative. I think kernel can maintain and tell user this most accurate information by one sysfs interface.

A real case I met have been shown in the previous mail to explain the use in real world.

At the process I introduce this feature, we found that the function of klp_find_ops can be optimized. If we can move klp_ops structure into klp_func, we don't need to maintain one global list again. And if we want to find an klp_ops, we just need to get the klp_func (which we will already have). This will be a nice efficiency improvement.

Regards.
Wardenjohn.

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Josh Poimboeuf]

On Wed, Sep 04, 2024 at 02:34:44PM +0800, zhang warden wrote:
> In the scenario where multiple people work together to maintain the
> same system, or a long time running system, the patch sets will
> inevitably be cumulative. I think kernel can maintain and tell user
> this most accurate information by one sysfs interface.

If there are multiple people applying patches independently from each
other (to the same function even!), you are playing with fire, as there
could easily be implicit dependencies and conflicts between the patches.

-- 
Josh

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden]

> On Sep 4, 2024, at 15:14, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> 
> On Wed, Sep 04, 2024 at 02:34:44PM +0800, zhang warden wrote:
>> In the scenario where multiple people work together to maintain the
>> same system, or a long time running system, the patch sets will
>> inevitably be cumulative. I think kernel can maintain and tell user
>> this most accurate information by one sysfs interface.
> 
> If there are multiple people applying patches independently from each
> other (to the same function even!), you are playing with fire, as there
> could easily be implicit dependencies and conflicts between the patches.
> 
> 
Yep, I agree with you. This is not a good practice.

But we can work further, livepatch can tell which function is now running. This feature can do more than that.

Afterall, users alway want to know if their newly patched function successfully enabled and using to fix the bug-existed kernel function.

With this feature, user can confirm their patch is successfully running instead of using crash to look into /proc/kcore to make sure this function is running now. (I always use this method to check my function patched ... lol).

And I think further, if we use kpatch-build[1], `kpatch list` can not only tell us which patch is enabled, but also tell us the relationship between running function and patched module.
I think this is an exciting feature...

[1] https://github.com/dynup/kpatch.git

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Josh Poimboeuf]

On Wed, Sep 04, 2024 at 03:30:22PM +0800, zhang warden wrote:
> > On Sep 4, 2024, at 15:14, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > If there are multiple people applying patches independently from each
> > other (to the same function even!), you are playing with fire, as there
> > could easily be implicit dependencies and conflicts between the patches.
> > 
> > 
> Yep, I agree with you. This is not a good practice.
> 
> But we can work further, livepatch can tell which function is now running. This feature can do more than that.
> 
> Afterall, users alway want to know if their newly patched function successfully enabled and using to fix the bug-existed kernel function.
> 
> With this feature, user can confirm their patch is successfully running instead of using crash to look into /proc/kcore to make sure this function is running now. (I always use this method to check my function patched ... lol).
> 
> And I think further, if we use kpatch-build[1], `kpatch list` can not only tell us which patch is enabled, but also tell us the relationship between running function and patched module.
> I think this is an exciting feature...

Most of this information is already available in sysfs, with the
exception of patch stacking order.

Every new feature adds maintenance burden.  It might not seem like much
to you, but the features add up, and livepatch needs to be solid.

We want patches that fix real world, tangible problems, not theoretical
problems that it *might* solve for a hypothetical user.

What is the motiviation behind this patch?  What real world problem does
it fix for you, or an actual user?  Have you considered other solutions,
like more organized patch management in user space?

-- 
Josh

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden]

Hi, Josh.
> Most of this information is already available in sysfs, with the
> exception of patch stacking order.
> 
Well, this is the problem my patch want to fix. But my patch is more simpler, it just shows the stack top of the target function, which is the only thing users care.

> 
> We want patches that fix real world, tangible problems, not theoretical
> problems that it *might* solve for a hypothetical user.
> 
> What is the motiviation behind this patch?  What real world problem does
> it fix for you, or an actual user?  

Here I can give you an example as I previous described:

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

>>>>>>
What's more, the scenario we easily face is that for the confidential environment, the system maintenance mainly depends on SREs. Different team may do bug fix or performance optimization to kernel function. 

Here usually some SREs comes to me and ask me how to make sure which version is now actually active because tow teams make tow livepatch modules, both of them make changes to one function. 

He wants to know if his system is under risk, he want the system run the right version of the function because one module is a bug fix and the other is just a performance optimization module, at this time, the bug fix version is much more important. dmesg is too long, he find it hard to find out the patch order from dmesg.

With this patch, he can just cat /sys/kernel/livepatch/<module>/<object>/<function>/using and get his answer.

> Have you considered other solutions,
> like more organized patch management in user space?

User space solutions seems unreliable. What we need is just the enabling version of target function. The order of livepatch module enable mainly from dmesg, which is easily flush away or being cleaned.

If we use an user space program to maintain the information of patch order, once the program is killed, the information is loss either.

Neither of the previous user space solutions seems reliable. Only kernel space will no one can change it. And it is the most accurate.

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Josh Poimboeuf]

On Thu, Sep 05, 2024 at 10:03:52PM +0800, zhang warden wrote:
> >>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.
> 
> >>>>>>
> What's more, the scenario we easily face is that for the confidential environment, the system maintenance mainly depends on SREs. Different team may do bug fix or performance optimization to kernel function. 
> 
> Here usually some SREs comes to me and ask me how to make sure which version is now actually active because tow teams make tow livepatch modules, both of them make changes to one function. 
> 
> He wants to know if his system is under risk, he want the system run the right version of the function because one module is a bug fix and the other is just a performance optimization module, at this time, the bug fix version is much more important. dmesg is too long, he find it hard to find out the patch order from dmesg.
> 
> With this patch, he can just cat /sys/kernel/livepatch/<module>/<object>/<function>/using and get his answer.

Thanks for the details, and sorry if I missed it before.  This would
have been helpful to have in the cover letter.

But again I want to stress that livepatching should be done with care.
Having different teams applying patches without coordination is not
recommended.  The teams' processes and/or tooling really need to be
improved.

> > Have you considered other solutions,
> > like more organized patch management in user space?
> 
> User space solutions seems unreliable. What we need is just the enabling version of target function. The order of livepatch module enable mainly from dmesg, which is easily flush away or being cleaned.

journalctl -b ?

> If we use an user space program to maintain the information of patch order, once the program is killed, the information is loss either.

Store the state in a file in /var/run?

-- 
Josh

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Miroslav Benes]

Hi,

On Wed, 28 Aug 2024, Wardenjohn wrote:

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

I am not a fan. Josh wrote most of my objections already so I will not 
repeat them. I understand that the attribute might be useful but the 
amount of code it adds to sensitive functions like 
klp_complete_transition() is no fun.

Would it be possible to just use klp_transition_patch and implement the 
logic just in using_show()? I have not thought through it completely but 
klp_transition_patch is also an indicator that there is a transition going 
on. It is set to NULL only after all func->transition are false. So if you 
check that, you can assign -1 in using_show() immediately and then just 
look at the top of func_stack.

If possible (and there are corner cases everywhere. Just take a look at 
barriers in all those functions.) and the resulting code is much simpler, 
we might take it. But otherwise this should really be solved in userspace 
using some live patch management tool as Josh said. I mean generally 
because you have much more serious problems without it.

Regards,
Miroslav

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden]

Hi Miroslav,
> 
> 
> I am not a fan. Josh wrote most of my objections already so I will not 
> repeat them. I understand that the attribute might be useful but the 
> amount of code it adds to sensitive functions like 
> klp_complete_transition() is no fun.
> 
OK, the point I make changes to klp_complete_transition is that when a transition is going to be complete, we can make sure the function state can go to an end state (0 or 1), which is the most easy way to do so...lol...

> Would it be possible to just use klp_transition_patch and implement the 
> logic just in using_show()? I have not thought through it completely but 
> klp_transition_patch is also an indicator that there is a transition going 
> on. It is set to NULL only after all func->transition are false. So if you 
> check that, you can assign -1 in using_show() immediately and then just 
> look at the top of func_stack.
> 

I will consider it later. If you have any suggestions or other solutions, please let me know.

> If possible (and there are corner cases everywhere. Just take a look at 
> barriers in all those functions.) and the resulting code is much simpler, 
> we might take it. But otherwise this should really be solved in userspace 
> using some live patch management tool as Josh said. I mean generally 
> because you have much more serious problems without it.
> 

I replied to Josh to explain my reason of not using user space tools to maintain livepatch information. Of cause, I put my patch here and tell you the problem I am facing, maybe there some people may face the same problem as me...hah...

We can discuss it, if you have a better idea for that patch, please fell free to tell me.

Also, I forgot to sign at the end of the email I sent Josh, I'm sorry...

Thanks.
Wardenjohn.

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Josh Poimboeuf]

On Thu, Sep 05, 2024 at 12:23:20PM +0200, Miroslav Benes wrote:
> I am not a fan. Josh wrote most of my objections already so I will not 
> repeat them. I understand that the attribute might be useful but the 
> amount of code it adds to sensitive functions like 
> klp_complete_transition() is no fun.
> 
> Would it be possible to just use klp_transition_patch and implement the 
> logic just in using_show()?

Yes, containing the logic to the sysfs file sounds a lot better.

> I have not thought through it completely but 
> klp_transition_patch is also an indicator that there is a transition going 
> on. It is set to NULL only after all func->transition are false. So if you 
> check that, you can assign -1 in using_show() immediately and then just 
> look at the top of func_stack.

sysfs already has per-patch 'transition' and 'enabled' files so I don't
like duplicating that information.

The only thing missing is the patch stack order.  How about a simple
per-patch file which indicates that?

  /sys/kernel/livepatch/<patchA>/order => 1
  /sys/kernel/livepatch/<patchB>/order => 2

The implementation should be trivial with the use of
klp_for_each_patch() to count the patches.

-- 
Josh

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Miroslav Benes]

On Thu, 5 Sep 2024, Josh Poimboeuf wrote:

> On Thu, Sep 05, 2024 at 12:23:20PM +0200, Miroslav Benes wrote:
> > I am not a fan. Josh wrote most of my objections already so I will not 
> > repeat them. I understand that the attribute might be useful but the 
> > amount of code it adds to sensitive functions like 
> > klp_complete_transition() is no fun.
> > 
> > Would it be possible to just use klp_transition_patch and implement the 
> > logic just in using_show()?
> 
> Yes, containing the logic to the sysfs file sounds a lot better.
> 
> > I have not thought through it completely but 
> > klp_transition_patch is also an indicator that there is a transition going 
> > on. It is set to NULL only after all func->transition are false. So if you 
> > check that, you can assign -1 in using_show() immediately and then just 
> > look at the top of func_stack.
> 
> sysfs already has per-patch 'transition' and 'enabled' files so I don't
> like duplicating that information.
> 
> The only thing missing is the patch stack order.  How about a simple
> per-patch file which indicates that?
> 
>   /sys/kernel/livepatch/<patchA>/order => 1
>   /sys/kernel/livepatch/<patchB>/order => 2
> 
> The implementation should be trivial with the use of
> klp_for_each_patch() to count the patches.

Yes, it should work as well.

Wardenjohn, you should then get all the information that you need. Also, 
please test your patches with livepatch kselftests before a submission 
next time. New sysfs attributes need to be documented in 
Documentation/ABI/testing/sysfs-kernel-livepatch and there should be a new 
kselftest for them.

Thank you,
Miroslav

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden]

Hi, John & Miroslav

>> 
>> Would it be possible to just use klp_transition_patch and implement the 
>> logic just in using_show()?
> 
> Yes, containing the logic to the sysfs file sounds a lot better.

Maybe I can try to use the state of klp_transition_patch to update the function's state instead of changing code in klp_complete_transition?

> 
>> I have not thought through it completely but 
>> klp_transition_patch is also an indicator that there is a transition going 
>> on. It is set to NULL only after all func->transition are false. So if you 
>> check that, you can assign -1 in using_show() immediately and then just 
>> look at the top of func_stack.
> 
> sysfs already has per-patch 'transition' and 'enabled' files so I don't
> like duplicating that information.
> 
> The only thing missing is the patch stack order.  How about a simple
> per-patch file which indicates that?
> 
>  /sys/kernel/livepatch/<patchA>/order => 1
>  /sys/kernel/livepatch/<patchB>/order => 2
> 
> The implementation should be trivial with the use of
> klp_for_each_patch() to count the patches.
> 
I think this is the second solution. It seems that adding an interface to patch level is an acceptable way. And if patch order is provided in /sys/kernel/livepatch/<patchA>/order, we should make a user space tool to calculate the function that is activate in the system. From my point to the original problem, it is more look like a workaround.

> journalctl -b ?
> Store the state in a file in /var/run?

Getting patch order from journal is the way I think not reliable. In fact, I don't recommend to get patch order in that way. ^_^

Thanks!
Wardenjohn

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Petr Mladek]

On Fri 2024-09-06 17:39:46, zhang warden wrote:
> Hi, John & Miroslav
> 
> >> 
> >> Would it be possible to just use klp_transition_patch and implement the 
> >> logic just in using_show()?
> > 
> > Yes, containing the logic to the sysfs file sounds a lot better.
> 
> Maybe I can try to use the state of klp_transition_patch to update the function's state instead of changing code in klp_complete_transition?
> 
> > 
> >> I have not thought through it completely but 
> >> klp_transition_patch is also an indicator that there is a transition going 
> >> on. It is set to NULL only after all func->transition are false. So if you 
> >> check that, you can assign -1 in using_show() immediately and then just 
> >> look at the top of func_stack.
> > 
> > sysfs already has per-patch 'transition' and 'enabled' files so I don't
> > like duplicating that information.
> > 
> > The only thing missing is the patch stack order.  How about a simple
> > per-patch file which indicates that?
> > 
> >  /sys/kernel/livepatch/<patchA>/order => 1
> >  /sys/kernel/livepatch/<patchB>/order => 2
> > 
> > The implementation should be trivial with the use of
> > klp_for_each_patch() to count the patches.
> > 
> I think this is the second solution. It seems that adding an
> interface to patch level is an acceptable way.

It seems to be the only acceptable idea at the moment.

> And if patch order
> is provided in /sys/kernel/livepatch/<patchA>/order, we should
> make a user space tool to calculate the function that
> is activate in the system. From my point to the original
> problem, it is more look like a workaround.

It is always a compromise between the complexity and the benefit.
Upstream will accept only changes which are worth it.

Here, the main problem is that you do not have coordinated developement
and installation of livepatches. This is dangerous and you should
not do it! Upstream will never support such a wild approach.

You could get upstream some features which would make your life
easier. But the features must be worth the effort.

Best Regards,
Petr

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden]

Hi, Petr

> On Sep 7, 2024, at 00:39, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Fri 2024-09-06 17:39:46, zhang warden wrote:
>> Hi, John & Miroslav
>> 
>>>> 
>>>> Would it be possible to just use klp_transition_patch and implement the 
>>>> logic just in using_show()?
>>> 
>>> Yes, containing the logic to the sysfs file sounds a lot better.
>> 
>> Maybe I can try to use the state of klp_transition_patch to update the function's state instead of changing code in klp_complete_transition?
>> 
>>> 
>>>> I have not thought through it completely but 
>>>> klp_transition_patch is also an indicator that there is a transition going 
>>>> on. It is set to NULL only after all func->transition are false. So if you 
>>>> check that, you can assign -1 in using_show() immediately and then just 
>>>> look at the top of func_stack.
>>> 
>>> sysfs already has per-patch 'transition' and 'enabled' files so I don't
>>> like duplicating that information.
>>> 
>>> The only thing missing is the patch stack order.  How about a simple
>>> per-patch file which indicates that?
>>> 
>>> /sys/kernel/livepatch/<patchA>/order => 1
>>> /sys/kernel/livepatch/<patchB>/order => 2
>>> 
>>> The implementation should be trivial with the use of
>>> klp_for_each_patch() to count the patches.
>>> 
>> I think this is the second solution. It seems that adding an
>> interface to patch level is an acceptable way.
> 
> It seems to be the only acceptable idea at the moment.
> 
>> And if patch order
>> is provided in /sys/kernel/livepatch/<patchA>/order, we should
>> make a user space tool to calculate the function that
>> is activate in the system. From my point to the original
>> problem, it is more look like a workaround.
> 
> It is always a compromise between the complexity and the benefit.
> Upstream will accept only changes which are worth it.
> 
> Here, the main problem is that you do not have coordinated developement
> and installation of livepatches. This is dangerous and you should
> not do it! Upstream will never support such a wild approach.
> 
> You could get upstream some features which would make your life
> easier. But the features must be worth the effort.

Yep, I have the same idea as you here. The problem we face now as Josh describes, livepatch now missing the information of patch order. If we can have it, the rest information user need can be processed my the original information from the kernel.

My intention to introduce "using" feature is to solve the missing info of livepatch, facing the original problem I met. But as livepatch becomes more and more widely used, this is a real problem that may be widely encountered.

If maintainer thinks the way changing klp_transition_patch is not worth it. Maybe I can try another way to fix this problem. ( For example, the patch-level order interface?) :) 

From my point of view, this information have its worth it provide, what we need to consider is how to implement this function specifically. Do you agree with me?

Best Regards,
Wardenjohn.

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Petr Mladek]

On Thu 2024-09-05 12:23:20, Miroslav Benes wrote:
> Hi,
> 
> On Wed, 28 Aug 2024, Wardenjohn wrote:
> 
> > 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>
> 
> I am not a fan. Josh wrote most of my objections already so I will not 
> repeat them. I understand that the attribute might be useful but the 
> amount of code it adds to sensitive functions like 
> klp_complete_transition() is no fun.
> 
> Would it be possible to just use klp_transition_patch and implement the 
> logic just in using_show()? I have not thought through it completely but 
> klp_transition_patch is also an indicator that there is a transition going 
> on. It is set to NULL only after all func->transition are false. So if you 
> check that, you can assign -1 in using_show() immediately and then just 
> look at the top of func_stack.

The 1st patch adds the pointer to struct klp_ops into struct
klp_func. We might check the state a similar way as klp_ftrace_handler().

I had something like this in mind when I suggested to move the pointer:

static ssize_t using_show(struct kobject *kobj,
				struct kobj_attribute *attr, char *buf)
{
	struct klp_func *func, *using_func;
	struct klp_ops *ops;
	int using;

	func = container_of(kobj, struct klp_func, kobj);

	rcu_read_lock();

	if (func->transition) {
		using = -1;
		goto out;
	}

	# FIXME: This requires releasing struct klp_ops via call_rcu()
	ops = func->ops;
	if (!ops) {
		using = 0;
		goto out;
	}

	using_func = list_first_or_null_rcu(&ops->func_stack,
					struct klp_func, stack_node);
	if (func == using_func)
		using = 1;
	else
		using = 0;

out:
	rcu_read_unlock();

	return sysfs_emit(buf, "%d\n", func->using);
}

It is racy and tricky. We probably should add some memory barriers.
And maybe even the ordering of reads should be different.

We could not take klp_mutex because it might cause a deadlock when
the sysfs file gets removed. kobject_put(&func->kobj) is called
by __klp_free_funcs() under klp_mutex.

It would be easier if we could take klp_mutex. But it would require
decrementing the kobject refcout without of klp_mutex. It might
be complicated.

I am afraid that this approach is not worth the effort and
is is not the way to go.

Best Regards,
Petr

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden]

Hi, Petr
> 
> The 1st patch adds the pointer to struct klp_ops into struct
> klp_func. We might check the state a similar way as klp_ftrace_handler().
> 
> I had something like this in mind when I suggested to move the pointer:
> 
> static ssize_t using_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> struct klp_func *func, *using_func;
> struct klp_ops *ops;
> int using;
> 
> func = container_of(kobj, struct klp_func, kobj);
> 
> rcu_read_lock();
> 
> if (func->transition) {
> using = -1;
> goto out;
> }
> 
> # FIXME: This requires releasing struct klp_ops via call_rcu()
> ops = func->ops;
> if (!ops) {
> using = 0;
> goto out;
> }
> 
> using_func = list_first_or_null_rcu(&ops->func_stack,
> struct klp_func, stack_node);
> if (func == using_func)
> using = 1;
> else
> using = 0;
> 
> out:
> rcu_read_unlock();
> 
> return sysfs_emit(buf, "%d\n", func->using);
> }

Bravo, I also have something like this in mind when Miroslav suggested to implement the logic in using_show. 

> 
> It is racy and tricky. We probably should add some memory barriers.
> And maybe even the ordering of reads should be different.
> 
> We could not take klp_mutex because it might cause a deadlock when
> the sysfs file gets removed. kobject_put(&func->kobj) is called
> by __klp_free_funcs() under klp_mutex.
> 
> It would be easier if we could take klp_mutex. But it would require
> decrementing the kobject refcout without of klp_mutex. It might
> be complicated.
> 
> I am afraid that this approach is not worth the effort and
> is is not the way to go.
> 

But I don't think I've thought as deeply as you do and may not have considered the possible risks. Therefore, I do need your help to make my patch perfect.  ^_^

Thank you.
Wardenjohn.

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[Petr Mladek]

On Sun 2024-09-08 10:51:14, zhang warden wrote:
> 
> Hi, Petr
> > 
> > The 1st patch adds the pointer to struct klp_ops into struct
> > klp_func. We might check the state a similar way as klp_ftrace_handler().
> > 
> > I had something like this in mind when I suggested to move the pointer:
> > 
> > static ssize_t using_show(struct kobject *kobj,
> > struct kobj_attribute *attr, char *buf)
> > {
> > struct klp_func *func, *using_func;
> > struct klp_ops *ops;
> > int using;
> > 
> > func = container_of(kobj, struct klp_func, kobj);
> > 
> > rcu_read_lock();
> > 
> > if (func->transition) {
> > using = -1;
> > goto out;
> > }
> > 
> > # FIXME: This requires releasing struct klp_ops via call_rcu()

This would require adding "struct rcu_head" into "struct klp_ops",
like:

struct klp_ops {
	struct list_head func_stack;
	struct ftrace_ops fops;
	struct rcu_head rcu;
};

and then freeing the structure using kfree_rcu():

diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 90408500e5a3..f096dd9390d2 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -149,7 +149,7 @@ static void klp_unpatch_func(struct klp_func *func)
 
 		list_del_rcu(&func->stack_node);
 		list_del(&ops->node);
-		kfree(ops);
+		kfree_rcu(ops, rcu);
 	} else {
 		list_del_rcu(&func->stack_node);
 	}
@@ -223,7 +223,7 @@ static int klp_patch_func(struct klp_func *func)
 err:
 	list_del_rcu(&func->stack_node);
 	list_del(&ops->node);
-	kfree(ops);
+	kfree_rcu(ops, rcu);
 	return ret;
 }

With this the function should be safe against accessing an invalid
pointer.

> > ops = func->ops;
> > if (!ops) {
> > using = 0;
> > goto out;
> > }
> > 
> > using_func = list_first_or_null_rcu(&ops->func_stack,
> > struct klp_func, stack_node);
> > if (func == using_func)
> > using = 1;
> > else
> > using = 0;
> > 
> > out:
> > rcu_read_unlock();
> > 
> > return sysfs_emit(buf, "%d\n", func->using);
> > }

But the function is still not correct according the order of reading.
A more correct solution would be something like:

static ssize_t using_show(struct kobject *kobj,
				struct kobj_attribute *attr, char *buf)
{
	struct klp_func *func, *using_func;
	struct klp_ops *ops;
	int using;

	func = container_of(kobj, struct klp_func, kobj);

	rcu_read_lock();

	/* This livepatch is used when the function is on top of the stack. */
	ops = func->ops;
	if (ops) {
		using_func = list_first_or_null_rcu(&ops->func_stack,
						struct klp_func, stack_node);
		if (func == using_func)
			using = 1;
		else
			using = 0;
	}

	/*
	 * The function stack gives the right information only when there
	 * is no transition in progress.
	 *
	 * Make sure that we see the updated ops->func_stack when
	 * func->transition is cleared. This matches with:
	 *
	 * The write barrier in  __klp_enable_patch() between
	 * klp_init_transition() and klp_patch_object().
	 *
	 * The write barrier in  __klp_disable_patch() between
	 * klp_init_transition() and klp_start_transition().
	 *
	 * The write barrier in klp_complete_transition()
	 * between klp_unpatch_objects() and func->transition = false.
	 */
	smp_rmb();

	if (func->transition)
		using = -1;

	rcu_read_unlock();

	return sysfs_emit(buf, "%d\n", func->using);
}

Now, the question is whether we want to maintain such a barrier. Any
lockless access and barrier adds a maintenance burden.

You might try to put the above into a patch see what others tell
about it.

Best Regards,
Petr

Re: [PATCH v4 2/2] livepatch: Add using attribute to klp_func for using function show

[zhang warden]

Hi, Petr

> On Sep 10, 2024, at 16:01, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Sun 2024-09-08 10:51:14, zhang warden wrote:
>> 
>> Hi, Petr
>>> 
>>> The 1st patch adds the pointer to struct klp_ops into struct
>>> klp_func. We might check the state a similar way as klp_ftrace_handler().
>>> 
>>> I had something like this in mind when I suggested to move the pointer:
>>> 
>>> static ssize_t using_show(struct kobject *kobj,
>>> struct kobj_attribute *attr, char *buf)
>>> {
>>> struct klp_func *func, *using_func;
>>> struct klp_ops *ops;
>>> int using;
>>> 
>>> func = container_of(kobj, struct klp_func, kobj);
>>> 
>>> rcu_read_lock();
>>> 
>>> if (func->transition) {
>>> using = -1;
>>> goto out;
>>> }
>>> 
>>> # FIXME: This requires releasing struct klp_ops via call_rcu()
> 
> This would require adding "struct rcu_head" into "struct klp_ops",
> like:
> 
> struct klp_ops {
> struct list_head func_stack;
> struct ftrace_ops fops;
> struct rcu_head rcu;
> };
> 
> and then freeing the structure using kfree_rcu():
> 
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 90408500e5a3..f096dd9390d2 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -149,7 +149,7 @@ static void klp_unpatch_func(struct klp_func *func)
> 
> list_del_rcu(&func->stack_node);
> list_del(&ops->node);
> - kfree(ops);
> + kfree_rcu(ops, rcu);
> } else {
> list_del_rcu(&func->stack_node);
> }
> @@ -223,7 +223,7 @@ static int klp_patch_func(struct klp_func *func)
> err:
> list_del_rcu(&func->stack_node);
> list_del(&ops->node);
> - kfree(ops);
> + kfree_rcu(ops, rcu);
> return ret;
> }
> 
> With this the function should be safe against accessing an invalid
> pointer.
> 
>>> ops = func->ops;
>>> if (!ops) {
>>> using = 0;
>>> goto out;
>>> }
>>> 
>>> using_func = list_first_or_null_rcu(&ops->func_stack,
>>> struct klp_func, stack_node);
>>> if (func == using_func)
>>> using = 1;
>>> else
>>> using = 0;
>>> 
>>> out:
>>> rcu_read_unlock();
>>> 
>>> return sysfs_emit(buf, "%d\n", func->using);
>>> }
> 
> But the function is still not correct according the order of reading.
> A more correct solution would be something like:
> 
> static ssize_t using_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> struct klp_func *func, *using_func;
> struct klp_ops *ops;
> int using;
> 
> func = container_of(kobj, struct klp_func, kobj);
> 
> rcu_read_lock();
> 
> /* This livepatch is used when the function is on top of the stack. */
> ops = func->ops;
> if (ops) {
> using_func = list_first_or_null_rcu(&ops->func_stack,
> struct klp_func, stack_node);
> if (func == using_func)
> using = 1;
> else
> using = 0;
> }
> 
> /*
>  * The function stack gives the right information only when there
>  * is no transition in progress.
>  *
>  * Make sure that we see the updated ops->func_stack when
>  * func->transition is cleared. This matches with:
>  *
>  * The write barrier in  __klp_enable_patch() between
>  * klp_init_transition() and klp_patch_object().
>  *
>  * The write barrier in  __klp_disable_patch() between
>  * klp_init_transition() and klp_start_transition().
>  *
>  * The write barrier in klp_complete_transition()
>  * between klp_unpatch_objects() and func->transition = false.
>  */
> smp_rmb();
> 
> if (func->transition)
> using = -1;
> 
> rcu_read_unlock();
> 
> return sysfs_emit(buf, "%d\n", func->using);
> }
> 
> Now, the question is whether we want to maintain such a barrier. Any
> lockless access and barrier adds a maintenance burden.
> 
> You might try to put the above into a patch see what others tell
> about it.
> 
> Best Regards,
> Petr

After the previous discussion, it seems that patch-level "order" feature
 is the more acceptable way to fulfill the patch order information.

Therefore, I am trying to go that way instead of adding "using" into klp_func.

Maybe patch-level interface will bring less maintenance burden.

Regards,
Wardenjohn.