Message-ID: <20181217162729.orxq6i53m4kgekp3@treble>
Date:   Mon, 17 Dec 2018 10:27:29 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Jiri Kosina <jikos@...nel.org>, Miroslav Benes <mbenes@...e.cz>,
        Jason Baron <jbaron@...mai.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v14 10/11] livepatch: Remove ordering and refuse loading
 conflicting patches

On Mon, Dec 17, 2018 at 05:07:09PM +0100, Petr Mladek wrote:
> On Thu 2018-12-13 17:06:52, Josh Poimboeuf wrote:
> > On Thu, Nov 29, 2018 at 10:44:30AM +0100, Petr Mladek wrote:
> > > The atomic replace and cumulative patches were introduced as a more secure
> > > way to handle dependent patches. They simplify the logic:
> > > 
> > >   + Any new cumulative patch is supposed to take over shadow variables
> > >     and changes made by callbacks from previous livepatches.
> > > 
> > >   + All replaced patches are discarded and the modules can be unloaded.
> > >     As a result, there is only one scenario when a cumulative livepatch
> > >     gets disabled.
> > > 
> > > The different handling of "normal" and cumulative patches might cause
> > > confusion. It would make sense to keep only one mode. On the other hand,
> > > it would be rude to enforce using the cumulative livepatches even for
> > > trivial and independent (hot) fixes.
> > > 
> > > This patch removes the stack of patches. The list of enabled patches
> > > is still needed but the ordering is not longer enforced.
> > > 
> > > Note that it is not possible to catch all possible dependencies. It is
> > > the responsibility of the livepatch authors to decide.
> > > 
> > > Nevertheless this patch prevents having two patches for the same function
> > > enabled at the same time after the transition finishes. It might help
> > > to catch obvious mistakes. But more importantly, we do not need to
> > > handle situation when a patch in the middle of the function stack
> > > (ops->func_stack) is being removed.
> > 
> > I'm not sure about this patch.  I like the removal of the stacking.  But
> > do we really want to enforce no dependencies between non-cumulative
> > patches?  It can be done correctly if the user is careful.
> > 
> > Maybe we should just let users do it if they want to.  And then that
> > also would mean less code for us to maintain.
> > 
> > And as usual, I apologize if I'm either contradicting or repeating past
> > versions of myself. :-)
> 
> This patch was actually motivated by you. On some conference, we
> discussed how to automatize the creation of livepatches. You wanted
> to make livepatching more safe in general (by tools, by checks, ...).
> Also you always wanted to make things easier and reduce possible
> scenarios. I thought that this might be in line with your wishes.
> 
> The problem with this patch is that it forces people to use
> cumulative patches. I am not sure if everyone is ready for it.
> 
> I do not resist on it. But I still think that it makes sense.

I do remember suggesting the removal of the stacking.  I think that's a
good idea.

I don't remember suggesting the other part: trying to detect and prevent
dependencies for non-replace users.  If I did suggest that, which is
very possible, I apologize for being wishy-washy :-)

The way I currently see it, there are two classes of users: cumulative
and non-cumulative.  IMO we should accept both as reasonable
possiblities.

Cumulative users will use 'replace'.  Non-cumulative users will do
whatever they want, and we shouldn't try to restrict them.

So I would propose that we remove the stacking, and not try to enforce
patch dependencies in any way.

-- 
Josh

Powered by blists - more mailing lists