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

[Jason Baron <jbaron@akamai.com>]

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

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

Thanks,

-Jason