Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Petr Mladek]

On Wed 2018-03-14 14:44:36, Josh Poimboeuf wrote:
> On Wed, Mar 14, 2018 at 03:27:02PM -0400, Joe Lawrence wrote:
> > On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> > > The existing API allows to pass a sample data to initialize the shadow
> > > data. It works well when the data are position independent. But it fails
> > > miserably when we need to set a pointer to the shadow structure itself.
> > > 
> > > Unfortunately, we might need to initialize the pointer surprisingly
> > > often because of struct list_head. It is even worse because the list
> > > might be hidden in other common structures, for example, struct mutex,
> > > struct wait_queue_head.
> > > 
> > > This patch makes the API more safe. A custom init function and data
> > > are passed to klp_shadow_*alloc() functions instead of the sample data.
> > 
> > Yup, this looks kinda familiar, I remember tinkering with the same idea
> > last year [1] before settling on the simpler API.
> > 
> > [1] 
> > https://github.com/torvalds/linux/compare/master...joe-lawrence:shadow_variables_v2_c
> > 
> > > Note that the init_data are not longer a template for the shadow->data.
> > > It might point to any data that might be necessary when the init
> > > function is called.
> > 
> > I'm not opposed to changing the API, but I was wondering if you had
> > thought about expanding it as an alternative? 
> > 
> > When working on this last summer, I remember holding onto to some less
> > than intuitive naming conventions so that I could support a basic API
> > and an extended API with bells and whistles like this patchset
> > implements.  It didn't seem too difficult to layer the basic API ontop
> > of one like this (see [1] for example), so maybe that's an option to
> > keep basic shadow variable usage a little simpler.  /two cents
> 
> I like Petr's new API.  It's not a big deal to just pass a couple of
> NULLs if you don't need the callback.
> 
> And I prefer fewer functions anyway -- maybe it's my functionitis
> allergies acting up again.

Yeah, I think that that two APIs might cause confusion. Especially
because *data and *init_data have different meaning. I would prefer
to keep only the first one.


> > Perhaps shadow variables are another candidate for some kind of
> > kselftest?
> 
> Indeed!

It would be great.


Best Regards,
Petr

PS: Thanks all for the feedback.

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Petr Mladek]

On Wed 2018-03-14 14:44:36, Josh Poimboeuf wrote:
> On Wed, Mar 14, 2018 at 03:27:02PM -0400, Joe Lawrence wrote:
> > On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> > > The existing API allows to pass a sample data to initialize the shadow
> > > data. It works well when the data are position independent. But it fails
> > > miserably when we need to set a pointer to the shadow structure itself.
> > > 
> > > Unfortunately, we might need to initialize the pointer surprisingly
> > > often because of struct list_head. It is even worse because the list
> > > might be hidden in other common structures, for example, struct mutex,
> > > struct wait_queue_head.
> > > 
> > > This patch makes the API more safe. A custom init function and data
> > > are passed to klp_shadow_*alloc() functions instead of the sample data.
> > 
> > Yup, this looks kinda familiar, I remember tinkering with the same idea
> > last year [1] before settling on the simpler API.
> > 
> > [1] 
> > https://github.com/torvalds/linux/compare/master...joe-lawrence:shadow_variables_v2_c
> > 
> > > Note that the init_data are not longer a template for the shadow->data.
> > > It might point to any data that might be necessary when the init
> > > function is called.
> > 
> > I'm not opposed to changing the API, but I was wondering if you had
> > thought about expanding it as an alternative? 
> > 
> > When working on this last summer, I remember holding onto to some less
> > than intuitive naming conventions so that I could support a basic API
> > and an extended API with bells and whistles like this patchset
> > implements.  It didn't seem too difficult to layer the basic API ontop
> > of one like this (see [1] for example), so maybe that's an option to
> > keep basic shadow variable usage a little simpler.  /two cents
> 
> I like Petr's new API.  It's not a big deal to just pass a couple of
> NULLs if you don't need the callback.
> 
> And I prefer fewer functions anyway -- maybe it's my functionitis
> allergies acting up again.

Yeah, I think that that two APIs might cause confusion. Especially
because *data and *init_data have different meaning. I would prefer
to keep only the first one.


> > Perhaps shadow variables are another candidate for some kind of
> > kselftest?
> 
> Indeed!

It would be great.


Best Regards,
Petr

PS: Thanks all for the feedback.

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Miroslav Benes]

> @@ -186,10 +198,13 @@ static void *__klp_shadow_get_or_alloc(void *obj, 
> unsigned long id, void *data,
>   * Return: the shadow variable data element, NULL on duplicate or
>   * failure.
>   */
> -void *klp_shadow_alloc(void *obj, unsigned long id, void *data,
> -size_t size, gfp_t gfp_flags)
> +void *klp_shadow_alloc(void *obj, unsigned long id,
> +size_t size, gfp_t gfp_flags,
> +klp_shadow_init_func_t init_func,
> +void *init_data)

The comment above the function should be also updated, because the 
function's parameters changed.

>  {
> - return __klp_shadow_get_or_alloc(obj, id, data, size, gfp_flags, true);
> + return __klp_shadow_get_or_alloc(obj, id, size, gfp_flags,
> +  init_func, init_data, true);
>  }
>  EXPORT_SYMBOL_GPL(klp_shadow_alloc);
>  
> @@ -212,10 +227,13 @@ EXPORT_SYMBOL_GPL(klp_shadow_alloc);
>   *
>   * Return: the shadow variable data element, NULL on failure.
>   */
> -void *klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
> -size_t size, gfp_t gfp_flags)
> +void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> +   size_t size, gfp_t gfp_flags,
> +   klp_shadow_init_func_t init_func,
> +   void *init_data)

Ditto.

Thanks,
Miroslav

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Miroslav Benes]

> @@ -186,10 +198,13 @@ static void *__klp_shadow_get_or_alloc(void *obj, 
> unsigned long id, void *data,
>   * Return: the shadow variable data element, NULL on duplicate or
>   * failure.
>   */
> -void *klp_shadow_alloc(void *obj, unsigned long id, void *data,
> -size_t size, gfp_t gfp_flags)
> +void *klp_shadow_alloc(void *obj, unsigned long id,
> +size_t size, gfp_t gfp_flags,
> +klp_shadow_init_func_t init_func,
> +void *init_data)

The comment above the function should be also updated, because the 
function's parameters changed.

>  {
> - return __klp_shadow_get_or_alloc(obj, id, data, size, gfp_flags, true);
> + return __klp_shadow_get_or_alloc(obj, id, size, gfp_flags,
> +  init_func, init_data, true);
>  }
>  EXPORT_SYMBOL_GPL(klp_shadow_alloc);
>  
> @@ -212,10 +227,13 @@ EXPORT_SYMBOL_GPL(klp_shadow_alloc);
>   *
>   * Return: the shadow variable data element, NULL on failure.
>   */
> -void *klp_shadow_get_or_alloc(void *obj, unsigned long id, void *data,
> -size_t size, gfp_t gfp_flags)
> +void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> +   size_t size, gfp_t gfp_flags,
> +   klp_shadow_init_func_t init_func,
> +   void *init_data)

Ditto.

Thanks,
Miroslav

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Josh Poimboeuf]

On Wed, Mar 14, 2018 at 03:43:01PM -0400, Joe Lawrence wrote:
> >> @@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, 
> >> unsigned long id, void *data,
> >>goto exists;
> >>}
> >>  
> >> +  new_shadow->obj = obj;
> >> +  new_shadow->id = id;
> >> +
> >> +  if (init_func) {
> >> +  int err;
> >> +
> >> +  err = init_func(obj, new_shadow->data, init_data);
> > 
> > Am I hallucinating, or will new_shadow->data always be NULL?  How did it
> > even work before?
> >
> struct klp_shadow {
> struct hlist_node node;
> struct rcu_head rcu_head;
> void *obj;
> unsigned long id;
> char data[];<< not a pointer
> };

Ah.  This code needs a nice comment above the kzalloc() call, so I won't
get confused again next time.

-- 
Josh

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Josh Poimboeuf]

