[PATCH v3] livepatch: shadow variables

[PATCH v3] livepatch: shadow variables

[Joe Lawrence]

Hi all,

This is v3 of the livepatch shadow variable API.  v2 collected a bunch
of great feedback on terminology, use cases and concurrency notes which
I've tried to incorporate here.

Here's a high-level sketch of v2 changes:

- Overall
  
  - Squash into a one patch, makes for finding terms and code across
    docs/implementation/example easier
  
  - Variable naming:
    - obj, original data -> obj, parent object
    - num, numerical description of new data -> id, data identifier
    - new_data -> data
    - new_size -> size
  
- Documentation
  
  - API summary: s/klp_shadow_detach/klp_shadow_detach_all
  - describe uses for the data id (versions, class/type, etc)
  - clarify that shadow data is a copy
  - fix use-cases - use one upstream commit and steal Petr's example for
                    klp_shadow_get_or_attach()
  
- Implementation
  
  - Modify klp_shadow_get_or_attach() to WARN and return NULL if shadow
    variable already exists
  
  - Add klp_shadow_update_or_attach() to update if shadow variable already
    exists
  
  
- Sample modules
  
  - s/_thread/_work_func
  - use system workqueue, DECLARE_DELAYED_WORK
  - use XXX_PERIOD constants
  - overhaul usage/comments, drop the verbose dmesg logs

Joe Lawrence (1):
  livepatch: introduce shadow variable API

 Documentation/livepatch/shadow-vars.txt   | 217 +++++++++++++++++
 include/linux/livepatch.h                 |  10 +
 kernel/livepatch/Makefile                 |   2 +-
 kernel/livepatch/shadow.c                 | 383 ++++++++++++++++++++++++++++++
 samples/Kconfig                           |   5 +-
 samples/livepatch/Makefile                |   3 +
 samples/livepatch/livepatch-shadow-mod.c  | 222 +++++++++++++++++
 samples/livepatch/livepatch-shadow-fix1.c | 174 ++++++++++++++
 samples/livepatch/livepatch-shadow-fix2.c | 167 +++++++++++++
 9 files changed, 1179 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/livepatch/shadow-vars.txt
 create mode 100644 kernel/livepatch/shadow.c
 create mode 100644 samples/livepatch/livepatch-shadow-mod.c
 create mode 100644 samples/livepatch/livepatch-shadow-fix1.c
 create mode 100644 samples/livepatch/livepatch-shadow-fix2.c

-- 
1.8.3.1

[PATCH v3] livepatch: introduce shadow variable API

[Joe Lawrence]

Add exported API for livepatch modules:

  klp_shadow_get()
  klp_shadow_attach()
  klp_shadow_get_or_attach()
  klp_shadow_update_or_attach()
  klp_shadow_detach()
  klp_shadow_detach_all()

that implement "shadow" variables, which allow callers to associate new
shadow fields to existing data structures.  This is intended to be used
by livepatch modules seeking to emulate additions to data structure
definitions.

See Documentation/livepatch/shadow-vars.txt for a summary of the new
shadow variable API, including a few common use cases.

See samples/livepatch/livepatch-shadow-* for exmaple modules that
demonstrate shadow variables.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 Documentation/livepatch/shadow-vars.txt   | 217 +++++++++++++++++
 include/linux/livepatch.h                 |  10 +
 kernel/livepatch/Makefile                 |   2 +-
 kernel/livepatch/shadow.c                 | 383 ++++++++++++++++++++++++++++++
 samples/Kconfig                           |   5 +-
 samples/livepatch/Makefile                |   3 +
 samples/livepatch/livepatch-shadow-mod.c  | 222 +++++++++++++++++
 samples/livepatch/livepatch-shadow-fix1.c | 174 ++++++++++++++
 samples/livepatch/livepatch-shadow-fix2.c | 167 +++++++++++++
 9 files changed, 1179 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/livepatch/shadow-vars.txt
 create mode 100644 kernel/livepatch/shadow.c
 create mode 100644 samples/livepatch/livepatch-shadow-mod.c
 create mode 100644 samples/livepatch/livepatch-shadow-fix1.c
 create mode 100644 samples/livepatch/livepatch-shadow-fix2.c

diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
new file mode 100644
index 000000000000..0dc2eb848615
--- /dev/null
+++ b/Documentation/livepatch/shadow-vars.txt
@@ -0,0 +1,217 @@
+================
+Shadow Variables
+================
+
+Shadow variables are a simple way for livepatch modules to associate
+additional "shadow" data with existing data structures.  Shadow data is
+allocated separately from parent data structures, which are left
+unmodified.  The shadow variable API described in this document is used
+to allocate/attach and detach/release shadow variables to their parents.
+
+The implementation introduces a global, in-kernel hashtable that
+associates pointers to parent objects and a numeric identifier of the
+shadow data.  The numeric identifier is a simple enumeration that may be
+used to describe shadow variable version, class or type, etc.  More
+specifically, the parent pointer serves as the hashtable key while the
+numeric id subsequently filters hashtable queries.  Multiple shadow
+variables may attach to the same parent object, but their numeric
+identifier distinguishes between them.
+
+
+1. Brief API summary
+====================
+
+(See the full API usage docbook notes in livepatch/shadow.c.)
+
+A hashtable references all shadow variables.  These references are
+stored and retrieved through a <obj, id> pair.
+
+* The klp_shadow variable data structure encapsulates both tracking
+meta-data and shadow-data:
+  - meta-data
+    - obj - pointer to parent object
+    - id - data identifier
+  - data[] - storage for shadow data
+
+  It is important to note that the klp_shadow_attach(),
+  klp_shadow_get_or_attach(), and klp_shadow_update_or_attach() calls,
+  described below, store a *copy* of the data that the functions are
+  provided.
+
+* klp_shadow_get() - retrieve a shadow variable data pointer
+  - search hashtable for <obj, id> pair
+
+* klp_shadow_attach() - allocate and add a new shadow variable
+  - search hashtable for <obj, id> pair
+  - if exists
+    - WARN and return NULL
+  - if <obj, id> doesn't already exist
+    - allocate a new shadow variable
+    - copy data into the new shadow variable
+    - add <obj, id> to the global hashtable
+
+* klp_shadow_get_or_attach() - get existing or attach a new shadow variable
+  - search hashtable for <obj, id> pair
+  - if exists
+    - return existing shadow variable
+  - if <obj, id> doesn't already exist
+    - allocate a new shadow variable
+    - copy data into the new shadow variable
+    - add <obj, id> pair to the global hashtable
+
+* klp_shadow_update_or_attach() - update or attach a new shadow variable
+  - search hashtable for <obj, id> pair
+  - if exists
+    -  update and return existing shadow variable
+  - if <obj, id> doesn't already exist
+    - allocate a new shadow variable
+    - copy data into the new shadow variable
+    - add <obj, id> pair to the global hashtable
+
+* klp_shadow_detach() - detach and free a <obj, id> shadow variable
+  - find and remove a <obj, id> reference from global hashtable
+    - if found, free shadow variable
+
+* klp_shadow_detach_all() - detach and free all <*, id> shadow variables
+  - find and remove any <*, id> references from global hashtable
+    - if found, free shadow variable
+
+
+2. Use cases
+============
+
+(See the example shadow variable livepatch modules in samples/livepatch/
+for full working demonstrations.)
+
+For the following use-case examples, consider commit 1d147bfa6429
+("mac80211: fix AP powersave TX vs.  wakeup race"), which added a
+spinlock to net/mac80211/sta_info.h :: struct sta_info.  Each use-case
+example can be considered a stand-alone livepatch implementation of this
+fix.
+
+
+Matching parent's lifecycle
+---------------------------
+
+If parent data structures are frequently created and destroyed, it may
+be easiest to align its shadow variable lifetimes to the same allocation
+and release functions.  In this case, the parent data structure is
+typically allocated, initialized, then registered in some manner.
+Shadow variable allocation and setup can then be considered part of the
+parent's initialization and should be completed before the parent "goes
+live" (ie, any shadow variable get-API requests are made for this
+<obj, id> pair.
+
+For commit 1d147bfa6429, when a parent sta_info structure is allocated,
+attach a shadow copy of the ps_lock pointer, then initialize it:
+
+#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;
+
+	/* Parent structure is created */
+	sta = kzalloc(sizeof(*sta) + hw->sta_data_size, gfp);
+
+	/* Attach a corresponding shadow variable, then initialize it */
+	ps_lock = klp_shadow_attach(sta, PS_LOCK, NULL, sizeof(ps_lock), gfp);
+	if (!ps_lock)
+		goto shadow_fail;
+	spin_lock_init(ps_lock);
+	...
+
+When requiring a ps_lock, query the shadow variable API to retrieve one
+for a specific struct sta_info:
+
+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);
+	if (ps_lock)
+		spin_lock(ps_lock);
+	...
+
+When the parent sta_info structure is freed, first detach the shadow
+variable:
+
+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);
+
+	kfree(sta);
+	...
+
+
+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);
+	...
+
+This usage will create a shadow variable, only if needed, otherwise it
+will use one that was already created for this <obj, id> pair.
+
+Like the previous use-case, the shadow spinlock needs to be cleaned up.
+A shadow variable can be detached just before its parent object is
+freed, or even when the shadow variable itself is no longer required.
+
+
+Other use-cases
+---------------
+
+Like klp_shadow_get_or_attach(), klp_shadow_update_or_attach() can be
+used to attach shadow variables to already allocated parent data
+structures in-flight. Unlike the former function,
+klp_shadow_update_or_attach() will *update* an existing shadow
+variable's data area.  This behavior may be useful when shadowing data
+who's lifetime does *not* follow the parent object's.  For example, when
+adding a "jiffies" or "last task pointer" shadow variable, overwriting
+the existing shadow variable's data copy is expected.
+
+Shadow variables can also be used as a flag indicating that a data
+structure was allocated by new, livepatched code.  In this case, it
+doesn't matter what data value the shadow variable holds, its existence
+suggests how to handle the parent object.
+
+
+3. References
+=============
+
+* https://github.com/dynup/kpatch
+The livepatch implementation is based on the kpatch version of shadow
+variables.
+
+* http://files.mkgnu.net/files/dynamos/doc/papers/dynamos_eurosys_07.pdf
+Dynamic and Adaptive Updates of Non-Quiescent Subsystems in Commodity
+Operating System Kernels (Kritis Makris, Kyung Dong Ryu 2007) presented
+a datatype update technique called "shadow data structures".
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..0033aaea8572 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -164,6 +164,16 @@ static inline bool klp_have_reliable_stack(void)
 	       IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
 }
 
