[PATCH v4] livepatch: shadow variables

[PATCH v4] livepatch: shadow variables

[Joe Lawrence]

v4

- klp_shadow_attach(), klp_shadow_get_or_attach(), and
  klp_shadow_update_or_attach()
  - fix up return values depending on whether a new_shadow
    variable was allocated, or an existing one was used
  - kfree new_shadow, not shadow_data when shadow variable is found
    under the lock (2nd search try)
  - refactor away most of the exit labels
  - move klp_shadow_set() calls outside of the klp_shadow_lock
  - fix multiline comment format

- klp_shadow_attach()
  - drop unnecessary variable assignment for conditional

- s/shadow_match()/klp_shadow_match()/g

- klp_shadow_match(), klp_shadow_set(), klp_shadow_add()
  - add "caller should hold lock" comments

- Documentation
  - remove unnecessary klp_shadow_get() call in use-case
  - s/its shadow variable lifetimes/their shadow variables lifetimes/

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

 Documentation/livepatch/shadow-vars.txt   | 215 +++++++++++++++++
 include/linux/livepatch.h                 |  10 +
 kernel/livepatch/Makefile                 |   2 +-
 kernel/livepatch/shadow.c                 | 382 ++++++++++++++++++++++++++++++
 samples/Kconfig                           |   5 +-
 samples/livepatch/Makefile                |   3 +
 samples/livepatch/livepatch-shadow-fix1.c | 174 ++++++++++++++
 samples/livepatch/livepatch-shadow-fix2.c | 167 +++++++++++++
 samples/livepatch/livepatch-shadow-mod.c  | 224 ++++++++++++++++++
 9 files changed, 1178 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-fix1.c
 create mode 100644 samples/livepatch/livepatch-shadow-fix2.c
 create mode 100644 samples/livepatch/livepatch-shadow-mod.c

-- 
1.8.3.1

[PATCH v4] 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 example modules that
demonstrate shadow variables.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 Documentation/livepatch/shadow-vars.txt   | 215 +++++++++++++++++
 include/linux/livepatch.h                 |  10 +
 kernel/livepatch/Makefile                 |   2 +-
 kernel/livepatch/shadow.c                 | 382 ++++++++++++++++++++++++++++++
 samples/Kconfig                           |   5 +-
 samples/livepatch/Makefile                |   3 +
 samples/livepatch/livepatch-shadow-fix1.c | 174 ++++++++++++++
 samples/livepatch/livepatch-shadow-fix2.c | 167 +++++++++++++
 samples/livepatch/livepatch-shadow-mod.c  | 224 ++++++++++++++++++
 9 files changed, 1178 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-fix1.c
 create mode 100644 samples/livepatch/livepatch-shadow-fix2.c
 create mode 100644 samples/livepatch/livepatch-shadow-mod.c

diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
new file mode 100644
index 000000000000..b8e918a37078
--- /dev/null
+++ b/Documentation/livepatch/shadow-vars.txt
@@ -0,0 +1,215 @@
+================
+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 their shadow variables 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);
+	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..0ebd4b635e4f
--- /dev/null
+++ b/kernel/livepatch/shadow.c
@@ -0,0 +1,382 @@
+/*
+ * 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[];
+};
+
+/**
+ * klp_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.
+ *
+ * Callers should hold the klp_shadow_lock.
+ */
+static inline bool klp_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 (klp_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
+ *
+ * Callers should hold the klp_shadow_lock.
+ */
+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
+ *
+ * Callers should hold the klp_shadow_lock.
+ */
+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 */
+	if (unlikely(klp_shadow_get(obj, id)))
+		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;
+	klp_shadow_set(new_shadow, obj, id, data, size);
+
+	/* 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 was found, throw away speculative
+		 * allocation and update/return the existing one.
+		 */
+		spin_unlock_irqrestore(&klp_shadow_lock, flags);
+		kfree(new_shadow);
+		goto err_exists;
+	}
+
+	/* No <obj, id> found, add the newly allocated one */
+	klp_shadow_add(new_shadow);
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+
+	return new_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)
+		return shadow_data;
+
+	/* Allocate a new shadow variable for use inside the lock below */
+	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
+	if (!new_shadow)
+		return NULL;
+	klp_shadow_set(new_shadow, obj, id, data, size);
+
+	/* 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 was found, throw away speculative
+		 * allocation and update/return the existing one.
+		 */
+		spin_unlock_irqrestore(&klp_shadow_lock, flags);
+		kfree(new_shadow);
+		return shadow_data;
+	}
+
+	/* No <obj, id> found, so attach the newly allocated one */
+	klp_shadow_add(new_shadow);
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+
+	return new_shadow->data;
+}
+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, *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)
+		return NULL;
+	klp_shadow_set(new_shadow, obj, id, data, size);
+
+	/* 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 was found, throw away speculative
+		 * allocation and update/return the existing one.
+		 */
+		kfree(new_shadow);
+		goto update_unlock_ret;
+	}
+
+	/* Could not find one, so attach the new one */
+	klp_shadow_add(new_shadow);
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+
+	return new_shadow->data;
+
+update_unlock_ret:
+	/* Update already attached shadow variable */
+	shadow = container_of(shadow_data, struct klp_shadow, data);
+	klp_shadow_set(shadow, obj, id, data, size);
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+
+	return shadow->data;
+}
+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 (klp_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 (klp_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-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");
diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
new file mode 100644
index 000000000000..4c54b250332d
--- /dev/null
+++ b/samples/livepatch/livepatch-shadow-mod.c
@@ -0,0 +1,224 @@
+/*
+ * 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);
-- 
1.8.3.1

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

[Josh Poimboeuf]

On Mon, Aug 14, 2017 at 04:02:43PM -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()
> 
> 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 example modules that
> demonstrate shadow variables.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH v4] 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
> + *
> + * Callers should hold the klp_shadow_lock.
> + */
> +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_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 */
> +	if (unlikely(klp_shadow_get(obj, id)))
> +		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;
> +	klp_shadow_set(new_shadow, obj, id, data, size);

There is a comment above about locking and we do not take the spinlock 
here. That could surprise someone. So I'd keep only klp_shadow_add() 
comment, because there it is strictly needed. It depends on the context in 
all other cases.

Could you also add a comment above klp_shadow_lock definition about what 
it aims to protect?

> +	/* 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_data is not needed anywhere, so you could do the same as for the 
first speculative search and remove shadow_data variable all together.

> +		/*
> +		 * Shadow variable was found, throw away speculative
> +		 * allocation and update/return the existing one.
> +		 */
> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +		kfree(new_shadow);
> +		goto err_exists;
> +	}
> +
> +	/* No <obj, id> found, add the newly allocated one */
> +	klp_shadow_add(new_shadow);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return new_shadow->data;
> +
> +err_exists:
> +	WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> +err:
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);

