On Thu, Apr 4, 2024 at 10:04 PM Petr Mladek <[email protected]> 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