+void *klp_shadow_get(void *obj, unsigned long id);
+void *klp_shadow_attach(void *obj, unsigned long id, void *data,
+			size_t size, gfp_t gfp_flags);
+void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
+			       size_t size, gfp_t gfp_flags);
+void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
+				  size_t size, gfp_t gfp_flags);
+void klp_shadow_detach(void *obj, unsigned long id);
+void klp_shadow_detach_all(unsigned long id);
+
 #else /* !CONFIG_LIVEPATCH */
 
 static inline int klp_module_coming(struct module *mod) { return 0; }
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index 2b8bdb1925da..b36ceda6488e 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
-livepatch-objs := core.o patch.o transition.o
+livepatch-objs := core.o patch.o shadow.o transition.o
diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
new file mode 100644
index 000000000000..f1ee0360ff62
--- /dev/null
+++ b/kernel/livepatch/shadow.c
@@ -0,0 +1,383 @@
+/*
+ * shadow.c - Shadow Variables
+ *
+ * Copyright (C) 2014 Josh Poimboeuf <jpoimboe@redhat.com>
+ * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/**
+ * DOC: Shadow variable API concurrency notes:
+ *
+ * The shadow variable API provides a simple relationship between an
+ * <obj, id> pair and a pointer value.  It is the responsibility of the
+ * caller to provide any mutual exclusion required of the shadow data.
+ *
+ * Once a shadow variable is "attached" to its parent object via the
+ * klp_shadow_*attach() API calls, it is considered live: any subsequent
+ * call to klp_shadow_get() may then return the shadow variable's data
+ * pointer.  Callers of klp_shadow_*attach() should prepare shadow data
+ * accordingly.
+ *
+ * The klp_shadow_*attach() API calls may allocate memory for new shadow
+ * variable structures.  Their implementation does not call kmalloc
+ * inside any spinlocks, but API callers should pass GFP flags according
+ * to their specific needs.
+ *
+ * The klp_shadow_hash is an RCU-enabled hashtable and is safe against
+ * concurrent klp_shadow_detach() and klp_shadow_get() operations.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/hashtable.h>
+#include <linux/slab.h>
+#include <linux/livepatch.h>
+
+static DEFINE_HASHTABLE(klp_shadow_hash, 12);
+static DEFINE_SPINLOCK(klp_shadow_lock);
+
+/**
+ * struct klp_shadow - shadow variable structure
+ * @node:	klp_shadow_hash hash table node
+ * @rcu_head:	RCU is used to safely free this structure
+ * @obj:	pointer to parent object
+ * @id:		data identifier
+ * @data:	data area
+ */
+struct klp_shadow {
+	struct hlist_node node;
+	struct rcu_head rcu_head;
+	void *obj;
+	unsigned long id;
+	char data[];
+};
+
+/**
+ * shadow_match() - verify a shadow variable matches given <obj, id>
+ * @shadow:	shadow variable to match
+ * @obj:	pointer to parent object
+ * @id:		data identifier
+ *
+ * Return: true if the shadow variable matches.
+ */
+static inline bool shadow_match(struct klp_shadow *shadow, void *obj,
+				unsigned long id)
+{
+	return shadow->obj == obj && shadow->id == id;
+}
+
+/**
+ * klp_shadow_get() - retrieve a shadow variable data pointer
+ * @obj:	pointer to parent object
+ * @id:		data identifier
+ *
+ * Return: the shadow variable data element, NULL on failure.
+ */
+void *klp_shadow_get(void *obj, unsigned long id)
+{
+	struct klp_shadow *shadow;
+
+	rcu_read_lock();
+
+	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
+				   (unsigned long)obj) {
+
+		if (shadow_match(shadow, obj, id)) {
+			rcu_read_unlock();
+			return shadow->data;
+		}
+	}
+
+	rcu_read_unlock();
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(klp_shadow_get);
+
+/*
+ * 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);
+}
+
+/**
+ * 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;
+
+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;
+
+	/* 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;
+	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;
+	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);
+
+/**
+ * klp_shadow_detach() - detach and free a <obj, id> shadow variable
+ * @obj:	pointer to parent object
+ * @id:		data identifier
+ */
+void klp_shadow_detach(void *obj, unsigned long id)
+{
+	struct klp_shadow *shadow;
+	unsigned long flags;
+
+	spin_lock_irqsave(&klp_shadow_lock, flags);
+
+	/* Delete <obj, id> from hash */
+	hash_for_each_possible(klp_shadow_hash, shadow, node,
+			       (unsigned long)obj) {
+
+		if (shadow_match(shadow, obj, id)) {
+			hash_del_rcu(&shadow->node);
+			kfree_rcu(shadow, rcu_head);
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+}
+EXPORT_SYMBOL_GPL(klp_shadow_detach);
+
+/**
+ * klp_shadow_detach_all() - detach all <*, id> shadow variables
+ * @id:		data identifier
+ */
+void klp_shadow_detach_all(unsigned long id)
+{
+	struct klp_shadow *shadow;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&klp_shadow_lock, flags);
+
+	/* Delete all <*, id> from hash */
+	hash_for_each(klp_shadow_hash, i, shadow, node) {
+		if (shadow_match(shadow, shadow->obj, id)) {
+			hash_del_rcu(&shadow->node);
+			kfree_rcu(shadow, rcu_head);
+		}
+	}
+
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+}
+EXPORT_SYMBOL_GPL(klp_shadow_detach_all);
diff --git a/samples/Kconfig b/samples/Kconfig
index 9cb63188d3ef..c332a3b9de05 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -71,11 +71,10 @@ config SAMPLE_RPMSG_CLIENT
 	  the rpmsg bus.
 
 config SAMPLE_LIVEPATCH
-	tristate "Build live patching sample -- loadable modules only"
+	tristate "Build live patching samples -- loadable modules only"
 	depends on LIVEPATCH && m
 	help
-	  Builds a sample live patch that replaces the procfs handler
-	  for /proc/cmdline to print "this has been live patched".
+	  Build sample live patch demonstrations.
 
 config SAMPLE_CONFIGFS
 	tristate "Build configfs patching sample -- loadable modules only"
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 10319d7ea0b1..539e81d433cd 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1 +1,4 @@
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
new file mode 100644
index 000000000000..cdba7905a0df
--- /dev/null
+++ b/samples/livepatch/livepatch-shadow-mod.c
@@ -0,0 +1,222 @@
+/*
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * livepatch-shadow-mod.c - Shadow variables, buggy module demo
+ *
+ * Purpose
+ * -------
+ *
+ * As a demonstration of livepatch shadow variable API, this module
+ * introduces memory leak behavior that livepatch modules
+ * livepatch-shadow-fix1.ko and livepatch-shadow-fix2.ko correct and
+ * enhance.
+ *
+ * WARNING - even though the livepatch-shadow-fix modules patch the
+ * memory leak, please load these modules at your own risk -- some
+ * amount of memory may leaked before the bug is patched.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * Step 1 - Load the buggy demonstration module:
+ *
+ *   insmod samples/livepatch/livepatch-shadow-mod.ko
+ *
+ * Watch dmesg output for a few moments to see new dummy being allocated
+ * and a periodic cleanup check.  (Note: a small amount of memory is
+ * being leaked.)
+ *
+ *
+ * Step 2 - Load livepatch fix1:
+ *
+ *   insmod samples/livepatch/livepatch-shadow-fix1.ko
+ *
+ * Continue watching dmesg and note that now livepatch_fix1_dummy_free()
+ * and livepatch_fix1_dummy_alloc() are logging messages about leaked
+ * memory and eventually leaks prevented.
+ *
+ *
+ * Step 3 - Load livepatch fix2 (on top of fix1):
+ *
+ *   insmod samples/livepatch/livepatch-shadow-fix2.ko
+ *
+ * This module extends functionality through shadow variables, as a new
+ * "check" counter is added to the dummy structure.  Periodic dmesg
+ * messages will log these as dummies are cleaned up.
+ *
+ *
+ * Step 4 - Cleanup
+ *
+ * Unwind the demonstration by disabling the livepatch fix modules, then
+ * removing them and the demo module:
+ *
+ *   echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix2/enabled
+ *   echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix1/enabled
+ *   rmmod livepatch-shadow-fix2
+ *   rmmod livepatch-shadow-fix1
+ *   rmmod livepatch-shadow-mod
+ */
+
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/stat.h>
+#include <linux/workqueue.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joe Lawrence <joe.lawrence@redhat.com>");
+MODULE_DESCRIPTION("Buggy module for shadow variable demo");
+
+/* Allocate new dummies every second */
+#define ALLOC_PERIOD	1
+/* Check for expired dummies after a few new ones have been allocated */
+#define CLEANUP_PERIOD	(3 * ALLOC_PERIOD)
+/* Dummies expire after a few cleanup instances */
+#define EXPIRE_PERIOD	(4 * CLEANUP_PERIOD)
+
+/* Keep a list of all the dummies so we can clean up any residual ones
+ * on module exit */
+LIST_HEAD(dummy_list);
+DEFINE_MUTEX(dummy_list_mutex);
+
+struct dummy {
+	struct list_head list;
+	unsigned long jiffies_expire;
+};
+
+noinline struct dummy *dummy_alloc(void)
+{
+	struct dummy *d;
+	void *leak;
+
+	d = kzalloc(sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return NULL;
+
+	d->jiffies_expire = jiffies +
+		msecs_to_jiffies(1000 * EXPIRE_PERIOD);
+
+	/* Oops, forgot to save leak! */
+	leak = kzalloc(sizeof(int), GFP_KERNEL);
+
+	pr_info("%s: dummy @ %p, expires @ %lx\n",
+		__func__, d, d->jiffies_expire);
+
+	return d;
+}
+
+noinline void dummy_free(struct dummy *d)
+{
+	pr_info("%s: dummy @ %p, expired = %lx\n",
+		__func__, d, d->jiffies_expire);
+
+	kfree(d);
+}
+
+noinline bool dummy_check(struct dummy *d, unsigned long jiffies)
+{
+	return time_after(jiffies, d->jiffies_expire);
+}
+
+/*
+ * alloc_work_func: allocates new dummy structures, allocates additional
+ *                  memory, aptly named "leak", but doesn't keep
+ *                  permanent record of it.
+ */
+
+static void alloc_work_func(struct work_struct *work);
+static DECLARE_DELAYED_WORK(alloc_dwork, alloc_work_func);
+
+static void alloc_work_func(struct work_struct *work)
+{
+	struct dummy *d;
+
+	d = dummy_alloc();
+	if (!d)
+		return;
+
+	mutex_lock(&dummy_list_mutex);
+	list_add(&d->list, &dummy_list);
+	mutex_unlock(&dummy_list_mutex);
+
+        schedule_delayed_work(&alloc_dwork,
+		msecs_to_jiffies(1000 * ALLOC_PERIOD));
+}
+
+/*
+ * cleanup_work_func: frees dummy structures.  Without knownledge of
+ *                    "leak", it leaks the additional memory that
+ *                    alloc_work_func created.
+ */
+
+static void cleanup_work_func(struct work_struct *work);
+static DECLARE_DELAYED_WORK(cleanup_dwork, cleanup_work_func);
+
+static void cleanup_work_func(struct work_struct *work)
+{
+	struct dummy *d, *tmp;
+	unsigned long j;
+
+	j = jiffies;
+	pr_info("%s: jiffies = %lx\n", __func__, j);
+
+	mutex_lock(&dummy_list_mutex);
+	list_for_each_entry_safe(d, tmp, &dummy_list, list) {
+
+		/* Kick out and free any expired dummies */
+		if (dummy_check(d, j)) {
+			list_del(&d->list);
+			dummy_free(d);
+		}
+	}
+	mutex_unlock(&dummy_list_mutex);
+
+        schedule_delayed_work(&cleanup_dwork,
+		msecs_to_jiffies(1000 * CLEANUP_PERIOD));
+}
+
+static int livepatch_shadow_mod_init(void)
+{
+	schedule_delayed_work(&alloc_dwork,
+		msecs_to_jiffies(1000 * ALLOC_PERIOD));
+	schedule_delayed_work(&cleanup_dwork,
+		msecs_to_jiffies(1000 * CLEANUP_PERIOD));
+
+	return 0;
+}
+
+static void livepatch_shadow_mod_exit(void)
+{
+	struct dummy *d, *tmp;
+
+	/* Wait for any dummies at work */
+	cancel_delayed_work_sync(&alloc_dwork);
+	cancel_delayed_work_sync(&cleanup_dwork);
+
+	/* Cleanup residual dummies */
+	list_for_each_entry_safe(d, tmp, &dummy_list, list) {
+		list_del(&d->list);
+		dummy_free(d);
+	}
+}
+
+module_init(livepatch_shadow_mod_init);
+module_exit(livepatch_shadow_mod_exit);
diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
new file mode 100644
index 000000000000..5acc838463d1
--- /dev/null
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -0,0 +1,174 @@
+/*
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * livepatch-shadow-fix1.c - Shadow variables, livepatch demo
+ *
+ * Purpose
+ * -------
+ *
+ * Fixes the memory leak introduced in livepatch-shadow-mod through the
+ * use of a shadow variable.  This fix demonstrates the "extending" of
+ * short-lived data structures by patching its allocation and release
+ * functions.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * This module is not intended to be standalone.  See the "Usage"
+ * section of livepatch-shadow-mod.c.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include <linux/slab.h>
+
+/* Shadow variable enums */
+#define SV_LEAK		1
+
+/* Allocate new dummies every second */
+#define ALLOC_PERIOD	1
+/* Check for expired dummies after a few new ones have been allocated */
+#define CLEANUP_PERIOD	(3 * ALLOC_PERIOD)
+/* Dummies expire after a few cleanup instances */
+#define EXPIRE_PERIOD	(4 * CLEANUP_PERIOD)
+
+struct dummy {
+	struct list_head list;
+	unsigned long jiffies_expire;
+};
+
+struct dummy *livepatch_fix1_dummy_alloc(void)
+{
+	struct dummy *d;
+	void *leak;
+	void **shadow_leak;
+
+	d = kzalloc(sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return NULL;
+
+	d->jiffies_expire = jiffies +
+		msecs_to_jiffies(1000 * EXPIRE_PERIOD);
+
+	/*
+	 * Patch: save the extra memory location into a SV_LEAK shadow
+	 * variable.  A patched dummy_free routine can later fetch this
+	 * pointer to handle resource release.
+	 */
+	leak = kzalloc(sizeof(int), GFP_KERNEL);
+	shadow_leak =
+		klp_shadow_attach(d, SV_LEAK, &leak, sizeof(leak), GFP_KERNEL);
+
+	pr_info("%s: dummy @ %p, expires @ %lx\n",
+		__func__, d, d->jiffies_expire);
+
+	return d;
+}
+
+void livepatch_fix1_dummy_free(struct dummy *d)
+{
+	void **shadow_leak;
+
+	/*
+	 * Patch: fetch the saved SV_LEAK shadow variable, detach and
+	 * free it.  Note: handle cases where this shadow variable does
+	 * not exist (ie, dummy structures allocated before this livepatch
+	 * was loaded.)
+	 */
+	shadow_leak = klp_shadow_get(d, SV_LEAK);
+	if (shadow_leak) {
+		klp_shadow_detach(d, SV_LEAK);
+		kfree(*shadow_leak);
+		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
+			 __func__, d, *shadow_leak);
+	} else {
+		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
+	}
+
+	kfree(d);
+}
+
+static struct klp_func funcs[] = {
+	{
+		.old_name = "dummy_alloc",
+		.new_func = livepatch_fix1_dummy_alloc,
+	},
+	{
+		.old_name = "dummy_free",
+		.new_func = livepatch_fix1_dummy_free,
+	}, { }
+};
+
+static struct klp_object objs[] = {
+	{
+		.name = "livepatch_shadow_mod",
+		.funcs = funcs,
+	}, { }
+};
+
+static struct klp_patch patch = {
+	.mod = THIS_MODULE,
+	.objs = objs,
+};
+
+static int livepatch_shadow_fix1_init(void)
+{
+	int ret;
+
+	if (!klp_have_reliable_stack() && !patch.immediate) {
+		/*
+		 * WARNING: Be very careful when using 'patch.immediate' in
+		 * your patches.  It's ok to use it for simple patches like
+		 * this, but for more complex patches which change function
+		 * semantics, locking semantics, or data structures, it may not
+		 * be safe.  Use of this option will also prevent removal of
+		 * the patch.
+		 *
+		 * See Documentation/livepatch/livepatch.txt for more details.
+		 */
+		patch.immediate = true;
+		pr_notice("The consistency model isn't supported for your architecture.  Bypassing safety mechanisms and applying the patch immediately.\n");
+	}
+
+	ret = klp_register_patch(&patch);
+	if (ret)
+		return ret;
+	ret = klp_enable_patch(&patch);
+	if (ret) {
+		WARN_ON(klp_unregister_patch(&patch));
+		return ret;
+	}
+	return 0;
+}
+
+static void livepatch_shadow_fix1_exit(void)
+{
+	/* Cleanup any existing SV_LEAK shadow variables */
+	klp_shadow_detach_all(SV_LEAK);
+
+	WARN_ON(klp_unregister_patch(&patch));
+}
+
+module_init(livepatch_shadow_fix1_init);
+module_exit(livepatch_shadow_fix1_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c
new file mode 100644
index 000000000000..fbaac0170285
--- /dev/null
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -0,0 +1,167 @@
+/*
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * livepatch-shadow-fix2.c - Shadow variables, livepatch demo
+ *
+ * Purpose
+ * -------
+ *
+ * Adds functionality to livepatch-shadow-mod's in-flight data
+ * structures through a shadow variable.  The livepatch patches a
+ * routine that periodically inspects data structures, incrementing a
+ * per-data-structure counter, creating the counter if needed.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * This module is not intended to be standalone.  See the "Usage"
+ * section of livepatch-shadow-mod.c.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include <linux/slab.h>
+
+/* Shadow variable enums */
+#define SV_LEAK		1
+#define SV_COUNTER	2
+
+struct dummy {
+	struct list_head list;
+	unsigned long jiffies_expire;
+};
+
+bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies)
+{
+	int *shadow_count;
+	int count;
+
+	/*
+	 * Patch: handle in-flight dummy structures, if they do not
+	 * already have a SV_COUNTER shadow variable, then attach a
+	 * new one.
+	 */
+	count = 0;
+	shadow_count = klp_shadow_get_or_attach(d, SV_COUNTER,
+						&count, sizeof(count),
+						GFP_NOWAIT);
+	if (shadow_count)
+		*shadow_count += 1;
+
+	return time_after(jiffies, d->jiffies_expire);
+}
+
+void livepatch_fix2_dummy_free(struct dummy *d)
+{
+	void **shadow_leak;
+	int *shadow_count;
+
+	/* Patch: copy the memory leak patch from the fix1 module. */
+	shadow_leak = klp_shadow_get(d, SV_LEAK);
+	if (shadow_leak) {
+		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
+			 __func__, d, *shadow_leak);
+		klp_shadow_detach(d, SV_LEAK);
+		kfree(*shadow_leak);
+	} else {
+		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
+	}
+
+	/*
+	 * Patch: fetch the SV_COUNTER shadow variable and display
+	 * the final count.  Detach the shadow variable.
+	 */
+	shadow_count = klp_shadow_get(d, SV_COUNTER);
+	if (shadow_count) {
+		pr_info("%s: dummy @ %p, check counter = %d\n",
+			__func__, d, *shadow_count);
+		klp_shadow_detach(d, SV_COUNTER);
+	}
+
+	kfree(d);
+}
+
+static struct klp_func funcs[] = {
+	{
+		.old_name = "dummy_check",
+		.new_func = livepatch_fix2_dummy_check,
+	},
+	{
+		.old_name = "dummy_free",
+		.new_func = livepatch_fix2_dummy_free,
+	}, { }
+};
+
+static struct klp_object objs[] = {
+	{
+		.name = "livepatch_shadow_mod",
+		.funcs = funcs,
+	}, { }
+};
+
+static struct klp_patch patch = {
+	.mod = THIS_MODULE,
+	.objs = objs,
+};
+
+static int livepatch_shadow_fix2_init(void)
+{
+	int ret;
+
+	if (!klp_have_reliable_stack() && !patch.immediate) {
+		/*
+		 * WARNING: Be very careful when using 'patch.immediate' in
+		 * your patches.  It's ok to use it for simple patches like
+		 * this, but for more complex patches which change function
+		 * semantics, locking semantics, or data structures, it may not
+		 * be safe.  Use of this option will also prevent removal of
+		 * the patch.
+		 *
+		 * See Documentation/livepatch/livepatch.txt for more details.
+		 */
+		patch.immediate = true;
+		pr_notice("The consistency model isn't supported for your architecture.  Bypassing safety mechanisms and applying the patch immediately.\n");
+	}
+
+	ret = klp_register_patch(&patch);
+	if (ret)
+		return ret;
+	ret = klp_enable_patch(&patch);
+	if (ret) {
+		WARN_ON(klp_unregister_patch(&patch));
+		return ret;
+	}
+	return 0;
+}
+
+static void livepatch_shadow_fix2_exit(void)
+{
+	/* Cleanup any existing SV_COUNTER shadow variables */
+	klp_shadow_detach_all(SV_COUNTER);
+
+	WARN_ON(klp_unregister_patch(&patch));
+}
+
+module_init(livepatch_shadow_fix2_init);
+module_exit(livepatch_shadow_fix2_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
-- 
1.8.3.1

Re: [PATCH v3] livepatch: introduce shadow variable API

[Miroslav Benes]

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

Re: [PATCH v3] livepatch: introduce shadow variable API

[Joe Lawrence]

On Thu, Aug 10, 2017 at 04:40:05PM +0200, Miroslav Benes wrote:
> 
> It generally looks ok. Only few questions below...
> 
> [...]
> 
> > +In-flight parent objects
> > +------------------------
> > +	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?

Correct, I'll remove 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.

Since shadow_match(), klp_shadow_set(), and klp_shadow_add() are all in
the same boat, I can mention the lock for those functions as well.  BTW,
is there a convention to drop the "klp_" for static, local routines?  I
should be consistent here.
 
> > +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> > +			size_t size, gfp_t gfp_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.
> 
> > +void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> > +			       size_t size, gfp_t gfp_flags)
> > +	shadow_data = data;
> 
> Again. "shadow_data = new_shadow->data;"?
> 
> > +void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
> > +				  size_t size, gfp_t gfp_flags)
> > +	shadow_data = data;
> 
> Dtto.
> 