Otherwise it looks good. You can add my

Acked-by: Miroslav Benes <mbenes@suse.cz>

with those nits fixed.

Thanks,
Miroslav

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

[Joe Lawrence]

On 08/16/2017 08:43 AM, Miroslav Benes wrote:
> 
>> [ ... snip ... ]
> 
> There is a comment above about locking and we do not take the spinlock 
> here. That could surprise someone. So I'd keep only klp_shadow_add() 
> comment, because there it is strictly needed. It depends on the context in 
> all other cases.

Good catch, I think this changed in this last version when I moved some
of the work outside the lock.

> Could you also add a comment above klp_shadow_lock definition about what 
> it aims to protect?
> 

How about "klp_shadow_lock provides exclusive access to the
klp_shadow_hash and the shadow variables it references."  or were
thinking of something more detailed?

>> +	/* 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_data is not needed anywhere, so you could do the same as for the 
> first speculative search and remove shadow_data variable all together.

Ok.

>> [ ... snip ... ]
> 
> Otherwise it looks good. You can add my
> 
> Acked-by: Miroslav Benes <mbenes@suse.cz>
> 
> with those nits fixed.

Thank you for all the suggestions and reviews!

-- Joe

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

[Miroslav Benes]

> > Could you also add a comment above klp_shadow_lock definition about what 
> > it aims to protect?
> > 
> 
> How about "klp_shadow_lock provides exclusive access to the
> klp_shadow_hash and the shadow variables it references."  or were
> thinking of something more detailed?

No, this is good. Thanks.

Miroslav

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

[Petr Mladek]

On Mon 2017-08-14 16:02:43, 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()
> 
> 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 example modules that
> demonstrate shadow variables.
> 
> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> new file mode 100644
> index 000000000000..0ebd4b635e4f
> --- /dev/null
> +++ b/kernel/livepatch/shadow.c
> +/**
> + * klp_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.
> + *
> + * Callers should hold the klp_shadow_lock.
> + */
> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
> +				unsigned long id)
> +{
> +	return shadow->obj == obj && shadow->id == id;
> +}

Do we really need this function? It is called only in situations
where shadow->obj == obj is always true. Especially the use in
klp_shadow_detach_all() is funny because we pass shadow->obj as
the shadow parameter.

> +
> +/**
> + * 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 (klp_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
> + *
> + * Callers should hold the klp_shadow_lock.
> + */
> +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);
> +}

The function name suggests that it is a counterpart of
klp_shadow_get() but it is not. Which is a bit confusing.

Hmm, the purpose of this function is to reduce the size of cut&pasted
code between all that klp_shadow_*attach() variants. But there
is still too much cut&pasted code. In fact, the base logic of all
variants is basically the same. The only small difference should be
how they handle the situation when the variable is already there.

OK, there is a locking difference in the update variant but
it is questionable, see below.

I would suggest to do something like this:

static enum klp_shadow_attach_existing_handling {
     KLP_SHADOW_EXISTING_RETURN,
     KLP_SHADOW_EXISTING_WARN,
     KLP_SHADOW_EXISING_UPDATE,
};

