Re: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

[Miroslav Benes]

On Tue, 10 Oct 2017, Josh Poimboeuf wrote:

> On Tue, Oct 10, 2017 at 11:15:33AM -0400, Jason Baron wrote:
> > 
> > 
> > On 10/06/2017 05:22 PM, Josh Poimboeuf wrote:
> > > On Wed, Sep 27, 2017 at 11:41:29PM -0400, Jason Baron wrote:
> > >> In preparation to introducing atomic replace, introduce iterators for
> > >> klp_func and klp_object, such that objects and functions can be dynamically
> > >> allocated (needed for atomic replace). This patch is intended to
> > >> effectively be a no-op until atomic replace is introduced.
> > > 
> > > Hi Jason,
> > > 
> > > Sorry for coming in so late on this.  This will be a nice feature.  I
> > > haven't gotten to the second patch yet, but I have a few comments
> > > already.
> > > 
> > 
> > 
> > Hi Josh,
> > 
> > > 1) Having both static *and* dynamic lists seems to add a lot of
> > >    complexity and brittleness.
> > > 
> > >    The livepatch API is easier to use with static data structures, so I
> > >    don't think we should change the API.  But I also don't think we
> > >    should allow the API to overly constrain our internal representation
> > >    of the data.
> > > 
> > >    What I'd *really* like to do is completely separate the user-supplied
> > >    data structures from our internal data .  At registration time, we
> > >    can convert all the arrays to dynamic lists, and then never touch the
> > >    arrays again.  Then managing the lists is completely straightforward.
> > > 
> > >    That would also allow us to have an external public struct and an
> > >    internal private struct, so the interface is more clear and we don't
> > >    have to worry about any patch modules accidentally messing with our
> > >    private data.
> > > 
> > 
> > Separating the external/internal data structures makes sense. Another
> > idea here would be to simply walk the static data structures at register
> > time and then add them onto the dynamic lists. Then, all subsequent
> > access can be done using the simple list walks. That would remove the
> > complex iterators here. The external/internal division could still be
> > done later but maybe doesn't couple as much to this patchset...
> 
> Sounds good.

I haven't seen v4 yet, but this seems to be the best approach so far.

I was the one who opposed external/internal division in the past, but 
given atomic replace feature we may end up there in the end. But I'd keep 
it separate.
 
> > > 2) There seems to be a general consensus that the cumulative "replace"
> > >    model is the best one.  In light of that, I wonder if we can simplify
> > >    our code a bit.  Specifically, can we get rid of the func stack?
> > > 
> > >    If the user always uses replace, then the func stack will never have
> > >    more than one func.  And we can reject a patch if the user doesn't
> > >    use replace when they're trying to patch a function which has already
> > >    been patched.
> > > 
> > >    Then the func stack doesn't have to be a stack, it can just be a
> > >    single func.
> > > 
> > >    That would allow us to simplify the code in several places for the
> > >    common case, yet still allow those who want to apply non-cumulative
> > >    patches to do so most of the time.
> > > 
> > >    It's not a *huge* simplification, but we've already made livepatch so
> > >    flexible that it's too complex to fit everything in your head at the
> > >    same time, and any simplification we can get away with is well worth
> > >    it IMO.  And once we have replace, I wonder if anybody would really
> > >    miss the func stack.
> > > 
> > 
> > I think that means instead of having a list of functions or the func
> > stack, you have 2 pointers - one to the active patch and one to the
> > previous patch. Maybe it would make sense to do in steps? IE have this
> > patchset reject patches that patch a patch to existing function, leaving
> > the func stack. And if that doesn't break any use-cases, switch to a
> > model where there is just an active and previous patch?
> 
> Hm.  Maybe I didn't think it through.  I guess the func stack could have
> at most *two* funcs -- not one.  Let's just forget this idea for now...

Yes, there would still be some kind of stacking needed just to make atomic 
replace work (if I understand you correctly).

Moreover, I'd be hesitant to remove stacking. Yes, we use cumulative 
patches at SUSE, because they correspond well to our particular use case. 
Some other company or even an ordinary user of linux might do things 
differently. If we removed it now, it would be almost impossible to add it 
later, because it is so complex. So if there's way to preserve it, I'd 
keep. As long as possible.

Regards,
Miroslav