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

[Josh Poimboeuf]

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.

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

-- 
Josh