[PATCH V3 0/1] livepatch: Add "stack_order" sysfs attribute

[PATCH V3 0/1] livepatch: Add "stack_order" sysfs attribute

[Wardenjohn]

As previous discussion, maintainers think that patch-level sysfs interface is the
only acceptable way to maintain the information of the order that klp_patch is 
applied to the system.

However, the previous patch introduce klp_ops into klp_func is a optimization 
methods of the patch introducing 'using' feature to klp_func.

But now, we don't support 'using' feature to klp_func and make 'klp_ops' patch
not necessary.

Therefore, this new version is only introduce the sysfs feature of klp_patch 
'stack_order'.

V1 -> V2:
1. According to the suggestion from Petr, to make the meaning more clear, rename
'order' to 'stack_order'.
2. According to the suggestion from Petr and Miroslav, this patch now move the 
calculating process to stack_order_show function. Adding klp_mutex lock protection.

V2 -> V3:
1. Squash 2 patches into 1. Update description of stack_order in ABI Document.
(Suggested by Miroslav)
2. Update subject and commit log. (Suggested by Miroslav)
3. Update code format for some line of the patch. (Suggested by Miroslav)

Regards.
Wardenjohn.

[PATCH V3 1/1] livepatch: Add "stack_order" sysfs attribute

[Wardenjohn]

Add "stack_order" sysfs attribute which holds the order in which a live
patch module was loaded into the system. A user can then determine an
active live patched version of a function.

cat /sys/kernel/livepatch/livepatch_1/stack_order -> 1

means that livepatch_1 is the first live patch applied

cat /sys/kernel/livepatch/livepatch_module/stack_order -> N

means that livepatch_module is the Nth live patch applied

Suggested-by: Petr Mladek <pmladek@suse.com>
Suggested-by: Miroslav Benes <mbenes@suse.cz>
Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Wardenjohn <zhangwarden@gmail.com>
---
 .../ABI/testing/sysfs-kernel-livepatch        |  8 ++++++
 kernel/livepatch/core.c                       | 25 +++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index a5df9b4910dc..2a60b49aa9a5 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -47,6 +47,14 @@ Description:
 		disabled when the feature is used. See
 		Documentation/livepatch/livepatch.rst for more information.
 
+What:           /sys/kernel/livepatch/<patch>/stack_order
+Date:           Sep 2024
+KernelVersion:  6.12.0
+Contact:        live-patching@vger.kernel.org
+Description:
+		This attribute holds the stack order of a livepatch module applied
+		to the running system.
+
 What:		/sys/kernel/livepatch/<patch>/<object>
 Date:		Nov 2014
 KernelVersion:	3.19.0
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ecbc9b6aba3a..30ab3668c59e 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -346,6 +346,7 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
  * /sys/kernel/livepatch/<patch>/enabled
  * /sys/kernel/livepatch/<patch>/transition
  * /sys/kernel/livepatch/<patch>/force
+ * /sys/kernel/livepatch/<patch>/stack_order
  * /sys/kernel/livepatch/<patch>/<object>
  * /sys/kernel/livepatch/<patch>/<object>/patched
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
@@ -443,13 +444,37 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
 	return count;
 }
 
+static ssize_t stack_order_show(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	struct klp_patch *patch, *this_patch;
+	int stack_order = 0;
+
+	this_patch = container_of(kobj, struct klp_patch, kobj);
+
+	mutex_lock(&klp_mutex);
+
+	klp_for_each_patch(patch) {
+		stack_order++;
+		if (patch == this_patch)
+			break;
+	}
+
+	mutex_unlock(&klp_mutex);
+
+	return sysfs_emit(buf, "%d\n", stack_order);
+}
+
+
 static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
 static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition);
 static struct kobj_attribute force_kobj_attr = __ATTR_WO(force);
