From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751425AbdGRMpF (ORCPT ); Tue, 18 Jul 2017 08:45:05 -0400 Received: from mx2.suse.de ([195.135.220.15]:36914 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751336AbdGRMpD (ORCPT ); Tue, 18 Jul 2017 08:45:03 -0400 Date: Tue, 18 Jul 2017 14:45:00 +0200 From: Petr Mladek To: Joe Lawrence Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh Poimboeuf , Jessica Yu , Jiri Kosina , Miroslav Benes 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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