[PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Yafang Shao]

Add "replace" sysfs attribute to show whether a livepatch supports
atomic replace or not.

A minor cleanup is also included in this patchset.

v1->v2:
- Refine the subject (Miroslav)
- Use sysfs_emit() instead and replace other snprintf() as well (Miroslav)
- Add selftests (Marcos) 

v1: https://lore.kernel.org/live-patching/20240607070157.33828-1-laoar.shao@gmail.com/

Yafang Shao (3):
  livepatch: Add "replace" sysfs attribute
  selftests/livepatch: Add selftests for "replace" sysfs attribute
  livepatch: Replace snprintf() with sysfs_emit()

 .../ABI/testing/sysfs-kernel-livepatch        |  8 ++++
 kernel/livepatch/core.c                       | 17 +++++--
 .../testing/selftests/livepatch/test-sysfs.sh | 48 +++++++++++++++++++
 3 files changed, 70 insertions(+), 3 deletions(-)

-- 
2.39.1

[PATCH v2 1/3] livepatch: Add "replace" sysfs attribute

[Yafang Shao]

When building a livepatch, a user can set it to be either an atomic replace
livepatch or a non atomic replace livepatch. However, it is not easy to
identify whether a livepatch is atomic replace or not until it actually
replaces some old livepatches. It will be beneficial to show it directly.

A new sysfs interface called 'replace' is introduced in this patch. The
result after this change is as follows:

  $ cat /sys/kernel/livepatch/livepatch-non_replace/replace
  0

  $ cat /sys/kernel/livepatch/livepatch-replace/replace
  1

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 Documentation/ABI/testing/sysfs-kernel-livepatch |  8 ++++++++
 kernel/livepatch/core.c                          | 12 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index a5df9b4910dc..3735d868013d 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>/replace
+Date:		Jun 2024
+KernelVersion:	6.11.0
+Contact:	live-patching@vger.kernel.org
+Description:
+		An attribute which indicates whether the patch supports
+		atomic-replace.
+
 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 52426665eecc..ad28617bfd75 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>/replace
  * /sys/kernel/livepatch/<patch>/<object>
  * /sys/kernel/livepatch/<patch>/<object>/patched
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
@@ -443,13 +444,24 @@ static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
 	return count;
 }
 
+static ssize_t replace_show(struct kobject *kobj,
+			    struct kobj_attribute *attr, char *buf)
+{
+	struct klp_patch *patch;
+
+	patch = container_of(kobj, struct klp_patch, kobj);
+	return sysfs_emit(buf, "%d\n", patch->replace);
+}
+
 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 replace_kobj_attr = __ATTR_RO(replace);
 static struct attribute *klp_patch_attrs[] = {
 	&enabled_kobj_attr.attr,
 	&transition_kobj_attr.attr,
 	&force_kobj_attr.attr,
+	&replace_kobj_attr.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(klp_patch);
-- 
2.39.1

Re: [PATCH v2 1/3] livepatch: Add "replace" sysfs attribute

[Petr Mladek]

On Mon 2024-06-10 09:32:35, Yafang Shao wrote:
> When building a livepatch, a user can set it to be either an atomic replace
> livepatch or a non atomic replace livepatch. However, it is not easy to
> identify whether a livepatch is atomic replace or not until it actually
> replaces some old livepatches. It will be beneficial to show it directly.
>
> A new sysfs interface called 'replace' is introduced in this patch. The
> result after this change is as follows:
> 
>   $ cat /sys/kernel/livepatch/livepatch-non_replace/replace
>   0
> 
>   $ cat /sys/kernel/livepatch/livepatch-replace/replace
>   1

The description is not sufficient. It does not explain why this
information is useful.

The proposed change allows to see the replace flag only when
the livepatch is already installed. But the value does
not have any effect at this point. It has effect only when
the livepatch is being installed.

I would propose something like:

<proposal>
There are situations when it might make sense to combine livepatches
with and without the atomic replace on the same system. For example,
the livepatch without the atomic replace might provide a hotfix
or extra tuning.

Managing livepatches on such systems might be challenging. And the
information which of the installed livepatches do not use the atomic
replace would be useful.

Add new sysfs interface 'replace'. It works as follows:

   $ cat /sys/kernel/livepatch/livepatch-non_replace/replace
   0

   $ cat /sys/kernel/livepatch/livepatch-replace/replace
   1
</proposal>

> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Otherwise the change looks good.

With a better description:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v2 1/3] livepatch: Add "replace" sysfs attribute

[Yafang Shao]

On Mon, Jun 24, 2024 at 9:46 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Mon 2024-06-10 09:32:35, Yafang Shao wrote:
> > When building a livepatch, a user can set it to be either an atomic replace
> > livepatch or a non atomic replace livepatch. However, it is not easy to
> > identify whether a livepatch is atomic replace or not until it actually
> > replaces some old livepatches. It will be beneficial to show it directly.
> >
> > A new sysfs interface called 'replace' is introduced in this patch. The
> > result after this change is as follows:
> >
> >   $ cat /sys/kernel/livepatch/livepatch-non_replace/replace
> >   0
> >
> >   $ cat /sys/kernel/livepatch/livepatch-replace/replace
> >   1
>
> The description is not sufficient. It does not explain why this
> information is useful.
>
> The proposed change allows to see the replace flag only when
> the livepatch is already installed. But the value does
> not have any effect at this point. It has effect only when
> the livepatch is being installed.
>
> I would propose something like:
>
> <proposal>
> There are situations when it might make sense to combine livepatches
> with and without the atomic replace on the same system. For example,
> the livepatch without the atomic replace might provide a hotfix
> or extra tuning.
>
> Managing livepatches on such systems might be challenging. And the
> information which of the installed livepatches do not use the atomic
> replace would be useful.
>
> Add new sysfs interface 'replace'. It works as follows:
>
>    $ cat /sys/kernel/livepatch/livepatch-non_replace/replace
>    0
>
>    $ cat /sys/kernel/livepatch/livepatch-replace/replace
>    1
> </proposal>
>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>
> Otherwise the change looks good.
>
> With a better description:
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
>

Thanks for your review and suggestion. I will do it.

