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

[Miroslav Benes]

On Tue, 10 Apr 2018, Josh Poimboeuf wrote:

> On Tue, Apr 10, 2018 at 10:34:55AM +0200, Petr Mladek wrote:
> > > > > > > We were just recently discussing the possibility of not allowing the
> > > > > > > disabling of patches at all.  If we're not going that far, let's at
> > > > > > > least further restrict it, for the sanity of our code, so we don't have
> > > > > > > to worry about a nasty mismatched stack of enabled/disabled/enabled/etc,
> > > > > > > at least for the cases where 'replace' patches aren't used.
> > > > > > 
> > > > > > I am not completely sure where all these fears come from. From my
> > > > > > point of view, this change is pretty safe and trivial thanks to NOPs
> > > > > > and overall design. It would be a shame to do not have it. But I
> > > > > > might be blind after spending so much time with the feature.
> > > > > 
> > > > > I think you're missing my point.  This isn't about your patch set, per
> > > > > se.  It's really about our existing code.  Today, our patch stack
> > > > > doesn't follow real stack semantics, because patches in the middle might
> > > > > be disabled.  I see that as a problem.
> > > 
> > > No, please read it again.  I wasn't talking about replaced patches.
> > 
> > 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.
 
> > Do I understand it correctly that you do not like that the patches
> > on the stack might be in two states (disabled/enabled). This might
> > be translated that you do not like the state when the patch is
> > registered and disabled.
> 
> No, that wasn't really what I meant, but I have often wondered whether
> we need such a distinction.
> 
> > I wonder if the problem is in the "stack" abstraction. Would it help
> > if we call it "sorted list" or whatever more suitable?
> 
> But I don't even see a reason to have a sorted list (more on that
> below).
> 
> > 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.

> > Or we could rename the operation do add/remove or anything else. In
> > fact, this is how it worked in kGraft.
> 
> I'm not sure what renaming would solve, unless you mean to combine
> registration and enablement into a single concept?  Sounds good to me.
> 
> > AFAIK, the enable/disabled state made more sense for immediate
> > patches that could not be unloaded. We still need to keep the patches
> > when the transaction is forced. The question is if we need to keep
> > the sysfs entries for loaded but unused patches.
> 
> I think we wouldn't need the sysfs entries.  Just disable/unregister
> forced patches like normal, except skipping the module_put().
> 
> > > Either way, why do we still need a stack?
> > 
> > Good question. I suggest to agree on some terms first:
> > 
> >    + Independent patches make unrelated changes. Any new function
> >      must not rely on changes done by any other patch.
> > 
> >    + Dependent patches mean that a later patch depend on changes
> >      done by an earlier patch. For example, a new function might
> >      use function added by an earlier patch.
> > 
> >    + Each cumulative patch include all necessary changes. I would say
> >      that it is self-containing and independent. Except that they should
> >      be able to continue using changes made by earlier patches (shadow
> >      variables, callbacks).
> > 
> > 
> > Then we could say:
> > 
> >    + The stack helps to enforce dependencies between dependent
> >      patches. But there is needed also some external solution
> >      that forces loading the patches in the right order.
> > 
> >    + The "replace" feature is useful for cumulative patches.
> >      It allows to remove existing changes and remove unused stuff.
> >      But cumulative patches might be done even _without_ the atomic
> >      replace.
> > 
> >    + Cumulative patches _with_ "replace" flag might be in theory
> >      installed in random order because they handle not-longer patched
> >      functions. Well, some incompatibility might be caused by shadow
> >      variables and callbacks. Therefore it still might make sense
> >      to install them in the right order.
> > 
> >      Cumulative patches _without_ "replace" flag must be installed
> >      in the right order because they do not handle not-longer patched
> >      functions.
> > 
> >      Anyway, for cumulative patches is important the external
> >      ordering (loading modules) and not the stack.
> > 
> > 
> > Back to your question:
> > 
> > The stack is needed for dependent non-cumulative patches.
> > 
> > The cumulative patches with "replace" flag seems the be
> > the most safe and most flexible solution. The question is
> > if we want to force all users to use this mode.
> 
> If they have dependencies between modules, they can either a) enforce it
> with tooling, or they can instead b) use 'replace'.
> 
> But let's get the module load order enforcement out of the kernel.
> There's no real need for the kernel to do it, and we're not even doing a
> good job at it.
> 
> > > > Or we have replace patches that are
> > > > standalone and we allow a smooth transfer from one to another one.
> > > > 
> > > > Anyway, for us, it is much more important the removal of replaced
> > > > patches. We could probably live without the possibility to replace
> > > > disabled patches.
> > > 
> > > I think replacing disabled patches is ok, *if* we get rid of the
> > > illusion of a stack.  The current stack isn't really a stack, it's just
> > > awkward.
> > 
> > I personally do not have problems with it. As I said, I see this as
> > two different modes how the life patches are distributed. The stack
> > is needed for dependent patches. The cumulative patches with
> > "replace" flag are self-contained and independent. They might
> > replace anything.
> > 
> > Well, it would make sense to reduce the amount of possible
> > situations and use cases.
> 
> A big +1.
> 
> > The question is what is acceptable to others
> 
> If there are any objections, this is their chance to speak up :-)
> 
> > and if it needs to be done as part of this patch set.
> 
> Maybe so, for at least a few reasons:
> 
> - This patch set makes the 'stack' obsolete, so it makes sense to remove
>   the 'stack' with it.

Not necessarily. I like Petr's rebase explanation here.

Miroslav
 
> - This patch set will already affect tooling, let's make tooling's life
>   easier by making all the related changes at the same time.
>   
>   (Though I'm not quite convinced on this point, would removing the
>   stack affect tooling at all?  If we also combined registration and
>   enablement into a single concept, then it definitely would.)
> 
> -- 
> Josh
>