void *__klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
		       size_t size, gfp_t gfp_flags,
		       enum klp_shadow_attach_existing_handling existing_handling)
{
	struct klp_shadow *new_shadow;
	void *shadow_data;
	unsigned long flags;

	/* Check if the shadow variable if <obj, id> already exists */
	shadow_data = klp_shadow_get(obj, id);
	if (shadow_data)
		goto exists;

	/* Allocate a new shadow variable for use inside the lock below */
	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
	if (!new_shadow) {
		pr_error("failed to allocate shadow variable <0x%p, %ul>\n",
				 obj, id);
		return NULL;
	}

	new_shadow->obj = obj;
	new_shadow->id = id;

	/* initialize the shadow variable if if data provided */
	if (data)
		memcpy(new_shadow->data, data, size);

	/* 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 was found, throw away speculative
		 * allocation and update/return the existing one.
		 */
		spin_unlock_irqrestore(&klp_shadow_lock, flags);
		kfree(new_shadow);
		goto exists;
	}

	/* No <obj, id> found, so attach the newly allocated one */
	hash_add_rcu(klp_shadow_hash, &new_shadow->node,
		     (unsigned long)new_shadow->obj);
	spin_unlock_irqrestore(&klp_shadow_lock, flags);

	return new_shadow->data;

exists:
	switch(existing_handling) {
	case KLP_SHADOW_EXISTING_RETURN:
		break;

	case KLP_SHADOW_EXISTING_WARN:
		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
		shadow_data = NULL;
		break;

	case KLP_SHADOW_EXISING_UPDATE:
		if (data)
			memcpy(shadow_data, data, size);
		break;
	}
	return shadow_data;
}

void *klp_shadow_attach(void *obj, unsigned long id, void *data,
			size_t size, gfp_t gfp_flags)
{
	return __klp_shadow_get_or_attach(obj, id, data, size,
			gfp_flags, KLP_SHADOW_EXISTING_RETURN);
}

void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
			       size_t size, gfp_t gfp_flags)
{
	return __klp_shadow_get_or_attach(obj, id, data, size,
			gfp_flags, KLP_SHADOW_EXISTING_WARN);
}

void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
			       size_t size, gfp_t gfp_flags)
{
	return __klp_shadow_get_or_attach(obj, id, data, size,
			gfp_flags, KLP_SHADOW_EXISTING_UPDATE);
}

Note that the above code is not even compile tested.

It removes a lot of cut&pasted code and the difference
is more clear.

It updates the data outside klp_shadow_lock. But it should be fine.
The lock is there to keep the hast table consistent (avoid
duplicates). Users are responsible to synchronize the data
stored in the variables their own way.

Hmm, the more I think about it the more I would suggest to remove
klp_shadow_update_or_attach() variant. It has very weird logic.
IMHO, it is not obvious that it must be called under the locks
that synchronize the shadow data. IMHO, the other two variants
are much more clear and enough. Or do you have a good real life
example for it?



> +/**
> + * klp_shadow_add() - add a shadow variable to the hashtable
> + * @shadow:	shadow variable to add
> + *
> + * Callers should hold the klp_shadow_lock.
> + */
> +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.

I would swap the two paragraphs. We should describe the main function
first and corner cases later.


> + * 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 */
> +	if (unlikely(klp_shadow_get(obj, id)))
> +		goto err_exists;
> +
> +	/* Allocate a new shadow variable for use inside the lock below */
> +	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);

We should print an error message when the memory cannot be allocated.
Otherwise we will return NULL without explanation. It will be
especially helpful when a caller forgets to check for NULL.


> +	if (!new_shadow)
> +		goto err;
> +	klp_shadow_set(new_shadow, obj, id, data, size);
> +
> +	/* 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 was found, throw away speculative
> +		 * allocation and update/return the existing one.
> +		 */
> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +		kfree(new_shadow);
> +		goto err_exists;
> +	}
> +
> +	/* No <obj, id> found, add the newly allocated one */
> +	klp_shadow_add(new_shadow);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return new_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.

There is a lot of duplicated text here. Also you need to read the first
paragraph very carefully. Otherwise, you miss that the 2nd paragraph
is true only in special situation.

I would make it more clear, something like:

It returns pointer to the shadow data when they already exist.
Otherwise, it attaches and new shadow variable like
klp_shadow_attach().

It guarantees that only one shadow variable will exists with
the given @id for the given @obj. Also it guarantees that
the variable will be initialized by the given @data only when
it did not exist before.


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

This should get removed. The buffer used for the shadow variable
is freed by kfree_rcu() called from klp_shadow_detach().

Same problem is also in the other livepatch.

> +		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
> +			 __func__, d, *shadow_leak);

This might access shadow_leak after it was (double) freed.

> +	} else {
> +		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
> +	}
> +
> +	kfree(d);
> +}


I am sorry for the late review. I had vacation.

I know that this patch already got some acks. Most of my comments
are about code clean up and tiny bugs that might be fixed later.