-- 
Regards
Yafang

[PATCH v2 2/3] selftests/livepatch: Add selftests for "replace" sysfs attribute

[Yafang Shao]

Add selftests for both atomic replace and non atomic replace
livepatches. The result is as follows,

  TEST: sysfs test ... ok
  TEST: sysfs test object/patched ... ok
  TEST: sysfs test replace enabled ... ok
  TEST: sysfs test replace disabled ... ok

Suggested-by: Marcos Paulo de Souza <mpdesouza@suse.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 .../testing/selftests/livepatch/test-sysfs.sh | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/tools/testing/selftests/livepatch/test-sysfs.sh b/tools/testing/selftests/livepatch/test-sysfs.sh
index 6c646afa7395..05a14f5a7bfb 100755
--- a/tools/testing/selftests/livepatch/test-sysfs.sh
+++ b/tools/testing/selftests/livepatch/test-sysfs.sh
@@ -18,6 +18,7 @@ check_sysfs_rights "$MOD_LIVEPATCH" "" "drwxr-xr-x"
 check_sysfs_rights "$MOD_LIVEPATCH" "enabled" "-rw-r--r--"
 check_sysfs_value  "$MOD_LIVEPATCH" "enabled" "1"
 check_sysfs_rights "$MOD_LIVEPATCH" "force" "--w-------"
+check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
 check_sysfs_rights "$MOD_LIVEPATCH" "transition" "-r--r--r--"
 check_sysfs_value  "$MOD_LIVEPATCH" "transition" "0"
 check_sysfs_rights "$MOD_LIVEPATCH" "vmlinux/patched" "-r--r--r--"
@@ -83,4 +84,51 @@ test_klp_callbacks_demo: post_unpatch_callback: vmlinux
 livepatch: 'test_klp_callbacks_demo': unpatching complete
 % rmmod test_klp_callbacks_demo"
 
+start_test "sysfs test replace enabled"
+
+MOD_LIVEPATCH=test_klp_atomic_replace
+load_lp $MOD_LIVEPATCH replace=1
+
+check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
+check_sysfs_value  "$MOD_LIVEPATCH" "replace" "1"
+
+disable_lp $MOD_LIVEPATCH
+unload_lp $MOD_LIVEPATCH
+
+check_result "% insmod test_modules/$MOD_LIVEPATCH.ko replace=1
+livepatch: enabling patch '$MOD_LIVEPATCH'
+livepatch: '$MOD_LIVEPATCH': initializing patching transition
+livepatch: '$MOD_LIVEPATCH': starting patching transition
+livepatch: '$MOD_LIVEPATCH': completing patching transition
+livepatch: '$MOD_LIVEPATCH': patching complete
+% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
+livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
+livepatch: '$MOD_LIVEPATCH': starting unpatching transition
+livepatch: '$MOD_LIVEPATCH': completing unpatching transition
+livepatch: '$MOD_LIVEPATCH': unpatching complete
+% rmmod $MOD_LIVEPATCH"
+
+start_test "sysfs test replace disabled"
+
+load_lp $MOD_LIVEPATCH replace=0
+
+check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
+check_sysfs_value  "$MOD_LIVEPATCH" "replace" "0"
+
+disable_lp $MOD_LIVEPATCH
+unload_lp $MOD_LIVEPATCH
+
+check_result "% insmod test_modules/$MOD_LIVEPATCH.ko replace=0
+livepatch: enabling patch '$MOD_LIVEPATCH'
+livepatch: '$MOD_LIVEPATCH': initializing patching transition
+livepatch: '$MOD_LIVEPATCH': starting patching transition
+livepatch: '$MOD_LIVEPATCH': completing patching transition
+livepatch: '$MOD_LIVEPATCH': patching complete
+% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
+livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
+livepatch: '$MOD_LIVEPATCH': starting unpatching transition
+livepatch: '$MOD_LIVEPATCH': completing unpatching transition
+livepatch: '$MOD_LIVEPATCH': unpatching complete
+% rmmod $MOD_LIVEPATCH"
+
 exit 0
-- 
2.39.1

Re: [PATCH v2 2/3] selftests/livepatch: Add selftests for "replace" sysfs attribute

[Marcos Paulo de Souza]

On Mon, 2024-06-10 at 09:32 +0800, Yafang Shao wrote:
> Add selftests for both atomic replace and non atomic replace
> livepatches. The result is as follows,
> 
>   TEST: sysfs test ... ok
>   TEST: sysfs test object/patched ... ok
>   TEST: sysfs test replace enabled ... ok
>   TEST: sysfs test replace disabled ... ok
> 
> Suggested-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

LGTM,

Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>
Tested-by: Marcos Paulo de Souza <mpdesouza@suse.com>