On Wed, Mar 14, 2018 at 03:43:01PM -0400, Joe Lawrence wrote:
> >> @@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, 
> >> unsigned long id, void *data,
> >>goto exists;
> >>}
> >>  
> >> +  new_shadow->obj = obj;
> >> +  new_shadow->id = id;
> >> +
> >> +  if (init_func) {
> >> +  int err;
> >> +
> >> +  err = init_func(obj, new_shadow->data, init_data);
> > 
> > Am I hallucinating, or will new_shadow->data always be NULL?  How did it
> > even work before?
> >
> struct klp_shadow {
> struct hlist_node node;
> struct rcu_head rcu_head;
> void *obj;
> unsigned long id;
> char data[];<< not a pointer
> };

Ah.  This code needs a nice comment above the kzalloc() call, so I won't
get confused again next time.

-- 
Josh

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Josh Poimboeuf]

On Wed, Mar 14, 2018 at 03:27:02PM -0400, Joe Lawrence wrote:
> On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> > The existing API allows to pass a sample data to initialize the shadow
> > data. It works well when the data are position independent. But it fails
> > miserably when we need to set a pointer to the shadow structure itself.
> > 
> > Unfortunately, we might need to initialize the pointer surprisingly
> > often because of struct list_head. It is even worse because the list
> > might be hidden in other common structures, for example, struct mutex,
> > struct wait_queue_head.
> > 
> > This patch makes the API more safe. A custom init function and data
> > are passed to klp_shadow_*alloc() functions instead of the sample data.
> 
> Yup, this looks kinda familiar, I remember tinkering with the same idea
> last year [1] before settling on the simpler API.
> 
> [1] 
> https://github.com/torvalds/linux/compare/master...joe-lawrence:shadow_variables_v2_c
> 
> > Note that the init_data are not longer a template for the shadow->data.
> > It might point to any data that might be necessary when the init
> > function is called.
> 
> I'm not opposed to changing the API, but I was wondering if you had
> thought about expanding it as an alternative? 
> 
> When working on this last summer, I remember holding onto to some less
> than intuitive naming conventions so that I could support a basic API
> and an extended API with bells and whistles like this patchset
> implements.  It didn't seem too difficult to layer the basic API ontop
> of one like this (see [1] for example), so maybe that's an option to
> keep basic shadow variable usage a little simpler.  /two cents

I like Petr's new API.  It's not a big deal to just pass a couple of
NULLs if you don't need the callback.

And I prefer fewer functions anyway -- maybe it's my functionitis
allergies acting up again.

> Perhaps shadow variables are another candidate for some kind of
> kselftest?

Indeed!

-- 
Josh

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Josh Poimboeuf]

On Wed, Mar 14, 2018 at 03:27:02PM -0400, Joe Lawrence wrote:
> On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> > The existing API allows to pass a sample data to initialize the shadow
> > data. It works well when the data are position independent. But it fails
> > miserably when we need to set a pointer to the shadow structure itself.
> > 
> > Unfortunately, we might need to initialize the pointer surprisingly
> > often because of struct list_head. It is even worse because the list
> > might be hidden in other common structures, for example, struct mutex,
> > struct wait_queue_head.
> > 
> > This patch makes the API more safe. A custom init function and data
> > are passed to klp_shadow_*alloc() functions instead of the sample data.
> 
> Yup, this looks kinda familiar, I remember tinkering with the same idea
> last year [1] before settling on the simpler API.
> 
> [1] 
> https://github.com/torvalds/linux/compare/master...joe-lawrence:shadow_variables_v2_c
> 
> > Note that the init_data are not longer a template for the shadow->data.
> > It might point to any data that might be necessary when the init
> > function is called.
> 
> I'm not opposed to changing the API, but I was wondering if you had
> thought about expanding it as an alternative? 
> 
> When working on this last summer, I remember holding onto to some less
> than intuitive naming conventions so that I could support a basic API
> and an extended API with bells and whistles like this patchset
> implements.  It didn't seem too difficult to layer the basic API ontop
> of one like this (see [1] for example), so maybe that's an option to
> keep basic shadow variable usage a little simpler.  /two cents

I like Petr's new API.  It's not a big deal to just pass a couple of
NULLs if you don't need the callback.

And I prefer fewer functions anyway -- maybe it's my functionitis
allergies acting up again.

> Perhaps shadow variables are another candidate for some kind of
> kselftest?

Indeed!

-- 
Josh

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Joe Lawrence]

