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

[Miroslav Benes]

> > +struct klp_shadow {
> > +   struct hlist_node node;
> > +   struct rcu_head rcu_head;
> > +   void *obj;
> > +   char *var;
> > +   void *data;
> 
> I would make the meaning more obvious. What about renaming?
> 
>   var -> key or id
>   data -> shadow_obj or new_obj

But var is not a key to a hash table. obj is. Renaming obj to key would be 
misleading in my opinion, because it IS a pointer to an object. And data 
is ok too, as far as I'm concerned. Just saying.

But yes, I'd welcome a description too.

Miroslav

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

[Miroslav Benes]

> > +struct klp_shadow {
> > +   struct hlist_node node;
> > +   struct rcu_head rcu_head;
> > +   void *obj;
> > +   char *var;
> > +   void *data;
> 
> I would make the meaning more obvious. What about renaming?
> 
>   var -> key or id
>   data -> shadow_obj or new_obj

But var is not a key to a hash table. obj is. Renaming obj to key would be 
misleading in my opinion, because it IS a pointer to an object. And data 
is ok too, as far as I'm concerned. Just saying.

But yes, I'd welcome a description too.

Miroslav

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

[Petr Mladek]

On Wed 2017-06-14 08:17:44, Josh Poimboeuf wrote:
> On Wed, Jun 14, 2017 at 02:59:13PM +0200, Petr Mladek wrote:
> > > +}
> > > +
> > > +void klp_shadow_detach(void *obj, char *var)
> > > +{
> > > + unsigned long flags;
> > > + struct klp_shadow *shadow;
> > > +
> > > + spin_lock_irqsave(_shadow_lock, flags);
> > > +
> > > + hash_for_each_possible(klp_shadow_hash, shadow, node,
> > > +(unsigned long)obj) {
> > > + if (shadow->obj == obj && !strcmp(shadow->var, var)) {
> > 
> > Do we need to test "shadow->obj == obj" here? If it is not true,
> > there would be a bug in the hashtable implementation or in
> > klp_shadow_attach().
> > 
> > Well, it might make sense to add a consistency check:
> > 
> >   WARN_ON(shadow->obj != obj);
> > 
> 
> It would make sense if hash_for_each_possible() worked that way, but for
> some reason it doesn't. :-/  It gives you all the hash collisions.

I see. Shame on me. The original code makes perfect sense then.

Best Regards,
Petr

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

[Petr Mladek]

On Wed 2017-06-14 08:17:44, Josh Poimboeuf wrote:
> On Wed, Jun 14, 2017 at 02:59:13PM +0200, Petr Mladek wrote:
> > > +}
> > > +
> > > +void klp_shadow_detach(void *obj, char *var)
> > > +{
> > > + unsigned long flags;
> > > + struct klp_shadow *shadow;
> > > +
> > > + spin_lock_irqsave(_shadow_lock, flags);
> > > +
> > > + hash_for_each_possible(klp_shadow_hash, shadow, node,
> > > +(unsigned long)obj) {
> > > + if (shadow->obj == obj && !strcmp(shadow->var, var)) {
> > 
> > Do we need to test "shadow->obj == obj" here? If it is not true,
> > there would be a bug in the hashtable implementation or in
> > klp_shadow_attach().
> > 
> > Well, it might make sense to add a consistency check:
> > 
> >   WARN_ON(shadow->obj != obj);
> > 
> 
> It would make sense if hash_for_each_possible() worked that way, but for
> some reason it doesn't. :-/  It gives you all the hash collisions.

I see. Shame on me. The original code makes perfect sense then.

Best Regards,
Petr

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

[Josh Poimboeuf]

On Wed, Jun 14, 2017 at 02:59:13PM +0200, Petr Mladek wrote:
> > +}
> > +
> > +void klp_shadow_detach(void *obj, char *var)
> > +{
> > +   unsigned long flags;
> > +   struct klp_shadow *shadow;
> > +
> > +   spin_lock_irqsave(_shadow_lock, flags);
> > +
> > +   hash_for_each_possible(klp_shadow_hash, shadow, node,
> > +  (unsigned long)obj) {
> > +   if (shadow->obj == obj && !strcmp(shadow->var, var)) {
> 
> Do we need to test "shadow->obj == obj" here? If it is not true,
> there would be a bug in the hashtable implementation or in
> klp_shadow_attach().
> 
> Well, it might make sense to add a consistency check:
> 
>   WARN_ON(shadow->obj != obj);
> 

It would make sense if hash_for_each_possible() worked that way, but for
some reason it doesn't. :-/  It gives you all the hash collisions.

-- 
Josh

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

[Josh Poimboeuf]

On Wed, Jun 14, 2017 at 02:59:13PM +0200, Petr Mladek wrote:
> > +}
> > +
> > +void klp_shadow_detach(void *obj, char *var)
> > +{
> > +   unsigned long flags;
> > +   struct klp_shadow *shadow;
> > +
> > +   spin_lock_irqsave(_shadow_lock, flags);
> > +
> > +   hash_for_each_possible(klp_shadow_hash, shadow, node,
> > +  (unsigned long)obj) {
> > +   if (shadow->obj == obj && !strcmp(shadow->var, var)) {
> 
> Do we need to test "shadow->obj == obj" here? If it is not true,
> there would be a bug in the hashtable implementation or in
> klp_shadow_attach().
> 
> Well, it might make sense to add a consistency check:
> 
>   WARN_ON(shadow->obj != obj);
> 

It would make sense if hash_for_each_possible() worked that way, but for
some reason it doesn't. :-/  It gives you all the hash collisions.

-- 
Josh

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

[Petr Mladek]

On Thu 2017-06-01 14:25:24, Joe Lawrence wrote:
> Add three exported API for livepatch modules:
> 
>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>   void klp_shadow_detach(void *obj, char *var);
>   void *klp_shadow_get(void *obj, char *var);
> 
> that implement "shadow" variables, which allow callers to associate new
> shadow fields to existing data structures.

It would be great to explain what shadow variables mean
here. Alternatively I would sqash the 2nd patch into this one.

People might use git blame to search what this API/code is for
and this commit not give much hints.

> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> new file mode 100644
> index ..72d5e567dff9
> --- /dev/null
> +++ b/kernel/livepatch/shadow.c
> @@ -0,0 +1,115 @@
> +/*
> + * shadow.c - Shadow Variables
> + *
> + * Copyright (C) 2014 Josh Poimboeuf 
> + * Copyright (C) 2014 Seth Jennings 
> + * Copyright (C) 2017 Joe Lawrence 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +
> +static DEFINE_HASHTABLE(klp_shadow_hash, 12);
> +static DEFINE_SPINLOCK(klp_shadow_lock);
> +

Please, add comment that will explain the structure members.
See include/linux/livepatch.h for explanation.

> +struct klp_shadow {
> + struct hlist_node node;
> + struct rcu_head rcu_head;
> + void *obj;
> + char *var;
> + void *data;

I would make the meaning more obvious. What about renaming?

  var -> key or id
  data -> shadow_obj or new_obj

> +};
> +

Also the function would deserve a comment explaining the meaning
and parameters.

> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
> +{
> + unsigned long flags;
> + struct klp_shadow *shadow;
> +
> + shadow = kmalloc(sizeof(*shadow), gfp);
> + if (!shadow)
> + return NULL;
> +
> + shadow->obj = obj;
> +
> + shadow->var = kstrdup(var, gfp);

I would use int or long instead of "char *". You do not know
in which context the API will be used. strdup() might be
unnecessarily expensive. You could always use enum or #define
to get readable names for the key.


> + if (!shadow->var) {
> + kfree(shadow);
> + return NULL;
> + }
> +
> + shadow->data = data;
> +
> + spin_lock_irqsave(_shadow_lock, flags);
> + hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
> + spin_unlock_irqrestore(_shadow_lock, flags);
> +
> + return shadow->data;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> +
> +static void klp_shadow_rcu_free(struct rcu_head *head)
> +{
> + struct klp_shadow *shadow;
> +
> + shadow = container_of(head, struct klp_shadow, rcu_head);
> +
> + kfree(shadow->var);
> + kfree(shadow);

If we use int/long instead of the string, we will do not
need a custom free function. free_rcu() will work then.

> +}
> +
> +void klp_shadow_detach(void *obj, char *var)
> +{
> + unsigned long flags;
> + struct klp_shadow *shadow;
> +
> + spin_lock_irqsave(_shadow_lock, flags);
> +
> + hash_for_each_possible(klp_shadow_hash, shadow, node,
> +(unsigned long)obj) {
> + if (shadow->obj == obj && !strcmp(shadow->var, var)) {

Do we need to test "shadow->obj == obj" here? If it is not true,
there would be a bug in the hashtable implementation or in
klp_shadow_attach().

Well, it might make sense to add a consistency check:

  WARN_ON(shadow->obj != obj);


> + hash_del_rcu(>node);
> + spin_unlock_irqrestore(_shadow_lock, flags);
> + call_rcu(>rcu_head, klp_shadow_rcu_free);

call_rcu() just queues the request. It does not wait for it.
It can be called inside the lock and the code might
be easier:

hash_del_rcu(>node);
call_rcu(>rcu_head,
  klp_shadow_rcu_free);
break;
}
}
spin_unlock_irqrestore(_shadow_lock, flags);
}


Otherwise, I like that the API rather trivial and still useful.

Best Regards,
Petr

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

[Petr Mladek]

On Thu 2017-06-01 14:25:24, Joe Lawrence wrote:
> Add three exported API for livepatch modules:
> 
>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>   void klp_shadow_detach(void *obj, char *var);
>   void *klp_shadow_get(void *obj, char *var);
> 
> that implement "shadow" variables, which allow callers to associate new
> shadow fields to existing data structures.

It would be great to explain what shadow variables mean
here. Alternatively I would sqash the 2nd patch into this one.

People might use git blame to search what this API/code is for
and this commit not give much hints.

> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> new file mode 100644
> index ..72d5e567dff9
> --- /dev/null
> +++ b/kernel/livepatch/shadow.c
> @@ -0,0 +1,115 @@
> +/*
> + * shadow.c - Shadow Variables
> + *
> + * Copyright (C) 2014 Josh Poimboeuf 
> + * Copyright (C) 2014 Seth Jennings 
> + * Copyright (C) 2017 Joe Lawrence 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see .
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +
> +static DEFINE_HASHTABLE(klp_shadow_hash, 12);
> +static DEFINE_SPINLOCK(klp_shadow_lock);
> +

Please, add comment that will explain the structure members.
See include/linux/livepatch.h for explanation.

> +struct klp_shadow {
> + struct hlist_node node;
> + struct rcu_head rcu_head;
> + void *obj;
> + char *var;
> + void *data;

I would make the meaning more obvious. What about renaming?

  var -> key or id
  data -> shadow_obj or new_obj

> +};
> +

Also the function would deserve a comment explaining the meaning
and parameters.

> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
> +{
> + unsigned long flags;
> + struct klp_shadow *shadow;
> +
> + shadow = kmalloc(sizeof(*shadow), gfp);
> + if (!shadow)
> + return NULL;
> +
> + shadow->obj = obj;
> +
> + shadow->var = kstrdup(var, gfp);

I would use int or long instead of "char *". You do not know
in which context the API will be used. strdup() might be
unnecessarily expensive. You could always use enum or #define
to get readable names for the key.


> + if (!shadow->var) {
> + kfree(shadow);
> + return NULL;
> + }
> +
> + shadow->data = data;
> +
> + spin_lock_irqsave(_shadow_lock, flags);
> + hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
> + spin_unlock_irqrestore(_shadow_lock, flags);
> +
> + return shadow->data;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> +
> +static void klp_shadow_rcu_free(struct rcu_head *head)
> +{
> + struct klp_shadow *shadow;
> +
> + shadow = container_of(head, struct klp_shadow, rcu_head);
> +
> + kfree(shadow->var);
> + kfree(shadow);

If we use int/long instead of the string, we will do not
need a custom free function. free_rcu() will work then.

> +}
> +
> +void klp_shadow_detach(void *obj, char *var)
> +{
> + unsigned long flags;
> + struct klp_shadow *shadow;
> +
> + spin_lock_irqsave(_shadow_lock, flags);
> +
> + hash_for_each_possible(klp_shadow_hash, shadow, node,
> +(unsigned long)obj) {
> + if (shadow->obj == obj && !strcmp(shadow->var, var)) {

Do we need to test "shadow->obj == obj" here? If it is not true,
there would be a bug in the hashtable implementation or in
klp_shadow_attach().

Well, it might make sense to add a consistency check:

  WARN_ON(shadow->obj != obj);


> + hash_del_rcu(>node);
> + spin_unlock_irqrestore(_shadow_lock, flags);
> + call_rcu(>rcu_head, klp_shadow_rcu_free);

call_rcu() just queues the request. It does not wait for it.
It can be called inside the lock and the code might
be easier:

hash_del_rcu(>node);
call_rcu(>rcu_head,
  klp_shadow_rcu_free);
break;
}
}
spin_unlock_irqrestore(_shadow_lock, flags);
}


Otherwise, I like that the API rather trivial and still useful.

Best Regards,
Petr

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

[Joe Lawrence]

On 06/09/2017 02:34 PM, Josh Poimboeuf wrote:
> On Fri, Jun 09, 2017 at 11:36:27AM -0400, Joe Lawrence wrote:
>> On 06/08/2017 12:49 PM, Josh Poimboeuf wrote:
>>> On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
 Add three exported API for livepatch modules:

   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
   void klp_shadow_detach(void *obj, char *var);
   void *klp_shadow_get(void *obj, char *var);

 that implement "shadow" variables, which allow callers to associate new
 shadow fields to existing data structures.

 Signed-off-by: Joe Lawrence 
>>>
>>> Overall the patch looks good to me.  It's a simple API but we've found
>>> it to be very useful for certain patches.
>>>
>>> One comment below:
>>>
 +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
 +{
 +  unsigned long flags;
 +  struct klp_shadow *shadow;
 +
 +  shadow = kmalloc(sizeof(*shadow), gfp);
 +  if (!shadow)
 +  return NULL;
 +
 +  shadow->obj = obj;
 +
 +  shadow->var = kstrdup(var, gfp);
 +  if (!shadow->var) {
 +  kfree(shadow);
 +  return NULL;
 +  }
 +
 +  shadow->data = data;
 +
 +  spin_lock_irqsave(_shadow_lock, flags);
 +  hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
 +  spin_unlock_irqrestore(_shadow_lock, flags);
 +
 +  return shadow->data;
 +}
 +EXPORT_SYMBOL_GPL(klp_shadow_attach);
>>>
>>> I wonder if we should worry about people misusing the API by calling
>>> klp_shadow_attach() for a shadow variable that already exists.  Maybe we
>>> should add a check and return NULL if it already exists.
>>>
>>
>> I don't think the API (the shadow or the underlying hash table calls)
>> currently protects against double-adds...  adding a check to do so would
>> probably need to occur with the klp_shadow_lock to protect against
>> concurrent detach calls.
>>
>> I could implement this protection in a v2, or leave it up to the caller.
>>  What do you think?
> 
> Yeah, I don't have a strong opinion either way.  It's fine with me to
> leave it as it is and trust the patch author not to mess it up.
> 

Without having encountered this in our kpatch shadow variable
experiences, I'm not sure which is better:

  1 - leave it as is, caller beware
  2 - return the existing shadow var
  3 - fail the double-add, differentiate failure with ERR_PTR

I'm leaning (1) to keep it simple, but since this an exported interface,
if any of the other options are any more "future-proof", I'd take that
into consideration.

-- Joe

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

[Joe Lawrence]

On 06/09/2017 02:34 PM, Josh Poimboeuf wrote:
> On Fri, Jun 09, 2017 at 11:36:27AM -0400, Joe Lawrence wrote:
>> On 06/08/2017 12:49 PM, Josh Poimboeuf wrote:
>>> On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
 Add three exported API for livepatch modules:

   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
   void klp_shadow_detach(void *obj, char *var);
   void *klp_shadow_get(void *obj, char *var);

 that implement "shadow" variables, which allow callers to associate new
 shadow fields to existing data structures.

 Signed-off-by: Joe Lawrence 
>>>
>>> Overall the patch looks good to me.  It's a simple API but we've found
>>> it to be very useful for certain patches.
>>>
>>> One comment below:
>>>
 +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
 +{
 +  unsigned long flags;
 +  struct klp_shadow *shadow;
 +
 +  shadow = kmalloc(sizeof(*shadow), gfp);
 +  if (!shadow)
 +  return NULL;
 +
 +  shadow->obj = obj;
 +
 +  shadow->var = kstrdup(var, gfp);
 +  if (!shadow->var) {
 +  kfree(shadow);
 +  return NULL;
 +  }
 +
 +  shadow->data = data;
 +
 +  spin_lock_irqsave(_shadow_lock, flags);
 +  hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
 +  spin_unlock_irqrestore(_shadow_lock, flags);
 +
 +  return shadow->data;
 +}
 +EXPORT_SYMBOL_GPL(klp_shadow_attach);
>>>
>>> I wonder if we should worry about people misusing the API by calling
>>> klp_shadow_attach() for a shadow variable that already exists.  Maybe we
>>> should add a check and return NULL if it already exists.
>>>
>>
>> I don't think the API (the shadow or the underlying hash table calls)
>> currently protects against double-adds...  adding a check to do so would
>> probably need to occur with the klp_shadow_lock to protect against
>> concurrent detach calls.
>>
>> I could implement this protection in a v2, or leave it up to the caller.
>>  What do you think?
> 
> Yeah, I don't have a strong opinion either way.  It's fine with me to
> leave it as it is and trust the patch author not to mess it up.
> 

Without having encountered this in our kpatch shadow variable
experiences, I'm not sure which is better:

  1 - leave it as is, caller beware
  2 - return the existing shadow var
  3 - fail the double-add, differentiate failure with ERR_PTR

I'm leaning (1) to keep it simple, but since this an exported interface,
if any of the other options are any more "future-proof", I'd take that
into consideration.

-- Joe

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

[Josh Poimboeuf]

On Fri, Jun 09, 2017 at 11:36:27AM -0400, Joe Lawrence wrote:
> On 06/08/2017 12:49 PM, Josh Poimboeuf wrote:
> > On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
> >> Add three exported API for livepatch modules:
> >>
> >>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
> >>   void klp_shadow_detach(void *obj, char *var);
> >>   void *klp_shadow_get(void *obj, char *var);
> >>
> >> that implement "shadow" variables, which allow callers to associate new
> >> shadow fields to existing data structures.
> >>
> >> Signed-off-by: Joe Lawrence 
> > 
> > Overall the patch looks good to me.  It's a simple API but we've found
> > it to be very useful for certain patches.
> > 
> > One comment below:
> > 
> >> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
> >> +{
> >> +  unsigned long flags;
> >> +  struct klp_shadow *shadow;
> >> +
> >> +  shadow = kmalloc(sizeof(*shadow), gfp);
> >> +  if (!shadow)
> >> +  return NULL;
> >> +
> >> +  shadow->obj = obj;
> >> +
> >> +  shadow->var = kstrdup(var, gfp);
> >> +  if (!shadow->var) {
> >> +  kfree(shadow);
> >> +  return NULL;
> >> +  }
> >> +
> >> +  shadow->data = data;
> >> +
> >> +  spin_lock_irqsave(_shadow_lock, flags);
> >> +  hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
> >> +  spin_unlock_irqrestore(_shadow_lock, flags);
> >> +
> >> +  return shadow->data;
> >> +}
> >> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> > 
> > I wonder if we should worry about people misusing the API by calling
> > klp_shadow_attach() for a shadow variable that already exists.  Maybe we
> > should add a check and return NULL if it already exists.
> > 
> 
> I don't think the API (the shadow or the underlying hash table calls)
> currently protects against double-adds...  adding a check to do so would
> probably need to occur with the klp_shadow_lock to protect against
> concurrent detach calls.
> 
> I could implement this protection in a v2, or leave it up to the caller.
>  What do you think?

Yeah, I don't have a strong opinion either way.  It's fine with me to
leave it as it is and trust the patch author not to mess it up.

-- 
Josh

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

[Josh Poimboeuf]

On Fri, Jun 09, 2017 at 11:36:27AM -0400, Joe Lawrence wrote:
> On 06/08/2017 12:49 PM, Josh Poimboeuf wrote:
> > On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
> >> Add three exported API for livepatch modules:
> >>
> >>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
> >>   void klp_shadow_detach(void *obj, char *var);
> >>   void *klp_shadow_get(void *obj, char *var);
> >>
> >> that implement "shadow" variables, which allow callers to associate new
> >> shadow fields to existing data structures.
> >>
> >> Signed-off-by: Joe Lawrence 
> > 
> > Overall the patch looks good to me.  It's a simple API but we've found
> > it to be very useful for certain patches.
> > 
> > One comment below:
> > 
> >> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
> >> +{
> >> +  unsigned long flags;
> >> +  struct klp_shadow *shadow;
> >> +
> >> +  shadow = kmalloc(sizeof(*shadow), gfp);
> >> +  if (!shadow)
> >> +  return NULL;
> >> +
> >> +  shadow->obj = obj;
> >> +
> >> +  shadow->var = kstrdup(var, gfp);
> >> +  if (!shadow->var) {
> >> +  kfree(shadow);
> >> +  return NULL;
> >> +  }
> >> +
> >> +  shadow->data = data;
> >> +
> >> +  spin_lock_irqsave(_shadow_lock, flags);
> >> +  hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
> >> +  spin_unlock_irqrestore(_shadow_lock, flags);
> >> +
> >> +  return shadow->data;
> >> +}
> >> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> > 
> > I wonder if we should worry about people misusing the API by calling
> > klp_shadow_attach() for a shadow variable that already exists.  Maybe we
> > should add a check and return NULL if it already exists.
> > 
> 
> I don't think the API (the shadow or the underlying hash table calls)
> currently protects against double-adds...  adding a check to do so would
> probably need to occur with the klp_shadow_lock to protect against
> concurrent detach calls.
> 
> I could implement this protection in a v2, or leave it up to the caller.
>  What do you think?

Yeah, I don't have a strong opinion either way.  It's fine with me to
leave it as it is and trust the patch author not to mess it up.

-- 
Josh

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

[Joe Lawrence]

On 06/08/2017 12:49 PM, Josh Poimboeuf wrote:
> On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
>> Add three exported API for livepatch modules:
>>
>>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>>   void klp_shadow_detach(void *obj, char *var);
>>   void *klp_shadow_get(void *obj, char *var);
>>
>> that implement "shadow" variables, which allow callers to associate new
>> shadow fields to existing data structures.
>>
>> Signed-off-by: Joe Lawrence 
> 
> Overall the patch looks good to me.  It's a simple API but we've found
> it to be very useful for certain patches.
> 
> One comment below:
> 
>> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
>> +{
>> +unsigned long flags;
>> +struct klp_shadow *shadow;
>> +
>> +shadow = kmalloc(sizeof(*shadow), gfp);
>> +if (!shadow)
>> +return NULL;
>> +
>> +shadow->obj = obj;
>> +
>> +shadow->var = kstrdup(var, gfp);
>> +if (!shadow->var) {
>> +kfree(shadow);
>> +return NULL;
>> +}
>> +
>> +shadow->data = data;
>> +
>> +spin_lock_irqsave(_shadow_lock, flags);
>> +hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
>> +spin_unlock_irqrestore(_shadow_lock, flags);
>> +
>> +return shadow->data;
>> +}
>> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> 
> I wonder if we should worry about people misusing the API by calling
> klp_shadow_attach() for a shadow variable that already exists.  Maybe we
> should add a check and return NULL if it already exists.
> 

I don't think the API (the shadow or the underlying hash table calls)
currently protects against double-adds...  adding a check to do so would
probably need to occur with the klp_shadow_lock to protect against
concurrent detach calls.

I could implement this protection in a v2, or leave it up to the caller.
 What do you think?

-- Joe

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

[Joe Lawrence]

On 06/08/2017 12:49 PM, Josh Poimboeuf wrote:
> On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
>> Add three exported API for livepatch modules:
>>
>>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>>   void klp_shadow_detach(void *obj, char *var);
>>   void *klp_shadow_get(void *obj, char *var);
>>
>> that implement "shadow" variables, which allow callers to associate new
>> shadow fields to existing data structures.
>>
>> Signed-off-by: Joe Lawrence 
> 
> Overall the patch looks good to me.  It's a simple API but we've found
> it to be very useful for certain patches.
> 
> One comment below:
> 
>> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
>> +{
>> +unsigned long flags;
>> +struct klp_shadow *shadow;
>> +
>> +shadow = kmalloc(sizeof(*shadow), gfp);
>> +if (!shadow)
>> +return NULL;
>> +
>> +shadow->obj = obj;
>> +
>> +shadow->var = kstrdup(var, gfp);
>> +if (!shadow->var) {
>> +kfree(shadow);
>> +return NULL;
>> +}
>> +
>> +shadow->data = data;
>> +
>> +spin_lock_irqsave(_shadow_lock, flags);
>> +hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
>> +spin_unlock_irqrestore(_shadow_lock, flags);
>> +
>> +return shadow->data;
>> +}
>> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> 
> I wonder if we should worry about people misusing the API by calling
> klp_shadow_attach() for a shadow variable that already exists.  Maybe we
> should add a check and return NULL if it already exists.
> 

I don't think the API (the shadow or the underlying hash table calls)
currently protects against double-adds...  adding a check to do so would
probably need to occur with the klp_shadow_lock to protect against
concurrent detach calls.

I could implement this protection in a v2, or leave it up to the caller.
 What do you think?

-- Joe

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

[Josh Poimboeuf]

On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
> Add three exported API for livepatch modules:
> 
>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>   void klp_shadow_detach(void *obj, char *var);
>   void *klp_shadow_get(void *obj, char *var);
> 
> that implement "shadow" variables, which allow callers to associate new
> shadow fields to existing data structures.
> 
> Signed-off-by: Joe Lawrence 

Overall the patch looks good to me.  It's a simple API but we've found
it to be very useful for certain patches.

One comment below:

> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
> +{
> + unsigned long flags;
> + struct klp_shadow *shadow;
> +
> + shadow = kmalloc(sizeof(*shadow), gfp);
> + if (!shadow)
> + return NULL;
> +
> + shadow->obj = obj;
> +
> + shadow->var = kstrdup(var, gfp);
> + if (!shadow->var) {
> + kfree(shadow);
> + return NULL;
> + }
> +
> + shadow->data = data;
> +
> + spin_lock_irqsave(_shadow_lock, flags);
> + hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
> + spin_unlock_irqrestore(_shadow_lock, flags);
> +
> + return shadow->data;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);

I wonder if we should worry about people misusing the API by calling
klp_shadow_attach() for a shadow variable that already exists.  Maybe we
should add a check and return NULL if it already exists.

-- 
Josh

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

[Josh Poimboeuf]

On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
> Add three exported API for livepatch modules:
> 
>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>   void klp_shadow_detach(void *obj, char *var);
>   void *klp_shadow_get(void *obj, char *var);
> 
> that implement "shadow" variables, which allow callers to associate new
> shadow fields to existing data structures.
> 
> Signed-off-by: Joe Lawrence 

Overall the patch looks good to me.  It's a simple API but we've found
it to be very useful for certain patches.

One comment below:

> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
> +{
> + unsigned long flags;
> + struct klp_shadow *shadow;
> +
> + shadow = kmalloc(sizeof(*shadow), gfp);
> + if (!shadow)
> + return NULL;
> +
> + shadow->obj = obj;
> +
> + shadow->var = kstrdup(var, gfp);
> + if (!shadow->var) {
> + kfree(shadow);
> + return NULL;
> + }
> +
> + shadow->data = data;
> +
> + spin_lock_irqsave(_shadow_lock, flags);
> + hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
> + spin_unlock_irqrestore(_shadow_lock, flags);
> +
> + return shadow->data;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);

I wonder if we should worry about people misusing the API by calling
klp_shadow_attach() for a shadow variable that already exists.  Maybe we
should add a check and return NULL if it already exists.

-- 
Josh

[PATCH 1/3] livepatch: introduce shadow variable API

[Joe Lawrence]

Add three exported API for livepatch modules:

  void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
  void klp_shadow_detach(void *obj, char *var);
  void *klp_shadow_get(void *obj, char *var);

that implement "shadow" variables, which allow callers to associate new
shadow fields to existing data structures.

Signed-off-by: Joe Lawrence 
---
 include/linux/livepatch.h |   4 ++
 kernel/livepatch/Makefile |   2 +-
 kernel/livepatch/shadow.c | 115 ++
 3 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 kernel/livepatch/shadow.c

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..a0d068ecf7f8 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -164,6 +164,10 @@ static inline bool klp_have_reliable_stack(void)
   IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
 }
 
+void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
+void klp_shadow_detach(void *obj, char *var);
+void *klp_shadow_get(void *obj, char *var);
+
 #else /* !CONFIG_LIVEPATCH */
 
 static inline int klp_module_coming(struct module *mod) { return 0; }
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index 2b8bdb1925da..b36ceda6488e 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
-livepatch-objs := core.o patch.o transition.o
+livepatch-objs := core.o patch.o shadow.o transition.o
diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
new file mode 100644
index ..72d5e567dff9
--- /dev/null
+++ b/kernel/livepatch/shadow.c
@@ -0,0 +1,115 @@
+/*
+ * shadow.c - Shadow Variables
+ *
+ * Copyright (C) 2014 Josh Poimboeuf 
+ * Copyright (C) 2014 Seth Jennings 
+ * Copyright (C) 2017 Joe Lawrence 
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include 
+#include 
+#include 
+
+static DEFINE_HASHTABLE(klp_shadow_hash, 12);
+static DEFINE_SPINLOCK(klp_shadow_lock);
+
+struct klp_shadow {
+   struct hlist_node node;
+   struct rcu_head rcu_head;
+   void *obj;
+   char *var;
+   void *data;
+};
+
+void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
+{
+   unsigned long flags;
+   struct klp_shadow *shadow;
+
+   shadow = kmalloc(sizeof(*shadow), gfp);
+   if (!shadow)
+   return NULL;
+
+   shadow->obj = obj;
+
+   shadow->var = kstrdup(var, gfp);
+   if (!shadow->var) {
+   kfree(shadow);
+   return NULL;
+   }
+
+   shadow->data = data;
+
+   spin_lock_irqsave(_shadow_lock, flags);
+   hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
+   spin_unlock_irqrestore(_shadow_lock, flags);
+
+   return shadow->data;
+}
+EXPORT_SYMBOL_GPL(klp_shadow_attach);
+
+static void klp_shadow_rcu_free(struct rcu_head *head)
+{
+   struct klp_shadow *shadow;
+
+   shadow = container_of(head, struct klp_shadow, rcu_head);
+
+   kfree(shadow->var);
+   kfree(shadow);
+}
+
+void klp_shadow_detach(void *obj, char *var)
+{
+   unsigned long flags;
+   struct klp_shadow *shadow;
+
+   spin_lock_irqsave(_shadow_lock, flags);
+
+   hash_for_each_possible(klp_shadow_hash, shadow, node,
+  (unsigned long)obj) {
+   if (shadow->obj == obj && !strcmp(shadow->var, var)) {
+   hash_del_rcu(>node);
+   spin_unlock_irqrestore(_shadow_lock, flags);
+   call_rcu(>rcu_head, klp_shadow_rcu_free);
+   return;
+   }
+   }
+
+   spin_unlock_irqrestore(_shadow_lock, flags);
+}
+EXPORT_SYMBOL_GPL(klp_shadow_detach);
+
+void *klp_shadow_get(void *obj, char *var)
+{
+   struct klp_shadow *shadow;
+
+   rcu_read_lock();
+
+   hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
+  (unsigned long)obj) {
+   if (shadow->obj == obj && !strcmp(shadow->var, var)) {
+   rcu_read_unlock();
+   return shadow->data;
+   }
+   }
+
+   rcu_read_unlock();
+
+   

[PATCH 1/3] livepatch: introduce shadow variable API

[Joe Lawrence]

Add three exported API for livepatch modules:

  void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
  void klp_shadow_detach(void *obj, char *var);
  void *klp_shadow_get(void *obj, char *var);

that implement "shadow" variables, which allow callers to associate new
shadow fields to existing data structures.

Signed-off-by: Joe Lawrence 
---
 include/linux/livepatch.h |   4 ++
 kernel/livepatch/Makefile |   2 +-
 kernel/livepatch/shadow.c | 115 ++
 3 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 kernel/livepatch/shadow.c

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..a0d068ecf7f8 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -164,6 +164,10 @@ static inline bool klp_have_reliable_stack(void)
   IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
 }
 
+void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
+void klp_shadow_detach(void *obj, char *var);
+void *klp_shadow_get(void *obj, char *var);
+
 #else /* !CONFIG_LIVEPATCH */
 
 static inline int klp_module_coming(struct module *mod) { return 0; }
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index 2b8bdb1925da..b36ceda6488e 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
-livepatch-objs := core.o patch.o transition.o
+livepatch-objs := core.o patch.o shadow.o transition.o
diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
new file mode 100644
index ..72d5e567dff9
--- /dev/null
+++ b/kernel/livepatch/shadow.c
@@ -0,0 +1,115 @@
+/*
+ * shadow.c - Shadow Variables
+ *
+ * Copyright (C) 2014 Josh Poimboeuf 
+ * Copyright (C) 2014 Seth Jennings 
+ * Copyright (C) 2017 Joe Lawrence 
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include 
+#include 
+#include 
+
+static DEFINE_HASHTABLE(klp_shadow_hash, 12);
+static DEFINE_SPINLOCK(klp_shadow_lock);
+
+struct klp_shadow {
+   struct hlist_node node;
+   struct rcu_head rcu_head;
+   void *obj;
+   char *var;
+   void *data;
+};
+
+void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
+{
+   unsigned long flags;
+   struct klp_shadow *shadow;
+
+   shadow = kmalloc(sizeof(*shadow), gfp);
+   if (!shadow)
+   return NULL;
+
+   shadow->obj = obj;
+
+   shadow->var = kstrdup(var, gfp);
+   if (!shadow->var) {
+   kfree(shadow);
+   return NULL;
+   }
+
+   shadow->data = data;
+
+   spin_lock_irqsave(_shadow_lock, flags);
+   hash_add_rcu(klp_shadow_hash, >node, (unsigned long)obj);
+   spin_unlock_irqrestore(_shadow_lock, flags);
+
+   return shadow->data;
+}
+EXPORT_SYMBOL_GPL(klp_shadow_attach);
+
+static void klp_shadow_rcu_free(struct rcu_head *head)
+{
+   struct klp_shadow *shadow;
+
+   shadow = container_of(head, struct klp_shadow, rcu_head);
+
+   kfree(shadow->var);
+   kfree(shadow);
+}
+
+void klp_shadow_detach(void *obj, char *var)
+{
+   unsigned long flags;
+   struct klp_shadow *shadow;
+
+   spin_lock_irqsave(_shadow_lock, flags);
+
+   hash_for_each_possible(klp_shadow_hash, shadow, node,
+  (unsigned long)obj) {
+   if (shadow->obj == obj && !strcmp(shadow->var, var)) {
+   hash_del_rcu(>node);
+   spin_unlock_irqrestore(_shadow_lock, flags);
+   call_rcu(>rcu_head, klp_shadow_rcu_free);
+   return;
+   }
+   }
+
+   spin_unlock_irqrestore(_shadow_lock, flags);
+}
+EXPORT_SYMBOL_GPL(klp_shadow_detach);
+
+void *klp_shadow_get(void *obj, char *var)
+{
+   struct klp_shadow *shadow;
+
+   rcu_read_lock();
+
+   hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
+  (unsigned long)obj) {
+   if (shadow->obj == obj && !strcmp(shadow->var, var)) {
+   rcu_read_unlock();
+   return shadow->data;
+   }
+   }
+
+   rcu_read_unlock();
+
+   return NULL;
+}
+EXPORT_SYMBOL_GPL(klp_shadow_get);
-- 
1.8.3.1