Re: [PATCH v10 05/10] livepatch: Support separate list for replaced patches.

[Petr Mladek]

On Wed 2018-04-11 07:32:14, Josh Poimboeuf wrote:
> On Wed, Apr 11, 2018 at 10:07:31AM +0200, Miroslav Benes wrote:
> > > > I was confused by wording "in the middle". It suggested that there
> > > > might had been enabled patches on the top and the bottom of the stack
> > > > and some disabled patches in between at the same time (or vice versa).
> > > > This was not true.
> > > 
> > > That *was* what I meant.  Consider the following sequence of events:
> > > 
> > > - Register patch 1
> > > - Enable patch 1
> > > - Register patch 2
> > > - Enable patch 2
> > > - Disable patch 2
> > > - Register patch 3
> > > - Enable patch 3
> > > 
> > > Notice that patch 2 (in the middle) is disabled, whereas patch 1 (on the
> > > bottom) and patch 3 (on the top) are enabled.
> > 
> > This should not be possible at all.
> > 
> > __klp_enable_patch:
> > 
> >         if (patch->list.prev != &klp_patches &&
> >             !list_prev_entry(patch, list)->enabled)
> >                 return -EBUSY;
> > 
> > When patch 3 is enabled, list_prev_entry() returns patch 2 and its 
> > ->enabled is false.
> 
> Hm, you're right.  I'm not sure how I got that idea...

It is great that we finally understand each other.

> I still agree with my original conclusion that enforcing stack order no
> longer makes sense though.

The question is what we will get if we remove the stack. Will it
really make the code easier and livepatching more safe?

First, note that stack is the main trick used by the ftrace handler.
It gets the patch from top of the stack and use the new_addr from
that patch.

If we remove the stack, we still need to handle 3 possibilities.
The handler will need to continue either with original_code,
old_patch or new_patch.

Now, just imagine the code. We will need variables orig_addr,
old_addr, new_addr or so which might be confusing. It will be
even more confusion if we do revert/disable. Also new_addr will
become old_addr if we install yet another patch.

We had exactly this in kGraft and it was a mess. I said "wow,
that is genius" when I saw the stack approach in the upstream
code proposal.


Second, unrelated patches must never patch the same functions.
Otherwise we would not be able to define which implementation
should be used. This is especially important when a patch is
removed and we need to fallback either to another patch or
original code. Yes, it makes perfect sense. But it needs code
that will check it, refuse loading the patch, ... It is not
complicated. But it is rather additional code than
simplification. I might make livepatching more safe
but probably not simplify the code.


> > > > Another possibility would be to get rid of the enable/disable states.
> > > > I mean that the patch will be automatically enabled during
> > > > registration and removed during unregistration.
> > > 
> > > I don't see how disabling during unregistration would be possible, since
> > > the unregister is called from the patch module exit function, which
> > > can only be called *after* the patch is disabled.
> > > 
> > > However, we could unregister immediately after disabling (i.e., in
> > > enabled_store context).
> > 
> > I think this is what Petr meant. So there would be nothing in the patch 
> > module exit function. Well, not exactly. We'd need to remove sysfs dir and 
> > maybe something more.
> 
> Sounds good to me, though aren't the livepatch sysfs entries removed by
> klp during unregister?

This is why I asked in my earlier mail if we need to keep sysfs
entries for unused patches. We could remove them when the patch
gets disabled (transition finishes). Then we do not need to
do anything in module_exit().

The patch module can be removed only when the transition is not
forced and we call module_put().

Best Regards,
Petr