On 03/14/2018 03:28 PM, Josh Poimboeuf wrote:
> On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> 
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 4754f01c1abb..fc7c64ce0992 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -186,11 +186,20 @@ static inline bool klp_have_reliable_stack(void)
>> IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
>>  }
>>  
>> +struct klp_shadow;
> 
> Why is this forward struct declaration needed?

Compiles ok w/o it, so shouldn't be needed.

>> @@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, 
>> unsigned long id, void *data,
>>  goto exists;
>>  }
>>  
>> +new_shadow->obj = obj;
>> +new_shadow->id = id;
>> +
>> +if (init_func) {
>> +int err;
>> +
>> +err = init_func(obj, new_shadow->data, init_data);
> 
> Am I hallucinating, or will new_shadow->data always be NULL?  How did it
> even work before?
>
struct klp_shadow {
struct hlist_node node;
struct rcu_head rcu_head;
void *obj;
unsigned long id;
char data[];<< not a pointer
};

In the past, this function would allocate the klp_shadow struct size
accordingly, then memcpy in its data contents.  This patch pushes the
responsibility of data initialization out to the init_func().

-- Joe

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Joe Lawrence]

On 03/14/2018 03:28 PM, Josh Poimboeuf wrote:
> On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> 
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 4754f01c1abb..fc7c64ce0992 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -186,11 +186,20 @@ static inline bool klp_have_reliable_stack(void)
>> IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
>>  }
>>  
>> +struct klp_shadow;
> 
> Why is this forward struct declaration needed?

Compiles ok w/o it, so shouldn't be needed.

>> @@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, 
>> unsigned long id, void *data,
>>  goto exists;
>>  }
>>  
>> +new_shadow->obj = obj;
>> +new_shadow->id = id;
>> +
>> +if (init_func) {
>> +int err;
>> +
>> +err = init_func(obj, new_shadow->data, init_data);
> 
> Am I hallucinating, or will new_shadow->data always be NULL?  How did it
> even work before?
>
struct klp_shadow {
struct hlist_node node;
struct rcu_head rcu_head;
void *obj;
unsigned long id;
char data[];<< not a pointer
};

In the past, this function would allocate the klp_shadow struct size
accordingly, then memcpy in its data contents.  This patch pushes the
responsibility of data initialization out to the init_func().

-- Joe

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Josh Poimboeuf]

On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> The existing API allows to pass a sample data to initialize the shadow
> data. It works well when the data are position independent. But it fails
> miserably when we need to set a pointer to the shadow structure itself.
> 
> Unfortunately, we might need to initialize the pointer surprisingly
> often because of struct list_head. It is even worse because the list
> might be hidden in other common structures, for example, struct mutex,
> struct wait_queue_head.

Can you provide a specific example in the changelog of where this was
needed?

> This patch makes the API more safe. A custom init function and data
> are passed to klp_shadow_*alloc() functions instead of the sample data.
> 
> Note that the init_data are not longer a template for the shadow->data.
> It might point to any data that might be necessary when the init
> function is called.
> 
> In addition, the newly allocated shadow structure is initialized
> only when it is really used.

I don't understand this sentence.  It makes it sound like the init
function is called when you do klp_shadow_get().  However, looking at
the code, the init function is always called after allocation.

> For this, the init function must be
> called under klp_shadow_lock. On one hand, this adds a risk of
> ABBA deadlocks. On the other hand, it allows to do some operations
> safely. For example, we could add the new structure into an
> existing list.

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 4754f01c1abb..fc7c64ce0992 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -186,11 +186,20 @@ static inline bool klp_have_reliable_stack(void)
>  IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
>  }
>  
> +struct klp_shadow;

Why is this forward struct declaration needed?

> @@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, 
> unsigned long id, void *data,
>   goto exists;
>   }
>  
> + new_shadow->obj = obj;
> + new_shadow->id = id;
> +
> + if (init_func) {
> + int err;
> +
> + err = init_func(obj, new_shadow->data, init_data);

Am I hallucinating, or will new_shadow->data always be NULL?  How did it
even work before?

-- 
Josh

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Josh Poimboeuf]

On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> The existing API allows to pass a sample data to initialize the shadow
> data. It works well when the data are position independent. But it fails
> miserably when we need to set a pointer to the shadow structure itself.
> 
> Unfortunately, we might need to initialize the pointer surprisingly
> often because of struct list_head. It is even worse because the list
> might be hidden in other common structures, for example, struct mutex,
> struct wait_queue_head.