But I would still suggests to re-evaluate the usefulness and
logic of klp_shadow_update_or_attach(). I think that it was
the primary reason for the many cut&pasted code. The locking
logic is weird there. It does too many things at once because
it also manipulates the existing data. All other functions just
get pointer to the data and eventually initialize them when
they did not exist before.

Best Regards,
Petr

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

[Joe Lawrence]

On 08/17/2017 10:05 AM, Petr Mladek wrote:
> On Mon 2017-08-14 16:02:43, 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()
>>
>> 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 example modules that
>> demonstrate shadow variables.
>>
>> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
>> new file mode 100644
>> index 000000000000..0ebd4b635e4f
>> --- /dev/null
>> +++ b/kernel/livepatch/shadow.c
>> +/**
>> + * klp_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.
>> + *
>> + * Callers should hold the klp_shadow_lock.
>> + */
>> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
>> +				unsigned long id)
>> +{
>> +	return shadow->obj == obj && shadow->id == id;
>> +}
> 
> Do we really need this function? It is called only in situations
> where shadow->obj == obj is always true. Especially the use in
> klp_shadow_detach_all() is funny because we pass shadow->obj as
> the shadow parameter.

Personal preference.  Abstracting out all of the routines that operated
on the shadow variables (setting up, comparison) did save some code
lines and centralized these common bits.

>> +
>> +/**
>> + * 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 (klp_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
>> + *
>> + * Callers should hold the klp_shadow_lock.
>> + */
>> +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);
>> +}
> 
> The function name suggests that it is a counterpart of
> klp_shadow_get() but it is not. Which is a bit confusing.
I should have called it "setup" or "init", but perhaps that's moot ...

> Hmm, the purpose of this function is to reduce the size of cut&pasted
> code between all that klp_shadow_*attach() variants. But there
> is still too much cut&pasted code. In fact, the base logic of all
> variants is basically the same. The only small difference should be
> how they handle the situation when the variable is already there.

... this is true.  An earlier draft revision that I had discarded
attempted combining all cases.  I had used two extra function arguments,
"update" and "duplicates", to key off for each behavior... it turned
into a complicated, full-screen page of conditional logic, so I threw it
out.

However, I like the way you pulled it off using a jump-to-switch
statement at the bottom of the function...

> OK, there is a locking difference in the update variant but
> it is questionable, see below.
> 
> I would suggest to do something like this:
> 
> static enum klp_shadow_attach_existing_handling {
>      KLP_SHADOW_EXISTING_RETURN,
>      KLP_SHADOW_EXISTING_WARN,
>      KLP_SHADOW_EXISING_UPDATE,
> };
> 
> void *__klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> 		       size_t size, gfp_t gfp_flags,
> 		       enum klp_shadow_attach_existing_handling existing_handling)
> {
> 	struct klp_shadow *new_shadow;
> 	void *shadow_data;
> 	unsigned long flags;
> 
> 	/* Check if the shadow variable if <obj, id> already exists */
> 	shadow_data = klp_shadow_get(obj, id);
> 	if (shadow_data)
> 		goto exists;
> 
> 	/* Allocate a new shadow variable for use inside the lock below */
> 	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> 	if (!new_shadow) {
> 		pr_error("failed to allocate shadow variable <0x%p, %ul>\n",
> 				 obj, id);
> 		return NULL;
> 	}
> 
> 	new_shadow->obj = obj;
> 	new_shadow->id = id;
> 
> 	/* initialize the shadow variable if if data provided */
> 	if (data)
> 		memcpy(new_shadow->data, data, size);
> 
> 	/* 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 was found, throw away speculative
> 		 * allocation and update/return the existing one.
> 		 */
> 		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> 		kfree(new_shadow);
> 		goto exists;
> 	}
> 
> 	/* No <obj, id> found, so attach the newly allocated one */
> 	hash_add_rcu(klp_shadow_hash, &new_shadow->node,
> 		     (unsigned long)new_shadow->obj);
> 	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> 
> 	return new_shadow->data;
> 
> exists:
> 	switch(existing_handling) {
> 	case KLP_SHADOW_EXISTING_RETURN:
> 		break;
> 
> 	case KLP_SHADOW_EXISTING_WARN:
> 		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
> 		shadow_data = NULL;
> 		break;
> 
> 	case KLP_SHADOW_EXISING_UPDATE:
> 		if (data)
> 			memcpy(shadow_data, data, size);
> 		break;
> 	}
> 	return shadow_data;
> }
> 
> void *klp_shadow_attach(void *obj, unsigned long id, void *data,
> 			size_t size, gfp_t gfp_flags)
> {
> 	return __klp_shadow_get_or_attach(obj, id, data, size,
> 			gfp_flags, KLP_SHADOW_EXISTING_RETURN);
> }
> 
> void *klp_shadow_get_or_attach(void *obj, unsigned long id, void *data,
> 			       size_t size, gfp_t gfp_flags)
> {
> 	return __klp_shadow_get_or_attach(obj, id, data, size,
> 			gfp_flags, KLP_SHADOW_EXISTING_WARN);
> }
> 
> void *klp_shadow_update_or_attach(void *obj, unsigned long id, void *data,
> 			       size_t size, gfp_t gfp_flags)
> {
> 	return __klp_shadow_get_or_attach(obj, id, data, size,
> 			gfp_flags, KLP_SHADOW_EXISTING_UPDATE);
> }
> 
> Note that the above code is not even compile tested.
> 
> It removes a lot of cut&pasted code and the difference
> is more clear.
>
> It updates the data outside klp_shadow_lock. But it should be fine.
> The lock is there to keep the hast table consistent (avoid
> duplicates). 
>
>              Users are responsible to synchronize the data
> stored in the variables their own way.

^^^ this is probably the best argument to ditch
klp_shadow_update_or_attach().

> 
> Hmm, the more I think about it the more I would suggest to remove
> klp_shadow_update_or_attach() variant. It has very weird logic.
> IMHO, it is not obvious that it must be called under the locks
> that synchronize the shadow data. IMHO, the other two variants
> are much more clear and enough. Or do you have a good real life
> example for it?

>From the API caller's point of view, the intent was: "I want a shadow
variable and it needs to have this current value."

klp_shadow_get_or_attach() doesn't pass back information about the
underlying shadow variable, ie, was an existing one found, or did it
allocate a new one.  In the former case, the data will be stale.

I don't have a real-world example for such use-case, so I'm willing to
drop the routine if it simplifies the code.  I'd be curious to see how
you might solve this situation using klp_shadow_get() and/or
klp_shadow_get_or_attach().

>> +/**
>> + * klp_shadow_add() - add a shadow variable to the hashtable
>> + * @shadow:	shadow variable to add
>> + *
>> + * Callers should hold the klp_shadow_lock.
>> + */
>> +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.
> 
> I would swap the two paragraphs. We should describe the main function
> first and corner cases later.

Okay, that makes sense.

>> + * 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 */
>> +	if (unlikely(klp_shadow_get(obj, id)))
>> +		goto err_exists;
>> +
>> +	/* Allocate a new shadow variable for use inside the lock below */
>> +	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> 
> We should print an error message when the memory cannot be allocated.
> Otherwise we will return NULL without explanation. It will be
> especially helpful when a caller forgets to check for NULL.

That's a good idea.  Silent failure could be confusing.

>> +	if (!new_shadow)
>> +		goto err;
>> +	klp_shadow_set(new_shadow, obj, id, data, size);
>> +
>> +	/* 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 was found, throw away speculative
>> +		 * allocation and update/return the existing one.
>> +		 */
>> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
>> +		kfree(new_shadow);
>> +		goto err_exists;
>> +	}
>> +
>> +	/* No <obj, id> found, add the newly allocated one */
>> +	klp_shadow_add(new_shadow);
>> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
>> +
>> +	return new_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.
> 
> There is a lot of duplicated text here. 

Copy/pasting -- it was a lot easier to keep the comments in sync with
the code as it was evolving through the revisions, especially with three
variants of the nearly same routine :)

>                                        Also you need to read the first
> paragraph very carefully. Otherwise, you miss that the 2nd paragraph
> is true only in special situation.
> 
> I would make it more clear, something like:
> 
> It returns pointer to the shadow data when they already exist.
> Otherwise, it attaches and new shadow variable like
> klp_shadow_attach().
> 
> It guarantees that only one shadow variable will exists with
> the given @id for the given @obj. Also it guarantees that
> the variable will be initialized by the given @data only when
> it did not exist before.

Good feedback, I'll incorporate something like this for the next version.

>> 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
>> +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);
> 
> This should get removed. The buffer used for the shadow variable
> is freed by kfree_rcu() called from klp_shadow_detach().
> 
> Same problem is also in the other livepatch.
> 
>> +		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
>> +			 __func__, d, *shadow_leak);
> 
> This might access shadow_leak after it was (double) freed.
> 
>> +	} else {
>> +		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
>> +	}
>> +
>> +	kfree(d);
>> +}

Let me double check these double frees (though I have been running the
tests w/slub_debug poisoning turned on).  At first glance, they look
like another holdover from the shadow-data-is-just-a pointer versions.

> 
> I am sorry for the late review. I had vacation.

No worries, this is a good review!

> I know that this patch already got some acks. Most of my comments
> are about code clean up and tiny bugs that might be fixed later.
> 
> But I would still suggests to re-evaluate the usefulness and
> logic of klp_shadow_update_or_attach(). I think that it was
> the primary reason for the many cut&pasted code. The locking
> logic is weird there. It does too many things at once because
> it also manipulates the existing data. All other functions just
> get pointer to the data and eventually initialize them when
> they did not exist before.

Without a good real-world example, you've convinced me that
klp_shadow_update_or_attach() could be dropped.  I think this will also
simplify the requirements of a shared __klp_shadow_get_or_attach() like
you sketched out earlier.

If Josh and Miroslav don't mind, I'd like to continue churning this
patch with the suggestions that Petr has made.

-- Joe

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

[Josh Poimboeuf]

