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.lawre...@redhat.com>
>>>
>>> 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
