[PATCH v2 0/2] livepatch, module: Delete the associated module of disabled livepatch

[PATCH v2 0/2] livepatch, module: Delete the associated module of disabled livepatch

[Yafang Shao]

In our production environment, upon loading a new atomic replace livepatch,
we encountered an issue where the kernel module of the old livepatch
remained, despite being replaced by the new one. The detailed steps to
reproduce that issue can be found in patch #2.

Detecting which livepatch will be replaced by the new one from userspace is
not reliable, necessitating the need for the operation to be performed
within the kernel itself.

This patchset aims to address this issue by automatically deleting the
associated module of a disabled livepatch. Since a disabled livepatch can't
be enabled again and the kernel module becomes redundant, it is safe to
remove it in this manner.

Changes:
- v1->v2:
  - Avoid using kpatch utility in the example (Joe, Petr)
  - Fix race around changing mod->state (Joe, Petr)
  - Don't set mod->state outside of kernel/module dir (Joe, Petr)
  - Alter selftests accordingly (Joe)
  - Split it into two patches (Petr, Miroslav)
  - Don't delete module from the path klp_enable_patch() (Petr, Miroslav)
  - Make delete_module() safe (Petr)  

Yafang Shao (2):
  module: Add a new helper delete_module()
  livepatch: Delete the associated module of disabled livepatch

 include/linux/module.h                        |  1 +
 kernel/livepatch/core.c                       | 16 ++--
 kernel/module/main.c                          | 82 +++++++++++++++----
 .../testing/selftests/livepatch/functions.sh  |  2 +
 .../selftests/livepatch/test-callbacks.sh     | 24 ++----
 .../selftests/livepatch/test-ftrace.sh        |  3 +-
 .../selftests/livepatch/test-livepatch.sh     | 11 +--
 .../testing/selftests/livepatch/test-state.sh | 15 +---
 .../selftests/livepatch/test-syscall.sh       |  3 +-
 .../testing/selftests/livepatch/test-sysfs.sh |  6 +-
 10 files changed, 95 insertions(+), 68 deletions(-)

-- 
2.39.1

[PATCH v2 1/2] module: Add a new helper delete_module()

[Yafang Shao]

Introduce a new helper function, delete_module(), designed to delete kernel
modules from locations outside of the `kernel/module` directory.

No functional change.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/module.h |  1 +
 kernel/module/main.c   | 82 ++++++++++++++++++++++++++++++++----------
 2 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 1153b0d99a80..c24557f1b795 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -75,6 +75,7 @@ extern struct module_attribute module_uevent;
 /* These are either module local, or the kernel's dummy ones. */
 extern int init_module(void);
 extern void cleanup_module(void);
+extern int delete_module(struct module *mod);
 
 #ifndef MODULE
 /**
diff --git a/kernel/module/main.c b/kernel/module/main.c
index e1e8a7a9d6c1..3b48ee66db41 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -695,12 +695,74 @@ EXPORT_SYMBOL(module_refcount);
 /* This exists whether we can unload or not */
 static void free_module(struct module *mod);
 
