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 <joe.lawrence@xxxxxxxxxx>
>>>
>>> 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(&klp_shadow_lock, flags);
>>>> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
>>>> +	spin_unlock_irqrestore(&klp_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