Can you provide a specific example in the changelog of where this was
needed?

> This patch makes the API more safe. A custom init function and data
> are passed to klp_shadow_*alloc() functions instead of the sample data.
> 
> Note that the init_data are not longer a template for the shadow->data.
> It might point to any data that might be necessary when the init
> function is called.
> 
> In addition, the newly allocated shadow structure is initialized
> only when it is really used.

I don't understand this sentence.  It makes it sound like the init
function is called when you do klp_shadow_get().  However, looking at
the code, the init function is always called after allocation.

> For this, the init function must be
> called under klp_shadow_lock. On one hand, this adds a risk of
> ABBA deadlocks. On the other hand, it allows to do some operations
> safely. For example, we could add the new structure into an
> existing list.

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 4754f01c1abb..fc7c64ce0992 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -186,11 +186,20 @@ static inline bool klp_have_reliable_stack(void)
>  IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
>  }
>  
> +struct klp_shadow;

Why is this forward struct declaration needed?

> @@ -150,6 +145,23 @@ static void *__klp_shadow_get_or_alloc(void *obj, 
> unsigned long id, void *data,
>   goto exists;
>   }
>  
> + new_shadow->obj = obj;
> + new_shadow->id = id;
> +
> + if (init_func) {
> + int err;
> +
> + err = init_func(obj, new_shadow->data, init_data);

Am I hallucinating, or will new_shadow->data always be NULL?  How did it
even work before?

-- 
Josh

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Joe Lawrence]

On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> The existing API allows to pass a sample data to initialize the shadow
> data. It works well when the data are position independent. But it fails
> miserably when we need to set a pointer to the shadow structure itself.
> 
> Unfortunately, we might need to initialize the pointer surprisingly
> often because of struct list_head. It is even worse because the list
> might be hidden in other common structures, for example, struct mutex,
> struct wait_queue_head.
> 
> This patch makes the API more safe. A custom init function and data
> are passed to klp_shadow_*alloc() functions instead of the sample data.

Yup, this looks kinda familiar, I remember tinkering with the same idea
last year [1] before settling on the simpler API.

[1] 
https://github.com/torvalds/linux/compare/master...joe-lawrence:shadow_variables_v2_c

> Note that the init_data are not longer a template for the shadow->data.
> It might point to any data that might be necessary when the init
> function is called.

I'm not opposed to changing the API, but I was wondering if you had
thought about expanding it as an alternative? 

When working on this last summer, I remember holding onto to some less
than intuitive naming conventions so that I could support a basic API
and an extended API with bells and whistles like this patchset
implements.  It didn't seem too difficult to layer the basic API ontop
of one like this (see [1] for example), so maybe that's an option to
keep basic shadow variable usage a little simpler.  /two cents

> In addition, the newly allocated shadow structure is initialized
> only when it is really used. For this, the init function must be
> called under klp_shadow_lock. On one hand, this adds a risk of
> ABBA deadlocks. On the other hand, it allows to do some operations
> safely. For example, we could add the new structure into an
> existing list.
> 
> Reported-by: Nicolai Stange 
> Signed-off-by: Petr Mladek 
> ---
>  Documentation/livepatch/shadow-vars.txt   | 32 +++--
>  include/linux/livepatch.h | 17 ---
>  kernel/livepatch/shadow.c | 48 
> +--
>  samples/livepatch/livepatch-shadow-fix1.c | 19 +++-
>  samples/livepatch/livepatch-shadow-fix2.c |  6 ++--
>  5 files changed, 89 insertions(+), 33 deletions(-)
> 
> diff --git a/Documentation/livepatch/shadow-vars.txt 
> b/Documentation/livepatch/shadow-vars.txt
> [ ... snip ...]
> @@ -148,16 +154,24 @@ shadow variables to parents already in-flight.
>  For commit 1d147bfa6429, a good spot to allocate a shadow spinlock is
>  inside ieee80211_sta_ps_deliver_wakeup():
>  
> +int ps_lock_shadow_init(void *obj, void *shadow_data, void *data)
> +{
> + spinlock_t *lock = shadow_data;
> +
> + spin_lock_init(lock);
> + return 0;
> +}
> +
>  #define PS_LOCK 1
>  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_alloc(sta, PS_LOCK,
> - _lock_fallback, sizeof(ps_lock_fallback),
> - GFP_ATOMIC);
> + sizeof(ps_lock_fallback), GFP_ATOMIC,

