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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Apr 4, 2024 at 10:04 PM Petr Mladek <pmladek@xxxxxxxx> 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





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux