[PATCH] livepatch: Delete the associated module when replacing an old livepatch

[PATCH] livepatch: Delete the associated module when replacing an old livepatch

[Yafang Shao]

Enhance the functionality of kpatch to automatically remove the associated
module when replacing an old livepatch with a new one. This ensures that no
leftover modules remain in the system. For instance:

- Load the first livepatch
  $ kpatch load 6.9.0-rc1+/livepatch-test_0.ko
  loading patch module: 6.9.0-rc1+/livepatch-test_0.ko
  waiting (up to 15 seconds) for patch transition to complete...
  transition complete (2 seconds)

  $ kpatch list
  Loaded patch modules:
  livepatch_test_0 [enabled]

  $ lsmod |grep livepatch
  livepatch_test_0       16384  1

- Load a new livepatch
  $ kpatch load 6.9.0-rc1+/livepatch-test_1.ko
  loading patch module: 6.9.0-rc1+/livepatch-test_1.ko
  waiting (up to 15 seconds) for patch transition to complete...
  transition complete (2 seconds)

  $ kpatch list
  Loaded patch modules:
  livepatch_test_1 [enabled]

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

With this improvement, executing
`kpatch load 6.9.0-rc1+/livepatch-test_1.ko` will automatically remove the
livepatch-test_0.ko module.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/module.h  |  1 +
 kernel/livepatch/core.c | 11 +++++++++--
 kernel/module/main.c    | 43 ++++++++++++++++++++++++-----------------
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 1153b0d99a80..9a95174a919b 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 void delete_module(struct module *mod);
 
 #ifndef MODULE
 /**
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ecbc9b6aba3a..f1edc999f3ef 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -711,6 +711,8 @@ static void klp_free_patch_start(struct klp_patch *patch)
  */
 static void klp_free_patch_finish(struct klp_patch *patch)
 {
+	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,13 @@ 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 (module_refcount(mod))
+			return;
+		mod->state = MODULE_STATE_GOING;
+		delete_module(mod);
+	}
 }
 
 /*
diff --git a/kernel/module/main.c b/kernel/module/main.c
index e1e8a7a9d6c1..e863e1f87dfd 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -695,12 +695,35 @@ EXPORT_SYMBOL(module_refcount);
 /* This exists whether we can unload or not */
 static void free_module(struct module *mod);
 