On Thu, Aug 17, 2017 at 12:01:33PM -0400, Joe Lawrence wrote:
> Without a good real-world example, you've convinced me that
> klp_shadow_update_or_attach() could be dropped.  I think this will also
> simplify the requirements of a shared __klp_shadow_get_or_attach() like
> you sketched out earlier.
> 
> If Josh and Miroslav don't mind, I'd like to continue churning this
> patch with the suggestions that Petr has made.

By all means, go ahead.  I rescind my Ack ;-)

-- 
Josh

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

[Petr Mladek]

On Thu 2017-08-17 12:01:33, Joe Lawrence wrote:
> On 08/17/2017 10:05 AM, Petr Mladek wrote:
> >> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> >> new file mode 100644
> >> index 000000000000..0ebd4b635e4f
> >> --- /dev/null
> >> +++ b/kernel/livepatch/shadow.c
> >> +/**
> >> + * klp_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.
> >> + *
> >> + * Callers should hold the klp_shadow_lock.
> >> + */
> >> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
> >> +				unsigned long id)
> >> +{
> >> +	return shadow->obj == obj && shadow->id == id;
> >> +}
> > 
> > Do we really need this function? It is called only in situations
> > where shadow->obj == obj is always true. Especially the use in
> > klp_shadow_detach_all() is funny because we pass shadow->obj as
> > the shadow parameter.
> 
> Personal preference.  Abstracting out all of the routines that operated
> on the shadow variables (setting up, comparison) did save some code
> lines and centralized these common bits.

I take this back. We actually need to check obj because different
objects might have the same hash.

I think that I did the same mistake also the last time. I hope that
I will be able to fix this in my mind faster than "never" vs. "newer"
typo that I do for years.

Also I forgot to say that you did great work. Each version of the
patch is much better than the previous one.

Best Regards,
Petr

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

[Josh Poimboeuf]

On Fri, Aug 18, 2017 at 11:42:50AM +0200, Petr Mladek wrote:
> On Thu 2017-08-17 12:01:33, Joe Lawrence wrote:
> > On 08/17/2017 10:05 AM, Petr Mladek wrote:
> > >> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> > >> new file mode 100644
> > >> index 000000000000..0ebd4b635e4f
> > >> --- /dev/null
> > >> +++ b/kernel/livepatch/shadow.c
> > >> +/**
> > >> + * klp_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.
> > >> + *
> > >> + * Callers should hold the klp_shadow_lock.
> > >> + */
> > >> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
> > >> +				unsigned long id)
> > >> +{
> > >> +	return shadow->obj == obj && shadow->id == id;
> > >> +}
> > > 
> > > Do we really need this function? It is called only in situations
> > > where shadow->obj == obj is always true. Especially the use in
> > > klp_shadow_detach_all() is funny because we pass shadow->obj as
> > > the shadow parameter.
> > 
> > Personal preference.  Abstracting out all of the routines that operated
> > on the shadow variables (setting up, comparison) did save some code
> > lines and centralized these common bits.
> 
> I take this back. We actually need to check obj because different
> objects might have the same hash.
> 
> I think that I did the same mistake also the last time. I hope that
> I will be able to fix this in my mind faster than "never" vs. "newer"
> typo that I do for years.

It's an easy mistake to make.  hash_for_each_possible() is not
intuitive, IMO.  Maybe some brave soul should fix it.

-- 
Josh

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

[Joe Lawrence]

On 08/17/2017 10:05 AM, Petr Mladek wrote:
> On Mon 2017-08-14 16:02:43, Joe Lawrence wrote:
>> [ ... snip ... ]
>> 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
>> +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);
> 
> This should get removed. The buffer used for the shadow variable
> is freed by kfree_rcu() called from klp_shadow_detach().
> 
> Same problem is also in the other livepatch.
> 
>> +		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
>> +			 __func__, d, *shadow_leak);
> 
> This might access shadow_leak after it was (double) freed.
> 
>> +	} else {
>> +		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
>> +	}
>> +
>> +	kfree(d);
>> +}

Hi Petr,

I think you're half correct.

The kfree is the crux of the memory leak patch, so it needs to stay.
However, the shadow variable is holding a copy of the pointer to the
memory leak area, so you're right that it can't be safely dereferenced
after the shadow variable is detached*.

The code should to be rearranged like:

