[prev in list] [next in list] [prev in thread] [next in thread]
List: live-patching
Subject: Re: unload and reload modules with patched function
From: Song Liu <songliubraving () fb ! com>
Date: 2022-07-22 16:53:28
Message-ID: 914D9BAD-40D8-4D3C-BAE9-44B00222F296 () fb ! com
[Download RAW message or body]
Hi Petr,
> On Jul 22, 2022, at 3:40 AM, Petr Mladek <pmladek@suse.com> wrote:
>
> 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.
+1 on naming is so hard for this.
>
>
> 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.
Thanks for sharing these experiences! Let's hope option 2) or 3)
would fix the issue.
Song
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic