Re: [RFC PATCH 2/2] livepatch: Implement livepatch hybrid mode

[Yafang Shao]

On Fri, Feb 7, 2025 at 9:58 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Thu 2025-02-06 10:35:11, Yafang Shao wrote:
> > On Thu, Feb 6, 2025 at 12:03 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
> > >
> > > On Wed 2025-02-05 10:54:47, Yafang Shao wrote:
> > > > On Tue, Feb 4, 2025 at 9:21 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
> > > > >
> > > > > On Mon 2025-01-27 23:34:50, Yafang Shao wrote:
>
> I am not sure if you still would want the hybrid model.

I believe the ability to selectively replace specific livepatches will
be a highly valuable feature.

> It is possible that the timeout in klp_try_complete_transition()
> would remove the hardlockup watchdog warnings, see
> https://lore.kernel.org/r/Z6Tmqro6CSm0h-E3@xxxxxxxxxxxxxxx
>
> I reply to this mail just for record because there were some
> unanswered questions...
>
> > > > To clarify the hybrid model, I'll illustrate it with the following Fixes:
> > > >
> > > > Fix1 livepatches: funcA
> > > > Fix2  livepatches: funcC
> > > > Fix3 livepatches: funcA, funcB
> > >
> > > How does look the livepatched FuncA here?
> > > Does it contain changes only for Fix3?
> > > Or is it cummulative livepatches_funA includes both Fix1 + Fix3?
> >
> > It is cumulative livepatches_funA includes both Fix1 + Fix3.
>
> It makes sense.
>
> I have missed this in the previous mail. I see it there now (after
> re-reading). But trick was somehow encrypted in long sentences.
>
>
> > > > Fix4  livepatches: funcD
> > > > Fix5 livepatches: funcB
> > > >
> > > > Each FixN represents a single /sys/kernel/livepatch/FixN.
> > > > By default, all Fixes are set to non-replaceable.
> > > >
> > > > Step-by-step process:
> > > > 1. Loading Fix1
> > > >    Loaded Fixes: Fix1
> > > > 2. Loading Fix2
> > > >    Loaded Fixes: Fix1 Fix2
> > > > 3. Loading Fix3
> > > >     Before loading, set Fix1 to replaceable and replace it with Fix3,
> > > > which is a cumulative patch of Fix1 and Fix3.
> > >
> > > Who will modify the "replaceable" flag of "Fix1"? Userspace or kernel?
> >
> > Userspace scripts. Modify it before loading a new one.
>
> This is one mine concern. Such a usespace script would be
> more complex for the the hybrid model then for cumulative livepatches.
> Any user of the hybrid model would have their own scripts
> and eventual bugs.

In the old model, we maintained a large script to deploy individual
livepatches. In the atomic replace model, we maintain another complex
script to deploy cumulative livepatches. We always end up creating our
own scripts to adapt to the complexities of the production
environment.

>
> Anyway, the more possibilities there more ways to break things
> and the more complicated is to debug eventual problems.

We still have some servers running the old 4.19 kernel, with over 20
livepatches deployed. These livepatches are managed using the old
model since the atomic replace model isn't supported yet. The
maintenance of these livepatches has been running smoothly with user
scripts. Things would be much simpler if we could rely on user scripts
to handle this process.

>
> If anyone would still like the hybrid model then I would like
> to enforce some safe behavior from the kernel. I mean to do
> as much as possible in the common (kernel) code.
>
> I have the following ideas:
>
>   1. Allow to load a livepatch with .replace enabled only when
>      all conflicting[*] livepatches are allowed to be replaced.

Makes sense.

>
>   2. Automatically replace all conflicting[*] livepatches.

Makes sense.

>
>   3. Allow to define a list of to-be-replaced livepatches
>      into struct patch.

Seems like a great idea.

>
>
> The 1st or 2nd idea would make the hybrid model more safe.
>
> The 2nd idea would even work without the .replaceable flag.
>
> The 3rd idea would allow to replace even non-conflicting[*]
> patches.
>
> [*] I would define that two livepatches are "conflicting" when
>     at least one function is modified by both of them. e.g.
>
>         + Patch1: funcA, funcB   \
>         + Patch2: funcC, funcD   - non-conflicting
>
>         + Patch1: funcA, funcB          \
>         + Patch2:        funcB, funcC   - conflicting
>
>     Or a bit weaker definition. Two patches are "conflicting"
>     when the new livepatch provides only partial replacement
>     for already livepatch functions, .e.g.
>
>         + Patch1: funcA, funcB                \
>         + Patch2:               funcC, funcD  - non-conflicting anyway
>
>         + Patch1: funcA, funcB          - Patch1 can't replace Patch2 (conflict)
>         + Patch2: funcA, funcB, funcC   - Patch2 can replace Patch1 (no conflict)
>
>         + Patch1: funcA, funcB          \
>         + Patch2:        funcB, funcC   - conflicting anyway
>
>
> Especially, the automatic replacement of conflicting patches might
> make the hybrid model safe and easier to use. And it would resolve
> most of my fears with this approach.

Many thanks for your suggestion. It’s been incredibly helpful. I’ll
definitely give it some thought.

-- 
Regards
Yafang