void livepatch_fix1_dummy_free(struct dummy *d)
{
        void **p_shadow_leak, *shadow_leak;

        p_shadow_leak = klp_shadow_get(d, SV_LEAK);
        if (p_shadow_leak) {
                shadow_leak = *p_shadow_leak;   << deref before detach
                klp_shadow_detach(d, SV_LEAK);
                kfree(shadow_leak);
        ...

* Aside: I usually develop with slub_debug=FZPU set to catch silly
use-after-frees like this.  However, since the shadow variable is
released via kfree_rcu(), I think there was a window before the grace
period where this one worked out okay...  once I added a
synchronize_rcu() call in between the klp_shadow_detch() and kfree()
calls, I did see the poison pattern.  This is my first time using
kfree_rcu(), so it was interesting to dig into.

Thanks,

-- Joe

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

[Petr Mladek]

On Fri 2017-08-18 09:46:08, Joe Lawrence wrote:
> On 08/17/2017 10:05 AM, Petr Mladek wrote:
> > On Mon 2017-08-14 16:02:43, Joe Lawrence wrote:
> >> [ ... snip ... ]
> >> 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
> >> +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);
> > 
> > This should get removed. The buffer used for the shadow variable
> > is freed by kfree_rcu() called from klp_shadow_detach().
> > 
> > Same problem is also in the other livepatch.
> > 
> >> +		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
> >> +			 __func__, d, *shadow_leak);
> > 
> > This might access shadow_leak after it was (double) freed.
> > 
> >> +	} else {
> >> +		pr_info("%s: dummy @ %p leaked!\n", __func__, d);
> >> +	}
> >> +
> >> +	kfree(d);
> >> +}
> 
> Hi Petr,
> 
> I think you're half correct.
> 
> The kfree is the crux of the memory leak patch, so it needs to stay.
> However, the shadow variable is holding a copy of the pointer to the
> memory leak area, so you're right that it can't be safely dereferenced
> after the shadow variable is detached*.

Ah, I see. The extra kftree does not free the shadow->data but
it frees the data that the shadow variable points to.