Gah!  Thank you for spotting this!  It was leftover from v1 when there
were only pointers (and not data copies) being thrown about.

Thanks,

-- Joe

Re: [PATCH v3] livepatch: introduce shadow variable API

[Miroslav Benes]

> > > +/*
> > > + * 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.
> 
> Since shadow_match(), klp_shadow_set(), and klp_shadow_add() are all in
> the same boat, I can mention the lock for those functions as well. 

Yes, please.

> BTW,
> is there a convention to drop the "klp_" for static, local routines?  I
> should be consistent here.

Oh, I didn't notice missing "klp_" in shadow_match. We have a prefix 
everywhere, so add it, please.

Thanks,
Miroslav

Re: [PATCH v3] livepatch: introduce shadow variable API

[Josh Poimboeuf]

On Fri, Jul 28, 2017 at 01:25:22PM -0400, Joe Lawrence wrote:
> Add exported API for livepatch modules:
> 
>   klp_shadow_get()
>   klp_shadow_attach()
>   klp_shadow_get_or_attach()
>   klp_shadow_update_or_attach()
>   klp_shadow_detach()
>   klp_shadow_detach_all()

I like the API.

> +Matching parent's lifecycle
> +---------------------------
> +
> +If parent data structures are frequently created and destroyed, it may
> +be easiest to align its shadow variable lifetimes to the same allocation

"its shadow variable lifetimes" -> "their shadow variables' lifetimes" ?

> +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> +			size_t size, gfp_t gfp_flags)

[ Note: some of the following comments also apply to
  klp_shadow_get_or_attach and klp_shadow_update_or_attach. ]

> +{
> +	struct klp_shadow *new_shadow;

nit: The "new" is implied, and there's only one shadow struct used in
the function, so maybe just call it "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;

The return value of klp_shadow_get() can be tested directly, no need for
a variable.

> +	/* 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;

The goto destination is just a single line return, so I think it's
clearer to just return NULL here and get rid of the err label.

> +
> +	/* 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 */

