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

[Joe Lawrence <joe.lawrence@redhat.com>]

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