I think this should be "sizeof(*ps_lock)" here since we've removed the
ps_lock_fallback.

> + ps_lock_shadow_init, NULL);
> +
>   if (ps_lock)
>   spin_lock(ps_lock);

The rest of this patchset looks pretty good.  I gave the samples a
test-run and they still operate as advertised.  Perhaps shadow variables
are another candidate for some kind of kselftest?

Regards,

-- Joe

Re: [PATCH 1/2] livepatch: Initialize shadow variables by init function safely

[Joe Lawrence]

On Tue, Mar 13, 2018 at 04:54:47PM +0100, Petr Mladek wrote:
> The existing API allows to pass a sample data to initialize the shadow
> data. It works well when the data are position independent. But it fails
> miserably when we need to set a pointer to the shadow structure itself.
> 
> Unfortunately, we might need to initialize the pointer surprisingly
> often because of struct list_head. It is even worse because the list
> might be hidden in other common structures, for example, struct mutex,
> struct wait_queue_head.
> 
> This patch makes the API more safe. A custom init function and data
> are passed to klp_shadow_*alloc() functions instead of the sample data.

Yup, this looks kinda familiar, I remember tinkering with the same idea
last year [1] before settling on the simpler API.

[1] 
https://github.com/torvalds/linux/compare/master...joe-lawrence:shadow_variables_v2_c

> Note that the init_data are not longer a template for the shadow->data.
> It might point to any data that might be necessary when the init
> function is called.

I'm not opposed to changing the API, but I was wondering if you had
thought about expanding it as an alternative? 

When working on this last summer, I remember holding onto to some less
than intuitive naming conventions so that I could support a basic API
and an extended API with bells and whistles like this patchset
implements.  It didn't seem too difficult to layer the basic API ontop
of one like this (see [1] for example), so maybe that's an option to
keep basic shadow variable usage a little simpler.  /two cents

> In addition, the newly allocated shadow structure is initialized
> only when it is really used. For this, the init function must be
> called under klp_shadow_lock. On one hand, this adds a risk of
> ABBA deadlocks. On the other hand, it allows to do some operations
> safely. For example, we could add the new structure into an
> existing list.
> 
> Reported-by: Nicolai Stange 
> Signed-off-by: Petr Mladek 
> ---
>  Documentation/livepatch/shadow-vars.txt   | 32 +++--
>  include/linux/livepatch.h | 17 ---
>  kernel/livepatch/shadow.c | 48 
> +--
>  samples/livepatch/livepatch-shadow-fix1.c | 19 +++-
>  samples/livepatch/livepatch-shadow-fix2.c |  6 ++--
>  5 files changed, 89 insertions(+), 33 deletions(-)
> 
> diff --git a/Documentation/livepatch/shadow-vars.txt 
> b/Documentation/livepatch/shadow-vars.txt
> [ ... snip ...]
> @@ -148,16 +154,24 @@ shadow variables to parents already in-flight.
>  For commit 1d147bfa6429, a good spot to allocate a shadow spinlock is
>  inside ieee80211_sta_ps_deliver_wakeup():
>  
> +int ps_lock_shadow_init(void *obj, void *shadow_data, void *data)
> +{
> + spinlock_t *lock = shadow_data;
> +
> + spin_lock_init(lock);
> + return 0;
> +}
> +
>  #define PS_LOCK 1
>  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_alloc(sta, PS_LOCK,
> - _lock_fallback, sizeof(ps_lock_fallback),
> - GFP_ATOMIC);
> + sizeof(ps_lock_fallback), GFP_ATOMIC,

I think this should be "sizeof(*ps_lock)" here since we've removed the
ps_lock_fallback.

> + ps_lock_shadow_init, NULL);
> +
>   if (ps_lock)
>   spin_lock(ps_lock);

The rest of this patchset looks pretty good.  I gave the samples a
test-run and they still operate as advertised.  Perhaps shadow variables
are another candidate for some kind of kselftest?

Regards,

-- Joe