> ---
>  .../testing/selftests/livepatch/test-sysfs.sh | 48
> +++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/tools/testing/selftests/livepatch/test-sysfs.sh
> b/tools/testing/selftests/livepatch/test-sysfs.sh
> index 6c646afa7395..05a14f5a7bfb 100755
> --- a/tools/testing/selftests/livepatch/test-sysfs.sh
> +++ b/tools/testing/selftests/livepatch/test-sysfs.sh
> @@ -18,6 +18,7 @@ check_sysfs_rights "$MOD_LIVEPATCH" "" "drwxr-xr-x"
>  check_sysfs_rights "$MOD_LIVEPATCH" "enabled" "-rw-r--r--"
>  check_sysfs_value  "$MOD_LIVEPATCH" "enabled" "1"
>  check_sysfs_rights "$MOD_LIVEPATCH" "force" "--w-------"
> +check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
>  check_sysfs_rights "$MOD_LIVEPATCH" "transition" "-r--r--r--"
>  check_sysfs_value  "$MOD_LIVEPATCH" "transition" "0"
>  check_sysfs_rights "$MOD_LIVEPATCH" "vmlinux/patched" "-r--r--r--"
> @@ -83,4 +84,51 @@ test_klp_callbacks_demo: post_unpatch_callback:
> vmlinux
>  livepatch: 'test_klp_callbacks_demo': unpatching complete
>  % rmmod test_klp_callbacks_demo"
>  
> +start_test "sysfs test replace enabled"
> +
> +MOD_LIVEPATCH=test_klp_atomic_replace
> +load_lp $MOD_LIVEPATCH replace=1
> +
> +check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
> +check_sysfs_value  "$MOD_LIVEPATCH" "replace" "1"
> +
> +disable_lp $MOD_LIVEPATCH
> +unload_lp $MOD_LIVEPATCH
> +
> +check_result "% insmod test_modules/$MOD_LIVEPATCH.ko replace=1
> +livepatch: enabling patch '$MOD_LIVEPATCH'
> +livepatch: '$MOD_LIVEPATCH': initializing patching transition
> +livepatch: '$MOD_LIVEPATCH': starting patching transition
> +livepatch: '$MOD_LIVEPATCH': completing patching transition
> +livepatch: '$MOD_LIVEPATCH': patching complete
> +% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
> +livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
> +livepatch: '$MOD_LIVEPATCH': starting unpatching transition
> +livepatch: '$MOD_LIVEPATCH': completing unpatching transition
> +livepatch: '$MOD_LIVEPATCH': unpatching complete
> +% rmmod $MOD_LIVEPATCH"
> +
> +start_test "sysfs test replace disabled"
> +
> +load_lp $MOD_LIVEPATCH replace=0
> +
> +check_sysfs_rights "$MOD_LIVEPATCH" "replace" "-r--r--r--"
> +check_sysfs_value  "$MOD_LIVEPATCH" "replace" "0"
> +
> +disable_lp $MOD_LIVEPATCH
> +unload_lp $MOD_LIVEPATCH
> +
> +check_result "% insmod test_modules/$MOD_LIVEPATCH.ko replace=0
> +livepatch: enabling patch '$MOD_LIVEPATCH'
> +livepatch: '$MOD_LIVEPATCH': initializing patching transition
> +livepatch: '$MOD_LIVEPATCH': starting patching transition
> +livepatch: '$MOD_LIVEPATCH': completing patching transition
> +livepatch: '$MOD_LIVEPATCH': patching complete
> +% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
> +livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
> +livepatch: '$MOD_LIVEPATCH': starting unpatching transition
> +livepatch: '$MOD_LIVEPATCH': completing unpatching transition
> +livepatch: '$MOD_LIVEPATCH': unpatching complete
> +% rmmod $MOD_LIVEPATCH"
> +
>  exit 0

Re: [PATCH v2 2/3] selftests/livepatch: Add selftests for "replace" sysfs attribute

[Petr Mladek]

On Mon 2024-06-10 09:32:36, Yafang Shao wrote:
> Add selftests for both atomic replace and non atomic replace
> livepatches. The result is as follows,
> 
>   TEST: sysfs test ... ok
>   TEST: sysfs test object/patched ... ok
>   TEST: sysfs test replace enabled ... ok
>   TEST: sysfs test replace disabled ... ok
> 
> Suggested-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH v2 3/3] livepatch: Replace snprintf() with sysfs_emit()

[Yafang Shao]

Let's use sysfs_emit() instead of snprintf().

Suggested-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/livepatch/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ad28617bfd75..3c21c31796db 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -402,7 +402,7 @@ static ssize_t enabled_show(struct kobject *kobj,
 	struct klp_patch *patch;
 
 	patch = container_of(kobj, struct klp_patch, kobj);
-	return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled);
+	return sysfs_emit(buf, "%d\n", patch->enabled);
 }
 
 static ssize_t transition_show(struct kobject *kobj,
@@ -411,8 +411,7 @@ static ssize_t transition_show(struct kobject *kobj,
 	struct klp_patch *patch;
 
 	patch = container_of(kobj, struct klp_patch, kobj);
-	return snprintf(buf, PAGE_SIZE-1, "%d\n",
-			patch == klp_transition_patch);
+	return sysfs_emit(buf, "%d\n", patch == klp_transition_patch);
 }
 
 static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
-- 
2.39.1

Re: [PATCH v2 3/3] livepatch: Replace snprintf() with sysfs_emit()

[Petr Mladek]

On Mon 2024-06-10 09:32:37, Yafang Shao wrote:
> Let's use sysfs_emit() instead of snprintf().
> 
> Suggested-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Song Liu]

Hi Yafang,

On Sun, Jun 9, 2024 at 6:33 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Add "replace" sysfs attribute to show whether a livepatch supports
> atomic replace or not.

I am curious about the use case here. AFAICT, the "replace" flag
matters _before_ the livepatch is loaded. Once the livepatch is
loaded, the "replace" part is already finished. Therefore, I think
we really need a way to check the replace flag before loading the
.ko file? Maybe in modinfo?

Thanks,
Song

>
> A minor cleanup is also included in this patchset.
>
> v1->v2:
> - Refine the subject (Miroslav)
> - Use sysfs_emit() instead and replace other snprintf() as well (Miroslav)
> - Add selftests (Marcos)
>
> v1: https://lore.kernel.org/live-patching/20240607070157.33828-1-laoar.shao@gmail.com/
>
> Yafang Shao (3):
>   livepatch: Add "replace" sysfs attribute
>   selftests/livepatch: Add selftests for "replace" sysfs attribute
>   livepatch: Replace snprintf() with sysfs_emit()
>
>  .../ABI/testing/sysfs-kernel-livepatch        |  8 ++++
>  kernel/livepatch/core.c                       | 17 +++++--
>  .../testing/selftests/livepatch/test-sysfs.sh | 48 +++++++++++++++++++
>  3 files changed, 70 insertions(+), 3 deletions(-)
>
> --
> 2.39.1
>
>

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Yafang Shao]

On Tue, Jun 11, 2024 at 1:19 AM Song Liu <song@kernel.org> wrote:
>
> Hi Yafang,
>
> On Sun, Jun 9, 2024 at 6:33 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > Add "replace" sysfs attribute to show whether a livepatch supports
> > atomic replace or not.
>
> I am curious about the use case here.

We will use this flag to check if there're both atomic replace
livepatch and non atomic replace livepatch running on a single server
at the same time. That can happen if we install a new non atomic
replace livepatch to a server which has already applied an atomic
replace livepatch.