+static struct kobj_attribute stack_order_kobj_attr = __ATTR_RO(stack_order);
 static struct attribute *klp_patch_attrs[] = {
 	&enabled_kobj_attr.attr,
 	&transition_kobj_attr.attr,
 	&force_kobj_attr.attr,
+	&stack_order_kobj_attr.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(klp_patch);
-- 
2.18.2

Re: [PATCH V3 1/1] livepatch: Add "stack_order" sysfs attribute

[Josh Poimboeuf]

On Sun, Sep 29, 2024 at 10:43:34PM +0800, Wardenjohn wrote:
> Add "stack_order" sysfs attribute which holds the order in which a live
> patch module was loaded into the system. A user can then determine an
> active live patched version of a function.
> 
> cat /sys/kernel/livepatch/livepatch_1/stack_order -> 1
> 
> means that livepatch_1 is the first live patch applied
> 
> cat /sys/kernel/livepatch/livepatch_module/stack_order -> N
> 
> means that livepatch_module is the Nth live patch applied
> 
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Suggested-by: Miroslav Benes <mbenes@suse.cz>
> Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org>
> Signed-off-by: Wardenjohn <zhangwarden@gmail.com>
> ---
>  .../ABI/testing/sysfs-kernel-livepatch        |  8 ++++++
>  kernel/livepatch/core.c                       | 25 +++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> index a5df9b4910dc..2a60b49aa9a5 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> @@ -47,6 +47,14 @@ Description:
>  		disabled when the feature is used. See
>  		Documentation/livepatch/livepatch.rst for more information.
>  
> +What:           /sys/kernel/livepatch/<patch>/stack_order
> +Date:           Sep 2024
> +KernelVersion:  6.12.0

These will probably need to be updated (can probably be done by Petr when
applying).

> +Contact:        live-patching@vger.kernel.org
> +Description:
> +		This attribute holds the stack order of a livepatch module applied
> +		to the running system.

It's probably a good idea to clarify what "stack order" means.  Also,
try to keep the text under 80 columns for consistency.

How about:

		This attribute indicates the order the patch was applied
		compared to other patches.  For example, a stack_order value of
		'2' indicates the patch was applied after the patch with stack
		order '1' and before any other currently applied patches.

-- 
Josh

Re: [PATCH V3 1/1] livepatch: Add "stack_order" sysfs attribute

[zhang warden]

Hi, Josh!
> On Oct 1, 2024, at 07:26, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>> 
>> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
>> index a5df9b4910dc..2a60b49aa9a5 100644
>> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
>> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
>> @@ -47,6 +47,14 @@ Description:
>> disabled when the feature is used. See
>> Documentation/livepatch/livepatch.rst for more information.
>> 
>> +What:           /sys/kernel/livepatch/<patch>/stack_order
>> +Date:           Sep 2024
>> +KernelVersion:  6.12.0
> 
> These will probably need to be updated (can probably be done by Petr when
> applying).
> 
True, kernel version may need Petr to decide.

>> +Contact:        live-patching@vger.kernel.org
>> +Description:
>> + This attribute holds the stack order of a livepatch module applied
>> + to the running system.
> 
> It's probably a good idea to clarify what "stack order" means.  Also,
> try to keep the text under 80 columns for consistency.
> 
> How about:
> 
> This attribute indicates the order the patch was applied
> compared to other patches.  For example, a stack_order value of
> '2' indicates the patch was applied after the patch with stack
> order '1' and before any other currently applied patches.
> 

Or how about:

This attribute indicates the order of the livepatch module 
applied to the system. The stack_order value N means 
that this module is the Nth applied to the system. If there
are serval patches changing the same function, the function
version of the biggest stack_order is enabling in the system.

Regards.
Wardenjohn.

Re: [PATCH V3 1/1] livepatch: Add "stack_order" sysfs attribute

[Petr Mladek]

On Thu 2024-10-03 14:26:19, zhang warden wrote:
> 
> Hi, Josh!
> > On Oct 1, 2024, at 07:26, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >> 
> >> diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
> >> index a5df9b4910dc..2a60b49aa9a5 100644
> >> --- a/Documentation/ABI/testing/sysfs-kernel-livepatch
> >> +++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
> >> @@ -47,6 +47,14 @@ Description:
> >> disabled when the feature is used. See
> >> Documentation/livepatch/livepatch.rst for more information.
> >> 
> >> +What:           /sys/kernel/livepatch/<patch>/stack_order
> >> +Date:           Sep 2024
> >> +KernelVersion:  6.12.0
> > 
> > These will probably need to be updated (can probably be done by Petr when
> > applying).
> > 
> True, kernel version may need Petr to decide.

It would be for 6.13 if the changes are accepted in time before the
next merge window.

Also please rebase the patch on top of current Linus' master or
v6.11. There are conflicts with the commit adb68ed26a3e922
("livepatch: Add "replace" sysfs attribute").


> >> +Contact:        live-patching@vger.kernel.org
> >> +Description:
> >> + This attribute holds the stack order of a livepatch module applied
> >> + to the running system.
> > 
> > It's probably a good idea to clarify what "stack order" means.  Also,
> > try to keep the text under 80 columns for consistency.
> > 
> > How about:
> > 
> > This attribute indicates the order the patch was applied
> > compared to other patches.  For example, a stack_order value of
> > '2' indicates the patch was applied after the patch with stack
> > order '1' and before any other currently applied patches.
> > 
> 
> Or how about:
> 
> This attribute indicates the order of the livepatch module 
> applied to the system. The stack_order value N means 
> that this module is the Nth applied to the system. If there
> are serval patches changing the same function, the function
> version of the biggest stack_order is enabling in the system.

The 2nd sentence looks superfluous. The 3rd sentence explains
the important effect.

Well, the part "is enabling in the system" is a bit cryptic.
I would write something like:

This attribute specifies the sequence in which live patch modules
are applied to the system. If multiple live patches modify the same
function, the implementation with the highest stack order is used,
unless a transition is currently in progress.

Best Regards,
Petr

Re: [PATCH V3 1/1] livepatch: Add "stack_order" sysfs attribute

[zhang warden]

Hi, Petr.
> Also please rebase the patch on top of current Linus' master or
> v6.11. There are conflicts with the commit adb68ed26a3e922
> ("livepatch: Add "replace" sysfs attribute").
> 
OK, will fix it.

>>>> +Contact:        live-patching@vger.kernel.org
>>>> +Description:
>>>> + This attribute holds the stack order of a livepatch module applied
>>>> + to the running system.
>>> 
>>> It's probably a good idea to clarify what "stack order" means.  Also,
>>> try to keep the text under 80 columns for consistency.
>>> 
>>> How about:
>>> 
>>> This attribute indicates the order the patch was applied
>>> compared to other patches.  For example, a stack_order value of
>>> '2' indicates the patch was applied after the patch with stack
>>> order '1' and before any other currently applied patches.
>>> 
>> 
>> Or how about:
>> 
>> This attribute indicates the order of the livepatch module 
>> applied to the system. The stack_order value N means 
>> that this module is the Nth applied to the system. If there
>> are serval patches changing the same function, the function
>> version of the biggest stack_order is enabling in the system.
> 
> The 2nd sentence looks superfluous. The 3rd sentence explains
> the important effect.
> 
> Well, the part "is enabling in the system" is a bit cryptic.
> I would write something like:
> 
> This attribute specifies the sequence in which live patch modules
> are applied to the system. If multiple live patches modify the same
> function, the implementation with the highest stack order is used,
> unless a transition is currently in progress.

This description looks good to me. What's the suggestion of 
other maintainers ?

Regards.
Wardenjohn.

Re: [PATCH V3 1/1] livepatch: Add "stack_order" sysfs attribute

[Josh Poimboeuf]

On Thu, Oct 03, 2024 at 10:59:11PM +0800, zhang warden wrote:
> > This attribute specifies the sequence in which live patch modules
> > are applied to the system. If multiple live patches modify the same
> > function, the implementation with the highest stack order is used,
> > unless a transition is currently in progress.
> 
> This description looks good to me. What's the suggestion of 
> other maintainers ?

I like it, though "highest stack order" is still a bit arbitrary, since
the highest stack order is actually the lowest number.

-- 
Josh

Re: [PATCH V3 1/1] livepatch: Add "stack_order" sysfs attribute

[zhang warden]

> On Oct 3, 2024, at 23:25, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> 
> On Thu, Oct 03, 2024 at 10:59:11PM +0800, zhang warden wrote:
>>> This attribute specifies the sequence in which live patch modules
>>> are applied to the system. If multiple live patches modify the same
>>> function, the implementation with the highest stack order is used,
>>> unless a transition is currently in progress.
>> 
>> This description looks good to me. What's the suggestion of 
>> other maintainers ?
> 
> I like it, though "highest stack order" is still a bit arbitrary, since
> the highest stack order is actually the lowest number.
> 
> -- 
> Josh

How about:

This attribute specifies the sequence in which live patch module 
are applied to the system. If multiple live patches modify the same
function, the implementation with the biggest 'stack_order' number
is used, unless a transition is currently in progress.

Regards.
Wardenjohn.

Re: [PATCH V3 0/1] livepatch: Add "stack_order" sysfs attribute

[Miroslav Benes]

Hello,

On Sun, 29 Sep 2024, Wardenjohn wrote:

> As previous discussion, maintainers think that patch-level sysfs interface is the
> only acceptable way to maintain the information of the order that klp_patch is 
> applied to the system.
> 
> However, the previous patch introduce klp_ops into klp_func is a optimization 
> methods of the patch introducing 'using' feature to klp_func.
> 
> But now, we don't support 'using' feature to klp_func and make 'klp_ops' patch
> not necessary.
> 
> Therefore, this new version is only introduce the sysfs feature of klp_patch 
> 'stack_order'.

could you also include the selftests as discussed before, please?

Miroslav

Re: [PATCH V3 0/1] livepatch: Add "stack_order" sysfs attribute

[zhang warden]

Hi, Miroslav.

> On Oct 2, 2024, at 19:44, Miroslav Benes <mbenes@suse.cz> wrote:
> 
> Hello,
> 
> could you also include the selftests as discussed before, please?
> 
> Miroslav

Should I include selftests in one patch?

Regards.
Wardenjohn.

Re: [PATCH V3 0/1] livepatch: Add "stack_order" sysfs attribute

[Petr Mladek]

On Thu 2024-10-03 16:06:59, zhang warden wrote:
> Hi,Miroslav.
> 
> >  could you also include the selftests as discussed before, please?
> 
> Should I submit the selftests in one patch?

I would put it into a separate patch from the patch adding the
"stack_order" attribute.

You might split the selftests into more patches if many selftests
are added.

Best Regards,
Petr