Re: [PATCH v2 2/2] livepatch/module: Correctly handle going modules

[Petr Mladek <pmladek@suse.cz>]

On Sat 2015-03-07 11:34:36, Rusty Russell wrote:
> Petr Mladek <pmladek@suse.cz> writes:
> > Existing live patches are removed from going modules using a notify handler.
> > There are two problems with the current implementation.
> >
> > First, new patch could still see the module in the GOING state even after
> > the notifier has been called. It will try to initialize the related
> > object structures but the module could disappear at any time. There will
> > stay mess in the structures. It might even cause an invalid memory access.
> >
> > Second, if we start supporting patches with semantic changes between function
> > calls, we would need to apply any new patch even for going modules. Note that
> > the code from the module could be called even in the GOING state until
> > mod->exit() finishes. See below for example.
> 
> I don't think you should handle going modules at all.  Rarely happens,
> and it should happen fast.

I would like to handle it correctly. It would be pity to break a system
just because of a module removal. Also the extra overhead will be
very small and it will happen only very rarely.

We will apply one new patch and remove it quickly after that. But this
will happen only when a module is removed and a patch is added at at
the "same" time.


> If you can hold the module_lock, the easiest thing to do is have us wake
> module_wq when a module is freed, then you can just:

Unfortunately, we could not use a waitqueue easily. We would need to
release klp_mutex to do not block going modules. But we could not
do so in the middle of a patch adding.

BTW: It seems that module_wq is used for coming modules. We could not
use it for coming modules from the same reason. In addition, waiters
are weaken after mod->init(). But we would need to apply the patch
before mod->init() to avoid any inconsistency.

Anyway, thanks for feedback.

Best Regards,
Petr

 
>         retry:
>                 err = wait_event_interruptible(module_wq,
>                                                !modules_unloading());
>                 if (err)
>                         goto out;
> 
>                 /* Now re-check under lock. */
>                 mutex_lock(&module_lock);
>                 if (unlikely(modules_unloading()) {
>                         mutex_unlock(&module_lock);
>                         goto retry;
>                 }
> 
> Cheers,
> Rusty.
> 
>