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

[Josh Poimboeuf]

On Wed, Apr 11, 2018 at 04:17:11PM +0200, Petr Mladek wrote:
> > 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.

You're confusing the func stack with the patch stack.

My proposal is to get rid of the patch stack.

We can keep the func stack.  It will be needed anyway for the 'replace'
case, where the stack may be of size 2.

> 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.

We don't need to enforce that.  The func stack can stay.  If somebody
wants to patch the same function multiple times (without using
'replace'), that's inadvisable, but it's also their business.  They're
responsible for the tooling to ensure the patch stack order is sane.

> > > > > 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.

If they are permanently disabled then I think the sysfs entries should
be removed.

> We could remove them when the patch gets disabled (transition
> finishes). Then we do not need to do anything in module_exit().

Agreed, though what happens if the transition finishes while still
running in the context of the write to the sysfs entry?  Can we remove a
sysfs entry while executing code associated with it?

-- 
Josh