+static void __delete_module(struct module *mod)
+{
+	char buf[MODULE_FLAGS_BUF_SIZE];
+
+	WARN_ON_ONCE(mod->state != MODULE_STATE_GOING);
+
+	/* Final destruction now no one is using it. */
+	if (mod->exit != NULL)
+		mod->exit();
+	blocking_notifier_call_chain(&module_notify_list,
+				     MODULE_STATE_GOING, mod);
+	klp_module_going(mod);
+	ftrace_release_mod(mod);
+
+	async_synchronize_full();
+
+	/* Store the name and taints of the last unloaded module for diagnostic purposes */
+	strscpy(last_unloaded_module.name, mod->name, sizeof(last_unloaded_module.name));
+	strscpy(last_unloaded_module.taints, module_flags(mod, buf, false),
+		sizeof(last_unloaded_module.taints));
+
+	free_module(mod);
+	/* someone could wait for the module in add_unformed_module() */
+	wake_up_all(&module_wq);
+}
+
+int delete_module(struct module *mod)
+{
+	int ret;
+
+	mutex_lock(&module_mutex);
+	if (!list_empty(&mod->source_list)) {
+		/* Other modules depend on us: get rid of them first. */
+		ret = -EWOULDBLOCK;
+		goto out;
+	}
+
+	/* Doing init or already dying? */
+	if (mod->state != MODULE_STATE_LIVE) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	/* If it has an init func, it must have an exit func to unload */
+	if (mod->init && !mod->exit) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	if (try_release_module_ref(mod) != 0) {
+		ret = -EWOULDBLOCK;
+		goto out;
+	}
+	mod->state = MODULE_STATE_GOING;
+	mutex_unlock(&module_mutex);
+	__delete_module(mod);
+	return 0;
+
+out:
+	mutex_unlock(&module_mutex);
+	return ret;
+}
+
 SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 		unsigned int, flags)
 {
 	struct module *mod;
 	char name[MODULE_NAME_LEN];
-	char buf[MODULE_FLAGS_BUF_SIZE];
 	int ret, forced = 0;
 
 	if (!capable(CAP_SYS_MODULE) || modules_disabled)
@@ -750,23 +812,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 		goto out;
 
 	mutex_unlock(&module_mutex);
-	/* Final destruction now no one is using it. */
-	if (mod->exit != NULL)
-		mod->exit();
-	blocking_notifier_call_chain(&module_notify_list,
-				     MODULE_STATE_GOING, mod);
-	klp_module_going(mod);
-	ftrace_release_mod(mod);
-
-	async_synchronize_full();
-
-	/* Store the name and taints of the last unloaded module for diagnostic purposes */
-	strscpy(last_unloaded_module.name, mod->name, sizeof(last_unloaded_module.name));
-	strscpy(last_unloaded_module.taints, module_flags(mod, buf, false), sizeof(last_unloaded_module.taints));
-
-	free_module(mod);
-	/* someone could wait for the module in add_unformed_module() */
-	wake_up_all(&module_wq);
+	__delete_module(mod);
 	return 0;
 out:
 	mutex_unlock(&module_mutex);
-- 
2.39.1

Re: [PATCH v2 1/2] module: Add a new helper delete_module()

[Yafang Shao]

On Sun, Apr 7, 2024 at 11:58 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Introduce a new helper function, delete_module(), designed to delete kernel
> modules from locations outside of the `kernel/module` directory.
>
> No functional change.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/module.h |  1 +
>  kernel/module/main.c   | 82 ++++++++++++++++++++++++++++++++----------
>  2 files changed, 65 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 1153b0d99a80..c24557f1b795 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -75,6 +75,7 @@ extern struct module_attribute module_uevent;
>  /* These are either module local, or the kernel's dummy ones. */
>  extern int init_module(void);
>  extern void cleanup_module(void);
> +extern int delete_module(struct module *mod);
>
>  #ifndef MODULE
>  /**
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index e1e8a7a9d6c1..3b48ee66db41 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -695,12 +695,74 @@ EXPORT_SYMBOL(module_refcount);
>  /* This exists whether we can unload or not */
>  static void free_module(struct module *mod);
>
> +static void __delete_module(struct module *mod)
> +{
> +       char buf[MODULE_FLAGS_BUF_SIZE];
> +
> +       WARN_ON_ONCE(mod->state != MODULE_STATE_GOING);
> +
> +       /* Final destruction now no one is using it. */
> +       if (mod->exit != NULL)
> +               mod->exit();
> +       blocking_notifier_call_chain(&module_notify_list,
> +                                    MODULE_STATE_GOING, mod);
> +       klp_module_going(mod);
> +       ftrace_release_mod(mod);
> +
> +       async_synchronize_full();
> +
> +       /* Store the name and taints of the last unloaded module for diagnostic purposes */
> +       strscpy(last_unloaded_module.name, mod->name, sizeof(last_unloaded_module.name));
> +       strscpy(last_unloaded_module.taints, module_flags(mod, buf, false),
> +               sizeof(last_unloaded_module.taints));
> +
> +       free_module(mod);
> +       /* someone could wait for the module in add_unformed_module() */
> +       wake_up_all(&module_wq);
> +}
> +
> +int delete_module(struct module *mod)
> +{
> +       int ret;
> +
> +       mutex_lock(&module_mutex);
> +       if (!list_empty(&mod->source_list)) {
> +               /* Other modules depend on us: get rid of them first. */
> +               ret = -EWOULDBLOCK;
> +               goto out;
> +       }
> +
> +       /* Doing init or already dying? */
> +       if (mod->state != MODULE_STATE_LIVE) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +       /* If it has an init func, it must have an exit func to unload */
> +       if (mod->init && !mod->exit) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +       if (try_release_module_ref(mod) != 0) {
> +               ret = -EWOULDBLOCK;
> +               goto out;
> +       }
> +       mod->state = MODULE_STATE_GOING;
> +       mutex_unlock(&module_mutex);
> +       __delete_module(mod);
> +       return 0;
> +
> +out:
> +       mutex_unlock(&module_mutex);
> +       return ret;
> +}
> +
>  SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>                 unsigned int, flags)
>  {
>         struct module *mod;
>         char name[MODULE_NAME_LEN];
> -       char buf[MODULE_FLAGS_BUF_SIZE];
>         int ret, forced = 0;
>
>         if (!capable(CAP_SYS_MODULE) || modules_disabled)
> @@ -750,23 +812,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>                 goto out;
>
>         mutex_unlock(&module_mutex);
> -       /* Final destruction now no one is using it. */
> -       if (mod->exit != NULL)
> -               mod->exit();
> -       blocking_notifier_call_chain(&module_notify_list,
> -                                    MODULE_STATE_GOING, mod);
> -       klp_module_going(mod);
> -       ftrace_release_mod(mod);
> -
> -       async_synchronize_full();
> -
> -       /* Store the name and taints of the last unloaded module for diagnostic purposes */
> -       strscpy(last_unloaded_module.name, mod->name, sizeof(last_unloaded_module.name));
> -       strscpy(last_unloaded_module.taints, module_flags(mod, buf, false), sizeof(last_unloaded_module.taints));
> -
> -       free_module(mod);
> -       /* someone could wait for the module in add_unformed_module() */
> -       wake_up_all(&module_wq);
> +       __delete_module(mod);
>         return 0;
>  out:
>         mutex_unlock(&module_mutex);
> --
> 2.39.1
>

Luis, Greg,

Since the last version, there hasn't been any response. Would you mind
taking a moment to review it and provide your feedback on the
kernel/module changes?

-- 
Regards
Yafang

Re: [PATCH v2 1/2] module: Add a new helper delete_module()

[Greg KH]

On Wed, Apr 24, 2024 at 08:09:05PM +0800, Yafang Shao wrote:
> On Sun, Apr 7, 2024 at 11:58 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > Introduce a new helper function, delete_module(), designed to delete kernel
> > modules from locations outside of the `kernel/module` directory.
> >
> > No functional change.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/linux/module.h |  1 +
> >  kernel/module/main.c   | 82 ++++++++++++++++++++++++++++++++----------
> >  2 files changed, 65 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 1153b0d99a80..c24557f1b795 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -75,6 +75,7 @@ extern struct module_attribute module_uevent;
> >  /* These are either module local, or the kernel's dummy ones. */
> >  extern int init_module(void);
> >  extern void cleanup_module(void);
> > +extern int delete_module(struct module *mod);
> >
> >  #ifndef MODULE
> >  /**
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index e1e8a7a9d6c1..3b48ee66db41 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -695,12 +695,74 @@ EXPORT_SYMBOL(module_refcount);
> >  /* This exists whether we can unload or not */
> >  static void free_module(struct module *mod);
> >
> > +static void __delete_module(struct module *mod)
> > +{
> > +       char buf[MODULE_FLAGS_BUF_SIZE];
> > +
> > +       WARN_ON_ONCE(mod->state != MODULE_STATE_GOING);
> > +
> > +       /* Final destruction now no one is using it. */
> > +       if (mod->exit != NULL)
> > +               mod->exit();
> > +       blocking_notifier_call_chain(&module_notify_list,
> > +                                    MODULE_STATE_GOING, mod);
> > +       klp_module_going(mod);
> > +       ftrace_release_mod(mod);
> > +
> > +       async_synchronize_full();
> > +
> > +       /* Store the name and taints of the last unloaded module for diagnostic purposes */
> > +       strscpy(last_unloaded_module.name, mod->name, sizeof(last_unloaded_module.name));
> > +       strscpy(last_unloaded_module.taints, module_flags(mod, buf, false),
> > +               sizeof(last_unloaded_module.taints));
> > +
> > +       free_module(mod);
> > +       /* someone could wait for the module in add_unformed_module() */
> > +       wake_up_all(&module_wq);
> > +}
> > +
> > +int delete_module(struct module *mod)
> > +{
> > +       int ret;
> > +
> > +       mutex_lock(&module_mutex);
> > +       if (!list_empty(&mod->source_list)) {
> > +               /* Other modules depend on us: get rid of them first. */
> > +               ret = -EWOULDBLOCK;
> > +               goto out;
> > +       }
> > +
> > +       /* Doing init or already dying? */
> > +       if (mod->state != MODULE_STATE_LIVE) {
> > +               ret = -EBUSY;
> > +               goto out;
> > +       }
> > +
> > +       /* If it has an init func, it must have an exit func to unload */
> > +       if (mod->init && !mod->exit) {
> > +               ret = -EBUSY;
> > +               goto out;
> > +       }
> > +
> > +       if (try_release_module_ref(mod) != 0) {
> > +               ret = -EWOULDBLOCK;
> > +               goto out;
> > +       }
> > +       mod->state = MODULE_STATE_GOING;
> > +       mutex_unlock(&module_mutex);
> > +       __delete_module(mod);
> > +       return 0;
> > +
> > +out:
> > +       mutex_unlock(&module_mutex);
> > +       return ret;
> > +}
> > +
> >  SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> >                 unsigned int, flags)
> >  {
> >         struct module *mod;
> >         char name[MODULE_NAME_LEN];
> > -       char buf[MODULE_FLAGS_BUF_SIZE];
> >         int ret, forced = 0;
> >
> >         if (!capable(CAP_SYS_MODULE) || modules_disabled)
> > @@ -750,23 +812,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> >                 goto out;
> >
> >         mutex_unlock(&module_mutex);
> > -       /* Final destruction now no one is using it. */
> > -       if (mod->exit != NULL)
> > -               mod->exit();
> > -       blocking_notifier_call_chain(&module_notify_list,
> > -                                    MODULE_STATE_GOING, mod);
> > -       klp_module_going(mod);
> > -       ftrace_release_mod(mod);
> > -
> > -       async_synchronize_full();
> > -
> > -       /* Store the name and taints of the last unloaded module for diagnostic purposes */
> > -       strscpy(last_unloaded_module.name, mod->name, sizeof(last_unloaded_module.name));
> > -       strscpy(last_unloaded_module.taints, module_flags(mod, buf, false), sizeof(last_unloaded_module.taints));
> > -
> > -       free_module(mod);
> > -       /* someone could wait for the module in add_unformed_module() */
> > -       wake_up_all(&module_wq);
> > +       __delete_module(mod);
> >         return 0;
> >  out:
> >         mutex_unlock(&module_mutex);
> > --
> > 2.39.1
> >
> 
> Luis, Greg,
> 
> Since the last version, there hasn't been any response. Would you mind
> taking a moment to review it and provide your feedback on the
> kernel/module changes?

There was response on patch 2/2, which is why I deleted this from my
review queue a long time ago.

Please address that if you wish to, and then resend if you feel this is
still needed.

Personally, I really don't like this function you added...

thanks,

greg k-h

Re: [PATCH v2 1/2] module: Add a new helper delete_module()

[Josh Poimboeuf]

On Sat, May 04, 2024 at 06:53:05PM +0200, Greg KH wrote:
> > Luis, Greg,
> > 
> > Since the last version, there hasn't been any response. Would you mind
> > taking a moment to review it and provide your feedback on the
> > kernel/module changes?
> 
> There was response on patch 2/2, which is why I deleted this from my
> review queue a long time ago.

Assuming you're referring to my comment (which is the only one I've
seen), that was only yesterday ;-)

