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

[Petr Mladek <pmladek@suse.cz>]

On Tue 2015-03-10 12:53:21, Rusty Russell wrote:
> Petr Mladek <pmladek@suse.cz> writes:
> > On Sat 2015-03-07 11:34:36, Rusty Russell wrote:
> >> 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.
> 
> I don't understand why you don't just stop modules.  I'm happy to write
> "int stop_module_changes() / void restart_module_changes()" for you.
> 
> This is far far simpler.  Stop module changes before you start patching.
> Restart after it's done.

Interesting idea. stop_module_changes() would need to prevent anyone
from entering COMING and GOING module states. Also it would need to
wait for other modules to leave these states.

As you mentioned later, we would need to exclude the current module
because patches are added by a module and stop_module_changes() would
need to be called from mod->init() script.

In fact, we would need to exclude all modules that called
stop_module_changes() to prevent a deadlock.


> Is your intent to apply patches to modules which are applied (long)
> after the original patch?

Yes, we want to apply already loaded patches to coming modules.

> Or leave that problem to userspace (ie. assume you've updated the
> on-disk modules)?

It would make things too complicated for our use case. We allow to
install any kernel build in parallel with another kernel build. It makes
it easier to reboot with the old working kernel if things went wrong.
Most (default) modules are distributed with the kernel, so we
would need to update the kernel as well and make it special to override
the patched one.

Also it would create a strange mix. The kernel would be able to
load modules from different builds. Therefore crashdump would be much
harder to analyze.

Another problem will be with the module providing the patch. It will
be needed for the original kernel but it might[*] be incompatible with
the fixed one.

[*] There is a possibility to hardcode relocation tables. It is handy
    when the patch is generated automatically or when the patch need
    to modify a module function with an ambiguous name.

> >> 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.
> 
> You grab the module mutex using stop_module_changes() before anything
> else.
> 
> Or are you using the "failed module loading" hack to apply patches?
> That would imply that the current module would have to be excluded
> from the stop_module_changes() check, but should still be possible.

Yes, we will need to be careful to avoid deadlocks. I think that
stop_module_changes/restart_module_changes feature makes sense only if
it will have more users. Otherwise, the approach with the extra flag
looks much easier to me. Note that there is only one boolean/bit
needed with the last version of the patch.

Thanks a lot for review and feedback. I could try to implement it
another way if you give me hints.

Best Regards,
Petr