> AFAICT, the "replace" flag
> matters _before_ the livepatch is loaded. Once the livepatch is
> loaded, the "replace" part is already finished. Therefore, I think
> we really need a way to check the replace flag before loading the
> .ko file? Maybe in modinfo?

It will be better if we can check it before loading it. However it
depends on how we build the livepatch ko, right? Take the
kpatch-build[0] for example, we have to modify the kpatch-build to add
support for it but we can't ensure all users will use it to build the
livepatch.

[0]. https://github.com/dynup/kpatch

-- 
Regards
Yafang

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Song Liu]

On Mon, Jun 10, 2024 at 7:47 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Tue, Jun 11, 2024 at 1:19 AM Song Liu <song@kernel.org> wrote:
> >
> > Hi Yafang,
> >
> > On Sun, Jun 9, 2024 at 6:33 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > Add "replace" sysfs attribute to show whether a livepatch supports
> > > atomic replace or not.
> >
> > I am curious about the use case here.
>
> We will use this flag to check if there're both atomic replace
> livepatch and non atomic replace livepatch running on a single server
> at the same time. That can happen if we install a new non atomic
> replace livepatch to a server which has already applied an atomic
> replace livepatch.
>
> > AFAICT, the "replace" flag
> > matters _before_ the livepatch is loaded. Once the livepatch is
> > loaded, the "replace" part is already finished. Therefore, I think
> > we really need a way to check the replace flag before loading the
> > .ko file? Maybe in modinfo?
>
> It will be better if we can check it before loading it. However it
> depends on how we build the livepatch ko, right? Take the
> kpatch-build[0] for example, we have to modify the kpatch-build to add
> support for it but we can't ensure all users will use it to build the
> livepatch.

Interesting. I guess we have very different use cases. In our systems,
we don't allow random users to load livepatch.

Thanks for the explanation.
Song

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Petr Mladek]

On Tue 2024-06-11 10:46:47, Yafang Shao wrote:
> On Tue, Jun 11, 2024 at 1:19 AM Song Liu <song@kernel.org> wrote:
> >
> > Hi Yafang,
> >
> > On Sun, Jun 9, 2024 at 6:33 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > Add "replace" sysfs attribute to show whether a livepatch supports
> > > atomic replace or not.
> >
> > I am curious about the use case here.
> 
> We will use this flag to check if there're both atomic replace
> livepatch and non atomic replace livepatch running on a single server
> at the same time. That can happen if we install a new non atomic
> replace livepatch to a server which has already applied an atomic
> replace livepatch.
> 
> > AFAICT, the "replace" flag
> > matters _before_ the livepatch is loaded. Once the livepatch is
> > loaded, the "replace" part is already finished. Therefore, I think
> > we really need a way to check the replace flag before loading the
> > .ko file? Maybe in modinfo?
> 
> It will be better if we can check it before loading it. However it
> depends on how we build the livepatch ko, right? Take the
> kpatch-build[0] for example, we have to modify the kpatch-build to add
> support for it but we can't ensure all users will use it to build the
> livepatch.

> [0]. https://github.com/dynup/kpatch

I still do not understand how the sysfs attribute would help here.
It will show the type of the currently used livepatches. But
it won't say what the to-be-installed livepatch would do.

Could you please describe how exactly you would use the information?
What would be the process/algorithm/logic which would prevent a mistake?

Honestly, it sounds like your processes are broken and you try
to fix them on the wrong end.

Allowing to load random livepatches which are built a random way
sounds like a hazard.

It should be possible to load only livepatches which passed some
testing (QA). And the testing (QA) should check if the livepatch
successfully replaced the previous version.

Or do you want to use the sysfs attribute in QA?
So that only livepatches witch "replace" flag set would pass QA?

Best Regards,
Petr

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Yafang Shao]

On Fri, Jun 21, 2024 at 3:31 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2024-06-11 10:46:47, Yafang Shao wrote:
> > On Tue, Jun 11, 2024 at 1:19 AM Song Liu <song@kernel.org> wrote:
> > >
> > > Hi Yafang,
> > >
> > > On Sun, Jun 9, 2024 at 6:33 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > >
> > > > Add "replace" sysfs attribute to show whether a livepatch supports
> > > > atomic replace or not.
> > >
> > > I am curious about the use case here.
> >
> > We will use this flag to check if there're both atomic replace
> > livepatch and non atomic replace livepatch running on a single server
> > at the same time. That can happen if we install a new non atomic
> > replace livepatch to a server which has already applied an atomic
> > replace livepatch.
> >
> > > AFAICT, the "replace" flag
> > > matters _before_ the livepatch is loaded. Once the livepatch is
> > > loaded, the "replace" part is already finished. Therefore, I think
> > > we really need a way to check the replace flag before loading the
> > > .ko file? Maybe in modinfo?
> >
> > It will be better if we can check it before loading it. However it
> > depends on how we build the livepatch ko, right? Take the
> > kpatch-build[0] for example, we have to modify the kpatch-build to add
> > support for it but we can't ensure all users will use it to build the
> > livepatch.
>
> > [0]. https://github.com/dynup/kpatch
>
> I still do not understand how the sysfs attribute would help here.
> It will show the type of the currently used livepatches. But
> it won't say what the to-be-installed livepatch would do.
>
> Could you please describe how exactly you would use the information?
> What would be the process/algorithm/logic which would prevent a mistake?
>
> Honestly, it sounds like your processes are broken and you try
> to fix them on the wrong end.
>
> Allowing to load random livepatches which are built a random way
> sounds like a hazard.

They are not random live patches. Some system administrators maintain
their own livepatches tailored for specific workloads or develop
livepatches for A/B testing. Our company’s kernel team maintains the
general livepatches. This worked well in the past when all livepatches
were independent. However, the situation changed when we switched from
non atomic replace livepatches to atomic replace livepatches, causing
issues due to the uncertain behavior of mixed atomic replace and non
atomic replace livepatches.

To address this change, we need a robust solution. One approach we
have identified is developing a CI build system for livepatches. All
livepatches must be built through this CI system, where they will
undergo CI tests to verify if they are atomic replace or not.