> The code should to be rearranged like:
> 
> void livepatch_fix1_dummy_free(struct dummy *d)
> {
>         void **p_shadow_leak, *shadow_leak;
> 
>         p_shadow_leak = klp_shadow_get(d, SV_LEAK);
>         if (p_shadow_leak) {
>                 shadow_leak = *p_shadow_leak;   << deref before detach

I would rename shadow_leak -> leak. It will make it more clear
that it is the original leak pointer.

Well, we could actually free the data before we detach/destroy
the shadow variable. But then it might deserve a comment to
avoid confusion that I had. I mean:

	shadow_leak = klp_shadow_get(d, SV_LEAK);
	if (shadow_leak) {
		pr_info("%s: dummy @ %p, prevented leak @ %p\n",
			 __func__, d, *shadow_leak);
		/* Free the previously leaked data */
		kfree(*shadow_leak);
		/* Free the shadow variable */
		klp_shadow_detach(d, SV_LEAK);

>                 klp_shadow_detach(d, SV_LEAK);
>                 kfree(shadow_leak);
>         ...
> 
> * Aside: I usually develop with slub_debug=FZPU set to catch silly
> use-after-frees like this.

Sounds like a good practice.

> released via kfree_rcu(), I think there was a window before the grace
> period where this one worked out okay...  once I added a
> synchronize_rcu() call in between the klp_shadow_detch() and kfree()
> calls, I did see the poison pattern.  This is my first time using
> kfree_rcu(), so it was interesting to dig into.

Yup.

Best Regards,
Petr

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

[Joe Lawrence]

On 08/17/2017 10:05 AM, Petr Mladek wrote:
> On Mon 2017-08-14 16:02:43, Joe Lawrence wrote:
>> [ ... snip ... ]
>> +	/* Allocate a new shadow variable for use inside the lock below */
>> +	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> 
> We should print an error message when the memory cannot be allocated.
> Otherwise we will return NULL without explanation. It will be
> especially helpful when a caller forgets to check for NULL.

Interesting, I hadn't seen this checkpatch complaint before:

WARNING: Possible unnecessary 'out of memory' message
#416: FILE: kernel/livepatch/shadow.c:143:
+       if (!new_shadow) {
+               pr_err("failed to allocate shadow variable <0x%p, %lu>\n",

Discussion thread:
https://lkml.org/lkml/2014/6/10/382

Think the stack trace that the memory subsystem would emit is good
enough, or would you like to see <obj, id> for debugging purposes?

-- Joe

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

[Petr Mladek]

On Fri 2017-08-18 16:25:42, Joe Lawrence wrote:
> On 08/17/2017 10:05 AM, Petr Mladek wrote:
> > On Mon 2017-08-14 16:02:43, Joe Lawrence wrote:
> >> [ ... snip ... ]
> >> +	/* Allocate a new shadow variable for use inside the lock below */
> >> +	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
> > 
> > We should print an error message when the memory cannot be allocated.
> > Otherwise we will return NULL without explanation. It will be
> > especially helpful when a caller forgets to check for NULL.
> 
> Interesting, I hadn't seen this checkpatch complaint before:
> 
> WARNING: Possible unnecessary 'out of memory' message
> #416: FILE: kernel/livepatch/shadow.c:143:
> +       if (!new_shadow) {
> +               pr_err("failed to allocate shadow variable <0x%p, %lu>\n",
> 
> Discussion thread:
> https://lkml.org/lkml/2014/6/10/382

Interesting, I was not aware of this.

> Think the stack trace that the memory subsystem would emit is good
> enough, or would you like to see <obj, id> for debugging purposes?

I agree that the backtrace should be enough to locate the problematic call
quickly. Feel free to omit it.

Now, I just need to update my patterns when looking for problematic
code.

Best Regards,
Petr

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

[Nicolai Stange]

Joe Lawrence <joe.lawrence@redhat.com> writes:

<snip>
> +
> +/**
> + * 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 (klp_shadow_match(shadow, obj, id)) {
> +			rcu_read_unlock();
> +			return shadow->data;

I had to think a moment about what protects shadow from getting freed by
a concurrent detach after that rcu_read_unlock(). Then I noticed that if
obj and the livepatch are alive, then so is shadow, because there
obviously hasn't been any reason to detach it.

So maybe it would be nice to have an additional comment at
klp_shadow_detach() that it's the API user's responsibility not to use a
shadow instance after detaching it...

Thanks,

Nicolai


> +		}
> +	}
> +
> +	rcu_read_unlock();
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_get);
<snap>


-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

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

[Petr Mladek]

On Fri 2017-08-18 15:44:29, Nicolai Stange wrote:
> Joe Lawrence <joe.lawrence@redhat.com> writes:
> 
> <snip>
> > +
> > +/**
> > + * 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 (klp_shadow_match(shadow, obj, id)) {
> > +			rcu_read_unlock();
> > +			return shadow->data;
> 
> I had to think a moment about what protects shadow from getting freed by
> a concurrent detach after that rcu_read_unlock(). Then I noticed that if
> obj and the livepatch are alive, then so is shadow, because there
> obviously hasn't been any reason to detach it.
> 
> So maybe it would be nice to have an additional comment at
> klp_shadow_detach() that it's the API user's responsibility not to use a
> shadow instance after detaching it...

Good point. In fact, it might make sense to rename the functions:

     attach -> create
     detach -> destroy

The name detach suggests that the variable is just not connected to
the parent object but that it is still accessible/usable.

Best Regards,
Petr

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

[Joe Lawrence]

On 08/18/2017 10:04 AM, Petr Mladek wrote:
> On Fri 2017-08-18 15:44:29, Nicolai Stange wrote:
>> Joe Lawrence <joe.lawrence@redhat.com> writes:
>>
>> <snip>
>>> +
>>> +/**
>>> + * 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 (klp_shadow_match(shadow, obj, id)) {
>>> +			rcu_read_unlock();
>>> +			return shadow->data;
>>
>> I had to think a moment about what protects shadow from getting freed by
>> a concurrent detach after that rcu_read_unlock(). Then I noticed that if
>> obj and the livepatch are alive, then so is shadow, because there
>> obviously hasn't been any reason to detach it.
>>
>> So maybe it would be nice to have an additional comment at
>> klp_shadow_detach() that it's the API user's responsibility not to use a
>> shadow instance after detaching it...

Nicolai, I can add something like "This function releases the memory for
this shadow variable instance, callers should stop referencing it
accordingly."  Similar text for klp_shadow_detach_all().

> Good point. In fact, it might make sense to rename the functions:
> 
>      attach -> create
>      detach -> destroy
> 
> The name detach suggests that the variable is just not connected to
> the parent object but that it is still accessible/usable.

FWIW, kpatch calls them "kpatch_shadow_alloc" and "kpatch_shadow_free".
Now that it's clear that we're not going separate shadow variable
allocation from hash table insertion, going back to alloc/create and
destroy/free is fine w/me.

-- Joe

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

[Nicolai Stange]

Joe Lawrence <joe.lawrence@redhat.com> writes:

> On 08/18/2017 10:04 AM, Petr Mladek wrote:
>> On Fri 2017-08-18 15:44:29, Nicolai Stange wrote:
>>> Joe Lawrence <joe.lawrence@redhat.com> writes:
>>>
>>> <snip>
>>>> +
>>>> +/**
>>>> + * 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 (klp_shadow_match(shadow, obj, id)) {
>>>> +			rcu_read_unlock();
>>>> +			return shadow->data;
>>>
>>> I had to think a moment about what protects shadow from getting freed by
>>> a concurrent detach after that rcu_read_unlock(). Then I noticed that if
>>> obj and the livepatch are alive, then so is shadow, because there
>>> obviously hasn't been any reason to detach it.
>>>
>>> So maybe it would be nice to have an additional comment at
>>> klp_shadow_detach() that it's the API user's responsibility not to use a
>>> shadow instance after detaching it...
>
> Nicolai, I can add something like "This function releases the memory for
> this shadow variable instance, callers should stop referencing it
> accordingly."  Similar text for klp_shadow_detach_all().

Perfect, thanks!


>> Good point. In fact, it might make sense to rename the functions:
>> 
>>      attach -> create
>>      detach -> destroy
>> 
>> The name detach suggests that the variable is just not connected to
>> the parent object but that it is still accessible/usable.
>
> FWIW, kpatch calls them "kpatch_shadow_alloc" and "kpatch_shadow_free".
> Now that it's clear that we're not going separate shadow variable
> allocation from hash table insertion, going back to alloc/create and
> destroy/free is fine w/me.

-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)