From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752745AbdHJOkI (ORCPT ); Thu, 10 Aug 2017 10:40:08 -0400 Received: from mx2.suse.de ([195.135.220.15]:52464 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752389AbdHJOkH (ORCPT ); Thu, 10 Aug 2017 10:40:07 -0400 Date: Thu, 10 Aug 2017 16:40:05 +0200 (CEST) From: Miroslav Benes To: Joe Lawrence cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh Poimboeuf , Jessica Yu , Jiri Kosina , Petr Mladek Subject: Re: [PATCH v3] livepatch: introduce shadow variable API In-Reply-To: <1501262722-26502-2-git-send-email-joe.lawrence@redhat.com> Message-ID: 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 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 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 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 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 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 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 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 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 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 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