Additionally, in our production environment, we need to ensure that
there are no non atomic replace livepatches in use. For instance, some
system administrators might still build livepatches outside of our CI
system. Detecting whether a single livepatch is atomic replace or not
is not easy. To simplify this, we propose adding a new sysfs attribute
to facilitate this check.

BTW, perhaps we could introduce a new sysctl setting to forcefully
forbid all non atomic replace livepatches?

>
> It should be possible to load only livepatches which passed some
> testing (QA). And the testing (QA) should check if the livepatch
> successfully replaced the previous version.
>
> Or do you want to use the sysfs attribute in QA?
> So that only livepatches witch "replace" flag set would pass QA?
>


-- 
Regards
Yafang

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Petr Mladek]

On Fri 2024-06-21 16:39:40, Yafang Shao wrote:
> On Fri, Jun 21, 2024 at 3:31 PM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Tue 2024-06-11 10:46:47, Yafang Shao wrote:
> > > On Tue, Jun 11, 2024 at 1:19 AM Song Liu <song@kernel.org> wrote:
> > > >
> > > > Hi Yafang,
> > > >
> > > > On Sun, Jun 9, 2024 at 6:33 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > > >
> > > > > Add "replace" sysfs attribute to show whether a livepatch supports
> > > > > atomic replace or not.
> > > >
> > > > I am curious about the use case here.
> > >
> > > We will use this flag to check if there're both atomic replace
> > > livepatch and non atomic replace livepatch running on a single server
> > > at the same time. That can happen if we install a new non atomic
> > > replace livepatch to a server which has already applied an atomic
> > > replace livepatch.
> > >
> > > > AFAICT, the "replace" flag
> > > > matters _before_ the livepatch is loaded. Once the livepatch is
> > > > loaded, the "replace" part is already finished. Therefore, I think
> > > > we really need a way to check the replace flag before loading the
> > > > .ko file? Maybe in modinfo?
> > >
> > > It will be better if we can check it before loading it. However it
> > > depends on how we build the livepatch ko, right? Take the
> > > kpatch-build[0] for example, we have to modify the kpatch-build to add
> > > support for it but we can't ensure all users will use it to build the
> > > livepatch.
> >
> > > [0]. https://github.com/dynup/kpatch
> >
> > I still do not understand how the sysfs attribute would help here.
> > It will show the type of the currently used livepatches. But
> > it won't say what the to-be-installed livepatch would do.
> >
> > Could you please describe how exactly you would use the information?
> > What would be the process/algorithm/logic which would prevent a mistake?
> >
> > Honestly, it sounds like your processes are broken and you try
> > to fix them on the wrong end.
> >
> > Allowing to load random livepatches which are built a random way
> > sounds like a hazard.
> 
> They are not random live patches. Some system administrators maintain
> their own livepatches tailored for specific workloads or develop
> livepatches for A/B testing. Our company’s kernel team maintains the
> general livepatches. This worked well in the past when all livepatches
> were independent. However, the situation changed when we switched from
> non atomic replace livepatches to atomic replace livepatches, causing
> issues due to the uncertain behavior of mixed atomic replace and non
> atomic replace livepatches.

I think that the uncertain behavior has been even before you started
using the atomic replace.

How did the system administrators detect whether the livepatches were
independent?

It looks to me that it worked only by luck. Well, I could imagine
that it has worked for a long time.


> To address this change, we need a robust solution. One approach we
> have identified is developing a CI build system for livepatches. All
> livepatches must be built through this CI system, where they will
> undergo CI tests to verify if they are atomic replace or not.

The important part is that the livepatch authors have to see
all already existing changes. They need to check that
the new change would not break other livepatched code and
vice versa.


> Additionally, in our production environment, we need to ensure that
> there are no non atomic replace livepatches in use. For instance, some
> system administrators might still build livepatches outside of our CI
> system. Detecting whether a single livepatch is atomic replace or not
> is not easy. To simplify this, we propose adding a new sysfs attribute
> to facilitate this check.
> 
> BTW, perhaps we could introduce a new sysctl setting to forcefully
> forbid all non atomic replace livepatches?

I like it. This looks like the most reliable solution. Would it
solve your problem.

Alternative solution would be to forbid installing non-replace
livepatches when there is already installed a livepatch with
the atomic replace. I could imagine that we enforce this for
everyone (without sysctl knob). Would this work for you?

That said, I am not completely against adding the sysfs attribute
"replace". I could imagine that it might be useful when a more
flexible solution is needed. But I think that it would be
hard to make a more flexible solution safe, especially
in your environment.

Best Regards,
Petr

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Yafang Shao]

On Fri, Jun 21, 2024 at 6:04 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Fri 2024-06-21 16:39:40, Yafang Shao wrote:
> > On Fri, Jun 21, 2024 at 3:31 PM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > On Tue 2024-06-11 10:46:47, Yafang Shao wrote:
> > > > On Tue, Jun 11, 2024 at 1:19 AM Song Liu <song@kernel.org> wrote:
> > > > >
> > > > > Hi Yafang,
> > > > >
> > > > > On Sun, Jun 9, 2024 at 6:33 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > > > >
> > > > > > Add "replace" sysfs attribute to show whether a livepatch supports
> > > > > > atomic replace or not.
> > > > >
> > > > > I am curious about the use case here.
> > > >
> > > > We will use this flag to check if there're both atomic replace
> > > > livepatch and non atomic replace livepatch running on a single server
> > > > at the same time. That can happen if we install a new non atomic
> > > > replace livepatch to a server which has already applied an atomic
> > > > replace livepatch.
> > > >
> > > > > AFAICT, the "replace" flag
> > > > > matters _before_ the livepatch is loaded. Once the livepatch is
> > > > > loaded, the "replace" part is already finished. Therefore, I think
> > > > > we really need a way to check the replace flag before loading the
> > > > > .ko file? Maybe in modinfo?
> > > >
> > > > It will be better if we can check it before loading it. However it
> > > > depends on how we build the livepatch ko, right? Take the
> > > > kpatch-build[0] for example, we have to modify the kpatch-build to add
> > > > support for it but we can't ensure all users will use it to build the
> > > > livepatch.
> > >
> > > > [0]. https://github.com/dynup/kpatch
> > >
> > > I still do not understand how the sysfs attribute would help here.
> > > It will show the type of the currently used livepatches. But
> > > it won't say what the to-be-installed livepatch would do.
> > >
> > > Could you please describe how exactly you would use the information?
> > > What would be the process/algorithm/logic which would prevent a mistake?
> > >
> > > Honestly, it sounds like your processes are broken and you try
> > > to fix them on the wrong end.
> > >
> > > Allowing to load random livepatches which are built a random way
> > > sounds like a hazard.
> >
> > They are not random live patches. Some system administrators maintain
> > their own livepatches tailored for specific workloads or develop
> > livepatches for A/B testing. Our company’s kernel team maintains the
> > general livepatches. This worked well in the past when all livepatches
> > were independent. However, the situation changed when we switched from
> > non atomic replace livepatches to atomic replace livepatches, causing
> > issues due to the uncertain behavior of mixed atomic replace and non
> > atomic replace livepatches.
>
> I think that the uncertain behavior has been even before you started
> using the atomic replace.
>
> How did the system administrators detect whether the livepatches were
> independent?
>
> It looks to me that it worked only by luck. Well, I could imagine
> that it has worked for a long time.

