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

Groups > linux.kernel > #1690241 > 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 Petr Mladek <pmladek@suse.com> - 2017-07-18 14:50 +0200
    Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-07-20 22:40 +0200
      Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Petr Mladek <pmladek@suse.com> - 2017-07-21 11:20 +0200
        Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-07-21 16:00 +0200
          Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-24 17:10 +0200
      Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Miroslav Benes <mbenes@suse.cz> - 2017-07-21 11:20 +0200
        Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Petr Mladek <pmladek@suse.com> - 2017-07-21 11:30 +0200

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

On Wed 2017-06-28 11:37:26, Joe Lawrence wrote:
> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
> new file mode 100644
> index 000000000000..7f28982e6b1c
> --- /dev/null
> +++ b/Documentation/livepatch/shadow-vars.txt
> +Use cases
> +---------
> +
> +See the example shadow variable livepatch modules in samples/livepatch
> +for full working demonstrations.
> +
> +Example 1: Commit 1d147bfa6429 ("mac80211: fix AP powersave TX vs.
> +wakeup race") added a spinlock to net/mac80211/sta_info.h :: struct
> +sta_info.  Implementing this change with a shadow variable is
> +straightforward.
> +
> +Allocation - when a host sta_info structure is allocated, attach a
> +shadow variable copy of the ps_lock:
> +
> +#define PS_LOCK 1
> +struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
> +				const u8 *addr, gfp_t gfp)
> +{
> +	struct sta_info *sta;
> +	spinlock_t *ps_lock;
> +	...
> +	sta = kzalloc(sizeof(*sta) + hw->sta_data_size, gfp);

klp_shadow_attach() does the allocation as well now.
Note that we could pass already initialized spin_lock.

> +	...
> +	ps_lock = klp_shadow_attach(sta, PS_LOCK, NULL, sizeof(*ps_lock), gfp);
> +	if (!ps_lock)
> +		goto shadow_fail;
> +	spin_lock_init(ps_lock);
> +	...
> +
> +Usage - when using the shadow spinlock, query the shadow variable API to
> +retrieve it:
> +
> +void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
> +{
> +	spinlock_t *ps_lock;
> +	...
> +	/* sync with ieee80211_tx_h_unicast_ps_buf */
> +	ps_lock = klp_shadow_get(sta, "ps_lock");

s/"ps_lock"/PS_LOCK/

The same problem is repeated many times below (also in the 2nd
example).

Also this is a nice example, where klp_shadow_get_or_attach()
would be useful. It would fix even already existing instances.

So, the code might look like:

  void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
  {
	DEFINE_SPINLOCK(ps_lock_fallback)
	spinlock_t *ps_lock;
	...
	/* sync with ieee80211_tx_h_unicast_ps_buf */
	ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
			&ps_lock_fallback, sizeof(ps_lock_fallback),
			GFP_ATOMIC);

It is a bit ugly that we always initialize ps_lock_fallback
even when it is not used. But it helps to avoid a custom
callback that would create the fallback variable. I think
that it is an acceptable deal.


> +	if (ps_lock)
> +		spin_lock(ps_lock);
> +	...
> +	if (ps_lock)
> +		spin_unlock(ps_lock);
> +	...
> +
> +Release - when the host sta_info structure is freed, first detach the
> +shadow variable and then free the shadow spinlock:
> +
> +void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
> +{
> +	spinlock_t *ps_lock;
> +	...
> +	ps_lock = klp_shadow_get(sta, "ps_lock");
> +	if (ps_lock)
> +		klp_shadow_detach(sta, "ps_lock");

Isn't klp_shadow_detach() enough? If it an optimization, 
klp_shadow_detach() might get optimized the same way.
But I am not sure if it is worth it.

> +	kfree(sta);
> +
> +


> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> new file mode 100644
> index 000000000000..d37a61c57e72
> --- /dev/null
> +++ b/kernel/livepatch/shadow.c
> +/**
> + * _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
> + *
> + * 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.
> + *
> + * Return: the shadow variable new_data element, NULL on failure.
> + */
> +static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> +				size_t new_size, gfp_t gfp_flags,
> +				bool lock)

Nested implementation is usually prefixed by two underlines __.
It is more visible and helps to distinguish it from the normal function.


> +{
> +	struct klp_shadow *shadow;
> +	unsigned long flags;
> +
> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> +	if (!shadow)
> +		return NULL;
> +
> +	shadow->obj = obj;
> +	shadow->num = num;
> +	if (new_data)
> +		memcpy(shadow->new_data, new_data, new_size);
> +
> +	if (lock)
> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);

We should check if the shadow variable already existed. Otherwise,
it would be possible to silently create many duplicates.

It would make klp_shadow_attach() and klp_shadow_get_or_attach()
to behave the same.

I would do WARN() in klp_shadow_attach() when the variable
already existed are return NULL. Of course it might be inoncent
duplication. But it might mean that someone else is using another
variable of the same name but with different content. klp_shadow_get()
would then return the same variable for two different purposes.
Then the whole system might end like a glass on a stony floor.


> +	if (lock)
> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return shadow->new_data;
> +}

Otherwise, I rather like the API. Thanks a lot for adding
klp_shadow_get_or_attach().

I did not comment things that were already discussed in
other threads.

Best Regards,
Petr

[toc] | [next] | [standalone]

#1693219

On 07/18/2017 08:45 AM, Petr Mladek wrote:
> On Wed 2017-06-28 11:37:26, Joe Lawrence wrote:
>> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
>> new file mode 100644
>> index 000000000000..7f28982e6b1c
>> --- /dev/null
>> +++ b/Documentation/livepatch/shadow-vars.txt
>> +Use cases
>> +---------
>> +
>> +See the example shadow variable livepatch modules in samples/livepatch
>> +for full working demonstrations.
>> +
>> +Example 1: Commit 1d147bfa6429 ("mac80211: fix AP powersave TX vs.
>> +wakeup race") added a spinlock to net/mac80211/sta_info.h :: struct
>> +sta_info.  Implementing this change with a shadow variable is
>> +straightforward.
>> +
>> +Allocation - when a host sta_info structure is allocated, attach a
>> +shadow variable copy of the ps_lock:
>> +
>> +#define PS_LOCK 1
>> +struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>> +				const u8 *addr, gfp_t gfp)
>> +{
>> +	struct sta_info *sta;
>> +	spinlock_t *ps_lock;
>> +	...
>> +	sta = kzalloc(sizeof(*sta) + hw->sta_data_size, gfp);
> 
> klp_shadow_attach() does the allocation as well now.
> Note that we could pass already initialized spin_lock.
> 
>> +	...
>> +	ps_lock = klp_shadow_attach(sta, PS_LOCK, NULL, sizeof(*ps_lock), gfp);
>> +	if (!ps_lock)
>> +		goto shadow_fail;
>> +	spin_lock_init(ps_lock);
>> +	...
>> +
>> +Usage - when using the shadow spinlock, query the shadow variable API to
>> +retrieve it:
>> +
>> +void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
>> +{
>> +	spinlock_t *ps_lock;
>> +	...
>> +	/* sync with ieee80211_tx_h_unicast_ps_buf */
>> +	ps_lock = klp_shadow_get(sta, "ps_lock");
> 
> s/"ps_lock"/PS_LOCK/
> 
> The same problem is repeated many times below (also in the 2nd
> example).
> 
> Also this is a nice example, where klp_shadow_get_or_attach()
> would be useful. It would fix even already existing instances.
> 
> So, the code might look like:
> 
>   void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
>   {
> 	DEFINE_SPINLOCK(ps_lock_fallback)
> 	spinlock_t *ps_lock;
> 	...
> 	/* sync with ieee80211_tx_h_unicast_ps_buf */
> 	ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
> 			&ps_lock_fallback, sizeof(ps_lock_fallback),
> 			GFP_ATOMIC);
> 
> It is a bit ugly that we always initialize ps_lock_fallback
> even when it is not used. But it helps to avoid a custom
> callback that would create the fallback variable. I think
> that it is an acceptable deal.

Yup, it's a tradeoff for the caller.  If they want a shadow variable
added and considered "live", they better have previously initialized it :)

> 
>> +	if (ps_lock)
>> +		spin_lock(ps_lock);
>> +	...
>> +	if (ps_lock)
>> +		spin_unlock(ps_lock);
>> +	...
>> +
>> +Release - when the host sta_info structure is freed, first detach the
>> +shadow variable and then free the shadow spinlock:
>> +
>> +void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
>> +{
>> +	spinlock_t *ps_lock;
>> +	...
>> +	ps_lock = klp_shadow_get(sta, "ps_lock");
>> +	if (ps_lock)
>> +		klp_shadow_detach(sta, "ps_lock");
> 
> Isn't klp_shadow_detach() enough? If it an optimization, 
> klp_shadow_detach() might get optimized the same way.
> But I am not sure if it is worth it.

Let me go back and review these inline examples for v3... I didn't
update them carefully enough when drafting v2.

>> +	kfree(sta);
>> +
>> +
> 
> 
>> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
>> new file mode 100644
>> index 000000000000..d37a61c57e72
>> --- /dev/null
>> +++ b/kernel/livepatch/shadow.c
>> +/**
>> + * _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
>> + *
>> + * 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.
>> + *
>> + * Return: the shadow variable new_data element, NULL on failure.
>> + */
>> +static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data,
>> +				size_t new_size, gfp_t gfp_flags,
>> +				bool lock)
> 
> Nested implementation is usually prefixed by two underlines __.
> It is more visible and helps to distinguish it from the normal function.

Noted for v3.

>> +{
>> +	struct klp_shadow *shadow;
>> +	unsigned long flags;
>> +
>> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
>> +	if (!shadow)
>> +		return NULL;
>> +
>> +	shadow->obj = obj;
>> +	shadow->num = num;
>> +	if (new_data)
>> +		memcpy(shadow->new_data, new_data, new_size);
>> +
>> +	if (lock)
>> +		spin_lock_irqsave(&klp_shadow_lock, flags);
>> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> 
> We should check if the shadow variable already existed. Otherwise,
> it would be possible to silently create many duplicates.
> 
> It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> to behave the same.

They would be almost exactly the same, except one version would bounce a
redundant entry while the other would return the existing one.  I could
envision callers wanting any of the following behavior:

If a shadow <obj, id> already exists:
  0 - add a second shadow variable (??? why)
  1 - return NULL, WARN
  2 - return the existing one
  3 - update the existing one with the new data and return it

* v2 klp_shadow_attach() currently implements #0, can be made to do #1
* v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
makes more sense

Going back to existing kpatch use-cases, since we paired shadow variable
creation to their parent object creation, -EEXIST was never an issue.  I
think we concocted one proof-of-concept kpatch where we created shadow
variables "in-flight", that is, we patched a routine that operated on
the parent object and created a shadow variable if one did not already
exist.  The in-flight patch was for single function and we knew that it
would never be called concurrently for the same parent object.  tl;dr =
kpatch never worried about existing shadow <obj, id>.

> I would do WARN() in klp_shadow_attach() when the variable
> already existed are return NULL. Of course it might be inoncent
> duplication. But it might mean that someone else is using another
> variable of the same name but with different content. klp_shadow_get()
> would then return the same variable for two different purposes.
> Then the whole system might end like a glass on a stony floor.

What do you think of expanding the API to include each the cases
outlined above?   Something like:

  1 - klp_attach = allocate and add a unique <obj, id> to the hash,
                   duplicates return NULL and a WARN

  2 - klp_get_or_attach = return <obj, id> if it already exists,
                          otherwise allocate a new one

  3 - klp_get_or_update = update and return <obj, id> if it already
                          exists, otherwise allocate a new one

IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
be dropped.  Since you suggested adding klp_get_or_attach(), what do you
think?

> 
>> +	if (lock)
>> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
>> +
>> +	return shadow->new_data;
>> +}
> 
> Otherwise, I rather like the API. Thanks a lot for adding
> klp_shadow_get_or_attach().
> 
> I did not comment things that were already discussed in
> other threads.

klp_shadow_get_or_attach() looks to be really useful in concurrent
situations, especially cases where we'd like to do in-flight shadow
variable creation.

Appreciate the comments as always.

-- Joe

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

#1693508

On Thu 2017-07-20 16:30:37, Joe Lawrence wrote:
> On 07/18/2017 08:45 AM, Petr Mladek wrote:
> > On Wed 2017-06-28 11:37:26, Joe Lawrence wrote:
> >> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> >> new file mode 100644
> >> index 000000000000..d37a61c57e72
> >> --- /dev/null
> >> +++ b/kernel/livepatch/shadow.c
> >> +static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> >> +				size_t new_size, gfp_t gfp_flags,
> >> +				bool lock)
> > 
> > Nested implementation is usually prefixed by two underlines __.
> > It is more visible and helps to distinguish it from the normal function.
> 
> Noted for v3.
> 
> >> +{
> >> +	struct klp_shadow *shadow;
> >> +	unsigned long flags;
> >> +
> >> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> >> +	if (!shadow)
> >> +		return NULL;
> >> +
> >> +	shadow->obj = obj;
> >> +	shadow->num = num;
> >> +	if (new_data)
> >> +		memcpy(shadow->new_data, new_data, new_size);
> >> +
> >> +	if (lock)
> >> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> >> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> > 
> > We should check if the shadow variable already existed. Otherwise,
> > it would be possible to silently create many duplicates.
> > 
> > It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> > to behave the same.
> 
> They would be almost exactly the same, except one version would bounce a
> redundant entry while the other would return the existing one.  I could
> envision callers wanting any of the following behavior:
> 
> If a shadow <obj, id> already exists:
>   0 - add a second shadow variable (??? why)
>   1 - return NULL, WARN
>   2 - return the existing one
>   3 - update the existing one with the new data and return it
> 
> * v2 klp_shadow_attach() currently implements #0, can be made to do #1
> * v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
> makes more sense
> 
> Going back to existing kpatch use-cases, since we paired shadow variable
> creation to their parent object creation, -EEXIST was never an issue.  I
> think we concocted one proof-of-concept kpatch where we created shadow
> variables "in-flight", that is, we patched a routine that operated on
> the parent object and created a shadow variable if one did not already
> exist.  The in-flight patch was for single function and we knew that it
> would never be called concurrently for the same parent object.  tl;dr =
> kpatch never worried about existing shadow <obj, id>.

I am not sure if you want to explain why you did not care. Or if
you want to suggest that we should not care :-)

I agree that if the API is used in simple/clear situations then
this might look like an overkill. But I am afraid that the API users
do not have this in hands. They usually have to create a livepatch
based on an upstream secutity fix. The fix need not be always simple.
Then it is handy to have an API that helps to catch mistakes
and keeps the patched system in a sane state.


> > I would do WARN() in klp_shadow_attach() when the variable
> > already existed are return NULL. Of course it might be inoncent
> > duplication. But it might mean that someone else is using another
> > variable of the same name but with different content. klp_shadow_get()
> > would then return the same variable for two different purposes.
> > Then the whole system might end like a glass on a stony floor.
> 
> What do you think of expanding the API to include each the cases
> outlined above?   Something like:
> 
>   1 - klp_attach = allocate and add a unique <obj, id> to the hash,
>                    duplicates return NULL and a WARN

Sounds good.

>   2 - klp_get_or_attach = return <obj, id> if it already exists,
>                           otherwise allocate a new one

Sounds good.

>   3 - klp_get_or_update = update and return <obj, id> if it already
>                           exists, otherwise allocate a new one

I am not sure where this behavior would make sense. See below.


> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
> be dropped.  Since you suggested adding klp_get_or_attach(), what do you
> think?

I do not agree. Let's look at the example with the missing lock.
The patch adds the lock if it did not exist. Then the lock can
be used to synchronize all further operations.

klp_get_or_update() would always replace the existing lock
with a freshly initialized one. We would loss the information
if it was locked or not.


> klp_shadow_get_or_attach() looks to be really useful in concurrent
> situations, especially cases where we'd like to do in-flight shadow
> variable creation.

Thanks a lot for working in the API. It will be handy.

Best Regards,
Petr

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

#1693720

On 07/21/2017 05:13 AM, Petr Mladek wrote:
> On Thu 2017-07-20 16:30:37, Joe Lawrence wrote:
>> Going back to existing kpatch use-cases, since we paired shadow variable
>> creation to their parent object creation, -EEXIST was never an issue.  I
>> think we concocted one proof-of-concept kpatch where we created shadow
>> variables "in-flight", that is, we patched a routine that operated on
>> the parent object and created a shadow variable if one did not already
>> exist.  The in-flight patch was for single function and we knew that it
>> would never be called concurrently for the same parent object.  tl;dr =
>> kpatch never worried about existing shadow <obj, id>.
> 
> I am not sure if you want to explain why you did not care. Or if
> you want to suggest that we should not care :-)

