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

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

 



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






[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