Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1690635 > unrolled thread

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-07-18 22:30 +0200
    Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-19 04:30 +0200
    Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Miroslav Benes <mbenes@suse.cz> - 2017-07-19 21:10 +0200
      Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Miroslav Benes <mbenes@suse.cz> - 2017-07-20 16:50 +0200
        Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-07-20 17:50 +0200
          Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-20 22:30 +0200
          Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Petr Mladek <pmladek@suse.com> - 2017-07-21 10:50 +0200
          Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Miroslav Benes <mbenes@suse.cz> - 2017-07-21 11:10 +0200

#1690635 — Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

On Mon, Jul 17, 2017 at 05:29:41PM +0200, Miroslav Benes wrote:
>
> On Wed, 28 Jun 2017, Joe Lawrence wrote:
> 
> > +Brief API summary
> > +-----------------
> > + [ ... snip ...]
> > +* klp_shadow_detach() - detach and free all <*, num> shadow variables
> > +  - find and remove any <*, num> references from hashtable
> > +    - if found, release shadow variable
> 
> I think that the second one should be klp_shadow_detach_all(), shouldn't 
> it?

Good catch, I'll fixup in v3.

> > +static DEFINE_HASHTABLE(klp_shadow_hash, 12);
> 
> Is there a reason, why you pick 12? I'm just curious.

The hashtable bit-size was inherited from the kpatch implementation.
Perhaps Josh knows why this value was picked?

Aside: we could have per-livepatch hashtables if that was desired, this
value could be then adjusted accordingly.  We haven't needed them for
kpatch, so I didn't see good reason to complicate things.

