Date: Wed, 16 Aug 2017 14:43:05 +0200 (CEST)
From: Miroslav Benes <mbenes@suse.cz>
To: Joe Lawrence <joe.lawrence@redhat.com>
cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@redhat.com>,
        Jiri Kosina <jikos@kernel.org>, Petr Mladek <pmladek@suse.com>
Subject: Re: [PATCH v4] livepatch: introduce shadow variable API
In-Reply-To: <1502740963-31310-2-git-send-email-joe.lawrence@redhat.com>
Message-ID: <alpine.LSU.2.20.1708161423040.4165@pobox.suse.cz>
References: <1502740963-31310-1-git-send-email-joe.lawrence@redhat.com> <1502740963-31310-2-git-send-email-joe.lawrence@redhat.com>
User-Agent: Alpine 2.20 (LSU 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3279
Lines: 106


> +/*
> + * klp_shadow_set() - initialize a shadow variable
> + * @shadow:	shadow variable to initialize
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + * @data:	pointer to data to attach to parent
> + * @size:	size of attached data
> + *
> + * Callers should hold the klp_shadow_lock.
> + */
> +static inline void klp_shadow_set(struct klp_shadow *shadow, void *obj,
> +				  unsigned long id, void *data, size_t size)
> +{
> +	shadow->obj = obj;
> +	shadow->id = id;
> +
> +	if (data)
> +		memcpy(shadow->data, data, size);
> +}

[...]

> +/**
> + * klp_shadow_attach() - allocate and add a new shadow variable
> + * @obj:	pointer to parent object
> + * @id:		data identifier
> + * @data:	pointer to data to attach to parent
> + * @size:	size of attached data
> + * @gfp_flags:	GFP mask for allocation
> + *
> + * If an existing <obj, id> shadow variable can be found, this routine
> + * will issue a WARN, exit early and return NULL.
> + *
> + * Allocates @size bytes for new shadow variable data using @gfp_flags
> + * and copies @size bytes from @data into the new shadow variable's own
> + * data space.  If @data is NULL, @size bytes are still allocated, but
> + * no copy is performed.  The new shadow variable is then added to the
> + * global hashtable.
> + *
> + * Return: the shadow variable data element, NULL on duplicate or
> + * failure.
> + */
> +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> +			size_t size, gfp_t gfp_flags)
> +{
> +	struct klp_shadow *new_shadow;
> +	void *shadow_data;
> +	unsigned long flags;
> +
> +	/* Take error exit path if <obj, id> already exists */
> +	if (unlikely(klp_shadow_get(obj, id)))
> +		goto err_exists;
> +
> +	/* Allocate a new shadow variable for use inside the lock below */
> +	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> +	if (!new_shadow)
> +		goto err;
> +	klp_shadow_set(new_shadow, obj, id, data, size);

There is a comment above about locking and we do not take the spinlock 
here. That could surprise someone. So I'd keep only klp_shadow_add() 
comment, because there it is strictly needed. It depends on the context in 
all other cases.

Could you also add a comment above klp_shadow_lock definition about what 
it aims to protect?

> +	/* Look for <obj, id> again under the lock */
> +	spin_lock_irqsave(&klp_shadow_lock, flags);
> +	shadow_data = klp_shadow_get(obj, id);
> +	if (unlikely(shadow_data)) {

shadow_data is not needed anywhere, so you could do the same as for the 
first speculative search and remove shadow_data variable all together.

> +		/*
> +		 * Shadow variable was found, throw away speculative
> +		 * allocation and update/return the existing one.
> +		 */
> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +		kfree(new_shadow);
> +		goto err_exists;
> +	}
> +
> +	/* No <obj, id> found, add the newly allocated one */
> +	klp_shadow_add(new_shadow);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return new_shadow->data;
> +
> +err_exists:
> +	WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> +err:
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);

Otherwise it looks good. You can add my

Acked-by: Miroslav Benes <mbenes@suse.cz>

with those nits fixed.

Thanks,
Miroslav