We have a limited number of livepatches in our system, primarily
intended to address critical issues that could potentially cause
system instability. Therefore, the likelihood of conflicts between two
livepatches is relatively low. While I acknowledge that there is
indeed a potential risk involved, it is the atomic replace livepatch
that poses this risk.

>
>
> > To address this change, we need a robust solution. One approach we
> > have identified is developing a CI build system for livepatches. All
> > livepatches must be built through this CI system, where they will
> > undergo CI tests to verify if they are atomic replace or not.
>
> The important part is that the livepatch authors have to see
> all already existing changes. They need to check that
> the new change would not break other livepatched code and
> vice versa.

Exactly. Through this CI system, all developers have visibility into
all the livepatches currently running on our system.

>
>
> > Additionally, in our production environment, we need to ensure that
> > there are no non atomic replace livepatches in use. For instance, some
> > system administrators might still build livepatches outside of our CI
> > system. Detecting whether a single livepatch is atomic replace or not
> > is not easy. To simplify this, we propose adding a new sysfs attribute
> > to facilitate this check.
> >
> > BTW, perhaps we could introduce a new sysctl setting to forcefully
> > forbid all non atomic replace livepatches?
>
> I like it. This looks like the most reliable solution. Would it
> solve your problem.
>
> Alternative solution would be to forbid installing non-replace
> livepatches when there is already installed a livepatch with
> the atomic replace. I could imagine that we enforce this for
> everyone (without sysctl knob). Would this work for you?

Perhaps we can add this sysctl knob as follows?

kernel.livepatch.forbid_non_atomic_replace:
    0 - Allow non atomic replace livepatch. (Default behavior)
    1 - Completely forbid non atomic replace livepatch.
    2 - Forbid non atomic replace livepatch only if there is already
an atomic replace livepatch running.

>
> That said, I am not completely against adding the sysfs attribute
> "replace". I could imagine that it might be useful when a more
> flexible solution is needed. But I think that it would be
> hard to make a more flexible solution safe, especially
> in your environment.

This sysfs attribute remains valuable as it aids in monitoring
livepatches, specifically allowing us to track which type of livepatch
is currently operating on a server.

--
Regards
Yafang

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Petr Mladek]

On Sun 2024-06-23 10:52:40, Yafang Shao wrote:
> On Fri, Jun 21, 2024 at 6:04 PM Petr Mladek <pmladek@suse.com> wrote:
> > On Fri 2024-06-21 16:39:40, Yafang Shao wrote:
> > > On Fri, Jun 21, 2024 at 3:31 PM Petr Mladek <pmladek@suse.com> wrote:
> > > > On Tue 2024-06-11 10:46:47, Yafang Shao wrote:
> > > > > On Tue, Jun 11, 2024 at 1:19 AM Song Liu <song@kernel.org> wrote:
> > > > > > On Sun, Jun 9, 2024 at 6:33 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > They are not random live patches. Some system administrators maintain
> > > their own livepatches tailored for specific workloads or develop
> > > livepatches for A/B testing. Our company’s kernel team maintains the
> > > general livepatches. This worked well in the past when all livepatches
> > > were independent. However, the situation changed when we switched from
> > > non atomic replace livepatches to atomic replace livepatches, causing
> > > issues due to the uncertain behavior of mixed atomic replace and non
> > > atomic replace livepatches.
> >
> > I think that the uncertain behavior has been even before you started
> > using the atomic replace.
> >
> > How did the system administrators detect whether the livepatches were
> > independent?
> >
> > It looks to me that it worked only by luck. Well, I could imagine
> > that it has worked for a long time.
> 
> We have a limited number of livepatches in our system, primarily
> intended to address critical issues that could potentially cause
> system instability. Therefore, the likelihood of conflicts between two
> livepatches is relatively low. While I acknowledge that there is
> indeed a potential risk involved, it is the atomic replace livepatch
> that poses this risk.

I see.

> >
> >
> > > To address this change, we need a robust solution. One approach we
> > > have identified is developing a CI build system for livepatches. All
> > > livepatches must be built through this CI system, where they will
> > > undergo CI tests to verify if they are atomic replace or not.
> >
> > The important part is that the livepatch authors have to see
> > all already existing changes. They need to check that
> > the new change would not break other livepatched code and
> > vice versa.
> 
> Exactly. Through this CI system, all developers have visibility into
> all the livepatches currently running on our system.

Sounds good.

> > > Additionally, in our production environment, we need to ensure that
> > > there are no non atomic replace livepatches in use. For instance, some
> > > system administrators might still build livepatches outside of our CI
> > > system. Detecting whether a single livepatch is atomic replace or not
> > > is not easy. To simplify this, we propose adding a new sysfs attribute
> > > to facilitate this check.
> > >
> > > BTW, perhaps we could introduce a new sysctl setting to forcefully
> > > forbid all non atomic replace livepatches?
> >
> > I like it. This looks like the most reliable solution. Would it
> > solve your problem.
> >
> > Alternative solution would be to forbid installing non-replace
> > livepatches when there is already installed a livepatch with
> > the atomic replace. I could imagine that we enforce this for
> > everyone (without sysctl knob). Would this work for you?
> 
> Perhaps we can add this sysctl knob as follows?
> 
> kernel.livepatch.forbid_non_atomic_replace:
>     0 - Allow non atomic replace livepatch. (Default behavior)
>     1 - Completely forbid non atomic replace livepatch.
>     2 - Forbid non atomic replace livepatch only if there is already
> an atomic replace livepatch running.

