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