We knew that in our limited use-cases for in-flight shadow variables,
concurrency was not an issue.  Josh has a better historical perspective,
but I think this particular use-case appeared way after the initial
kpatch implementation of shadow variables.  Now that we know we can use
them in this way, I agree that it's important to hash out the
implications while designing the livepatch counterpart.

> I agree that if the API is used in simple/clear situations then
> this might look like an overkill. But I am afraid that the API users
> do not have this in hands. They usually have to create a livepatch
> based on an upstream secutity fix. The fix need not be always simple.
> Then it is handy to have an API that helps to catch mistakes
> and keeps the patched system in a sane state.
Very true, though I wonder what interesting state that will be when
running patched code and partially shadowed variables. :)  At the very
least, I think this protection would be valuable during patch sanity
testing.

>>> I would do WARN() in klp_shadow_attach() when the variable
>>> already existed are return NULL. Of course it might be inoncent
>>> duplication. But it might mean that someone else is using another
>>> variable of the same name but with different content. klp_shadow_get()
>>> would then return the same variable for two different purposes.
>>> Then the whole system might end like a glass on a stony floor.
>>
>> What do you think of expanding the API to include each the cases
>> outlined above?   Something like:
>>
>>   1 - klp_attach = allocate and add a unique <obj, id> to the hash,
>>                    duplicates return NULL and a WARN
> 
> Sounds good.
> 
>>   2 - klp_get_or_attach = return <obj, id> if it already exists,
>>                           otherwise allocate a new one
> 
> Sounds good.
> 
>>   3 - klp_get_or_update = update and return <obj, id> if it already
>>                           exists, otherwise allocate a new one
> 
> I am not sure where this behavior would make sense. See below.
> 
> 
>> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
>> be dropped.  Since you suggested adding klp_get_or_attach(), what do you
>> think?
> 
> I do not agree. Let's look at the example with the missing lock.
> The patch adds the lock if it did not exist. Then the lock can
> be used to synchronize all further operations.
> 
> klp_get_or_update() would always replace the existing lock
> with a freshly initialized one. We would loss the information
> if it was locked or not.