> Please address that if you wish to, and then resend if you feel this is
> still needed.
> 
> Personally, I really don't like this function you added...

I tend to agree...

-- 
Josh

Re: [PATCH v2 1/2] module: Add a new helper delete_module()

[Luis Chamberlain]

On Wed, Apr 24, 2024 at 08:09:05PM +0800, Yafang Shao wrote:
> Luis, Greg,
> 
> Since the last version, there hasn't been any response. Would you mind
> taking a moment to review it and provide your feedback on the
> kernel/module changes?

Josh had feedback for you. Without any Acked-by from livepatch folks this
isn't capturing the full picture.

  Luis

Re: [PATCH v2 1/2] module: Add a new helper delete_module()

[Petr Mladek]

On Sun 2024-04-07 11:57:29, Yafang Shao wrote:
> Introduce a new helper function, delete_module(), designed to delete kernel
> modules from locations outside of the `kernel/module` directory.
> 
> No functional change.
> 
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -695,12 +695,74 @@ EXPORT_SYMBOL(module_refcount);
>  /* This exists whether we can unload or not */
>  static void free_module(struct module *mod);
>  
> +static void __delete_module(struct module *mod)
> +{
> +	char buf[MODULE_FLAGS_BUF_SIZE];
> +
> +	WARN_ON_ONCE(mod->state != MODULE_STATE_GOING);
> +
> +	/* Final destruction now no one is using it. */
> +	if (mod->exit != NULL)
> +		mod->exit();
> +	blocking_notifier_call_chain(&module_notify_list,
> +				     MODULE_STATE_GOING, mod);
> +	klp_module_going(mod);
> +	ftrace_release_mod(mod);
> +
> +	async_synchronize_full();
> +
> +	/* Store the name and taints of the last unloaded module for diagnostic purposes */
> +	strscpy(last_unloaded_module.name, mod->name, sizeof(last_unloaded_module.name));
> +	strscpy(last_unloaded_module.taints, module_flags(mod, buf, false),
> +		sizeof(last_unloaded_module.taints));
> +
> +	free_module(mod);
> +	/* someone could wait for the module in add_unformed_module() */
> +	wake_up_all(&module_wq);
> +}
> +
> +int delete_module(struct module *mod)
> +{
> +	int ret;
> +
> +	mutex_lock(&module_mutex);
> +	if (!list_empty(&mod->source_list)) {
> +		/* Other modules depend on us: get rid of them first. */
> +		ret = -EWOULDBLOCK;
> +		goto out;
> +	}

This is cut&paste from SYSCALL_DEFINE2(delete_module...

> +
> +	/* Doing init or already dying? */
> +	if (mod->state != MODULE_STATE_LIVE) {
> +		ret = -EBUSY;
> +		goto out;
> +	}

Same here. You only removed the debug message. Why?

> +
> +	/* If it has an init func, it must have an exit func to unload */
> +	if (mod->init && !mod->exit) {
> +		ret = -EBUSY;
> +		goto out;
> +	}

Same code, just without the "forced" handling.

> +
> +	if (try_release_module_ref(mod) != 0) {
> +		ret = -EWOULDBLOCK;
> +		goto out;
> +	}

This is the same as try_stop_module() without the "forced" handling.

> +	mod->state = MODULE_STATE_GOING;
> +	mutex_unlock(&module_mutex);
> +	__delete_module(mod);
> +	return 0;

I am sure that we could better refactor the code to remove
the code duplication.

> +
> +out:
> +	mutex_unlock(&module_mutex);
> +	return ret;
> +}
> +
>  SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  		unsigned int, flags)
>  {
>  	struct module *mod;
>  	char name[MODULE_NAME_LEN];
> -	char buf[MODULE_FLAGS_BUF_SIZE];
>  	int ret, forced = 0;
>  
>  	if (!capable(CAP_SYS_MODULE) || modules_disabled)

Otherwise, it looks good to me.

Best Regards,
Petr

[PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Yafang Shao]

In our production environment, upon loading a new atomic replace livepatch,
we encountered an issue where the kernel module of the old livepatch
remained, despite being replaced by the new one. This issue can be
reproduced using the following steps:

- Pre setup: Build two atomic replace livepatches

- First step: Load the first livepatch using the insmod command
  $ insmod livepatch-test_0.ko

  $ ls /sys/kernel/livepatch/
  livepatch_test_0

  $ cat /sys/kernel/livepatch/livepatch_test_0/enabled
  1

  $ lsmod  | grep livepatch
  livepatch_test_0       16384  1

- Second step: Load the new livepatch using the insmod command
  $ insmod livepatch-test_1.ko

  $ ls /sys/kernel/livepatch/
  livepatch_test_1                  <<<< livepatch_test_0 was replaced

  $ cat /sys/kernel/livepatch/livepatch_test_1/enabled
  1

  $ lsmod  | grep livepatch
  livepatch_test_1       16384  1
  livepatch_test_0       16384  0    <<<< leftover

Detecting which livepatch will be replaced by the new one from userspace is
not reliable, necessitating the need for the operation to be performed
within the kernel itself. With this improvement, executing
`insmod livepatch-test_1.ko` will automatically remove the
livepatch-test_0.ko module.

Following this change, the associated kernel module will be removed when
executing `echo 0 > /sys/kernel/livepatch/${livepath}/enabled`. Therefore,
adjustments need to be made to the selftests accordingly.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/livepatch/core.c                       | 16 +++++++++----
 .../testing/selftests/livepatch/functions.sh  |  2 ++
 .../selftests/livepatch/test-callbacks.sh     | 24 +++++--------------
 .../selftests/livepatch/test-ftrace.sh        |  3 +--
 .../selftests/livepatch/test-livepatch.sh     | 11 +++------
 .../testing/selftests/livepatch/test-state.sh | 15 ++++--------
 .../selftests/livepatch/test-syscall.sh       |  3 +--
 .../testing/selftests/livepatch/test-sysfs.sh |  6 ++---
 8 files changed, 30 insertions(+), 50 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ecbc9b6aba3a..6850158bcef4 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -709,8 +709,10 @@ static void klp_free_patch_start(struct klp_patch *patch)
  * the last function accessing the livepatch structures when the patch
  * gets disabled.
  */
-static void klp_free_patch_finish(struct klp_patch *patch)
+static void klp_free_patch_finish(struct klp_patch *patch, bool delete)
 {
+	struct module *mod = patch->mod;
+
 	/*
 	 * Avoid deadlock with enabled_store() sysfs callback by
 	 * calling this outside klp_mutex. It is safe because
@@ -721,8 +723,12 @@ static void klp_free_patch_finish(struct klp_patch *patch)
 	wait_for_completion(&patch->finish);
 
 	/* Put the module after the last access to struct klp_patch. */
-	if (!patch->forced)
-		module_put(patch->mod);
+	if (!patch->forced) {
+		module_put(mod);
+		if (!delete)
+			return;
+		delete_module(mod);
+	}
 }
 
 /*
@@ -735,7 +741,7 @@ static void klp_free_patch_work_fn(struct work_struct *work)
 	struct klp_patch *patch =
 		container_of(work, struct klp_patch, free_work);
 
-	klp_free_patch_finish(patch);
+	klp_free_patch_finish(patch, true);
 }
 
 void klp_free_patch_async(struct klp_patch *patch)
@@ -1124,7 +1130,7 @@ int klp_enable_patch(struct klp_patch *patch)
 
 	mutex_unlock(&klp_mutex);
 
-	klp_free_patch_finish(patch);
+	klp_free_patch_finish(patch, false);
 
 	return ret;
 }
diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index fc4c6a016d38..21c48f9b020e 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -220,6 +220,8 @@ function load_failing_mod() {
 function unload_mod() {
 	local mod="$1"
 
+	[[ ! -d "/sys/module/$mod" ]] && return
+
 	# Wait for module reference count to clear ...
 	loop_until '[[ $(cat "/sys/module/$mod/refcnt") == "0" ]]' ||
 		die "failed to unload module $mod (refcnt)"
diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh
index 32b150e25b10..833cc66915f7 100755
--- a/tools/testing/selftests/livepatch/test-callbacks.sh
+++ b/tools/testing/selftests/livepatch/test-callbacks.sh
@@ -55,7 +55,6 @@ livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
 $MOD_LIVEPATCH: post_unpatch_callback: $MOD_TARGET -> [MODULE_STATE_LIVE] Normal state
 livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH
 % rmmod $MOD_TARGET
 $MOD_TARGET: ${MOD_TARGET}_exit"
 
@@ -103,7 +102,6 @@ livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
 $MOD_LIVEPATCH: post_unpatch_callback: $MOD_TARGET -> [MODULE_STATE_LIVE] Normal state
 livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH
 % rmmod $MOD_TARGET
 $MOD_TARGET: ${MOD_TARGET}_exit"
 
@@ -152,8 +150,7 @@ $MOD_LIVEPATCH: pre_unpatch_callback: vmlinux
 livepatch: '$MOD_LIVEPATCH': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
-livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 
 # This test is similar to the previous test, however the livepatch is
@@ -201,8 +198,7 @@ $MOD_LIVEPATCH: pre_unpatch_callback: vmlinux
 livepatch: '$MOD_LIVEPATCH': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
-livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 
 # A simple test of loading a livepatch without one of its patch target
@@ -233,8 +229,7 @@ $MOD_LIVEPATCH: pre_unpatch_callback: vmlinux
 livepatch: '$MOD_LIVEPATCH': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
-livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 
 # Test a scenario where a vmlinux pre-patch callback returns a non-zero
@@ -316,8 +311,7 @@ $MOD_LIVEPATCH: pre_unpatch_callback: vmlinux
 livepatch: '$MOD_LIVEPATCH': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
-livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 
 # Test loading multiple targeted kernel modules.  This test-case is
@@ -373,7 +367,6 @@ livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
 $MOD_LIVEPATCH: post_unpatch_callback: $MOD_TARGET_BUSY -> [MODULE_STATE_LIVE] Normal state
 livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH
 % rmmod $MOD_TARGET_BUSY
 $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_exit"
 
@@ -445,7 +438,6 @@ livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
 $MOD_LIVEPATCH: post_unpatch_callback: $MOD_TARGET_BUSY -> [MODULE_STATE_LIVE] Normal state
 livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH
 % rmmod $MOD_TARGET_BUSY
 $MOD_TARGET_BUSY: busymod_work_func exit
 $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_exit"
@@ -496,9 +488,7 @@ $MOD_LIVEPATCH: pre_unpatch_callback: vmlinux
 livepatch: '$MOD_LIVEPATCH': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
-livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH2
-% rmmod $MOD_LIVEPATCH"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 
 # Load multiple livepatches, but the second as an 'atomic-replace'
@@ -545,9 +535,7 @@ $MOD_LIVEPATCH2: pre_unpatch_callback: vmlinux
 livepatch: '$MOD_LIVEPATCH2': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH2': completing unpatching transition
 $MOD_LIVEPATCH2: post_unpatch_callback: vmlinux
-livepatch: '$MOD_LIVEPATCH2': unpatching complete
-% rmmod $MOD_LIVEPATCH2
-% rmmod $MOD_LIVEPATCH"
+livepatch: '$MOD_LIVEPATCH2': unpatching complete"
 
 
 exit 0
diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh
index 730218bce99c..81539fe3c082 100755
--- a/tools/testing/selftests/livepatch/test-ftrace.sh
+++ b/tools/testing/selftests/livepatch/test-ftrace.sh
@@ -57,8 +57,7 @@ livepatch: sysctl: setting key \"kernel.ftrace_enabled\": Device or resource bus
 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"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 
 exit 0
diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
index e3455a6b1158..8209f4f239fb 100755
--- a/tools/testing/selftests/livepatch/test-livepatch.sh
+++ b/tools/testing/selftests/livepatch/test-livepatch.sh
@@ -41,8 +41,7 @@ livepatch: '$MOD_LIVEPATCH': patching complete
 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"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 
 # - load a livepatch that modifies the output from /proc/cmdline and
@@ -95,14 +94,12 @@ livepatch: '$MOD_REPLACE': initializing unpatching transition
 livepatch: '$MOD_REPLACE': starting unpatching transition
 livepatch: '$MOD_REPLACE': completing unpatching transition
 livepatch: '$MOD_REPLACE': unpatching complete
-% rmmod $MOD_REPLACE
 $MOD_LIVEPATCH: this has been live patched
 % 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"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 
 # - load a livepatch that modifies the output from /proc/cmdline and
@@ -149,14 +146,12 @@ livepatch: '$MOD_REPLACE': starting patching transition
 livepatch: '$MOD_REPLACE': completing patching transition
 livepatch: '$MOD_REPLACE': patching complete
 $MOD_REPLACE: this has been live patched
-% rmmod $MOD_LIVEPATCH
 $MOD_REPLACE: this has been live patched
 % echo 0 > /sys/kernel/livepatch/$MOD_REPLACE/enabled
 livepatch: '$MOD_REPLACE': initializing unpatching transition
 livepatch: '$MOD_REPLACE': starting unpatching transition
 livepatch: '$MOD_REPLACE': completing unpatching transition
-livepatch: '$MOD_REPLACE': unpatching complete
-% rmmod $MOD_REPLACE"
+livepatch: '$MOD_REPLACE': unpatching complete"
 
 
 exit 0
diff --git a/tools/testing/selftests/livepatch/test-state.sh b/tools/testing/selftests/livepatch/test-state.sh
index 10a52ac06185..2cc0ef3752c2 100755
--- a/tools/testing/selftests/livepatch/test-state.sh
+++ b/tools/testing/selftests/livepatch/test-state.sh
@@ -37,8 +37,7 @@ livepatch: '$MOD_LIVEPATCH': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH': completing unpatching transition
 $MOD_LIVEPATCH: post_unpatch_callback: vmlinux
 $MOD_LIVEPATCH: free_loglevel_state: freeing space for the stored console_loglevel
-livepatch: '$MOD_LIVEPATCH': unpatching complete
-% rmmod $MOD_LIVEPATCH"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 
 # Take over system state change by a cumulative patch
@@ -71,7 +70,6 @@ livepatch: '$MOD_LIVEPATCH2': completing patching transition
 $MOD_LIVEPATCH2: post_patch_callback: vmlinux
 $MOD_LIVEPATCH2: fix_console_loglevel: taking over the console_loglevel change
 livepatch: '$MOD_LIVEPATCH2': patching complete
-% rmmod $MOD_LIVEPATCH
 % echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH2/enabled
 livepatch: '$MOD_LIVEPATCH2': initializing unpatching transition
 $MOD_LIVEPATCH2: pre_unpatch_callback: vmlinux
@@ -80,8 +78,7 @@ livepatch: '$MOD_LIVEPATCH2': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH2': completing unpatching transition
 $MOD_LIVEPATCH2: post_unpatch_callback: vmlinux
 $MOD_LIVEPATCH2: free_loglevel_state: freeing space for the stored console_loglevel
-livepatch: '$MOD_LIVEPATCH2': unpatching complete
-% rmmod $MOD_LIVEPATCH2"
+livepatch: '$MOD_LIVEPATCH2': unpatching complete"
 
 
 # Take over system state change by a cumulative patch
@@ -116,7 +113,6 @@ livepatch: '$MOD_LIVEPATCH3': completing patching transition
 $MOD_LIVEPATCH3: post_patch_callback: vmlinux
 $MOD_LIVEPATCH3: fix_console_loglevel: taking over the console_loglevel change
 livepatch: '$MOD_LIVEPATCH3': patching complete
-% rmmod $MOD_LIVEPATCH2
 % insmod test_modules/$MOD_LIVEPATCH2.ko
 livepatch: enabling patch '$MOD_LIVEPATCH2'
 livepatch: '$MOD_LIVEPATCH2': initializing patching transition
@@ -135,9 +131,7 @@ livepatch: '$MOD_LIVEPATCH2': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH2': completing unpatching transition
 $MOD_LIVEPATCH2: post_unpatch_callback: vmlinux
 $MOD_LIVEPATCH2: free_loglevel_state: freeing space for the stored console_loglevel
-livepatch: '$MOD_LIVEPATCH2': unpatching complete
-% rmmod $MOD_LIVEPATCH2
-% rmmod $MOD_LIVEPATCH3"
+livepatch: '$MOD_LIVEPATCH2': unpatching complete"
 
 
 # Failure caused by incompatible cumulative livepatches
@@ -170,7 +164,6 @@ livepatch: '$MOD_LIVEPATCH2': starting unpatching transition
 livepatch: '$MOD_LIVEPATCH2': completing unpatching transition
 $MOD_LIVEPATCH2: post_unpatch_callback: vmlinux
 $MOD_LIVEPATCH2: free_loglevel_state: freeing space for the stored console_loglevel
-livepatch: '$MOD_LIVEPATCH2': unpatching complete
-% rmmod $MOD_LIVEPATCH2"
+livepatch: '$MOD_LIVEPATCH2': unpatching complete"
 
 exit 0
diff --git a/tools/testing/selftests/livepatch/test-syscall.sh b/tools/testing/selftests/livepatch/test-syscall.sh
index b76a881d4013..a4e26cb7f524 100755
--- a/tools/testing/selftests/livepatch/test-syscall.sh
+++ b/tools/testing/selftests/livepatch/test-syscall.sh
@@ -47,7 +47,6 @@ $MOD_SYSCALL: Remaining not livepatched processes: 0
 livepatch: '$MOD_SYSCALL': initializing unpatching transition
 livepatch: '$MOD_SYSCALL': starting unpatching transition
 livepatch: '$MOD_SYSCALL': completing unpatching transition
-livepatch: '$MOD_SYSCALL': unpatching complete
-% rmmod $MOD_SYSCALL"
+livepatch: '$MOD_SYSCALL': unpatching complete"
 
 exit 0
diff --git a/tools/testing/selftests/livepatch/test-sysfs.sh b/tools/testing/selftests/livepatch/test-sysfs.sh
index 6c646afa7395..9adfcead84f6 100755
--- a/tools/testing/selftests/livepatch/test-sysfs.sh
+++ b/tools/testing/selftests/livepatch/test-sysfs.sh
@@ -37,8 +37,7 @@ livepatch: '$MOD_LIVEPATCH': patching complete
 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"
+livepatch: '$MOD_LIVEPATCH': unpatching complete"
 
 start_test "sysfs test object/patched"
 
@@ -80,7 +79,6 @@ test_klp_callbacks_demo: pre_unpatch_callback: vmlinux
 livepatch: 'test_klp_callbacks_demo': starting unpatching transition
 livepatch: 'test_klp_callbacks_demo': completing unpatching transition
 test_klp_callbacks_demo: post_unpatch_callback: vmlinux
-livepatch: 'test_klp_callbacks_demo': unpatching complete
-% rmmod test_klp_callbacks_demo"
+livepatch: 'test_klp_callbacks_demo': unpatching complete"
 
 exit 0
-- 
2.39.1

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Josh Poimboeuf]

On Sun, Apr 07, 2024 at 11:57:30AM +0800, Yafang Shao wrote:
>   $ ls /sys/kernel/livepatch/
>   livepatch_test_1                  <<<< livepatch_test_0 was replaced
> 
>   $ cat /sys/kernel/livepatch/livepatch_test_1/enabled
>   1
> 
>   $ lsmod  | grep livepatch
>   livepatch_test_1       16384  1
>   livepatch_test_0       16384  0    <<<< leftover
> 
> Detecting which livepatch will be replaced by the new one from userspace is
> not reliable, necessitating the need for the operation to be performed
> within the kernel itself. With this improvement, executing
> `insmod livepatch-test_1.ko` will automatically remove the
> livepatch-test_0.ko module.
> 
> Following this change, the associated kernel module will be removed when
> executing `echo 0 > /sys/kernel/livepatch/${livepath}/enabled`. Therefore,
> adjustments need to be made to the selftests accordingly.

If the problem is that the user can't see which livepatch has been
disabled, we should just fix that problem directly by leaving the
disabled module in /sys/kernel/livepatch with an 'enabled' value of 0.

'enabled' could then be made read-only for replaced files.

That seems less disruptive to the user (and more consistent with the
previous interface), and continues to leave the policy up to the user to
decide if/when they want to remove the module.

It would also allow easily downgrading the replaced module in the future
(once we have proper support for that).

-- 
Josh

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Petr Mladek]

On Fri 2024-05-03 14:14:34, Josh Poimboeuf wrote:
> On Sun, Apr 07, 2024 at 11:57:30AM +0800, Yafang Shao wrote:
> >   $ ls /sys/kernel/livepatch/
> >   livepatch_test_1                  <<<< livepatch_test_0 was replaced
> > 
> >   $ cat /sys/kernel/livepatch/livepatch_test_1/enabled
> >   1
> > 
> >   $ lsmod  | grep livepatch
> >   livepatch_test_1       16384  1
> >   livepatch_test_0       16384  0    <<<< leftover
> > 
> > Detecting which livepatch will be replaced by the new one from userspace is
> > not reliable

BTW: we handle this in rpm post install script. It removes all not longer
used livepatch modules before installing the newly installed one.


> > , necessitating the need for the operation to be performed
> > within the kernel itself. With this improvement, executing
> > `insmod livepatch-test_1.ko` will automatically remove the
> > livepatch-test_0.ko module.
> >
> > Following this change, the associated kernel module will be removed when
> > executing `echo 0 > /sys/kernel/livepatch/${livepath}/enabled`. Therefore,
> > adjustments need to be made to the selftests accordingly.
> 
> If the problem is that the user can't see which livepatch has been
> disabled, we should just fix that problem directly by leaving the
> disabled module in /sys/kernel/livepatch with an 'enabled' value of 0.
> 'enabled' could then be made read-only for replaced files.

I agree that it might remove the race. We must make sure that the
value is false only when the module can be removed. Also it would
require adding an API to remove the sysfs files from the module_exit
callback.

> That seems less disruptive to the user (and more consistent with the
> previous interface), and continues to leave the policy up to the user to
> decide if/when they want to remove the module.

I do not see any reasonable reason to keep the replaced livepatch
module loaded. It is an unusable piece of code. IMHO, it would be
really convenient if the kernel removed it.

It would be a new behavior even for the module loader. But we could
see it as a version upgrade of a kernel module.

> It would also allow easily downgrading the replaced module in the future
> (once we have proper support for that).

Sigh, I still havn't found time to prepare v2 of the patchset
reworking the callbacks.

Best Regards,
Petr

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Josh Poimboeuf]

On Mon, May 06, 2024 at 01:32:19PM +0200, Petr Mladek wrote:
> Also it would require adding an API to remove the sysfs files from the
> module_exit callback.

Could the sysfs removal be triggered from klp_module_going() or a module
notifier?

> I do not see any reasonable reason to keep the replaced livepatch
> module loaded. It is an unusable piece of code. IMHO, it would be
> really convenient if the kernel removed it.

User space needs to be polling for the transition to complete so it can
reverse the patch if it stalls.  Otherwise the patch could stall forever
and go unnoticed.

Can't user space just unload the replaced module after it detects the
completed transition?

I'm not sure I see the benefit in complicating the kernel and possibly
introducing bugs, when unloading the module from user space seems to be
a perfectly valid option.

Also, an error returned by delete_module() to the kernel would be
ignored and the module might remain in memory forever without being
noticed.

-- 
Josh

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Yafang Shao]

On Tue, May 7, 2024 at 10:35 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Mon, May 06, 2024 at 01:32:19PM +0200, Petr Mladek wrote:
> > Also it would require adding an API to remove the sysfs files from the
> > module_exit callback.
>
> Could the sysfs removal be triggered from klp_module_going() or a module
> notifier?
>
> > I do not see any reasonable reason to keep the replaced livepatch
> > module loaded. It is an unusable piece of code. IMHO, it would be
> > really convenient if the kernel removed it.
>
> User space needs to be polling for the transition to complete so it can
> reverse the patch if it stalls.  Otherwise the patch could stall forever
> and go unnoticed.
>
> Can't user space just unload the replaced module after it detects the
> completed transition?

Are you referring to polling the
"/sys/kernel/livepatch/XXX/transition"? The challenge lies in the
uncertainty regarding which livepatches will be replaced and how many.
Even if we can poll the transition status, there's no guarantee that a
livepatch will be replaced by this operation.

>
> I'm not sure I see the benefit in complicating the kernel and possibly
> introducing bugs, when unloading the module from user space seems to be
> a perfectly valid option.
>
> Also, an error returned by delete_module() to the kernel would be
> ignored and the module might remain in memory forever without being
> noticed.

As Petr pointed out, we can enhance the functionality by checking the
return value and providing informative error messages. This aligns
with the user experience when deleting a module; if deletion fails,
users have the option to try again. Similarly, if error messages are
displayed, users can manually remove the module if needed.

-- 
Regards
Yafang

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Josh Poimboeuf]

On Tue, May 07, 2024 at 10:03:59PM +0800, Yafang Shao wrote:
> On Tue, May 7, 2024 at 10:35 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > User space needs to be polling for the transition to complete so it can
> > reverse the patch if it stalls.  Otherwise the patch could stall forever
> > and go unnoticed.
> >
> > Can't user space just unload the replaced module after it detects the
> > completed transition?
> 
> Are you referring to polling the
> "/sys/kernel/livepatch/XXX/transition"? The challenge lies in the
> uncertainty regarding which livepatches will be replaced and how many.
> Even if we can poll the transition status, there's no guarantee that a
> livepatch will be replaced by this operation.

If klp_patch.replace is set on the new patch then it will replace all
previous patches.

If the replaced patches remain in /sys/livepatch then you can simply
unload all the patches with enabled == 0, after the new patch succeeds
(enabled == 1 && transition == 0).

> > I'm not sure I see the benefit in complicating the kernel and possibly
> > introducing bugs, when unloading the module from user space seems to be
> > a perfectly valid option.
> >
> > Also, an error returned by delete_module() to the kernel would be
> > ignored and the module might remain in memory forever without being
> > noticed.
> 
> As Petr pointed out, we can enhance the functionality by checking the
> return value and providing informative error messages. This aligns
> with the user experience when deleting a module; if deletion fails,
> users have the option to try again. Similarly, if error messages are
> displayed, users can manually remove the module if needed.

Calling delete_module() from the kernel means there's no syscall with
which to return an error back to the user.

-- 
Josh

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Yafang Shao]

On Wed, May 8, 2024 at 1:16 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Tue, May 07, 2024 at 10:03:59PM +0800, Yafang Shao wrote:
> > On Tue, May 7, 2024 at 10:35 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > User space needs to be polling for the transition to complete so it can
> > > reverse the patch if it stalls.  Otherwise the patch could stall forever
> > > and go unnoticed.
> > >
> > > Can't user space just unload the replaced module after it detects the
> > > completed transition?
> >
> > Are you referring to polling the
> > "/sys/kernel/livepatch/XXX/transition"? The challenge lies in the
> > uncertainty regarding which livepatches will be replaced and how many.
> > Even if we can poll the transition status, there's no guarantee that a
> > livepatch will be replaced by this operation.
>
> If klp_patch.replace is set on the new patch then it will replace all
> previous patches.

A scenario exists wherein a user could simultaneously disable a loaded
livepatch, potentially resulting in it not being replaced by the new
patch. While theoretical, this possibility is not entirely
implausible.

>
> If the replaced patches remain in /sys/livepatch then you can simply
> unload all the patches with enabled == 0, after the new patch succeeds
> (enabled == 1 && transition == 0).
>
> > > I'm not sure I see the benefit in complicating the kernel and possibly
> > > introducing bugs, when unloading the module from user space seems to be
> > > a perfectly valid option.
> > >
> > > Also, an error returned by delete_module() to the kernel would be
> > > ignored and the module might remain in memory forever without being
> > > noticed.
> >
> > As Petr pointed out, we can enhance the functionality by checking the
> > return value and providing informative error messages. This aligns
> > with the user experience when deleting a module; if deletion fails,
> > users have the option to try again. Similarly, if error messages are
> > displayed, users can manually remove the module if needed.
>
> Calling delete_module() from the kernel means there's no syscall with
> which to return an error back to the user.

pr_error() can calso tell the user the error, right ?
If we must return an error to the user, probably we can use
klp_free_replaced_patches_sync() instead of
klp_free_replaced_patches_async().

Under what conditions might a kernel module of a disabled livepatch be
unable to be removed?

--
Regards
Yafang

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Josh Poimboeuf]

On Wed, May 08, 2024 at 02:01:29PM +0800, Yafang Shao wrote:
> On Wed, May 8, 2024 at 1:16 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > If klp_patch.replace is set on the new patch then it will replace all
> > previous patches.
> 
> A scenario exists wherein a user could simultaneously disable a loaded
> livepatch, potentially resulting in it not being replaced by the new
> patch. While theoretical, this possibility is not entirely
> implausible.

Why does it matter whether it was replaced, or was disabled beforehand?
Either way the end result is the same.

> > > As Petr pointed out, we can enhance the functionality by checking the
> > > return value and providing informative error messages. This aligns
> > > with the user experience when deleting a module; if deletion fails,
> > > users have the option to try again. Similarly, if error messages are
> > > displayed, users can manually remove the module if needed.
> >
> > Calling delete_module() from the kernel means there's no syscall with
> > which to return an error back to the user.
> 
> pr_error() can calso tell the user the error, right ?

The dmesg buffer isn't a reliable way to communicate errors to a user
space process.

> If we must return an error to the user, probably we can use
> klp_free_replaced_patches_sync() instead of
> klp_free_replaced_patches_async().

It's async for a reason :-)

> Under what conditions might a kernel module of a disabled livepatch be
> unable to be removed?

For example:

  - Some code grabbed a reference to it, or some module has a dependency
    on it

  - It has an init function but not an exit function

  - The module has already been removed due to some race

  - Some other unforeseen bug or race in the module exit path

-- 
Josh

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Yafang Shao]

On Wed, May 8, 2024 at 3:03 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Wed, May 08, 2024 at 02:01:29PM +0800, Yafang Shao wrote:
> > On Wed, May 8, 2024 at 1:16 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > If klp_patch.replace is set on the new patch then it will replace all
> > > previous patches.
> >
> > A scenario exists wherein a user could simultaneously disable a loaded
> > livepatch, potentially resulting in it not being replaced by the new
> > patch. While theoretical, this possibility is not entirely
> > implausible.
>
> Why does it matter whether it was replaced, or was disabled beforehand?
> Either way the end result is the same.

When users disable the livepatch, the corresponding kernel module may
sometimes be removed, while other times it remains intact. This
inconsistency has the potential to confuse users.

>
> > > > As Petr pointed out, we can enhance the functionality by checking the
> > > > return value and providing informative error messages. This aligns
> > > > with the user experience when deleting a module; if deletion fails,
> > > > users have the option to try again. Similarly, if error messages are
> > > > displayed, users can manually remove the module if needed.
> > >
> > > Calling delete_module() from the kernel means there's no syscall with
> > > which to return an error back to the user.
> >
> > pr_error() can calso tell the user the error, right ?
>
> The dmesg buffer isn't a reliable way to communicate errors to a user
> space process.
>
> > If we must return an error to the user, probably we can use
> > klp_free_replaced_patches_sync() instead of
> > klp_free_replaced_patches_async().
>
> It's async for a reason :-)
>
> > Under what conditions might a kernel module of a disabled livepatch be
> > unable to be removed?
>
> For example:
>
>   - Some code grabbed a reference to it, or some module has a dependency
>     on it
>
>   - It has an init function but not an exit function
>
>   - The module has already been removed due to some race
>
>   - Some other unforeseen bug or race in the module exit path

This scenario is typical for a regular kernel module, but I'm
uncertain if the same applies to a disabled livepatch. Is there a
mechanism in place to ensure that the corresponding kernel module is
always removed?

-- 
Regards
Yafang

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Josh Poimboeuf]

On Thu, May 09, 2024 at 10:17:43AM +0800, Yafang Shao wrote:
> On Wed, May 8, 2024 at 3:03 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >
> > On Wed, May 08, 2024 at 02:01:29PM +0800, Yafang Shao wrote:
> > > On Wed, May 8, 2024 at 1:16 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > > If klp_patch.replace is set on the new patch then it will replace all
> > > > previous patches.
> > >
> > > A scenario exists wherein a user could simultaneously disable a loaded
> > > livepatch, potentially resulting in it not being replaced by the new
> > > patch. While theoretical, this possibility is not entirely
> > > implausible.
> >
> > Why does it matter whether it was replaced, or was disabled beforehand?
> > Either way the end result is the same.
> 
> When users disable the livepatch, the corresponding kernel module may
> sometimes be removed, while other times it remains intact. This
> inconsistency has the potential to confuse users.

I'm afraid I don't understand.  Can you give an example scenario?

-- 
Josh

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Yafang Shao]

On Thu, May 9, 2024 at 1:20 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Thu, May 09, 2024 at 10:17:43AM +0800, Yafang Shao wrote:
> > On Wed, May 8, 2024 at 3:03 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > >
> > > On Wed, May 08, 2024 at 02:01:29PM +0800, Yafang Shao wrote:
> > > > On Wed, May 8, 2024 at 1:16 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > > > If klp_patch.replace is set on the new patch then it will replace all
> > > > > previous patches.
> > > >
> > > > A scenario exists wherein a user could simultaneously disable a loaded
> > > > livepatch, potentially resulting in it not being replaced by the new
> > > > patch. While theoretical, this possibility is not entirely
> > > > implausible.
> > >
> > > Why does it matter whether it was replaced, or was disabled beforehand?
> > > Either way the end result is the same.
> >
> > When users disable the livepatch, the corresponding kernel module may
> > sometimes be removed, while other times it remains intact. This
> > inconsistency has the potential to confuse users.
>
> I'm afraid I don't understand.  Can you give an example scenario?
>

As previously mentioned, this scenario may occur if user-space tools
remove all pertinent kernel modules from /sys/livepatch/* while a user
attempts to load a new atomic-replace livepatch.

For instance:

User-A                                                       User-B

echo 0 > /sys/livepatch/A/enable              insmod atomic-replace-patch.ko

From User-A's viewpoint, the A.ko module might sometimes be removed,
while at other times it remains intact. The reason is that User-B
removed a module that he shouldn't remove.

-- 
Regards
Yafang

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Petr Mladek]

On Thu 2024-05-09 13:53:17, Yafang Shao wrote:
> On Thu, May 9, 2024 at 1:20 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >
> > On Thu, May 09, 2024 at 10:17:43AM +0800, Yafang Shao wrote:
> > > On Wed, May 8, 2024 at 3:03 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > >
> > > > On Wed, May 08, 2024 at 02:01:29PM +0800, Yafang Shao wrote:
> > > > > On Wed, May 8, 2024 at 1:16 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > > > > If klp_patch.replace is set on the new patch then it will replace all
> > > > > > previous patches.
> > > > >
> > > > > A scenario exists wherein a user could simultaneously disable a loaded
> > > > > livepatch, potentially resulting in it not being replaced by the new
> > > > > patch. While theoretical, this possibility is not entirely
> > > > > implausible.
> > > >
> > > > Why does it matter whether it was replaced, or was disabled beforehand?
> > > > Either way the end result is the same.
> > >
> > > When users disable the livepatch, the corresponding kernel module may
> > > sometimes be removed, while other times it remains intact. This
> > > inconsistency has the potential to confuse users.
> >
> > I'm afraid I don't understand.  Can you give an example scenario?
> >
> 
> As previously mentioned, this scenario may occur if user-space tools
> remove all pertinent kernel modules from /sys/livepatch/* while a user
> attempts to load a new atomic-replace livepatch.
> 
> For instance:
> 
> User-A                                                       User-B
> 
> echo 0 > /sys/livepatch/A/enable              insmod atomic-replace-patch.ko
> 
> >From User-A's viewpoint, the A.ko module might sometimes be removed,
> while at other times it remains intact. The reason is that User-B
> removed a module that he shouldn't remove.

Why would User-A want to keep the module, please? The livepatches
could not longer be re-enabled since the commit 958ef1e39d24d6
("livepatch: Simplify API by removing registration step") which
was added in v5.1-rc1.

The only problem might be that User-A can't remove the module
because it has already been removed by User-B or vice versa.
Is this really a problem?

Have you seen the problem in practice or is it just theoretical?

Is anyone really combining livepatches with and without atomic
replace?

Best Regards,
Petr

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Yafang Shao]

On Thu, May 9, 2024 at 6:16 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Thu 2024-05-09 13:53:17, Yafang Shao wrote:
> > On Thu, May 9, 2024 at 1:20 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > >
> > > On Thu, May 09, 2024 at 10:17:43AM +0800, Yafang Shao wrote:
> > > > On Wed, May 8, 2024 at 3:03 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > > >
> > > > > On Wed, May 08, 2024 at 02:01:29PM +0800, Yafang Shao wrote:
> > > > > > On Wed, May 8, 2024 at 1:16 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > > > > > If klp_patch.replace is set on the new patch then it will replace all
> > > > > > > previous patches.
> > > > > >
> > > > > > A scenario exists wherein a user could simultaneously disable a loaded
> > > > > > livepatch, potentially resulting in it not being replaced by the new
> > > > > > patch. While theoretical, this possibility is not entirely
> > > > > > implausible.
> > > > >
> > > > > Why does it matter whether it was replaced, or was disabled beforehand?
> > > > > Either way the end result is the same.
> > > >
> > > > When users disable the livepatch, the corresponding kernel module may
> > > > sometimes be removed, while other times it remains intact. This
> > > > inconsistency has the potential to confuse users.
> > >
> > > I'm afraid I don't understand.  Can you give an example scenario?
> > >
> >
> > As previously mentioned, this scenario may occur if user-space tools
> > remove all pertinent kernel modules from /sys/livepatch/* while a user
> > attempts to load a new atomic-replace livepatch.
> >
> > For instance:
> >
> > User-A                                                       User-B
> >
> > echo 0 > /sys/livepatch/A/enable              insmod atomic-replace-patch.ko
> >
> > >From User-A's viewpoint, the A.ko module might sometimes be removed,
> > while at other times it remains intact. The reason is that User-B
> > removed a module that he shouldn't remove.
>
> Why would User-A want to keep the module, please? The livepatches
> could not longer be re-enabled since the commit 958ef1e39d24d6
> ("livepatch: Simplify API by removing registration step") which
> was added in v5.1-rc1.
>
> The only problem might be that User-A can't remove the module
> because it has already been removed by User-B or vice versa.
> Is this really a problem?
>
> Have you seen the problem in practice or is it just theoretical?

It is just theoretical.

>
> Is anyone really combining livepatches with and without atomic
> replace?

I don't know.

Previously, we utilized live patches without atomic-replace
functionality. However, we've now transitioned all patches to support
atomic-replace. It might be beneficial to introduce a
/sys/kernel/livepatch/XXX/replace entry to indicate whether a patch
supports atomic-replace or not


-- 
Regards
Yafang

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Petr Mladek]

On Mon 2024-05-06 19:35:22, Josh Poimboeuf wrote:
> On Mon, May 06, 2024 at 01:32:19PM +0200, Petr Mladek wrote:
> > Also it would require adding an API to remove the sysfs files from the
> > module_exit callback.
> 
> Could the sysfs removal be triggered from klp_module_going() or a module
> notifier?

Great idea!

> > I do not see any reasonable reason to keep the replaced livepatch
> > module loaded. It is an unusable piece of code. IMHO, it would be
> > really convenient if the kernel removed it.
> 
> User space needs to be polling for the transition to complete so it can
> reverse the patch if it stalls.  Otherwise the patch could stall forever
> and go unnoticed.

Triggering the revert and removing the module are two asynchronous
operations. User space might just trigger the revert when it detect
the stall and assume that the module will get removed.

Stalled revert is end of the game anyway.

> Can't user space just unload the replaced module after it detects the
> completed transition?
> 
> I'm not sure I see the benefit in complicating the kernel and possibly
> introducing bugs, when unloading the module from user space seems to be
> a perfectly valid option.
>
> Also, an error returned by delete_module() to the kernel would be
> ignored and the module might remain in memory forever without being
> noticed.

The same error would happen even when it was called via the syscall.
It is the same code.

Well, user space might want to know when the module was removed.
For example, it could not load the same livepatch module until
it was removed. Explicit rmmod would be useful in this case.

I agree that adding the auto deleting might create new problems
and probably is not worth it.

Best Regards,
Petr

Re: [PATCH v2 2/2] livepatch: Delete the associated module of disabled livepatch

[Petr Mladek]

On Sun 2024-04-07 11:57:30, Yafang Shao wrote:
> In our production environment, upon loading a new atomic replace livepatch,
> we encountered an issue where the kernel module of the old livepatch
> remained, despite being replaced by the new one. This issue can be
> reproduced using the following steps:
> 
> - Pre setup: Build two atomic replace livepatches
> 
> - First step: Load the first livepatch using the insmod command
>   $ insmod livepatch-test_0.ko
> 
>   $ ls /sys/kernel/livepatch/
>   livepatch_test_0
> 
>   $ cat /sys/kernel/livepatch/livepatch_test_0/enabled
>   1
> 
>   $ lsmod  | grep livepatch
>   livepatch_test_0       16384  1
> 
> - Second step: Load the new livepatch using the insmod command
>   $ insmod livepatch-test_1.ko
> 
>   $ ls /sys/kernel/livepatch/
>   livepatch_test_1                  <<<< livepatch_test_0 was replaced
> 
>   $ cat /sys/kernel/livepatch/livepatch_test_1/enabled
>   1
> 
>   $ lsmod  | grep livepatch
>   livepatch_test_1       16384  1
>   livepatch_test_0       16384  0    <<<< leftover
> 
> Detecting which livepatch will be replaced by the new one from userspace is
> not reliable, necessitating the need for the operation to be performed
> within the kernel itself. With this improvement, executing
> `insmod livepatch-test_1.ko` will automatically remove the
> livepatch-test_0.ko module.
> 
> Following this change, the associated kernel module will be removed when
> executing `echo 0 > /sys/kernel/livepatch/${livepath}/enabled`. Therefore,
> adjustments need to be made to the selftests accordingly.
> 
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -721,8 +723,12 @@ static void klp_free_patch_finish(struct klp_patch *patch)
>  	wait_for_completion(&patch->finish);
>  
>  	/* Put the module after the last access to struct klp_patch. */
> -	if (!patch->forced)
> -		module_put(patch->mod);
> +	if (!patch->forced) {
> +		module_put(mod);
> +		if (!delete)
> +			return;
> +		delete_module(mod);

We should check the return value and report an error.

> +	}
>  }