Feel free to send a patch if it would be useful for you.

> > That said, I am not completely against adding the sysfs attribute
> > "replace". I could imagine that it might be useful when a more
> > flexible solution is needed. But I think that it would be
> > hard to make a more flexible solution safe, especially
> > in your environment.
> 
> This sysfs attribute remains valuable as it aids in monitoring
> livepatches, specifically allowing us to track which type of livepatch
> is currently operating on a server.

Fair enough. I am fine with it. I would only like to improve the
commit message explaining the motivation, see my reply to the patch.

Best Regards,
Petr

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Yafang Shao]

On Mon, Jun 24, 2024 at 10:04 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Sun 2024-06-23 10:52:40, Yafang Shao wrote:
> > On Fri, Jun 21, 2024 at 6:04 PM Petr Mladek <pmladek@suse.com> wrote:
> > > On Fri 2024-06-21 16:39:40, Yafang Shao wrote:
> > > > On Fri, Jun 21, 2024 at 3:31 PM Petr Mladek <pmladek@suse.com> wrote:
> > > > > On Tue 2024-06-11 10:46:47, Yafang Shao wrote:
> > > > > > On Tue, Jun 11, 2024 at 1:19 AM Song Liu <song@kernel.org> wrote:
> > > > > > > On Sun, Jun 9, 2024 at 6:33 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > > They are not random live patches. Some system administrators maintain
> > > > their own livepatches tailored for specific workloads or develop
> > > > livepatches for A/B testing. Our company’s kernel team maintains the
> > > > general livepatches. This worked well in the past when all livepatches
> > > > were independent. However, the situation changed when we switched from
> > > > non atomic replace livepatches to atomic replace livepatches, causing
> > > > issues due to the uncertain behavior of mixed atomic replace and non
> > > > atomic replace livepatches.
> > >
> > > I think that the uncertain behavior has been even before you started
> > > using the atomic replace.
> > >
> > > How did the system administrators detect whether the livepatches were
> > > independent?
> > >
> > > It looks to me that it worked only by luck. Well, I could imagine
> > > that it has worked for a long time.
> >
> > We have a limited number of livepatches in our system, primarily
> > intended to address critical issues that could potentially cause
> > system instability. Therefore, the likelihood of conflicts between two
> > livepatches is relatively low. While I acknowledge that there is
> > indeed a potential risk involved, it is the atomic replace livepatch
> > that poses this risk.
>
> I see.
>
> > >
> > >
> > > > To address this change, we need a robust solution. One approach we
> > > > have identified is developing a CI build system for livepatches. All
> > > > livepatches must be built through this CI system, where they will
> > > > undergo CI tests to verify if they are atomic replace or not.
> > >
> > > The important part is that the livepatch authors have to see
> > > all already existing changes. They need to check that
> > > the new change would not break other livepatched code and
> > > vice versa.
> >
> > Exactly. Through this CI system, all developers have visibility into
> > all the livepatches currently running on our system.
>
> Sounds good.
>
> > > > Additionally, in our production environment, we need to ensure that
> > > > there are no non atomic replace livepatches in use. For instance, some
> > > > system administrators might still build livepatches outside of our CI
> > > > system. Detecting whether a single livepatch is atomic replace or not
> > > > is not easy. To simplify this, we propose adding a new sysfs attribute
> > > > to facilitate this check.
> > > >
> > > > BTW, perhaps we could introduce a new sysctl setting to forcefully
> > > > forbid all non atomic replace livepatches?
> > >
> > > I like it. This looks like the most reliable solution. Would it
> > > solve your problem.
> > >
> > > Alternative solution would be to forbid installing non-replace
> > > livepatches when there is already installed a livepatch with
> > > the atomic replace. I could imagine that we enforce this for
> > > everyone (without sysctl knob). Would this work for you?
> >
> > Perhaps we can add this sysctl knob as follows?
> >
> > kernel.livepatch.forbid_non_atomic_replace:
> >     0 - Allow non atomic replace livepatch. (Default behavior)
> >     1 - Completely forbid non atomic replace livepatch.
> >     2 - Forbid non atomic replace livepatch only if there is already
> > an atomic replace livepatch running.
>
> Feel free to send a patch if it would be useful for you.

I will do it in a separate patch.

-- 
Regards
Yafang

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Miroslav Benes]

Hi,

> > > Additionally, in our production environment, we need to ensure that
> > > there are no non atomic replace livepatches in use. For instance, some
> > > system administrators might still build livepatches outside of our CI
> > > system. Detecting whether a single livepatch is atomic replace or not
> > > is not easy. To simplify this, we propose adding a new sysfs attribute
> > > to facilitate this check.
> > >
> > > BTW, perhaps we could introduce a new sysctl setting to forcefully
> > > forbid all non atomic replace livepatches?
> >
> > I like it. This looks like the most reliable solution. Would it
> > solve your problem.
> >
> > Alternative solution would be to forbid installing non-replace
> > livepatches when there is already installed a livepatch with
> > the atomic replace. I could imagine that we enforce this for
> > everyone (without sysctl knob). Would this work for you?
> 
> Perhaps we can add this sysctl knob as follows?
> 
> kernel.livepatch.forbid_non_atomic_replace:
>     0 - Allow non atomic replace livepatch. (Default behavior)
>     1 - Completely forbid non atomic replace livepatch.
>     2 - Forbid non atomic replace livepatch only if there is already
> an atomic replace livepatch running.

I would be more comfortable if such policies were implemented in the 
userspace. It would allow for more flexibility when it comes to different 
use cases. The kernel may provide necessary information (sysfs attributes, 
modinfo flag) for that of course.

