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);
