Date: Thu, 10 Aug 2017 16:40: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 v3] livepatch: introduce shadow variable API
In-Reply-To: <1501262722-26502-2-git-send-email-joe.lawrence@redhat.com>
Message-ID: <alpine.LSU.2.20.1708101631080.7971@pobox.suse.cz>
References: <1501262722-26502-1-git-send-email-joe.lawrence@redhat.com> <1501262722-26502-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: 8928
Lines: 278


It generally looks ok. Only few questions below...

[...]

> +In-flight parent objects
> +------------------------
> +
> +Sometimes it may not be convenient or possible to allocate shadow
> +variables alongside their parent objects.  Or a livepatch fix may
> +require shadow varibles to only a subset of parent object instances.  In
> +these cases, the klp_shadow_get_or_attach() call can be used to attach
> +shadow variables to parents already in-flight.
> +
> +For commit 1d147bfa6429, a good spot to attach a shadow spinlock is
> +inside ieee80211_sta_ps_deliver_wakeup():
> +
> +#define PS_LOCK 1
> +void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
> +{
> +	DEFINE_SPINLOCK(ps_lock_fallback);
> +	spinlock_t *ps_lock;
> +
> +	/* sync with ieee80211_tx_h_unicast_ps_buf */
> +	ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
> +			&ps_lock_fallback, sizeof(ps_lock_fallback),
> +			GFP_ATOMIC);
> +
> +	ps_lock = klp_shadow_get(sta, PS_LOCK);
> +	if (ps_lock)
> +		spin_lock(ps_lock);

ps_lock = klp_shadow_get(sta, PS_LOCK); should not be needed, should it?

[...]

> +/*
> + * 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
> + */
> +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_add() - add a shadow variable to the hashtable
> + * @shadow:	shadow variable to add
> + */
> +static inline void klp_shadow_add(struct klp_shadow *shadow)
> +{
> +	hash_add_rcu(klp_shadow_hash, &shadow->node,
> +		     (unsigned long)shadow->obj);
> +}

It would be nice to add a comment that a caller must hold klp_shadow_lock 
spinlock.

> +/**
> + * 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 */
> +	shadow_data = klp_shadow_get(obj, id);
> +	if (unlikely(shadow_data))
> +		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;
> +
> +	/* 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 variable found, throw away allocation and take
> +		 * error exit path */
> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +		kfree(shadow_data);
> +		goto err_exists;
> +	}
> +
> +	/* No <obj, id> found, add the newly allocated one */
> +	shadow_data = data;
> +	klp_shadow_set(new_shadow, obj, id, data, size);
> +	klp_shadow_add(new_shadow);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return shadow_data;

I may be missing something, but shouldn't this return new_shadow->data? 
You return original data here which seems strange.

> +
> +err_exists:
> +	WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> +err:
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> +
> +/**
> + * klp_shadow_get_or_attach() - get existing or attach 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, it will be
> + * used (but *not* updated) in the return value of this function.
> + *
> + * 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 failure.
> + */
> +void *klp_shadow_get_or_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;
> +
> +	/* Return a shadow variable if <obj, id> already exists */
> +	shadow_data = klp_shadow_get(obj, id);
> +	if (shadow_data)
> +		goto ret;

This could be "return ret;" directly. But no big deal.

> +	/* 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;
> +
> +	/* 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 variable found, throw away allocation and
> +		 * return the shadow variable data */
> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +		kfree(shadow_data);
> +		goto ret;
> +	}
> +
> +	/* No <obj, id> found, so attach the newly allocated one */
> +	shadow_data = data;

Again. "shadow_data = new_shadow->data;"?

> +	klp_shadow_set(new_shadow, obj, id, data, size);
> +	klp_shadow_add(new_shadow);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +ret:
> +	return shadow_data;
> +err:
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_get_or_attach);
> +
> +/**
> + * klp_shadow_update_or_attach() - update or attach 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, it will be
> + * updated and used in the return value of this function.
> + *
> + * 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 failure.
> + */
> +void *klp_shadow_update_or_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;
> +
> +	/* Update/return a shadow variable if <obj, id> already exists */
> +	spin_lock_irqsave(&klp_shadow_lock, flags);
> +	shadow_data = klp_shadow_get(obj, id);
> +	if (shadow_data)
> +		goto update_unlock_ret;
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	/* 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;
> +
> +	/* Look for <obj, id> again under the lock, this time with an
> +	 * allocation in hand to use if need to use it. */
> +	spin_lock_irqsave(&klp_shadow_lock, flags);
> +	shadow_data = klp_shadow_get(obj, id);
> +	if (unlikely(shadow_data)) {
> +
> +		/* Shadow variable found, throw away allocation and
> +		 * update/return the existing one */
> +		kfree(new_shadow);
> +		goto update_unlock_ret;
> +	}
> +
> +	/* Could not find one, so attach the new one */
> +	shadow_data = data;

Dtto.

> +	klp_shadow_set(new_shadow, obj, id, data, size);
> +	klp_shadow_add(new_shadow);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return shadow_data;
> +
> +update_unlock_ret:
> +	/* Update already attached shadow variable */
> +	new_shadow = container_of(shadow_data, struct klp_shadow, data);
> +	klp_shadow_set(new_shadow, obj, id, data, size);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return shadow_data;
> +err:
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_update_or_attach);

The rest is good.

Thanks,
Miroslav