Multi-line comments should be in the kernel coding style:

		/*
		 * Shadow variable found, throw away allocation and take
		 * error exit path
		 */

Also, complete sentences preferred :-)

> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +		kfree(shadow_data);

Shouldn't it free new_shadow instead of 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);

To avoid doing extra work with the lock held, the klp_shadow_set() can
be done before getting the lock, after the kzalloc.


-- 
Josh

Re: [PATCH v3] livepatch: introduce shadow variable API

[Joe Lawrence]

On 08/11/2017 12:35 PM, Josh Poimboeuf wrote:
> On Fri, Jul 28, 2017 at 01:25:22PM -0400, Joe Lawrence wrote:
>> Add exported API for livepatch modules:
>>
>>   klp_shadow_get()
>>   klp_shadow_attach()
>>   klp_shadow_get_or_attach()
>>   klp_shadow_update_or_attach()
>>   klp_shadow_detach()
>>   klp_shadow_detach_all()
> 
> I like the API.
> 
>> +Matching parent's lifecycle
>> +---------------------------
>> +
>> +If parent data structures are frequently created and destroyed, it may
>> +be easiest to align its shadow variable lifetimes to the same allocation
> 
> "its shadow variable lifetimes" -> "their shadow variables' lifetimes" ?

"parent data structures" is plural, so I think you are correct.

