DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 11158C04B935
Subject: Re: [PATCH 1/3] livepatch: introduce shadow variable API
To: Josh Poimboeuf <jpoimboe@redhat.com>
References: <1496341526-19061-1-git-send-email-joe.lawrence@redhat.com>
 <1496341526-19061-2-git-send-email-joe.lawrence@redhat.com>
 <20170608164923.mkbiwporqw4i2kxy@treble>
 <a5627cd6-f094-e20d-de63-e94272c33019@redhat.com>
 <20170609183425.qn4n45mwqewxavmb@treble>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jessica Yu <jeyu@redhat.com>, Jiri Kosina <jikos@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>, Petr Mladek <pmladek@suse.com>
From: Joe Lawrence <joe.lawrence@redhat.com>
Organization: Red Hat
Message-ID: <943d8625-7d90-94a2-4a6e-68c3b85e353a@redhat.com>
Date: Mon, 12 Jun 2017 09:49:08 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170609183425.qn4n45mwqewxavmb@treble>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2651
Lines: 76

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@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