Re: unload and reload modules with patched function

[Petr Mladek <pmladek@suse.com>]

On Wed 2022-07-20 23:57:25, Song Liu wrote:
> Hi folks,
> 
> While testing livepatch kernel modules, we found that if a kernel module has
> patched functions, we cannot unload and load it again (rmmod, then insmod).
> This hasn't happened in production yet, but it feels very risky. We use
> automation (chef to be specific) to handle kernel modules and livepatch.
> It is totally possible some weird sequence of operations would cause insmod
> errors on thousands of servers. Therefore, we would like to fix this issue
> before it hits us hard.
> 
> A bit of searching with the error message shows it was a known issue [1], and
> a few options are discussed:
> 
> "Possible ways to fix it:
> 
> 1) Remove the error check in apply_relocate_add().  I don't think we
>    should do this, because the error is actually useful for detecting
>    corrupt modules.  And also, powerpc has the similar error so this
>    wouldn't be a universal solution.
> 
> 2) In klp_unpatch_object(), call an arch-specific arch_unpatch_object()
>    which reverses any arch-specific patching: on x86, clearing all
>    relocation targets to zero; on powerpc, converting the instructions
>    after relative link branches to nops.  I don't think we should do
>    this because it's not a global solution and requires fidgety
>    arch-specific patching code.
> 
> 3) Don't allow patched modules to be removed.  I think this makes the
>    most sense.  Nobody needs this functionality anyway (right?).
> "

Just for completeness there is one more possibility. We have sometimes
discussed a split of the livepatch module into per-object
(vmlinux + per-module) modules. So that modules can be loaded and
unloaded together with the respective livepatch counter parts.

I have played with this idea some (years) ago. It was quite
complicated because of the consistency model. If I remember correctly
the main challenges were:

1. The livepatch module must be loaded together with all related
   livepatch modules for all loaded modules before the transition
   is started.

2. If any module is loaded then it must wait in MODULE_STATE_COMMING
   until the related livepatch module is loaded and the livepatch
   ftrace callbacks applied.

3. The naming is a nightmare.


Ad 1. and 2.: It needs some "hacks" in the module loader. It requires
    calling modprobe from kernel code which some people hate.

Ad 3: Livepatch is a module. The per-object livepatch is set of related
    modules. The livepatch modules do livepatch vmlinux and "normal"
    modules. It is easy to get lost in the terms. Especially it hard
    to distinguish "livepatched modules" and "livepatch modules"
    in code (variable and function names) and comments.


I have never published the POC because it was not finished and it got
less important after removing the most of the arch-specific code.
I could put it somewhere when anyone is interested.

Anyway, I think that it is _not_ the way to go. IMHO, the split
livepatch modules bring more problems than they solve.

Best Regards,
Petr