Ah good point, perhaps we have two situations here:

  A - A shadow variable that's pointing to some object, like a lock,
      where the original object is required.  (Your example above.)

  B - A shadow variable that's storing the data itself.  In other words,
      instead of attaching a pointer, the whole object was attached:

        void patched_function()
        {
           ...
           klp_get_or_attach(obj, id, &jiffies, sizeof(jiffies), ...)
           ...

      in which case the caller is only interested in pushing in the
      latest version of jiffies.

For these I suggest klp_get_or_attach() for case A and
klp_get_or_update() for case B.

-- Joe

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

#1694796

On Fri, Jul 21, 2017 at 09:55:59AM -0400, Joe Lawrence wrote:
> >>> I would do WARN() in klp_shadow_attach() when the variable
> >>> already existed are return NULL. Of course it might be inoncent
> >>> duplication. But it might mean that someone else is using another
> >>> variable of the same name but with different content. klp_shadow_get()
> >>> would then return the same variable for two different purposes.
> >>> Then the whole system might end like a glass on a stony floor.
> >>
> >> What do you think of expanding the API to include each the cases
> >> outlined above?   Something like:
> >>
> >>   1 - klp_attach = allocate and add a unique <obj, id> to the hash,
> >>                    duplicates return NULL and a WARN
> > 
> > Sounds good.
> > 
> >>   2 - klp_get_or_attach = return <obj, id> if it already exists,
> >>                           otherwise allocate a new one
> > 
> > Sounds good.
> > 
> >>   3 - klp_get_or_update = update and return <obj, id> if it already
> >>                           exists, otherwise allocate a new one
> > 
> > I am not sure where this behavior would make sense. See below.
> > 
> > 
> >> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
> >> be dropped.  Since you suggested adding klp_get_or_attach(), what do you
> >> think?
> > 
> > I do not agree. Let's look at the example with the missing lock.
> > The patch adds the lock if it did not exist. Then the lock can
> > be used to synchronize all further operations.
> > 
> > klp_get_or_update() would always replace the existing lock
> > with a freshly initialized one. We would loss the information
> > if it was locked or not.
> 
> Ah good point, perhaps we have two situations here:
> 
>   A - A shadow variable that's pointing to some object, like a lock,
>       where the original object is required.  (Your example above.)
> 
>   B - A shadow variable that's storing the data itself.  In other words,
>       instead of attaching a pointer, the whole object was attached:
> 
>         void patched_function()
>         {
>            ...
>            klp_get_or_attach(obj, id, &jiffies, sizeof(jiffies), ...)
>            ...
> 
>       in which case the caller is only interested in pushing in the
>       latest version of jiffies.
> 
> For these I suggest klp_get_or_attach() for case A and
> klp_get_or_update() for case B.

klp_get_or_update() doesn't actually 'get', because even if it does, it
gets updated first.  I think a more precise name would be
klp_update_or_attach().

-- 
Josh

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

#1693509

> >> +{
> >> +	struct klp_shadow *shadow;
> >> +	unsigned long flags;
> >> +
> >> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> >> +	if (!shadow)
> >> +		return NULL;
> >> +
> >> +	shadow->obj = obj;
> >> +	shadow->num = num;
> >> +	if (new_data)
> >> +		memcpy(shadow->new_data, new_data, new_size);
> >> +
> >> +	if (lock)
> >> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> >> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> > 
> > We should check if the shadow variable already existed. Otherwise,
> > it would be possible to silently create many duplicates.
> > 
> > It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> > to behave the same.
> 
> They would be almost exactly the same, except one version would bounce a
> redundant entry while the other would return the existing one.  I could
> envision callers wanting any of the following behavior:
> 
> If a shadow <obj, id> already exists:
>   0 - add a second shadow variable (??? why)
>   1 - return NULL, WARN
>   2 - return the existing one
>   3 - update the existing one with the new data and return it
> 
> * v2 klp_shadow_attach() currently implements #0, can be made to do #1
> * v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
> makes more sense

I have a feeling that we're becoming overprotective here again. I think 
that klp_shadow_attach() adding a new entry makes sense. 
Although I can imagine #1. I think it is a responsibility of the user to 
know what to call. And that is what klp_shadow_get_or_attach() is for.

klp_shadow_get() and klp_shadow_attach() are two main API functions. 
klp_shadow_get_or_attach() is there to make things safe if needed 
(concurrency).
 
> Going back to existing kpatch use-cases, since we paired shadow variable
> creation to their parent object creation, -EEXIST was never an issue.  I
> think we concocted one proof-of-concept kpatch where we created shadow
> variables "in-flight", that is, we patched a routine that operated on
> the parent object and created a shadow variable if one did not already
> exist.  The in-flight patch was for single function and we knew that it
> would never be called concurrently for the same parent object.  tl;dr =
> kpatch never worried about existing shadow <obj, id>.

And this makes sense to me too.

> > I would do WARN() in klp_shadow_attach() when the variable
> > already existed are return NULL. Of course it might be inoncent
> > duplication. But it might mean that someone else is using another
> > variable of the same name but with different content. klp_shadow_get()
> > would then return the same variable for two different purposes.
> > Then the whole system might end like a glass on a stony floor.
> 
> What do you think of expanding the API to include each the cases
> outlined above?   Something like:
> 
>   1 - klp_attach = allocate and add a unique <obj, id> to the hash,
>                    duplicates return NULL and a WARN
> 
>   2 - klp_get_or_attach = return <obj, id> if it already exists,
>                           otherwise allocate a new one
> 
>   3 - klp_get_or_update = update and return <obj, id> if it already
>                           exists, otherwise allocate a new one
> 
> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
> be dropped.  Since you suggested adding klp_get_or_attach(), what do you
> think?

I don't know. I'd be prudent now. We can always add it later...

Miroslav

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

#1693512

On Fri 2017-07-21 11:12:18, Miroslav Benes wrote:
> 
> > >> +{
> > >> +	struct klp_shadow *shadow;
> > >> +	unsigned long flags;
> > >> +
> > >> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> > >> +	if (!shadow)
> > >> +		return NULL;
> > >> +
> > >> +	shadow->obj = obj;
> > >> +	shadow->num = num;
> > >> +	if (new_data)
> > >> +		memcpy(shadow->new_data, new_data, new_size);
> > >> +
> > >> +	if (lock)
> > >> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> > >> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> > > 
> > > We should check if the shadow variable already existed. Otherwise,
> > > it would be possible to silently create many duplicates.
> > > 
> > > It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> > > to behave the same.
> > 
> > They would be almost exactly the same, except one version would bounce a
> > redundant entry while the other would return the existing one.  I could
> > envision callers wanting any of the following behavior:
> > 
> > If a shadow <obj, id> already exists:
> >   0 - add a second shadow variable (??? why)
> >   1 - return NULL, WARN
> >   2 - return the existing one
> >   3 - update the existing one with the new data and return it
> > 
> > * v2 klp_shadow_attach() currently implements #0, can be made to do #1
> > * v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
> > makes more sense
> 
> I have a feeling that we're becoming overprotective here again. I think 
> that klp_shadow_attach() adding a new entry makes sense. 
> Although I can imagine #1. I think it is a responsibility of the user to 
> know what to call. And that is what klp_shadow_get_or_attach() is for.

The shadow id is an integer. This prevents also from using the same
id by two patches for a different purpose. The two livepatches might
be crated months after each other. There might be many fixes
accumulated in the livepatch. Is it really almost impossible
to make mistakes so this rather small change is not worth it?

Another motivation is that the author of the livepatch usually
is not familiar with the patched code. It makes it more prone
to mistakes.

Best Regards,
Petr

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web