>> +void *klp_shadow_attach(void *obj, unsigned long id, void *data,
>> +			size_t size, gfp_t gfp_flags)
> 
> [ Note: some of the following comments also apply to
>   klp_shadow_get_or_attach and klp_shadow_update_or_attach. ]
> 
>> +{
>> +	struct klp_shadow *new_shadow;
> 
> nit: The "new" is implied, and there's only one shadow struct used in
> the function, so maybe just call it "shadow"?

I'd like to keep the "new_" prefix convention to clearly mark that
structure as a (temporarily) allocated one.  v4 WIP modifies
klp_shadow_update_or_attach() to include both "new_shadow" and "shadow",
so there will be a convention established there.

>> +	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;
> 
> The return value of klp_shadow_get() can be tested directly, no need for
> a variable.

Yup.

> 
>> +	/* 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;
> 
> The goto destination is just a single line return, so I think it's
> clearer to just return NULL here and get rid of the err label.

Yeah, in isolation I'd agree with you, however I chose to error on the
side of the overall pattern: klp_shadow_attach(),
klp_shadow_get_or_attach(), and klp_shadow_update_or_attach() all follow
the same structure.  It felt more consistent to maintain similar exit
patterns and bend this style rule.  Either way, the point is moot as
fixing the problems Miroslav pointed out (and below) required
refactoring these routines and their return paths.

>> +
>> +	/* 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 */
> 
> Multi-line comments should be in the kernel coding style:
> 
> 		/*
> 		 * Shadow variable found, throw away allocation and take
> 		 * error exit path
> 		 */
> 
> Also, complete sentences preferred :-)

Looks like someone forgot to run through checkpatch.  Sorry about that.

> 
>> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
>> +		kfree(shadow_data);
> 
> Shouldn't it free new_shadow instead of shadow_data?

You are correct ... will fix in v4.

>> +		goto err_exists;
>> +	}
>> +
>> +	/* No <obj, id> found, add the newly allocated one */
>> +	shadow_data = data;
>> +	klp_shadow_set(new_shadow, obj, id, data, size);
> 
> To avoid doing extra work with the lock held, the klp_shadow_set() can
> be done before getting the lock, after the kzalloc.

The interesting work being a potentially speculative memcpy... moving
outside the lock, after the first search-miss, makes sense.

Thanks,

-- Joe