Date: Tue, 18 Jul 2017 14:45:00 +0200
From: Petr Mladek <pmladek@suse.com>
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>, Miroslav Benes <mbenes@suse.cz>
Subject: Re: [PATCH v2 1/2] livepatch: introduce shadow variable API
Message-ID: <20170718124500.GF3393@pathway.suse.cz>
References: <1498664247-12296-1-git-send-email-joe.lawrence@redhat.com>
 <1498664247-12296-2-git-send-email-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1498664247-12296-2-git-send-email-joe.lawrence@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5390
Lines: 176

On Wed 2017-06-28 11:37:26, Joe Lawrence wrote:
> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
> new file mode 100644
> index 000000000000..7f28982e6b1c
> --- /dev/null
> +++ b/Documentation/livepatch/shadow-vars.txt
> +Use cases
> +---------
> +
> +See the example shadow variable livepatch modules in samples/livepatch
> +for full working demonstrations.
> +
> +Example 1: Commit 1d147bfa6429 ("mac80211: fix AP powersave TX vs.
> +wakeup race") added a spinlock to net/mac80211/sta_info.h :: struct
> +sta_info.  Implementing this change with a shadow variable is
> +straightforward.
> +
> +Allocation - when a host sta_info structure is allocated, attach a
> +shadow variable copy of the ps_lock:
> +
> +#define PS_LOCK 1
> +struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
> +				const u8 *addr, gfp_t gfp)
> +{
> +	struct sta_info *sta;
> +	spinlock_t *ps_lock;
> +	...
> +	sta = kzalloc(sizeof(*sta) + hw->sta_data_size, gfp);

klp_shadow_attach() does the allocation as well now.
Note that we could pass already initialized spin_lock.

> +	...
> +	ps_lock = klp_shadow_attach(sta, PS_LOCK, NULL, sizeof(*ps_lock), gfp);
> +	if (!ps_lock)
> +		goto shadow_fail;
> +	spin_lock_init(ps_lock);
> +	...
> +
> +Usage - when using the shadow spinlock, query the shadow variable API to
> +retrieve it:
> +
> +void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
> +{
> +	spinlock_t *ps_lock;
> +	...
> +	/* sync with ieee80211_tx_h_unicast_ps_buf */
> +	ps_lock = klp_shadow_get(sta, "ps_lock");

s/"ps_lock"/PS_LOCK/

The same problem is repeated many times below (also in the 2nd
example).

Also this is a nice example, where klp_shadow_get_or_attach()
would be useful. It would fix even already existing instances.

So, the code might look like:

  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);

It is a bit ugly that we always initialize ps_lock_fallback
even when it is not used. But it helps to avoid a custom
callback that would create the fallback variable. I think
that it is an acceptable deal.


> +	if (ps_lock)
> +		spin_lock(ps_lock);
> +	...
> +	if (ps_lock)
> +		spin_unlock(ps_lock);
> +	...
> +
> +Release - when the host sta_info structure is freed, first detach the
> +shadow variable and then free the shadow spinlock:
> +
> +void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
> +{
> +	spinlock_t *ps_lock;
> +	...
> +	ps_lock = klp_shadow_get(sta, "ps_lock");
> +	if (ps_lock)
> +		klp_shadow_detach(sta, "ps_lock");

Isn't klp_shadow_detach() enough? If it an optimization, 
klp_shadow_detach() might get optimized the same way.
But I am not sure if it is worth it.

> +	kfree(sta);
> +
> +


> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> new file mode 100644
> index 000000000000..d37a61c57e72
> --- /dev/null
> +++ b/kernel/livepatch/shadow.c
> +/**
> + * _klp_shadow_attach() - allocate and add a new shadow variable
> + * @obj:	pointer to original data
> + * @num:	numerical description of new data
> + * @new_data:	pointer to new data
> + * @new_size:	size of new data
> + * @gfp_flags:	GFP mask for allocation
> + * @lock:	take klp_shadow_lock during klp_shadow_hash operations
> + *
> + * Note: allocates @new_size space for shadow variable data and copies
> + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> + * space.  If @new_data is NULL, @new_size is still allocated, but no
> + * copy is performed.
> + *
> + * Return: the shadow variable new_data element, NULL on failure.
> + */
> +static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> +				size_t new_size, gfp_t gfp_flags,
> +				bool lock)

Nested implementation is usually prefixed by two underlines __.
It is more visible and helps to distinguish it from the normal function.


> +{
> +	struct klp_shadow *shadow;
> +	unsigned long flags;
> +
> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> +	if (!shadow)
> +		return NULL;
> +
> +	shadow->obj = obj;
> +	shadow->num = num;
> +	if (new_data)
> +		memcpy(shadow->new_data, new_data, new_size);
> +
> +	if (lock)
> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);

We should check if the shadow variable already existed. Otherwise,
it would be possible to silently create many duplicates.

It would make klp_shadow_attach() and klp_shadow_get_or_attach()
to behave the same.

I would do WARN() in klp_shadow_attach() when the variable
already existed are return NULL. Of course it might be inoncent
duplication. But it might mean that someone else is using another
variable of the same name but with different content. klp_shadow_get()
would then return the same variable for two different purposes.
Then the whole system might end like a glass on a stony floor.


> +	if (lock)
> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return shadow->new_data;
> +}

Otherwise, I rather like the API. Thanks a lot for adding
klp_shadow_get_or_attach().

I did not comment things that were already discussed in
other threads.

Best Regards,
Petr