+void delete_module(struct module *mod)
+{
+	char buf[MODULE_FLAGS_BUF_SIZE];
+
+	/* 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);
+}
+
 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 +773,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] livepatch: Delete the associated module when replacing an old livepatch

[zhang warden]

It seems that you try to remove the disabled module by the kip replace. However, changing the code of sys call may introduce some unnecessary changes to non-livepatch module. Is that really a safe way to do so?

> On Mar 31, 2024, at 21:38, Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> Enhance the functionality of kpatch to automatically remove the associated
> module when replacing an old livepatch with a new one. This ensures that no
> leftover modules remain in the system. For instance:
> 
> - Load the first livepatch
> $ kpatch load 6.9.0-rc1+/livepatch-test_0.ko
> loading patch module: 6.9.0-rc1+/livepatch-test_0.ko
> waiting (up to 15 seconds) for patch transition to complete...
> transition complete (2 seconds)
> 
> $ kpatch list
> Loaded patch modules:
> livepatch_test_0 [enabled]
> 
> $ lsmod |grep livepatch
> livepatch_test_0       16384  1
> 
> - Load a new livepatch
> $ kpatch load 6.9.0-rc1+/livepatch-test_1.ko
> loading patch module: 6.9.0-rc1+/livepatch-test_1.ko
> waiting (up to 15 seconds) for patch transition to complete...
> transition complete (2 seconds)
> 
> $ kpatch list
> Loaded patch modules:
> livepatch_test_1 [enabled]
> 
> $ lsmod |grep livepatch
> livepatch_test_1       16384  1
> livepatch_test_0       16384  0   <<<< leftover
> 
> With this improvement, executing
> `kpatch load 6.9.0-rc1+/livepatch-test_1.ko` will automatically remove the
> livepatch-test_0.ko module.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> include/linux/module.h  |  1 +
> kernel/livepatch/core.c | 11 +++++++++--
> kernel/module/main.c    | 43 ++++++++++++++++++++++++-----------------
> 3 files changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 1153b0d99a80..9a95174a919b 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 void delete_module(struct module *mod);
> 
> #ifndef MODULE
> /**
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index ecbc9b6aba3a..f1edc999f3ef 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -711,6 +711,8 @@ static void klp_free_patch_start(struct klp_patch *patch)
> */
> static void klp_free_patch_finish(struct klp_patch *patch)
> {
> + 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,13 @@ 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 (module_refcount(mod))
> + return;
> + mod->state = MODULE_STATE_GOING;
> + delete_module(mod);
> + }
> }
> 
> /*
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index e1e8a7a9d6c1..e863e1f87dfd 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -695,12 +695,35 @@ EXPORT_SYMBOL(module_refcount);
> /* This exists whether we can unload or not */
> static void free_module(struct module *mod);
> 
> +void delete_module(struct module *mod)
> +{
> + char buf[MODULE_FLAGS_BUF_SIZE];
> +
> + /* 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);
> +}
> +
> 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 +773,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] livepatch: Delete the associated module when replacing an old livepatch

[Joe Lawrence]

On Sun, Mar 31, 2024 at 09:38:39PM +0800, Yafang Shao wrote:
> Enhance the functionality of kpatch to automatically remove the associated
> module when replacing an old livepatch with a new one. This ensures that no
> leftover modules remain in the system. For instance:
> 
> - Load the first livepatch
>   $ kpatch load 6.9.0-rc1+/livepatch-test_0.ko
>   loading patch module: 6.9.0-rc1+/livepatch-test_0.ko
>   waiting (up to 15 seconds) for patch transition to complete...
>   transition complete (2 seconds)
> 
>   $ kpatch list
>   Loaded patch modules:
>   livepatch_test_0 [enabled]
> 
>   $ lsmod |grep livepatch
>   livepatch_test_0       16384  1
> 
> - Load a new livepatch
>   $ kpatch load 6.9.0-rc1+/livepatch-test_1.ko
>   loading patch module: 6.9.0-rc1+/livepatch-test_1.ko
>   waiting (up to 15 seconds) for patch transition to complete...
>   transition complete (2 seconds)
> 
>   $ kpatch list
>   Loaded patch modules:
>   livepatch_test_1 [enabled]
> 
>   $ lsmod |grep livepatch
>   livepatch_test_1       16384  1
>   livepatch_test_0       16384  0   <<<< leftover
> 
> With this improvement, executing
> `kpatch load 6.9.0-rc1+/livepatch-test_1.ko` will automatically remove the
> livepatch-test_0.ko module.
> 

Hi Yafang,

I think it would be better if the commit message reasoning used
insmod/modprobe directly rather than the kpatch user utility wrapper.
That would be more generic and remove any potential kpatch utility
variants from the picture.  (For example, it is possible to add `rmmod`
in the wrapper and then this patch would be redundant.)

> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/module.h  |  1 +
>  kernel/livepatch/core.c | 11 +++++++++--
>  kernel/module/main.c    | 43 ++++++++++++++++++++++++-----------------
>  3 files changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 1153b0d99a80..9a95174a919b 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 void delete_module(struct module *mod);
>  
>  #ifndef MODULE
>  /**
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index ecbc9b6aba3a..f1edc999f3ef 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -711,6 +711,8 @@ static void klp_free_patch_start(struct klp_patch *patch)
>   */
>  static void klp_free_patch_finish(struct klp_patch *patch)
>  {
> +	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,13 @@ 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 (module_refcount(mod))
> +			return;
> +		mod->state = MODULE_STATE_GOING;
> +		delete_module(mod);
> +	}
>  }
>  
>  /*
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index e1e8a7a9d6c1..e863e1f87dfd 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -695,12 +695,35 @@ EXPORT_SYMBOL(module_refcount);
>  /* This exists whether we can unload or not */
>  static void free_module(struct module *mod);
>  
> +void delete_module(struct module *mod)
> +{
> +	char buf[MODULE_FLAGS_BUF_SIZE];
> +
> +	/* 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);
> +}
> +
>  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 +773,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
> 

It's been a while since atomic replace was added and so I forget why the
implementation doesn't try this -- is it possible for the livepatch
module to have additional references that this patch would force its way
through?

Also, this patch will break the "atomic replace livepatch" kselftest in
test-livepatch.sh [1].  I think it would need to drop the `unload_lp
$MOD_LIVEPATCH` command, the following 'live patched' greps and their
corresponding dmesg output in the test's final check_result() call.

--
Joe

Re: [PATCH] livepatch: Delete the associated module when replacing an old livepatch

[Yafang Shao]

On Tue, Apr 2, 2024 at 10:03 AM zhang warden <zhangwarden@gmail.com> wrote:
>
> It seems that you try to remove the disabled module by the kip replace. However, changing the code of sys call may introduce some unnecessary changes to non-livepatch module.

Could you please explain why klp_free_patch_finish() will impact the
non-livepatch module ?

> Is that really a safe way to do so?

Could you pls elaborate how you practice  the kpatch replace mode in your
production environment? Have you notice the commit df1e98f2c74
("kpatch: rmmod module of the same name before loading a module") in
the kpatch userspace tool that tries to workaround this issue ?
BTW, why do you think it is unsafe ?


--
Regards
Yafang

Re: [PATCH] livepatch: Delete the associated module when replacing an old livepatch

[Yafang Shao]

On Mon, Apr 1, 2024 at 11:02 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On Sun, Mar 31, 2024 at 09:38:39PM +0800, Yafang Shao wrote:
> > Enhance the functionality of kpatch to automatically remove the associated
> > module when replacing an old livepatch with a new one. This ensures that no
> > leftover modules remain in the system. For instance:
> >
> > - Load the first livepatch
> >   $ kpatch load 6.9.0-rc1+/livepatch-test_0.ko
> >   loading patch module: 6.9.0-rc1+/livepatch-test_0.ko
> >   waiting (up to 15 seconds) for patch transition to complete...
> >   transition complete (2 seconds)
> >
> >   $ kpatch list
> >   Loaded patch modules:
> >   livepatch_test_0 [enabled]
> >
> >   $ lsmod |grep livepatch
> >   livepatch_test_0       16384  1
> >
> > - Load a new livepatch
> >   $ kpatch load 6.9.0-rc1+/livepatch-test_1.ko
> >   loading patch module: 6.9.0-rc1+/livepatch-test_1.ko
> >   waiting (up to 15 seconds) for patch transition to complete...
> >   transition complete (2 seconds)
> >
> >   $ kpatch list
> >   Loaded patch modules:
> >   livepatch_test_1 [enabled]
> >
> >   $ lsmod |grep livepatch
> >   livepatch_test_1       16384  1
> >   livepatch_test_0       16384  0   <<<< leftover
> >
> > With this improvement, executing
> > `kpatch load 6.9.0-rc1+/livepatch-test_1.ko` will automatically remove the
> > livepatch-test_0.ko module.
> >
>
> Hi Yafang,
>
> I think it would be better if the commit message reasoning used
> insmod/modprobe directly rather than the kpatch user utility wrapper.
> That would be more generic and remove any potential kpatch utility
> variants from the picture.  (For example, it is possible to add `rmmod`
> in the wrapper and then this patch would be redundant.)

Hi Joe,

I attempted to incorporate an `rmmod` operation within the kpatch
replacement process, but encountered challenges in devising a safe and
effective solution. The difficulty arises from the uncertainty
regarding which livepatch will be replaced in userspace, necessitating
the operation to be conducted within the kernel itself.


>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/linux/module.h  |  1 +
> >  kernel/livepatch/core.c | 11 +++++++++--
> >  kernel/module/main.c    | 43 ++++++++++++++++++++++++-----------------
> >  3 files changed, 35 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 1153b0d99a80..9a95174a919b 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 void delete_module(struct module *mod);
> >
> >  #ifndef MODULE
> >  /**
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index ecbc9b6aba3a..f1edc999f3ef 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -711,6 +711,8 @@ static void klp_free_patch_start(struct klp_patch *patch)
> >   */
> >  static void klp_free_patch_finish(struct klp_patch *patch)
> >  {
> > +     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,13 @@ 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 (module_refcount(mod))
> > +                     return;
> > +             mod->state = MODULE_STATE_GOING;
> > +             delete_module(mod);
> > +     }
> >  }
> >
> >  /*
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index e1e8a7a9d6c1..e863e1f87dfd 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -695,12 +695,35 @@ EXPORT_SYMBOL(module_refcount);
> >  /* This exists whether we can unload or not */
> >  static void free_module(struct module *mod);
> >
> > +void delete_module(struct module *mod)
> > +{
> > +     char buf[MODULE_FLAGS_BUF_SIZE];
> > +
> > +     /* 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);
> > +}
> > +
> >  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 +773,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
> >
>
> It's been a while since atomic replace was added and so I forget why the
> implementation doesn't try this -- is it possible for the livepatch
> module to have additional references that this patch would force its way
> through?

In the klp_free_patch_finish() function, a check is performed on the
reference count of the livepatch module. If the reference count is not
zero, the function will skip further processing.

>
> Also, this patch will break the "atomic replace livepatch" kselftest in
> test-livepatch.sh [1].  I think it would need to drop the `unload_lp
> $MOD_LIVEPATCH` command, the following 'live patched' greps and their
> corresponding dmesg output in the test's final check_result() call.

Thanks for your information. I will check it.

-- 
Regards
Yafang

Re: [PATCH] livepatch: Delete the associated module when replacing an old livepatch

[zhang warden]

> On Apr 2, 2024, at 10:27, Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> df1e98f2c74

Hi Yafang!

To my first question, from your patch, klp_free_patch_finish may not affect non-livpatch module. However, if my reading is right, your patch make changes to SYSCALL of delete_module. Making changes to sys call may effect non-livepatch module, I think.

Tell you the truth, in my production env, I don’t use klp replace mode because my livepatch fixing process dose’t adjust the logic of replacing the previous patches. Therefore, klp-replace mode is not suitable in my situation. The reason why I ask for safety is that this patch seems to change the syscall, which may cause some other effects.

For the commit ("kpatch: rmmod module of the same name before loading a module”) in patch userspace, it seems to fix this issue, while this commit is working in userspace, under kpatch’s control.  

What’s more, your patch seems to be malformed	when I try to patch it. Is there any thing wrong when I copying your patch?

This is only my own option in reading your patch. Thanks!

--
Regards
Warden

Re: [PATCH] livepatch: Delete the associated module when replacing an old livepatch

[Yafang Shao]

On Tue, Apr 2, 2024 at 10:56 AM zhang warden <zhangwarden@gmail.com> wrote:
>
>
>
> > On Apr 2, 2024, at 10:27, Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > df1e98f2c74
>
> Hi Yafang!
>
> To my first question, from your patch, klp_free_patch_finish may not affect non-livpatch module. However, if my reading is right, your patch make changes to SYSCALL of delete_module. Making changes to sys call may effect non-livepatch module, I think.

I can't get your point here. Impact on what? The performance?

>
> Tell you the truth, in my production env, I don’t use klp replace mode because my livepatch fixing process dose’t adjust the logic of replacing the previous patches. Therefore, klp-replace mode is not suitable in my situation. The reason why I ask for safety is that this patch seems to change the syscall, which may cause some other effects.

Most code modifications within the kernel have the potential to
directly or indirectly alter one or more syscalls.

>
> For the commit ("kpatch: rmmod module of the same name before loading a module”) in patch userspace, it seems to fix this issue, while this commit is working in userspace, under kpatch’s control.

It appears there may have been a misunderstanding regarding the commit
("kpatch: rmmod module of the same name before loading a module"). I
recommend trying it out first before drawing any conclusions.

>
> What’s more, your patch seems to be malformed   when I try to patch it. Is there any thing wrong when I copying your patch?

I don't know what happened. Probably I should rebase it on the lastest
live-patching tree.

>
> This is only my own option in reading your patch. Thanks!
>
> --
> Regards
> Warden
>


-- 
Regards
Yafang

Re: [PATCH] livepatch: Delete the associated module when replacing an old livepatch

[Joe Lawrence]

On 4/1/24 22:45, Yafang Shao wrote:
> On Mon, Apr 1, 2024 at 11:02 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>>
>> On Sun, Mar 31, 2024 at 09:38:39PM +0800, Yafang Shao wrote:
>>> Enhance the functionality of kpatch to automatically remove the associated
>>> module when replacing an old livepatch with a new one. This ensures that no
>>> leftover modules remain in the system. For instance:
>>>
>>> - Load the first livepatch
>>>   $ kpatch load 6.9.0-rc1+/livepatch-test_0.ko
>>>   loading patch module: 6.9.0-rc1+/livepatch-test_0.ko
>>>   waiting (up to 15 seconds) for patch transition to complete...
>>>   transition complete (2 seconds)
>>>
>>>   $ kpatch list
>>>   Loaded patch modules:
>>>   livepatch_test_0 [enabled]
>>>
>>>   $ lsmod |grep livepatch
>>>   livepatch_test_0       16384  1
>>>
>>> - Load a new livepatch
>>>   $ kpatch load 6.9.0-rc1+/livepatch-test_1.ko
>>>   loading patch module: 6.9.0-rc1+/livepatch-test_1.ko
>>>   waiting (up to 15 seconds) for patch transition to complete...
>>>   transition complete (2 seconds)
>>>
>>>   $ kpatch list
>>>   Loaded patch modules:
>>>   livepatch_test_1 [enabled]
>>>
>>>   $ lsmod |grep livepatch
>>>   livepatch_test_1       16384  1
>>>   livepatch_test_0       16384  0   <<<< leftover
>>>
>>> With this improvement, executing
>>> `kpatch load 6.9.0-rc1+/livepatch-test_1.ko` will automatically remove the
>>> livepatch-test_0.ko module.
>>>
>>
>> Hi Yafang,
>>
>> I think it would be better if the commit message reasoning used
>> insmod/modprobe directly rather than the kpatch user utility wrapper.
>> That would be more generic and remove any potential kpatch utility
>> variants from the picture.  (For example, it is possible to add `rmmod`
>> in the wrapper and then this patch would be redundant.)
> 
> Hi Joe,
> 
> I attempted to incorporate an `rmmod` operation within the kpatch
> replacement process, but encountered challenges in devising a safe and
> effective solution. The difficulty arises from the uncertainty
> regarding which livepatch will be replaced in userspace, necessitating
> the operation to be conducted within the kernel itself.
> 

I wasn't suggesting that the kpatch user utility should or could solve
this problem, just that this scenario is not specific to kpatch.  And
since this is a kernel patch, it would be consistent to speak in terms
of livepatches: the repro can be phrased in terms of modprobe/insmod,
/sys/kernel/livepatch/ sysfs, rmmod, etc. for which those not using the
kpatch utility are more familiar with.

>>
>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>> ---
>>>  include/linux/module.h  |  1 +
>>>  kernel/livepatch/core.c | 11 +++++++++--
>>>  kernel/module/main.c    | 43 ++++++++++++++++++++++++-----------------
>>>  3 files changed, 35 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/include/linux/module.h b/include/linux/module.h
>>> index 1153b0d99a80..9a95174a919b 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 void delete_module(struct module *mod);
>>>
>>>  #ifndef MODULE
>>>  /**
>>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>>> index ecbc9b6aba3a..f1edc999f3ef 100644
>>> --- a/kernel/livepatch/core.c
>>> +++ b/kernel/livepatch/core.c
>>> @@ -711,6 +711,8 @@ static void klp_free_patch_start(struct klp_patch *patch)
>>>   */
>>>  static void klp_free_patch_finish(struct klp_patch *patch)
>>>  {
>>> +     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,13 @@ 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 (module_refcount(mod))
>>> +                     return;
>>> +             mod->state = MODULE_STATE_GOING;
>>> +             delete_module(mod);
>>> +     }

I'm gonna have to read study code in kernel/module/ to be confident that
this is completely safe.  What happens if this code races a concurrent
`rmmod` from the user (perhaps that pesky kpatch utility)?  Can a stray
module reference sneak between the code here.  Etc.  The existing
delete_module syscall does some additional safety checks under the
module_mutex, which may or may not make sense for this use case... Petr,
Miroslav any thoughts?

Also, code-wise, it would be nice if the mod->state were only assigned
inside the kernel/module/main.c code... maybe this little sequence can
be pushed into that file so it's all in one place?

>>>  }
>>>
>>>  /*
>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>> index e1e8a7a9d6c1..e863e1f87dfd 100644
>>> --- a/kernel/module/main.c
>>> +++ b/kernel/module/main.c
>>> @@ -695,12 +695,35 @@ EXPORT_SYMBOL(module_refcount);
>>>  /* This exists whether we can unload or not */
>>>  static void free_module(struct module *mod);
>>>
>>> +void delete_module(struct module *mod)
>>> +{
>>> +     char buf[MODULE_FLAGS_BUF_SIZE];
>>> +
>>> +     /* 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);
>>> +}
>>> +
>>>  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 +773,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
>>>
>>
>> It's been a while since atomic replace was added and so I forget why the
>> implementation doesn't try this -- is it possible for the livepatch
>> module to have additional references that this patch would force its way
>> through?
> 
> In the klp_free_patch_finish() function, a check is performed on the
> reference count of the livepatch module. If the reference count is not
> zero, the function will skip further processing.
> 
>>
>> Also, this patch will break the "atomic replace livepatch" kselftest in
>> test-livepatch.sh [1].  I think it would need to drop the `unload_lp
>> $MOD_LIVEPATCH` command, the following 'live patched' greps and their
>> corresponding dmesg output in the test's final check_result() call.
> 
> Thanks for your information. I will check it.
> 

Let me know if you have any questions, I'm more familiar with that code
than the atomic replace / module interactions :)

-- 
Joe

Re: [PATCH] livepatch: Delete the associated module when replacing an old livepatch

[Yafang Shao]

On Tue, Apr 2, 2024 at 9:39 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On 4/1/24 22:45, Yafang Shao wrote:
> > On Mon, Apr 1, 2024 at 11:02 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >>
> >> On Sun, Mar 31, 2024 at 09:38:39PM +0800, Yafang Shao wrote:
> >>> Enhance the functionality of kpatch to automatically remove the associated
> >>> module when replacing an old livepatch with a new one. This ensures that no
> >>> leftover modules remain in the system. For instance:
> >>>
> >>> - Load the first livepatch
> >>>   $ kpatch load 6.9.0-rc1+/livepatch-test_0.ko
> >>>   loading patch module: 6.9.0-rc1+/livepatch-test_0.ko
> >>>   waiting (up to 15 seconds) for patch transition to complete...
> >>>   transition complete (2 seconds)
> >>>
> >>>   $ kpatch list
> >>>   Loaded patch modules:
> >>>   livepatch_test_0 [enabled]
> >>>
> >>>   $ lsmod |grep livepatch
> >>>   livepatch_test_0       16384  1
> >>>
> >>> - Load a new livepatch
> >>>   $ kpatch load 6.9.0-rc1+/livepatch-test_1.ko
> >>>   loading patch module: 6.9.0-rc1+/livepatch-test_1.ko
> >>>   waiting (up to 15 seconds) for patch transition to complete...
> >>>   transition complete (2 seconds)
> >>>
> >>>   $ kpatch list
> >>>   Loaded patch modules:
> >>>   livepatch_test_1 [enabled]
> >>>
> >>>   $ lsmod |grep livepatch
> >>>   livepatch_test_1       16384  1
> >>>   livepatch_test_0       16384  0   <<<< leftover
> >>>
> >>> With this improvement, executing
> >>> `kpatch load 6.9.0-rc1+/livepatch-test_1.ko` will automatically remove the
> >>> livepatch-test_0.ko module.
> >>>
> >>
> >> Hi Yafang,
> >>
> >> I think it would be better if the commit message reasoning used
> >> insmod/modprobe directly rather than the kpatch user utility wrapper.
> >> That would be more generic and remove any potential kpatch utility
> >> variants from the picture.  (For example, it is possible to add `rmmod`
> >> in the wrapper and then this patch would be redundant.)
> >
> > Hi Joe,
> >
> > I attempted to incorporate an `rmmod` operation within the kpatch
> > replacement process, but encountered challenges in devising a safe and
> > effective solution. The difficulty arises from the uncertainty
> > regarding which livepatch will be replaced in userspace, necessitating
> > the operation to be conducted within the kernel itself.
> >
>
> I wasn't suggesting that the kpatch user utility should or could solve
> this problem, just that this scenario is not specific to kpatch.  And
> since this is a kernel patch, it would be consistent to speak in terms
> of livepatches: the repro can be phrased in terms of modprobe/insmod,
> /sys/kernel/livepatch/ sysfs, rmmod, etc. for which those not using the
> kpatch utility are more familiar with.

Understood. Thanks for your explanation. I will try it.

>
> >>
> >>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >>> ---
> >>>  include/linux/module.h  |  1 +
> >>>  kernel/livepatch/core.c | 11 +++++++++--
> >>>  kernel/module/main.c    | 43 ++++++++++++++++++++++++-----------------
> >>>  3 files changed, 35 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/include/linux/module.h b/include/linux/module.h
> >>> index 1153b0d99a80..9a95174a919b 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 void delete_module(struct module *mod);
> >>>
> >>>  #ifndef MODULE
> >>>  /**
> >>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >>> index ecbc9b6aba3a..f1edc999f3ef 100644
> >>> --- a/kernel/livepatch/core.c
> >>> +++ b/kernel/livepatch/core.c
> >>> @@ -711,6 +711,8 @@ static void klp_free_patch_start(struct klp_patch *patch)
> >>>   */
> >>>  static void klp_free_patch_finish(struct klp_patch *patch)
> >>>  {
> >>> +     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,13 @@ 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 (module_refcount(mod))
> >>> +                     return;
> >>> +             mod->state = MODULE_STATE_GOING;
> >>> +             delete_module(mod);
> >>> +     }
>
> I'm gonna have to read study code in kernel/module/ to be confident that
> this is completely safe.  What happens if this code races a concurrent
> `rmmod` from the user (perhaps that pesky kpatch utility)?  Can a stray
> module reference sneak between the code here.  Etc.  The existing
> delete_module syscall does some additional safety checks under the
> module_mutex, which may or may not make sense for this use case... Petr,
> Miroslav any thoughts?

A race condition may occur. It appears necessary to modify the
mod->state under the protection of module_mutex. If the state is not
MODULE_STATE_LIVE, it must be skipped.

>
> Also, code-wise, it would be nice if the mod->state were only assigned
> inside the kernel/module/main.c code... maybe this little sequence can
> be pushed into that file so it's all in one place?

good suggestion. will do it.

>
> >>>  }
> >>>
> >>>  /*
> >>> diff --git a/kernel/module/main.c b/kernel/module/main.c
> >>> index e1e8a7a9d6c1..e863e1f87dfd 100644
> >>> --- a/kernel/module/main.c
> >>> +++ b/kernel/module/main.c
> >>> @@ -695,12 +695,35 @@ EXPORT_SYMBOL(module_refcount);
> >>>  /* This exists whether we can unload or not */
> >>>  static void free_module(struct module *mod);
> >>>
> >>> +void delete_module(struct module *mod)
> >>> +{
> >>> +     char buf[MODULE_FLAGS_BUF_SIZE];
> >>> +
> >>> +     /* 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);
> >>> +}
> >>> +
> >>>  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 +773,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
> >>>
> >>
> >> It's been a while since atomic replace was added and so I forget why the
> >> implementation doesn't try this -- is it possible for the livepatch
> >> module to have additional references that this patch would force its way
> >> through?
> >
> > In the klp_free_patch_finish() function, a check is performed on the
> > reference count of the livepatch module. If the reference count is not
> > zero, the function will skip further processing.
> >
> >>
> >> Also, this patch will break the "atomic replace livepatch" kselftest in
> >> test-livepatch.sh [1].  I think it would need to drop the `unload_lp
> >> $MOD_LIVEPATCH` command, the following 'live patched' greps and their
> >> corresponding dmesg output in the test's final check_result() call.
> >
> > Thanks for your information. I will check it.
> >
>
> Let me know if you have any questions, I'm more familiar with that code
> than the atomic replace / module interactions :)
>

You're correct in noting that we need to discard certain unload_lps
and rmmods. This is because, after implementing the change, executing
`echo 0 > /sys/kernel/livepatch/${livepatch}/enabled` will remove the
associated kernel module.

The question then arises: is this change in behavior acceptable, or
should we avoid it?

-- 
Regards
Yafang

Re: [PATCH] livepatch: Delete the associated module when replacing an old livepatch

[Petr Mladek]

On Sun 2024-03-31 21:38:39, Yafang Shao wrote:
> Enhance the functionality of kpatch to automatically remove the associated
> module when replacing an old livepatch with a new one. This ensures that no
> leftover modules remain in the system. For instance:

I like this feature. I would suggest to split it into two parts:

  + 1st patch would implement the delete_module() API. It must be safe
    even for other potential in-kernel callers. And it must be
    acceptable for the module loader code maintainers.

  + 2nd patch() using the API in the livepatch code.
    We will need to make sure that the new delete_module()
    API is used correctly from the livepatching code side.

The 2nd patch should also fix the selftests.


> - Load the first livepatch
>   $ kpatch load 6.9.0-rc1+/livepatch-test_0.ko
>   loading patch module: 6.9.0-rc1+/livepatch-test_0.ko
>   waiting (up to 15 seconds) for patch transition to complete...
>   transition complete (2 seconds)
> 
>   $ kpatch list
>   Loaded patch modules:
>   livepatch_test_0 [enabled]
> 
>   $ lsmod |grep livepatch
>   livepatch_test_0       16384  1
> 
> - Load a new livepatch
>   $ kpatch load 6.9.0-rc1+/livepatch-test_1.ko
>   loading patch module: 6.9.0-rc1+/livepatch-test_1.ko
>   waiting (up to 15 seconds) for patch transition to complete...
>   transition complete (2 seconds)
> 
>   $ kpatch list
>   Loaded patch modules:
>   livepatch_test_1 [enabled]
> 
>   $ lsmod |grep livepatch
>   livepatch_test_1       16384  1
>   livepatch_test_0       16384  0   <<<< leftover
> 
> With this improvement, executing
> `kpatch load 6.9.0-rc1+/livepatch-test_1.ko` will automatically remove the
> livepatch-test_0.ko module.

As already mentioned by Joe, please replace "kpatch" with
the related "modprobe" and "echo 0 >/sys/kernel/livepatch/<name>/enable"
calls.

"kpatch" is a 3rd party tool and only few people know what it does
internally. The kernel commit message is there for current and future
kernel developers. They should be able to understand the behavior
even without digging details about "random" user-space tools.

> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -721,8 +723,13 @@ 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 (module_refcount(mod))
> +			return;
> +		mod->state = MODULE_STATE_GOING;

mod->state should be modified only by the code in kernel/module/.
It helps to keep the operation safe (under control of module
loader code maintainers).

The fact that this patch does the above without module_mutex is
a nice example of possible mistakes.

And there are more problems, see below.

> +		delete_module(mod);

klp_free_patch_finish() is called also from the error path
in klp_enable_patch(). We must not remove the module
in this case. do_init_module() will do the clean up
the right way.

> +	}
>  }
>  
>  /*
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index e1e8a7a9d6c1..e863e1f87dfd 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -695,12 +695,35 @@ EXPORT_SYMBOL(module_refcount);
>  /* This exists whether we can unload or not */
>  static void free_module(struct module *mod);
>  
> +void delete_module(struct module *mod)
> +{
> +	char buf[MODULE_FLAGS_BUF_SIZE];
> +

If we export this API via include/linux/module.h then
it could be used anywhere in the kernel. Therefore we need
to make it safe.

This function should do the same actions as the syscall
starting from:

	mutex_lock(&module_mutex); 

	if (!list_empty(&mod->source_list)) {
		/* Other modules depend on us: get rid of them first. */
		ret = -EWOULDBLOCK;
		goto out;
	}
...

Best Regards,
Petr

> +	/* 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);
> +}
> +
>  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 +773,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);

Re: [PATCH] livepatch: Delete the associated module when replacing an old livepatch

[Miroslav Benes]

> >>> --- a/kernel/livepatch/core.c
> >>> +++ b/kernel/livepatch/core.c
> >>> @@ -711,6 +711,8 @@ static void klp_free_patch_start(struct klp_patch *patch)
> >>>   */
> >>>  static void klp_free_patch_finish(struct klp_patch *patch)
> >>>  {
> >>> +     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,13 @@ 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 (module_refcount(mod))
> >>> +                     return;
> >>> +             mod->state = MODULE_STATE_GOING;
> >>> +             delete_module(mod);
> >>> +     }
> 
> I'm gonna have to read study code in kernel/module/ to be confident that
> this is completely safe.  What happens if this code races a concurrent
> `rmmod` from the user (perhaps that pesky kpatch utility)?  Can a stray
> module reference sneak between the code here.  Etc.  The existing
> delete_module syscall does some additional safety checks under the
> module_mutex, which may or may not make sense for this use case... Petr,
> Miroslav any thoughts?

Compared to the existing delete_module syscall we know at this point that 
the module was live and used which gives us a slight advantage (leaving 
the issue that this path is also used in klp_enable_patch() as Petr said 
aside). However as you and Petr pointed out already I do not think it is 
correct to do this here. Changing mod->state is possible without 
module_mutex but only in some cases. I need to refresh it.

Anyway, a separate patch with a preparation work might reveal some of 
these issues and would be easier to review hopefully.

Miroslav

Re: [PATCH] livepatch: Delete the associated module when replacing an old livepatch

[Yafang Shao]

On Thu, Apr 4, 2024 at 10:04 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Sun 2024-03-31 21:38:39, Yafang Shao wrote:
> > Enhance the functionality of kpatch to automatically remove the associated
> > module when replacing an old livepatch with a new one. This ensures that no
> > leftover modules remain in the system. For instance:
>
> I like this feature. I would suggest to split it into two parts:
>
>   + 1st patch would implement the delete_module() API. It must be safe
>     even for other potential in-kernel callers. And it must be
>     acceptable for the module loader code maintainers.
>
>   + 2nd patch() using the API in the livepatch code.
>     We will need to make sure that the new delete_module()
>     API is used correctly from the livepatching code side.
>
> The 2nd patch should also fix the selftests.

Thanks for your suggestion. I will do it.

>
>
> > - Load the first livepatch
> >   $ kpatch load 6.9.0-rc1+/livepatch-test_0.ko
> >   loading patch module: 6.9.0-rc1+/livepatch-test_0.ko
> >   waiting (up to 15 seconds) for patch transition to complete...
> >   transition complete (2 seconds)
> >
> >   $ kpatch list
> >   Loaded patch modules:
> >   livepatch_test_0 [enabled]
> >
> >   $ lsmod |grep livepatch
> >   livepatch_test_0       16384  1
> >
> > - Load a new livepatch
> >   $ kpatch load 6.9.0-rc1+/livepatch-test_1.ko
> >   loading patch module: 6.9.0-rc1+/livepatch-test_1.ko
> >   waiting (up to 15 seconds) for patch transition to complete...
> >   transition complete (2 seconds)
> >
> >   $ kpatch list
> >   Loaded patch modules:
> >   livepatch_test_1 [enabled]
> >
> >   $ lsmod |grep livepatch
> >   livepatch_test_1       16384  1
> >   livepatch_test_0       16384  0   <<<< leftover
> >
> > With this improvement, executing
> > `kpatch load 6.9.0-rc1+/livepatch-test_1.ko` will automatically remove the
> > livepatch-test_0.ko module.
>
> As already mentioned by Joe, please replace "kpatch" with
> the related "modprobe" and "echo 0 >/sys/kernel/livepatch/<name>/enable"
> calls.
>
> "kpatch" is a 3rd party tool and only few people know what it does
> internally. The kernel commit message is there for current and future
> kernel developers. They should be able to understand the behavior
> even without digging details about "random" user-space tools.

will do it.

>
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -721,8 +723,13 @@ 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 (module_refcount(mod))
> > +                     return;
> > +             mod->state = MODULE_STATE_GOING;
>
> mod->state should be modified only by the code in kernel/module/.
> It helps to keep the operation safe (under control of module
> loader code maintainers).
>
> The fact that this patch does the above without module_mutex is
> a nice example of possible mistakes.
>
> And there are more problems, see below.
>
> > +             delete_module(mod);
>
> klp_free_patch_finish() is called also from the error path
> in klp_enable_patch(). We must not remove the module
> in this case. do_init_module() will do the clean up
> the right way.

Thanks for pointing it out. will fix it.

>
> > +     }
> >  }
> >
> >  /*
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index e1e8a7a9d6c1..e863e1f87dfd 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -695,12 +695,35 @@ EXPORT_SYMBOL(module_refcount);
> >  /* This exists whether we can unload or not */
> >  static void free_module(struct module *mod);
> >
> > +void delete_module(struct module *mod)
> > +{
> > +     char buf[MODULE_FLAGS_BUF_SIZE];
> > +
>
> If we export this API via include/linux/module.h then
> it could be used anywhere in the kernel. Therefore we need
> to make it safe.
>
> This function should do the same actions as the syscall
> starting from:
>
>         mutex_lock(&module_mutex);
>
>         if (!list_empty(&mod->source_list)) {
>                 /* Other modules depend on us: get rid of them first. */
>                 ret = -EWOULDBLOCK;
>                 goto out;
>         }
> ...

good suggestion. will do it.


-- 
Regards
Yafang