Miroslav

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Yafang Shao]

On Thu, Jun 27, 2024 at 9:02 PM Miroslav Benes <mbenes@suse.cz> wrote:
>
> Hi,
>
> > > > Additionally, in our production environment, we need to ensure that
> > > > there are no non atomic replace livepatches in use. For instance, some
> > > > system administrators might still build livepatches outside of our CI
> > > > system. Detecting whether a single livepatch is atomic replace or not
> > > > is not easy. To simplify this, we propose adding a new sysfs attribute
> > > > to facilitate this check.
> > > >
> > > > BTW, perhaps we could introduce a new sysctl setting to forcefully
> > > > forbid all non atomic replace livepatches?
> > >
> > > I like it. This looks like the most reliable solution. Would it
> > > solve your problem.
> > >
> > > Alternative solution would be to forbid installing non-replace
> > > livepatches when there is already installed a livepatch with
> > > the atomic replace. I could imagine that we enforce this for
> > > everyone (without sysctl knob). Would this work for you?
> >
> > Perhaps we can add this sysctl knob as follows?
> >
> > kernel.livepatch.forbid_non_atomic_replace:
> >     0 - Allow non atomic replace livepatch. (Default behavior)
> >     1 - Completely forbid non atomic replace livepatch.
> >     2 - Forbid non atomic replace livepatch only if there is already
> > an atomic replace livepatch running.
>
> I would be more comfortable if such policies were implemented in the
> userspace. It would allow for more flexibility when it comes to different
> use cases. The kernel may provide necessary information (sysfs attributes,
> modinfo flag) for that of course.

The sysfs attributes serve as a valuable tool for monitoring and
identifying system occurrences, but they are unable to preempt
unexpected behaviors such as the occurrence of mixed atomic replace
livepatch and non atomic replace livepatch.

If the flexibility of the sysctl knob is limited, then perhaps we
could explore utilizing BPF LSM or fmod_ret as an alternative
implementation? We would simply define the necessary interfaces within
the kernel, and users would have the capability to define their own
policies through bpf programs.

-- 
Regards
Yafang

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Miroslav Benes]

[-- Attachment #1: Type: text/plain, Size: 2895 bytes --]

On Thu, 27 Jun 2024, Yafang Shao wrote:

> On Thu, Jun 27, 2024 at 9:02 PM Miroslav Benes <mbenes@suse.cz> wrote:
> >
> > Hi,
> >
> > > > > Additionally, in our production environment, we need to ensure that
> > > > > there are no non atomic replace livepatches in use. For instance, some
> > > > > system administrators might still build livepatches outside of our CI
> > > > > system. Detecting whether a single livepatch is atomic replace or not
> > > > > is not easy. To simplify this, we propose adding a new sysfs attribute
> > > > > to facilitate this check.
> > > > >
> > > > > BTW, perhaps we could introduce a new sysctl setting to forcefully
> > > > > forbid all non atomic replace livepatches?
> > > >
> > > > I like it. This looks like the most reliable solution. Would it
> > > > solve your problem.
> > > >
> > > > Alternative solution would be to forbid installing non-replace
> > > > livepatches when there is already installed a livepatch with
> > > > the atomic replace. I could imagine that we enforce this for
> > > > everyone (without sysctl knob). Would this work for you?
> > >
> > > Perhaps we can add this sysctl knob as follows?
> > >
> > > kernel.livepatch.forbid_non_atomic_replace:
> > >     0 - Allow non atomic replace livepatch. (Default behavior)
> > >     1 - Completely forbid non atomic replace livepatch.
> > >     2 - Forbid non atomic replace livepatch only if there is already
> > > an atomic replace livepatch running.
> >
> > I would be more comfortable if such policies were implemented in the
> > userspace. It would allow for more flexibility when it comes to different
> > use cases. The kernel may provide necessary information (sysfs attributes,
> > modinfo flag) for that of course.
> 
> The sysfs attributes serve as a valuable tool for monitoring and
> identifying system occurrences, but they are unable to preempt
> unexpected behaviors such as the occurrence of mixed atomic replace
> livepatch and non atomic replace livepatch.

True, but when you have sysfs attributes and eventually modinfo, you can 
easily write a wrapper around modprobe which would implement any policy 
that you need. For example, you would see which live patches are 
installed, which live patches you want to install and decide based on that 
what to do. You can bail out, install only a subset...

You can also integrate the wrapper to your "DevOps" system or so.

> If the flexibility of the sysctl knob is limited, then perhaps we
> could explore utilizing BPF LSM or fmod_ret as an alternative
> implementation? We would simply define the necessary interfaces within
> the kernel, and users would have the capability to define their own
> policies through bpf programs.

It sounds like a big hammer to me. If something like this happens, it 
might be better to implement it on the module loader level.

Anyway, others may feel differently about it.

Miroslav

Re: [PATCH v2 0/3] livepatch: Add "replace" sysfs attribute

[Yafang Shao]

On Mon, Jun 10, 2024 at 9:33 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Add "replace" sysfs attribute to show whether a livepatch supports
> atomic replace or not.
>
> A minor cleanup is also included in this patchset.
>
> v1->v2:
> - Refine the subject (Miroslav)
> - Use sysfs_emit() instead and replace other snprintf() as well (Miroslav)
> - Add selftests (Marcos)
>
> v1: https://lore.kernel.org/live-patching/20240607070157.33828-1-laoar.shao@gmail.com/
>
> Yafang Shao (3):
>   livepatch: Add "replace" sysfs attribute
>   selftests/livepatch: Add selftests for "replace" sysfs attribute
>   livepatch: Replace snprintf() with sysfs_emit()
>
>  .../ABI/testing/sysfs-kernel-livepatch        |  8 ++++
>  kernel/livepatch/core.c                       | 17 +++++--
>  .../testing/selftests/livepatch/test-sysfs.sh | 48 +++++++++++++++++++
>  3 files changed, 70 insertions(+), 3 deletions(-)
>
> --
> 2.39.1
>

Hi Maintainers,

Just following up.

Would you be able to accept this series, or do you have any additional
comments or feedback?

Thank you.

--
Regards
Yafang