> > +static DEFINE_SPINLOCK(klp_shadow_lock);
> > +
> > +/**
> > + * struct klp_shadow - shadow variable structure
> > + * @node:	klp_shadow_hash hash table node
> > + * @rcu_head:	RCU is used to safely free this structure
> > + * @obj:	pointer to original data
> > + * @num:	numerical description of new data
> 
> Josh proposed better description. Could we also have a note somewhere in 
> the documentation what this member is practically for? I mean versioning 
> and ability to attach new members to a data structure if live patches are 
> stacked.

That's a good idea and I posted a sample doc-blurb in my other reply to
Petr about terminology.

> > + * @new_data:	new data area
> > + */
> > +struct klp_shadow {
> > +	struct hlist_node node;
> > +	struct rcu_head rcu_head;
> > +	void *obj;
> > +	unsigned long num;
> > +	char new_data[];
> > +};
> 
> What is the reason to change 'void *new_data' to 'char new_data[]'? I 
> assume it is related to API changes below...
> 
> [...]
> 
> > +/**
> > + * _klp_shadow_attach() - allocate and add a new shadow variable
> > + * @obj:	pointer to original data
> > + * @num:	numerical description of new data
> > + * @new_data:	pointer to new data
> > + * @new_size:	size of new data
> > + * @gfp_flags:	GFP mask for allocation
> > + * @lock:	take klp_shadow_lock during klp_shadow_hash operations
> 
> I am not sure about lock argument. Do we need it? Common practice is to 
> have function foo() which takes a lock, and function __foo() which does 
> not.
> 
> In klp_shadow_get_or_attach(), you use it as I'd expect. You take the 
> spinlock, call this function and release the spinlock. Is it possible 
> to do the same in klp_shadow_attach() and have __klp_shadow_attach() 
> without lock argument?

Yes, this would be possible, though it would restrict
klp_shadow_attach() from accepting gfp_flags that might allow for
sleeping.  More on that below ...
 
> > + *
> > + * Note: allocates @new_size space for shadow variable data and copies
> > + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> > + * space.  If @new_data is NULL, @new_size is still allocated, but no
> > + * copy is performed.
> 
> I must say I'm not entirely happy with this. I don't know if this is what 
> Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> memcpy instead of a simple assignment in v1 seems worse. 

This change was a bit of a experiment on my part in reaction to
adding klp_shadow_get_or_attach().

I like the simplicity of v1's pointer assignment -- in fact, moving all
allocation responsiblity (klp_shadow meta-data and data[] area) out to
the caller is doable, though implementing klp_shadow_get_or_attach() and
and klp_shadow_detach_all() complicates matters, for example, adding an
alloc/release callback.  I originally attempted this for v2, but turned
back when the API and implementation grew complicated.  If the memcpy
and gfp_flag restrictions are too ugly, I can try revisting that
approach.  Ideas welcome :)

Regards,

-- Joe

[toc] | [next] | [standalone]

#1690914

On Tue, Jul 18, 2017 at 04:21:07PM -0400, Joe Lawrence wrote:
> On Mon, Jul 17, 2017 at 05:29:41PM +0200, Miroslav Benes wrote:
> >
> > On Wed, 28 Jun 2017, Joe Lawrence wrote:
> > 
> > > +Brief API summary
> > > +-----------------
> > > + [ ... snip ...]
> > > +* klp_shadow_detach() - detach and free all <*, num> shadow variables
> > > +  - find and remove any <*, num> references from hashtable
> > > +    - if found, release shadow variable
> > 
> > I think that the second one should be klp_shadow_detach_all(), shouldn't 
> > it?
> 
> Good catch, I'll fixup in v3.
> 
> > > +static DEFINE_HASHTABLE(klp_shadow_hash, 12);
> > 
> > Is there a reason, why you pick 12? I'm just curious.
>
> The hashtable bit-size was inherited from the kpatch implementation.
> Perhaps Josh knows why this value was picked?

My thinking was that it gives you about 4096 unique hash table entries
for 32k of RAM.  It was a rough guess.  It's hard to really predict what
size you need.

> Aside: we could have per-livepatch hashtables if that was desired, this
> value could be then adjusted accordingly.  We haven't needed them for
> kpatch, so I didn't see good reason to complicate things.

I think a global hash table is much better because it allows you to deal
more gracefully with patch upgrades.

> > > + *
> > > + * Note: allocates @new_size space for shadow variable data and copies
> > > + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> > > + * space.  If @new_data is NULL, @new_size is still allocated, but no
> > > + * copy is performed.
> > 
> > I must say I'm not entirely happy with this. I don't know if this is what 
> > Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> > memcpy instead of a simple assignment in v1 seems worse. 
> 
> This change was a bit of a experiment on my part in reaction to
> adding klp_shadow_get_or_attach().
> 
> I like the simplicity of v1's pointer assignment -- in fact, moving all
> allocation responsiblity (klp_shadow meta-data and data[] area) out to
> the caller is doable, though implementing klp_shadow_get_or_attach() and
> and klp_shadow_detach_all() complicates matters, for example, adding an
> alloc/release callback.  I originally attempted this for v2, but turned
> back when the API and implementation grew complicated.  If the memcpy
> and gfp_flag restrictions are too ugly, I can try revisting that
> approach.  Ideas welcome :)

Personally I'm not a fan of the callbacks, I like the v2 API.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1692116

> > > +/**
> > > + * _klp_shadow_attach() - allocate and add a new shadow variable
> > > + * @obj:	pointer to original data
> > > + * @num:	numerical description of new data
> > > + * @new_data:	pointer to new data
> > > + * @new_size:	size of new data
> > > + * @gfp_flags:	GFP mask for allocation
> > > + * @lock:	take klp_shadow_lock during klp_shadow_hash operations
> > 
> > I am not sure about lock argument. Do we need it? Common practice is to 
> > have function foo() which takes a lock, and function __foo() which does 
> > not.
> > 
> > In klp_shadow_get_or_attach(), you use it as I'd expect. You take the 
> > spinlock, call this function and release the spinlock. Is it possible 
> > to do the same in klp_shadow_attach() and have __klp_shadow_attach() 
> > without lock argument?
> 
> Yes, this would be possible, though it would restrict
> klp_shadow_attach() from accepting gfp_flags that might allow for
> sleeping.  More on that below ...

Ok, that is a good remark. The problem is that it applies to 
klp_shadow_get_or_attach() too. There you acquire a spin_lock and call 
_klp_shadow_attach() with gfp_flags, which are then used for kzalloc.

I might misread the code. It is getting late here.

> > > + *
> > > + * Note: allocates @new_size space for shadow variable data and copies
> > > + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> > > + * space.  If @new_data is NULL, @new_size is still allocated, but no
> > > + * copy is performed.
> > 
> > I must say I'm not entirely happy with this. I don't know if this is what 
> > Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> > memcpy instead of a simple assignment in v1 seems worse. 
> 
> This change was a bit of a experiment on my part in reaction to
> adding klp_shadow_get_or_attach().
> 
> I like the simplicity of v1's pointer assignment -- in fact, moving all
> allocation responsiblity (klp_shadow meta-data and data[] area) out to
> the caller is doable, though implementing klp_shadow_get_or_attach() and
> and klp_shadow_detach_all() complicates matters, for example, adding an
> alloc/release callback.  I originally attempted this for v2, but turned
> back when the API and implementation grew complicated.  If the memcpy
> and gfp_flag restrictions are too ugly, I can try revisting that
> approach.  Ideas welcome :)

Well, I didn't like callbacks either :). And no, I do not have a better 
idea. I still need to think about it.

Miroslav

[toc] | [prev] | [next] | [standalone]

#1693000

> > > > + *
> > > > + * Note: allocates @new_size space for shadow variable data and copies
> > > > + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> > > > + * space.  If @new_data is NULL, @new_size is still allocated, but no
> > > > + * copy is performed.
> > > 
> > > I must say I'm not entirely happy with this. I don't know if this is what 
> > > Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> > > memcpy instead of a simple assignment in v1 seems worse. 
> > 
> > This change was a bit of a experiment on my part in reaction to
> > adding klp_shadow_get_or_attach().
> > 
> > I like the simplicity of v1's pointer assignment -- in fact, moving all
> > allocation responsiblity (klp_shadow meta-data and data[] area) out to
> > the caller is doable, though implementing klp_shadow_get_or_attach() and
> > and klp_shadow_detach_all() complicates matters, for example, adding an
> > alloc/release callback.  I originally attempted this for v2, but turned
> > back when the API and implementation grew complicated.  If the memcpy
> > and gfp_flag restrictions are too ugly, I can try revisting that
> > approach.  Ideas welcome :)
> 
> Well, I didn't like callbacks either :). And no, I do not have a better 
> idea. I still need to think about it.

Done and I agree that memcpy approach is not so bad after all :). So I'm 
fine with it.

Miroslav

[toc] | [prev] | [next] | [standalone]

#1693038

On 07/20/2017 10:45 AM, Miroslav Benes wrote:
> 
>>>>> + *
>>>>> + * Note: allocates @new_size space for shadow variable data and copies
>>>>> + * @new_size bytes from @new_data into the shadow varaible's own @new_data
>>>>> + * space.  If @new_data is NULL, @new_size is still allocated, but no
>>>>> + * copy is performed.
>>>>
>>>> I must say I'm not entirely happy with this. I don't know if this is what 
>>>> Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
>>>> memcpy instead of a simple assignment in v1 seems worse. 
>>>
>>> This change was a bit of a experiment on my part in reaction to
>>> adding klp_shadow_get_or_attach().
>>>
>>> I like the simplicity of v1's pointer assignment -- in fact, moving all
>>> allocation responsiblity (klp_shadow meta-data and data[] area) out to
>>> the caller is doable, though implementing klp_shadow_get_or_attach() and
>>> and klp_shadow_detach_all() complicates matters, for example, adding an
>>> alloc/release callback.  I originally attempted this for v2, but turned
>>> back when the API and implementation grew complicated.  If the memcpy
>>> and gfp_flag restrictions are too ugly, I can try revisting that
>>> approach.  Ideas welcome :)
>>
>> Well, I didn't like callbacks either :). And no, I do not have a better 
>> idea. I still need to think about it.
> 
> Done and I agree that memcpy approach is not so bad after all :). So I'm 
> fine with it.

I looked at it again this morning and a "pass-your-own" allocation API
always comes back to adding callbacks and other complications :(  In the
end, most callers will be shadowing pointers and not entire structures,
so I think the copy isn't too bad.

On a related note, if we keep the allocations and memcpy, how about I
shift around the attach/get calls like so:

  __klp_shadow_attach
    set shadow variable member values
    memcpy
    add to hash

  klp_shadow_attach
    alloc new shadow var
    lock
    call __klp_shadow_attach with new alloc
    unlock

  klp_shadow_get_or_attach
    be optimistic, call klp_shadow_get (if found, return it)
    be pessimistic, alloc new shadow var
    lock
      call klp_shadow_get again
      if unlikely found
        kfree unneeded alloc
      else
        call __klp_shadow_attach with new alloc
    unlock
    return whichever shadow var we used

This way both calls can accept gfp_flags that may sleep, with the only
downside that klp_shadow_get_or_attach may allocate an unnecessary
shadow variable in the unlikely case that it's found on the second
klp_shadow_get attempt (under the lock).  No more clunky "bool lock"
flag either. :)

-- Joe

[toc] | [prev] | [next] | [standalone]

#1693216

On Thu, Jul 20, 2017 at 11:48:41AM -0400, Joe Lawrence wrote:
> On a related note, if we keep the allocations and memcpy, how about I
> shift around the attach/get calls like so:
> 
>   __klp_shadow_attach
>     set shadow variable member values
>     memcpy
>     add to hash
> 
>   klp_shadow_attach
>     alloc new shadow var
>     lock
>     call __klp_shadow_attach with new alloc
>     unlock
> 
>   klp_shadow_get_or_attach
>     be optimistic, call klp_shadow_get (if found, return it)
>     be pessimistic, alloc new shadow var
>     lock
>       call klp_shadow_get again
>       if unlikely found
>         kfree unneeded alloc
>       else
>         call __klp_shadow_attach with new alloc
>     unlock
>     return whichever shadow var we used
> 
> This way both calls can accept gfp_flags that may sleep, with the only
> downside that klp_shadow_get_or_attach may allocate an unnecessary
> shadow variable in the unlikely case that it's found on the second
> klp_shadow_get attempt (under the lock).  No more clunky "bool lock"
> flag either. :)

Sounds good to me!

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1693478

On Thu 2017-07-20 11:48:41, Joe Lawrence wrote:
> On 07/20/2017 10:45 AM, Miroslav Benes wrote:
> > 
> >>>>> + *
> >>>>> + * Note: allocates @new_size space for shadow variable data and copies
> >>>>> + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> >>>>> + * space.  If @new_data is NULL, @new_size is still allocated, but no
> >>>>> + * copy is performed.
> >>>>
> >>>> I must say I'm not entirely happy with this. I don't know if this is what 
> >>>> Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> >>>> memcpy instead of a simple assignment in v1 seems worse. 
> >>>
> >>> This change was a bit of a experiment on my part in reaction to
> >>> adding klp_shadow_get_or_attach().
> >>>
> >>> I like the simplicity of v1's pointer assignment -- in fact, moving all
> >>> allocation responsiblity (klp_shadow meta-data and data[] area) out to
> >>> the caller is doable, though implementing klp_shadow_get_or_attach() and
> >>> and klp_shadow_detach_all() complicates matters, for example, adding an
> >>> alloc/release callback.  I originally attempted this for v2, but turned
> >>> back when the API and implementation grew complicated.  If the memcpy
> >>> and gfp_flag restrictions are too ugly, I can try revisting that
> >>> approach.  Ideas welcome :)
> >>
> >> Well, I didn't like callbacks either :). And no, I do not have a better 
> >> idea. I still need to think about it.
> > 
> > Done and I agree that memcpy approach is not so bad after all :). So I'm 
> > fine with it.
> 
> I looked at it again this morning and a "pass-your-own" allocation API
> always comes back to adding callbacks and other complications :(  In the
> end, most callers will be shadowing pointers and not entire structures,
> so I think the copy isn't too bad.

I agree.

> On a related note, if we keep the allocations and memcpy, how about I
> shift around the attach/get calls like so:
> 
>   __klp_shadow_attach
>     set shadow variable member values
>     memcpy
>     add to hash
> 
>   klp_shadow_attach
>     alloc new shadow var
>     lock
>     call __klp_shadow_attach with new alloc
>     unlock
>
>   klp_shadow_get_or_attach
>     be optimistic, call klp_shadow_get (if found, return it)
>     be pessimistic, alloc new shadow var
>     lock
>       call klp_shadow_get again
>       if unlikely found
>         kfree unneeded alloc
>       else
>         call __klp_shadow_attach with new alloc
>     unlock
>     return whichever shadow var we used

I would really suggest that klp_shadow_attach() prevents adding
duplicates. We should make the API as safe as possible.
Catching unexpected duplicate could safe people a lot of
headaches.

Please read more on this in my review
https://lkml.kernel.org/r/20170718124500.GF3393@pathway.suse.cz

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1693499

On Thu, 20 Jul 2017, Joe Lawrence wrote:

> On 07/20/2017 10:45 AM, Miroslav Benes wrote:
> > 
> >>>>> + *
> >>>>> + * Note: allocates @new_size space for shadow variable data and copies
> >>>>> + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> >>>>> + * space.  If @new_data is NULL, @new_size is still allocated, but no
> >>>>> + * copy is performed.
> >>>>
> >>>> I must say I'm not entirely happy with this. I don't know if this is what 
> >>>> Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> >>>> memcpy instead of a simple assignment in v1 seems worse. 
> >>>
> >>> This change was a bit of a experiment on my part in reaction to
> >>> adding klp_shadow_get_or_attach().
> >>>
> >>> I like the simplicity of v1's pointer assignment -- in fact, moving all
> >>> allocation responsiblity (klp_shadow meta-data and data[] area) out to
> >>> the caller is doable, though implementing klp_shadow_get_or_attach() and
> >>> and klp_shadow_detach_all() complicates matters, for example, adding an
> >>> alloc/release callback.  I originally attempted this for v2, but turned
> >>> back when the API and implementation grew complicated.  If the memcpy
> >>> and gfp_flag restrictions are too ugly, I can try revisting that
> >>> approach.  Ideas welcome :)
> >>
> >> Well, I didn't like callbacks either :). And no, I do not have a better 
> >> idea. I still need to think about it.
> > 
> > Done and I agree that memcpy approach is not so bad after all :). So I'm 
> > fine with it.
> 
> I looked at it again this morning and a "pass-your-own" allocation API
> always comes back to adding callbacks and other complications :(  In the
> end, most callers will be shadowing pointers and not entire structures,
> so I think the copy isn't too bad.

I agree.
 
> On a related note, if we keep the allocations and memcpy, how about I
> shift around the attach/get calls like so:
> 
>   __klp_shadow_attach
>     set shadow variable member values
>     memcpy
>     add to hash
> 
>   klp_shadow_attach
>     alloc new shadow var
>     lock
>     call __klp_shadow_attach with new alloc
>     unlock
> 
>   klp_shadow_get_or_attach
>     be optimistic, call klp_shadow_get (if found, return it)
>     be pessimistic, alloc new shadow var
>     lock
>       call klp_shadow_get again
>       if unlikely found
>         kfree unneeded alloc
>       else
>         call __klp_shadow_attach with new alloc
>     unlock
>     return whichever shadow var we used
> 
> This way both calls can accept gfp_flags that may sleep, with the only
> downside that klp_shadow_get_or_attach may allocate an unnecessary
> shadow variable in the unlikely case that it's found on the second
> klp_shadow_get attempt (under the lock).  No more clunky "bool lock"
> flag either. :)

Yes, this looks good.

Thanks,
Miroslav

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web