Otherwise, it looks good to me.

I am personally in favor of this change. It removes one step
which otherwise has to be called from userspace after a non trivial
delay. The transition might take seconds.

It should simplify the scripting around livepatch modules handling.
It might reduce the need to over-optimize transition times.

Best Regards,
Petr

Re: [PATCH v2 0/2] livepatch, module: Delete the associated module of disabled livepatch

[Joe Lawrence]

On 4/6/24 23:57, Yafang Shao wrote:
> In our production environment, upon loading a new atomic replace livepatch,
> we encountered an issue where the kernel module of the old livepatch
> remained, despite being replaced by the new one. The detailed steps to
> reproduce that issue can be found in patch #2.
> 
> Detecting which livepatch will be replaced by the new one from userspace is
> not reliable, necessitating the need for the operation to be performed
> within the kernel itself.
> 
> This patchset aims to address this issue by automatically deleting the
> associated module of a disabled livepatch. Since a disabled livepatch can't
> be enabled again and the kernel module becomes redundant, it is safe to
> remove it in this manner.
> 
> Changes:
> - v1->v2:
>   - Avoid using kpatch utility in the example (Joe, Petr)
>   - Fix race around changing mod->state (Joe, Petr)
>   - Don't set mod->state outside of kernel/module dir (Joe, Petr)
>   - Alter selftests accordingly (Joe)
>   - Split it into two patches (Petr, Miroslav)
>   - Don't delete module from the path klp_enable_patch() (Petr, Miroslav)
>   - Make delete_module() safe (Petr)  
> 
> Yafang Shao (2):
>   module: Add a new helper delete_module()
>   livepatch: Delete the associated module of disabled livepatch
> 
>  include/linux/module.h                        |  1 +
>  kernel/livepatch/core.c                       | 16 ++--
>  kernel/module/main.c                          | 82 +++++++++++++++----
>  .../testing/selftests/livepatch/functions.sh  |  2 +
>  .../selftests/livepatch/test-callbacks.sh     | 24 ++----
>  .../selftests/livepatch/test-ftrace.sh        |  3 +-
>  .../selftests/livepatch/test-livepatch.sh     | 11 +--
>  .../testing/selftests/livepatch/test-state.sh | 15 +---
>  .../selftests/livepatch/test-syscall.sh       |  3 +-
>  .../testing/selftests/livepatch/test-sysfs.sh |  6 +-
>  10 files changed, 95 insertions(+), 68 deletions(-)
> 

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- 
Joe