[PATCH v2 0/2] livepatch: add shadow variable API

[PATCH v2 0/2] livepatch: add shadow variable API

[Joe Lawrence]

This is v2 of the shadow variable implementation patchset, incorporating
much of the feedback from the first version:

v2:
  - squashed the Documentation patch with the API implementation patch

  - converted API parameter/return documentation to docbook style comments
    in the .c implementation

  - converted the klp_shadow string descriptor to an unsigned long

  - combined shadow data and klp_shadow structure to one allocation

  - adopted kfree_rcu() suggestion

  - added klp_shadow_get_or_create() to the API to help avoid racing
    klp_shadow_get + klp_shadow_attach() instances

  - added klp_shadow_detach_all() to the API to cleanup a set of
    <*, num> shadow variables

  - created a new set of sample modules to demonstrate the API:
    - a buggy module
    - fix 1 to plug a memory leak in newly allocate data structures
    - fix 2 to add functionality to in-flight data structures

The sample modules are contrived to demonstrate the shadow variable API.
Instead of patching already in-tree code, I created a simple module to
avoid any kallsyms workarounds.  That said, the description and
demonstration debug printing could stand further refinement.  IMHO, the
code is easier to follow than the periodic kernel messages logged.
Suggestions welcome.

Joe Lawrence (2):
  livepatch: introduce shadow variable API
  livepatch: add shadow variable sample programs

 Documentation/livepatch/shadow-vars.txt   | 156 +++++++++++++
 include/linux/livepatch.h                 |   8 +
 kernel/livepatch/Makefile                 |   2 +-
 kernel/livepatch/shadow.c                 | 257 ++++++++++++++++++++++
 samples/Kconfig                           |   5 +-
 samples/livepatch/Makefile                |   3 +
 samples/livepatch/livepatch-shadow-fix1.c | 160 ++++++++++++++
 samples/livepatch/livepatch-shadow-fix2.c | 157 +++++++++++++
 samples/livepatch/livepatch-shadow-mod.c  | 353 ++++++++++++++++++++++++++++++
 9 files changed, 1097 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 v2 1/2] 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_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.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 Documentation/livepatch/shadow-vars.txt | 156 +++++++++++++++++++
 include/linux/livepatch.h               |   8 +
 kernel/livepatch/Makefile               |   2 +-
 kernel/livepatch/shadow.c               | 257 ++++++++++++++++++++++++++++++++
 4 files changed, 422 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/livepatch/shadow-vars.txt
 create mode 100644 kernel/livepatch/shadow.c

diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
new file mode 100644
index 000000000000..7f28982e6b1c
--- /dev/null
+++ b/Documentation/livepatch/shadow-vars.txt
@@ -0,0 +1,156 @@
+Shadow Variables
+================
+
+Shadow variables are a simple way for livepatch modules to associate new
+"shadow" data to existing data structures.  Original data structures
+(both their definition and storage) are left unmodified and "new" data
+is allocated separately.  A shadow variable hashtable associates a
+string key, enumeration pair with a pointer to the new data.
+
+
+Brief API summary
+-----------------
+
+See the full API usage docbook notes in the livepatch/shadow.c
+implementation.
+
+An in-kernel hashtable references all of the shadow variables.  These
+references are stored/retrieved through a <obj, num> key pair.
+
+* The klp_shadow variable data structure encapsulates both tracking
+meta-data and shadow-data:
+  - meta-data
+    - obj - pointer to original data
+    - num - numerical description of new data
+  - new_data[] - storage for shadow data
+
+* klp_shadow_attach() - allocate and add a new shadow variable:
+  - allocate a new shadow variable
+  - push a <obj, num> key pair into hashtable
+
+* klp_shadow_get() - retrieve a shadow variable new_data pointer
+  - search hashtable for <obj, num> key pair
+
+* klp_shadow_get_or_attach() - get existing or attach a new shadow variable
+  - search hashtable for <obj, num> key pair
+  - if not found, call klp_shadow_attach()
+
+* klp_shadow_detach() - detach and free a <obj, num> shadow variable
+  - find and remove any <obj, num> references from hashtable
+    - if found, release shadow variable
+
+* klp_shadow_detach() - detach and free all <*, num> shadow variables
+  - find and remove any <*, num> references from hashtable
+    - if found, release shadow variable
+
+
+Use cases
+---------
+
+See the example shadow variable livepatch modules in samples/livepatch
+for full working demonstrations.
+
+Example 1: Commit 1d147bfa6429 ("mac80211: fix AP powersave TX vs.
+wakeup race") added a spinlock to net/mac80211/sta_info.h :: struct
+sta_info.  Implementing this change with a shadow variable is
+straightforward.
+
+Allocation - when a host sta_info structure is allocated, attach a
+shadow variable copy of the ps_lock:
+
+#define PS_LOCK 1
+struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
+				const u8 *addr, gfp_t gfp)
+{
+	struct sta_info *sta;
+	spinlock_t *ps_lock;
+	...
+	sta = kzalloc(sizeof(*sta) + hw->sta_data_size, gfp);
+	...
+	ps_lock = klp_shadow_attach(sta, PS_LOCK, NULL, sizeof(*ps_lock), gfp);
+	if (!ps_lock)
+		goto shadow_fail;
+	spin_lock_init(ps_lock);
+	...
+
+Usage - when using the shadow spinlock, query the shadow variable API to
+retrieve it:
+
+void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
+{
+	spinlock_t *ps_lock;
+	...
+	/* sync with ieee80211_tx_h_unicast_ps_buf */
+	ps_lock = klp_shadow_get(sta, "ps_lock");
+	if (ps_lock)
+		spin_lock(ps_lock);
+	...
+	if (ps_lock)
+		spin_unlock(ps_lock);
+	...
+
+Release - when the host sta_info structure is freed, first detach the
+shadow variable and then free the shadow spinlock:
+
+void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
+{
+	spinlock_t *ps_lock;
+	...
+	ps_lock = klp_shadow_get(sta, "ps_lock");
+	if (ps_lock)
+		klp_shadow_detach(sta, "ps_lock");
+
+	kfree(sta);
+
+
+Example 2: Commit 82486aa6f1b9 ("ipv4: restore rt->fi for reference
+counting") added a struct fib_info pointer to include/net/route.h ::
+struct rtable. A shadow variable can be used to implement the new
+pointer.
+
+This implementation diverges from the original commit, as it can attach
+the shadow variable when the code actually uses it:
+
+#define FIB_INFO 1
+static void rt_init_metrics(struct rtable *rt, struct fib_info *fi)
+{
+	if (fi->fib_metrics != (u32 *)dst_default_metrics) {
+		fib_info_hold(&fi);
+		klp_shadow_attach(rt, FIB_INFO, &fi, sizeof(fi), GFP_KERNEL)
+	}
+
+	dst_init_metrics(&rt->dst, fi->fib_metrics, true);
+}
+
+The shadow variable can be detached when it's no longer needed:
+
+static void ipv4_dst_destroy(struct dst_entry *dst)
+{
+	struct rtable *rt = (struct rtable *) dst;
+	struct fib_info *shadow_fi;
+
+	shadow_fi = klp_shadow_get(rt, "fi");
+	if (shadow_fi) {
+		klp_shadow_detach(rt, "fi");
+		fib_info_put(shadow_fi);
+	}
+
+
+Other examples: shadow variables can also be used as a simple flag
+indicating that a data structure had been allocated by new, livepatched
+code.  In this case, it doesn't matter what new_data value the shadow
+variable holds, its existence can be keyed off of to handle the data
+structure accordingly.
+
+
+Reference
+==========
+
+* 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..4cf3c285784d 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -164,6 +164,14 @@ static inline bool klp_have_reliable_stack(void)
 	       IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
 }
 
+void *klp_shadow_get(void *obj, unsigned long num);
+void *klp_shadow_attach(void *obj, unsigned long num, void *new_data,
+			size_t new_size, gfp_t gfp_flags);
+void *klp_shadow_get_or_attach(void *obj, unsigned long num, void *new_data,
+			       size_t new_size, gfp_t gfp_flags);
+void klp_shadow_detach(void *obj, unsigned long num);
+void klp_shadow_detach_all(unsigned long num);
+
 #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..d37a61c57e72
--- /dev/null
+++ b/kernel/livepatch/shadow.c
@@ -0,0 +1,257 @@
+/*
+ * 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 simply provides a relationship between an
+ * <obj, num> pair and a pointer value.  It is the responsibility of the
+ * caller to provide any mutual exclusion required of the shadow data.
+ *
+ * Once klp_shadow_attach() adds a shadow variable to the
+ * klp_shadow_hash, it is considered live and klp_shadow_get() may
+ * return the shadow variable's new_data pointer.  Therefore,
+ * initialization of shadow new_data should be completed before
+ * attaching the shadow variable.
+ *
+ * Alternatively, the klp_shadow_get_or_attach() call may be used to
+ * safely fetch any existing <obj, num> match, or create a new
+ * <obj, num> shadow variable if none exists.
+ *
+ * If the API is called under a special context (like spinlocks), set
+ * the GFP flags passed to klp_shadow_attach() accordingly.
+ *
+ * The klp_shadow_hash is an RCU-enabled hashtable and should be 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 original data
+ * @num:	numerical description of new data
+ * @new_data:	new data area
+ */
+struct klp_shadow {
+	struct hlist_node node;
+	struct rcu_head rcu_head;
+	void *obj;
+	unsigned long num;
+	char new_data[];
+};
+
+/**
+ * shadow_match() - verify a shadow variable matches given <obj, num>
+ * @shadow:	shadow variable to match
+ * @obj:	pointer to original data
+ * @num:	numerical description of new data
+ *
+ * Return: true if the shadow variable matches.
+ */
+static inline bool shadow_match(struct klp_shadow *shadow, void *obj,
+				unsigned long num)
+{
+	return shadow->obj == obj && shadow->num == num;
+}
+
+/**
+ * klp_shadow_get() - retrieve a shadow variable new_data pointer
+ * @obj:	pointer to original data
+ * @num:	numerical description of new data
+ *
+ * Return: a pointer to shadow variable new data
+ */
+void *klp_shadow_get(void *obj, unsigned long num)
+{
+	struct klp_shadow *shadow;
+
+	rcu_read_lock();
+
+	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
+				   (unsigned long)obj) {
+
+		if (shadow_match(shadow, obj, num)) {
+			rcu_read_unlock();
+			return shadow->new_data;
+		}
+	}
+
+	rcu_read_unlock();
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(klp_shadow_get);
+
+/**
+ * _klp_shadow_attach() - allocate and add a new shadow variable
+ * @obj:	pointer to original data
+ * @num:	numerical description of new data
+ * @new_data:	pointer to new data
+ * @new_size:	size of new data
+ * @gfp_flags:	GFP mask for allocation
+ * @lock:	take klp_shadow_lock during klp_shadow_hash operations
+ *
+ * Note: allocates @new_size space for shadow variable data and copies
+ * @new_size bytes from @new_data into the shadow varaible's own @new_data
+ * space.  If @new_data is NULL, @new_size is still allocated, but no
+ * copy is performed.
+ *
+ * Return: the shadow variable new_data element, NULL on failure.
+ */
+static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data,
+				size_t new_size, gfp_t gfp_flags,
+				bool lock)
+{
+	struct klp_shadow *shadow;
+	unsigned long flags;
+
+	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
+	if (!shadow)
+		return NULL;
+
+	shadow->obj = obj;
+	shadow->num = num;
+	if (new_data)
+		memcpy(shadow->new_data, new_data, new_size);
+
+	if (lock)
+		spin_lock_irqsave(&klp_shadow_lock, flags);
+	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
+	if (lock)
+		spin_unlock_irqrestore(&klp_shadow_lock, flags);
+
+	return shadow->new_data;
+}
+
+/**
+ * klp_shadow_attach() - allocate and add a new shadow variable
+ * @obj:	pointer to original data
+ * @num:	numerical description of new num
+ * @new_data:	pointer to new data
+ * @new_size:	size of new data
+ * @gfp_flags:	GFP mask for allocation
+ *
+ * Return: the shadow variable new_data element, NULL on failure.
+ */
+void *klp_shadow_attach(void *obj, unsigned long num, void *new_data,
+			size_t new_size, gfp_t gfp_flags)
+{
+	return _klp_shadow_attach(obj, num, new_data, new_size,
+				  gfp_flags, true);
+}
+EXPORT_SYMBOL_GPL(klp_shadow_attach);
+
+/**
+ * klp_shadow_get_or_attach() - get existing or attach a new shadow variable
+ * @obj:	pointer to original data
+ * @num:	numerical description of new data
+ * @new_data:	pointer to new data
+ * @new_size:   size of new data
+ * @gfp_flags:	GFP mask used to allocate shadow variable metadata
+ *
+ * Note: if memory allocation is necessary, it will do so under a spinlock,
+ * so @gfp_flags should include GFP_NOWAIT, or GFP_ATOMIC, etc.
+ *
+ * Return: the shadow variable new_data element, NULL on failure.
+ */
+void *klp_shadow_get_or_attach(void *obj, unsigned long num, void *new_data,
+			       size_t new_size, gfp_t gfp_flags)
+{
+	void *nd;
+	unsigned long flags;
+
+	nd = klp_shadow_get(obj, num);
+
+	if (!nd) {
+		spin_lock_irqsave(&klp_shadow_lock, flags);
+		nd = klp_shadow_get(obj, num);
+		if (!nd)
+			nd = _klp_shadow_attach(obj, num, new_data, new_size,
+						gfp_flags, false);
+		spin_unlock_irqrestore(&klp_shadow_lock, flags);
+	}
+
+	return nd;
+
+}
+EXPORT_SYMBOL_GPL(klp_shadow_get_or_attach);
+
+/**
+ * klp_shadow_detach() - detach and free a <obj, num> shadow variable
+ * @obj:	pointer to original data
+ * @num:	numerical description of new data
+ */
+void klp_shadow_detach(void *obj, unsigned long num)
+{
+	struct klp_shadow *shadow;
+	unsigned long flags;
+
+	spin_lock_irqsave(&klp_shadow_lock, flags);
+
+	/* Delete all <obj, num> from hash */
+	hash_for_each_possible(klp_shadow_hash, shadow, node,
+			       (unsigned long)obj) {
+
+		if (shadow_match(shadow, obj, num)) {
+			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 <*, num> shadow variables
+ * @num:	numerical description of new data
+ */
+void klp_shadow_detach_all(unsigned long num)
+{
+	struct klp_shadow *shadow;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&klp_shadow_lock, flags);
+
+	/* Delete all <*, num> from hash */
+	hash_for_each(klp_shadow_hash, i, shadow, node) {
+		if (shadow_match(shadow, shadow->obj, num)) {
+			hash_del_rcu(&shadow->node);
+			kfree_rcu(shadow, rcu_head);
+		}
+	}
+
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+}
+EXPORT_SYMBOL_GPL(klp_shadow_detach_all);
-- 
1.8.3.1

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5979 bytes --]

Hi Joe,

[auto build test WARNING on jikos-livepatching/for-next]
[also build test WARNING on v4.12-rc7 next-20170630]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Joe-Lawrence/livepatch-introduce-shadow-variable-API/20170630-061942
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-next
config: s390-performance_defconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   In file included from include/linux/irqflags.h:15:0,
                    from arch/s390/include/asm/processor.h:35,
                    from arch/s390/include/asm/thread_info.h:24,
                    from include/linux/thread_info.h:37,
                    from arch/s390/include/asm/preempt.h:5,
                    from include/linux/preempt.h:80,
                    from include/linux/spinlock.h:50,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:10,
                    from include/linux/hashtable.h:13,
                    from kernel/livepatch/shadow.c:49:
   kernel/livepatch/shadow.c: In function '_klp_shadow_attach':
>> arch/s390/include/asm/irqflags.h:63:12: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (flags & ARCH_IRQ_ENABLED)
               ^
   kernel/livepatch/shadow.c:135:16: note: 'flags' was declared here
     unsigned long flags;
                   ^~~~~
--
   In file included from include/linux/irqflags.h:15:0,
                    from arch/s390/include/asm/processor.h:35,
                    from arch/s390/include/asm/thread_info.h:24,
                    from include/linux/thread_info.h:37,
                    from arch/s390/include/asm/preempt.h:5,
                    from include/linux/preempt.h:80,
                    from include/linux/spinlock.h:50,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:10,
                    from include/linux/hashtable.h:13,
                    from kernel//livepatch/shadow.c:49:
   kernel//livepatch/shadow.c: In function '_klp_shadow_attach':
>> arch/s390/include/asm/irqflags.h:63:12: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (flags & ARCH_IRQ_ENABLED)
               ^
   kernel//livepatch/shadow.c:135:16: note: 'flags' was declared here
     unsigned long flags;
                   ^~~~~

vim +/flags +63 arch/s390/include/asm/irqflags.h

94c12cc7d include/asm-s390/irqflags.h      Martin Schwidefsky    2006-09-28  47  }
94c12cc7d include/asm-s390/irqflags.h      Martin Schwidefsky    2006-09-28  48  
f433c4aec arch/s390/include/asm/irqflags.h Steven Rostedt        2011-07-24  49  static inline notrace void arch_local_irq_disable(void)
df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  50  {
df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  51  	arch_local_irq_save();
df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  52  }
1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  53  
f433c4aec arch/s390/include/asm/irqflags.h Steven Rostedt        2011-07-24  54  static inline notrace void arch_local_irq_enable(void)
94c12cc7d include/asm-s390/irqflags.h      Martin Schwidefsky    2006-09-28  55  {
df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  56  	__arch_local_irq_stosm(0x03);
94c12cc7d include/asm-s390/irqflags.h      Martin Schwidefsky    2006-09-28  57  }
1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  58  
204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11  59  /* This only restores external and I/O interrupt state */
f433c4aec arch/s390/include/asm/irqflags.h Steven Rostedt        2011-07-24  60  static inline notrace void arch_local_irq_restore(unsigned long flags)
df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  61  {
204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11  62  	/* only disabled->disabled and disabled->enabled is valid */
204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11 @63  	if (flags & ARCH_IRQ_ENABLED)
204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11  64  		arch_local_irq_enable();
df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  65  }
df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  66  
f433c4aec arch/s390/include/asm/irqflags.h Steven Rostedt        2011-07-24  67  static inline notrace bool arch_irqs_disabled_flags(unsigned long flags)
1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  68  {
204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11  69  	return !(flags & ARCH_IRQ_ENABLED);
1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  70  }
1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  71  

:::::: The code at line 63 was first introduced by commit
:::::: 204ee2c5643199a25181ec04ea645d00709c2a5a s390/irqflags: optimize irq restore

:::::: TO: Christian Borntraeger <borntraeger@de.ibm.com>
:::::: CC: Martin Schwidefsky <schwidefsky@de.ibm.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 16939 bytes --]

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Joe Lawrence]

On Fri, Jun 30, 2017 at 09:49:46PM +0800, kbuild test robot wrote:
> Date: Fri, 30 Jun 2017 21:49:46 +0800
> From: kbuild test robot <lkp@intel.com>
> To: Joe Lawrence <joe.lawrence@redhat.com>
> Cc: kbuild-all@01.org, live-patching@vger.kernel.org,
>  linux-kernel@vger.kernel.org, Josh Poimboeuf <jpoimboe@redhat.com>,
>  Jessica Yu <jeyu@redhat.com>, Jiri Kosina <jikos@kernel.org>, Miroslav
>  Benes <mbenes@suse.cz>, Petr Mladek <pmladek@suse.com>
> Subject: Re: [PATCH v2 1/2] livepatch: introduce shadow variable API
> User-Agent: Mutt/1.5.23 (2014-03-12)
> 
> Hi Joe,
> 
> [auto build test WARNING on jikos-livepatching/for-next]
> [also build test WARNING on v4.12-rc7 next-20170630]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Joe-Lawrence/livepatch-introduce-shadow-variable-API/20170630-061942
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git for-next
> config: s390-performance_defconfig (attached as .config)
> compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=s390 
> 
> Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
> 
> All warnings (new ones prefixed by >>):
> 
>    In file included from include/linux/irqflags.h:15:0,
>                     from arch/s390/include/asm/processor.h:35,
>                     from arch/s390/include/asm/thread_info.h:24,
>                     from include/linux/thread_info.h:37,
>                     from arch/s390/include/asm/preempt.h:5,
>                     from include/linux/preempt.h:80,
>                     from include/linux/spinlock.h:50,
>                     from include/linux/rcupdate.h:38,
>                     from include/linux/rculist.h:10,
>                     from include/linux/hashtable.h:13,
>                     from kernel/livepatch/shadow.c:49:
>    kernel/livepatch/shadow.c: In function '_klp_shadow_attach':
> >> arch/s390/include/asm/irqflags.h:63:12: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
>      if (flags & ARCH_IRQ_ENABLED)
>                ^
>    kernel/livepatch/shadow.c:135:16: note: 'flags' was declared here
>      unsigned long flags;
>                    ^~~~~
> --
>    In file included from include/linux/irqflags.h:15:0,
>                     from arch/s390/include/asm/processor.h:35,
>                     from arch/s390/include/asm/thread_info.h:24,
>                     from include/linux/thread_info.h:37,
>                     from arch/s390/include/asm/preempt.h:5,
>                     from include/linux/preempt.h:80,
>                     from include/linux/spinlock.h:50,
>                     from include/linux/rcupdate.h:38,
>                     from include/linux/rculist.h:10,
>                     from include/linux/hashtable.h:13,
>                     from kernel//livepatch/shadow.c:49:
>    kernel//livepatch/shadow.c: In function '_klp_shadow_attach':
> >> arch/s390/include/asm/irqflags.h:63:12: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
>      if (flags & ARCH_IRQ_ENABLED)
>                ^
>    kernel//livepatch/shadow.c:135:16: note: 'flags' was declared here
>      unsigned long flags;
>                    ^~~~~
> 
> vim +/flags +63 arch/s390/include/asm/irqflags.h
> 
> 94c12cc7d include/asm-s390/irqflags.h      Martin Schwidefsky    2006-09-28  47  }
> 94c12cc7d include/asm-s390/irqflags.h      Martin Schwidefsky    2006-09-28  48  
> f433c4aec arch/s390/include/asm/irqflags.h Steven Rostedt        2011-07-24  49  static inline notrace void arch_local_irq_disable(void)
> df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  50  {
> df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  51  	arch_local_irq_save();
> df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  52  }
> 1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  53  
> f433c4aec arch/s390/include/asm/irqflags.h Steven Rostedt        2011-07-24  54  static inline notrace void arch_local_irq_enable(void)
> 94c12cc7d include/asm-s390/irqflags.h      Martin Schwidefsky    2006-09-28  55  {
> df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  56  	__arch_local_irq_stosm(0x03);
> 94c12cc7d include/asm-s390/irqflags.h      Martin Schwidefsky    2006-09-28  57  }
> 1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  58  
> 204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11  59  /* This only restores external and I/O interrupt state */
> f433c4aec arch/s390/include/asm/irqflags.h Steven Rostedt        2011-07-24  60  static inline notrace void arch_local_irq_restore(unsigned long flags)
> df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  61  {
> 204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11  62  	/* only disabled->disabled and disabled->enabled is valid */
> 204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11 @63  	if (flags & ARCH_IRQ_ENABLED)
> 204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11  64  		arch_local_irq_enable();
> df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  65  }
> df9ee2927 arch/s390/include/asm/irqflags.h David Howells         2010-10-07  66  
> f433c4aec arch/s390/include/asm/irqflags.h Steven Rostedt        2011-07-24  67  static inline notrace bool arch_irqs_disabled_flags(unsigned long flags)
> 1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  68  {
> 204ee2c56 arch/s390/include/asm/irqflags.h Christian Borntraeger 2016-01-11  69  	return !(flags & ARCH_IRQ_ENABLED);
> 1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  70  }
> 1f194a4c3 include/asm-s390/irqflags.h      Heiko Carstens        2006-07-03  71  
> 
> :::::: The code at line 63 was first introduced by commit
> :::::: 204ee2c5643199a25181ec04ea645d00709c2a5a s390/irqflags: optimize irq restore
> 
> :::::: TO: Christian Borntraeger <borntraeger@de.ibm.com>
> :::::: CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Presumably the 0-day built bot doesn't like this construct:

	static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data,
					size_t new_size, gfp_t gfp_flags,
					bool lock)
	{
		unsigned long flags;
		...
		if (lock)
			spin_lock_irqsave(&klp_shadow_lock, flags);
		hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
		if (lock)
			spin_unlock_irqrestore(&klp_shadow_lock, flags);

Perhaps explicitly initializing flags to 0, or combining the conditional
will appease it?

-- Joe

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Josh Poimboeuf]

On Wed, Jun 28, 2017 at 11:37:26AM -0400, Joe Lawrence wrote:
> Add exported API for livepatch modules:
> 
>   klp_shadow_get()
>   klp_shadow_attach()
>   klp_shadow_get_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.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

The API, docs, and code all look really good.

A few comments below about some of the variable naming and descriptions.

> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
> new file mode 100644
> index 000000000000..7f28982e6b1c
> --- /dev/null
> +++ b/Documentation/livepatch/shadow-vars.txt
> @@ -0,0 +1,156 @@
> +Shadow Variables
> +================
> +
> +Shadow variables are a simple way for livepatch modules to associate new
> +"shadow" data to existing data structures.  Original data structures
> +(both their definition and storage) are left unmodified and "new" data
> +is allocated separately.  A shadow variable hashtable associates a
> +string key, enumeration pair with a pointer to the new data.

s/string key/numeric key/

> +Brief API summary
> +-----------------
> +
> +See the full API usage docbook notes in the livepatch/shadow.c
> +implementation.
> +
> +An in-kernel hashtable references all of the shadow variables.  These
> +references are stored/retrieved through a <obj, num> key pair.

"num" is rather vague, how about "key"?

(And note, this and some of the other comments also apply to the code as
well)

> +* The klp_shadow variable data structure encapsulates both tracking
> +meta-data and shadow-data:
> +  - meta-data
> +    - obj - pointer to original data

Instead of "original data", how about calling it the "parent object"?
That describes it better to me at least.  "Original data" sounds like
some of the data might be replaced.

> +    - num - numerical description of new data

"numerical description of new data" sounds a little confusing, how about
"unique identifier for new data"?

I'm also not sure about the phrase "new data".  Maybe something like
"new data field" would be more descriptive?  Or just "new field"?  I
view it kind of like adding a field to a struct.  Not a big deal either
way.

> +void *klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> +			size_t new_size, gfp_t gfp_flags);

It could be just me, but the "new_" prefixes threw me off a little bit.
The new is implied anyway.  How about just "data" and "size"?

And the same comment for the klp_shadow struct.

-- 
Josh

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Miroslav Benes]

On Thu, 13 Jul 2017, Josh Poimboeuf wrote:

> On Wed, Jun 28, 2017 at 11:37:26AM -0400, Joe Lawrence wrote:
> 
> > +Brief API summary
> > +-----------------
> > +
> > +See the full API usage docbook notes in the livepatch/shadow.c
> > +implementation.
> > +
> > +An in-kernel hashtable references all of the shadow variables.  These
> > +references are stored/retrieved through a <obj, num> key pair.
> 
> "num" is rather vague, how about "key"?
> 
> (And note, this and some of the other comments also apply to the code as
> well)
> 
> > +* The klp_shadow variable data structure encapsulates both tracking
> > +meta-data and shadow-data:
> > +  - meta-data
> > +    - obj - pointer to original data
> 
> Instead of "original data", how about calling it the "parent object"?
> That describes it better to me at least.  "Original data" sounds like
> some of the data might be replaced.

I agree that "original data" does not sound right. However, we use "parent 
object" for vmlinux or a module in our code. But I don't have a better 
name and "parent object" sounds good.
 
> > +    - num - numerical description of new data
> 
> "numerical description of new data" sounds a little confusing, how about
> "unique identifier for new data"?
> 
> I'm also not sure about the phrase "new data".  Maybe something like
> "new data field" would be more descriptive?  Or just "new field"?  I
> view it kind of like adding a field to a struct.  Not a big deal either
> way.
>
> > +void *klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> > +			size_t new_size, gfp_t gfp_flags);
> 
> It could be just me, but the "new_" prefixes threw me off a little bit.
> The new is implied anyway.  How about just "data" and "size"?
> 
> And the same comment for the klp_shadow struct.

I agree with Josh on all of this.

Miroslav

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Petr Mladek]

On Mon 2017-07-17 17:35:38, Miroslav Benes wrote:
> On Thu, 13 Jul 2017, Josh Poimboeuf wrote:
> 
> > On Wed, Jun 28, 2017 at 11:37:26AM -0400, Joe Lawrence wrote:
> > 
> > > +Brief API summary
> > > +-----------------
> > > +
> > > +See the full API usage docbook notes in the livepatch/shadow.c
> > > +implementation.
> > > +
> > > +An in-kernel hashtable references all of the shadow variables.  These
> > > +references are stored/retrieved through a <obj, num> key pair.
> > 
> > "num" is rather vague, how about "key"?

As Mirek said in the previous version. "obj" is the key for the hash
table.

Anyway, I agree that "num" is vague and even confusing. I would
suggest to use "id".

> > (And note, this and some of the other comments also apply to the code as
> > well)
> > 
> > > +* The klp_shadow variable data structure encapsulates both tracking
> > > +meta-data and shadow-data:
> > > +  - meta-data
> > > +    - obj - pointer to original data
> > 
> > Instead of "original data", how about calling it the "parent object"?
> > That describes it better to me at least.  "Original data" sounds like
> > some of the data might be replaced.
> 
> I agree that "original data" does not sound right. However, we use "parent 
> object" for vmlinux or a module in our code. But I don't have a better 
> name and "parent object" sounds good.

What about "primary object"? I took inspiration from
https://en.wikipedia.org/wiki/Shadow_table

> > > +    - num - numerical description of new data
> > 
> > "numerical description of new data" sounds a little confusing, how about
> > "unique identifier for new data"?

Here we come to the "id" ;-)

I wonder if each patch should register its own IDs and the size of the
data. The API could shout when anyone wants to use a not yet
registered ID or when the same ID with another size is being
registered. It might increase security. But I am not sure
if it is worth it.


> > I'm also not sure about the phrase "new data".  Maybe something like
> > "new data field" would be more descriptive?  Or just "new field"?  I
> > view it kind of like adding a field to a struct.  Not a big deal either
> > way.
> >
> > > +void *klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> > > +			size_t new_size, gfp_t gfp_flags);
> > 
> > It could be just me, but the "new_" prefixes threw me off a little bit.
> > The new is implied anyway.  How about just "data" and "size"?
> > 
> > And the same comment for the klp_shadow struct.
> 
> I agree with Josh on all of this.

You persuaded me that "data" and "size" make sense after all ;-)
new_obj would mean that we replace/copy the entire object.

Best Regards,
Petr

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Joe Lawrence]

On Tue, Jul 18, 2017 at 03:00:47PM +0200, Petr Mladek wrote:
> Date:   Tue, 18 Jul 2017 15:00:47 +0200
> From: Petr Mladek <pmladek@suse.com>
> To: Miroslav Benes <mbenes@suse.cz>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>, Joe Lawrence
>  <joe.lawrence@redhat.com>, live-patching@vger.kernel.org,
>  linux-kernel@vger.kernel.org, Jessica Yu <jeyu@redhat.com>, Jiri Kosina
>  <jikos@kernel.org>
> Subject: Re: [PATCH v2 1/2] livepatch: introduce shadow variable API
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Mon 2017-07-17 17:35:38, Miroslav Benes wrote:
> > On Thu, 13 Jul 2017, Josh Poimboeuf wrote:
> > 
> > > On Wed, Jun 28, 2017 at 11:37:26AM -0400, Joe Lawrence wrote:
> > > 
> > > > +Brief API summary
> > > > +-----------------
> > > > +
> > > > +See the full API usage docbook notes in the livepatch/shadow.c
> > > > +implementation.
> > > > +
> > > > +An in-kernel hashtable references all of the shadow variables.  These
> > > > +references are stored/retrieved through a <obj, num> key pair.
> > > 
> > > "num" is rather vague, how about "key"?
> 
> As Mirek said in the previous version. "obj" is the key for the hash
> table.
> 
> Anyway, I agree that "num" is vague and even confusing. I would
> suggest to use "id".
> 
> > > (And note, this and some of the other comments also apply to the code as
> > > well)
> > > 
> > > > +* The klp_shadow variable data structure encapsulates both tracking
> > > > +meta-data and shadow-data:
> > > > +  - meta-data
> > > > +    - obj - pointer to original data
> > > 
> > > Instead of "original data", how about calling it the "parent object"?
> > > That describes it better to me at least.  "Original data" sounds like
> > > some of the data might be replaced.
> > 
> > I agree that "original data" does not sound right. However, we use "parent 
> > object" for vmlinux or a module in our code. But I don't have a better 
> > name and "parent object" sounds good.
> 
> What about "primary object"? I took inspiration from
> https://en.wikipedia.org/wiki/Shadow_table
> 
> > > > +    - num - numerical description of new data
> > > 
> > > "numerical description of new data" sounds a little confusing, how about
> > > "unique identifier for new data"?
> 
> Here we come to the "id" ;-)
> 
> I wonder if each patch should register its own IDs and the size of the
> data. The API could shout when anyone wants to use a not yet
> registered ID or when the same ID with another size is being
> registered. It might increase security. But I am not sure
> if it is worth it.
> 
> 
> > > I'm also not sure about the phrase "new data".  Maybe something like
> > > "new data field" would be more descriptive?  Or just "new field"?  I
> > > view it kind of like adding a field to a struct.  Not a big deal either
> > > way.
> > >
> > > > +void *klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> > > > +			size_t new_size, gfp_t gfp_flags);
> > > 
> > > It could be just me, but the "new_" prefixes threw me off a little bit.
> > > The new is implied anyway.  How about just "data" and "size"?
> > > 
> > > And the same comment for the klp_shadow struct.
> > 
> > I agree with Josh on all of this.
> 
> You persuaded me that "data" and "size" make sense after all ;-)
> new_obj would mean that we replace/copy the entire object.

Who knew naming things was so difficult :)

There's been a bunch of feedback on terminology, so I'll just issue a
collective reply to Petr's last msg on the topic.  These were my
thoughts on naming clarification:

  v1,v2                                     v3
  --------------------------------------------------------------
  obj, original data                        obj, parent object
  num, numerical description of new data    id, data identifier
  new_data                                  data
  new_size                                  data_size

Miroslav also suggested additional text explaining the id / data
identifier field.  How about something like this:

---

================
Shadow Variables
================

...

A global, in-kernel hashtable associates parent pointers and a numeric
identifier with shadow variable data.  Specifically, the parent pointer
serves as the hashtable key, while the numeric id further filters
hashtable queries.  The numeric identifier is a simple enumeration that
may be used to describe shadow variable versions (for stacking
livepatches), class or type (for multiple shadow variables per parent),
etc.  Multiple shadow variables may attach to the same parent object,
but their numeric identifier distinguises between them.

---

Thanks for the review suggestions, let me know if I missed any other
terms flagged in one of the other legs of the discussion thread.

-- Joe

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Petr Mladek]

On Tue 2017-07-18 15:36:27, Joe Lawrence wrote:
> Who knew naming things was so difficult :)
> 
> There's been a bunch of feedback on terminology, so I'll just issue a
> collective reply to Petr's last msg on the topic.  These were my
> thoughts on naming clarification:
> 
>   v1,v2                                     v3
>   --------------------------------------------------------------
>   obj, original data                        obj, parent object
>   num, numerical description of new data    id, data identifier
>   new_data                                  data
>   new_size                                  data_size

IMHO, "size" might be enough in the context when it is used.

> 
> Miroslav also suggested additional text explaining the id / data
> identifier field.  How about something like this:
> 
> ---
> 
> ================
> Shadow Variables
> ================
> 
> ...
> 
> A global, in-kernel hashtable associates parent pointers and a numeric
> identifier with shadow variable data.

I would slightly reformulate the above sentece:

A global, in-kernel hashtable associates pointers to parent objects
and a numeric identifier of the shadow data.

> Specifically, the parent pointer
> serves as the hashtable key, while the numeric id further filters
> hashtable queries.  The numeric identifier is a simple enumeration that
> may be used to describe shadow variable versions (for stacking
> livepatches), class or type (for multiple shadow variables per parent),
> etc.  Multiple shadow variables may attach to the same parent object,
> but their numeric identifier distinguises between them.

Sounds good to me.

Best Regards,
Petr

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Miroslav Benes]

On Wed, 19 Jul 2017, Petr Mladek wrote:

> On Tue 2017-07-18 15:36:27, Joe Lawrence wrote:
> > Who knew naming things was so difficult :)
> > 
> > There's been a bunch of feedback on terminology, so I'll just issue a
> > collective reply to Petr's last msg on the topic.  These were my
> > thoughts on naming clarification:
> > 
> >   v1,v2                                     v3
> >   --------------------------------------------------------------
> >   obj, original data                        obj, parent object
> >   num, numerical description of new data    id, data identifier
> >   new_data                                  data
> >   new_size                                  data_size
> 
> IMHO, "size" might be enough in the context when it is used.

I agree.

> > 
> > Miroslav also suggested additional text explaining the id / data
> > identifier field.  How about something like this:
> > 
> > ---
> > 
> > ================
> > Shadow Variables
> > ================
> > 
> > ...
> > 
> > A global, in-kernel hashtable associates parent pointers and a numeric
> > identifier with shadow variable data.
> 
> I would slightly reformulate the above sentece:
> 
> A global, in-kernel hashtable associates pointers to parent objects
> and a numeric identifier of the shadow data.
> 
> > Specifically, the parent pointer
> > serves as the hashtable key, while the numeric id further filters
> > hashtable queries.  The numeric identifier is a simple enumeration that
> > may be used to describe shadow variable versions (for stacking
> > livepatches), class or type (for multiple shadow variables per parent),
> > etc.  Multiple shadow variables may attach to the same parent object,
> > but their numeric identifier distinguises between them.

s/distinguises/distinguishes/
 
> Sounds good to me.

Yes, thanks for the paragraph. It sounds good combined with Petr's 
proposal.

Miroslav

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Miroslav Benes]

On Wed, 28 Jun 2017, Joe Lawrence wrote:

> +Brief API summary
> +-----------------
> +
> +See the full API usage docbook notes in the livepatch/shadow.c
> +implementation.
> +
> +An in-kernel hashtable references all of the shadow variables.  These
> +references are stored/retrieved through a <obj, num> key pair.
> +
> +* The klp_shadow variable data structure encapsulates both tracking
> +meta-data and shadow-data:
> +  - meta-data
> +    - obj - pointer to original data
> +    - num - numerical description of new data
> +  - new_data[] - storage for shadow data
> +
> +* klp_shadow_attach() - allocate and add a new shadow variable:
> +  - allocate a new shadow variable
> +  - push a <obj, num> key pair into hashtable
> +
> +* klp_shadow_get() - retrieve a shadow variable new_data pointer
> +  - search hashtable for <obj, num> key pair
> +
> +* klp_shadow_get_or_attach() - get existing or attach a new shadow variable
> +  - search hashtable for <obj, num> key pair
> +  - if not found, call klp_shadow_attach()
> +
> +* klp_shadow_detach() - detach and free a <obj, num> shadow variable
> +  - find and remove any <obj, num> references from hashtable
> +    - if found, release shadow variable
> +
> +* klp_shadow_detach() - detach and free all <*, num> shadow variables
> +  - find and remove any <*, num> references from hashtable
> +    - if found, release shadow variable

I think that the second one should be klp_shadow_detach_all(), shouldn't 
it?

[...]

> +static DEFINE_HASHTABLE(klp_shadow_hash, 12);

Is there a reason, why you pick 12? I'm just curious.

> +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 original data
> + * @num:	numerical description of new data

Josh proposed better description. Could we also have a note somewhere in 
the documentation what this member is practically for? I mean versioning 
and ability to attach new members to a data structure if live patches are 
stacked.

> + * @new_data:	new data area
> + */
> +struct klp_shadow {
> +	struct hlist_node node;
> +	struct rcu_head rcu_head;
> +	void *obj;
> +	unsigned long num;
> +	char new_data[];
> +};

What is the reason to change 'void *new_data' to 'char new_data[]'? I 
assume it is related to API changes below...

[...]

> +/**
> + * _klp_shadow_attach() - allocate and add a new shadow variable
> + * @obj:	pointer to original data
> + * @num:	numerical description of new data
> + * @new_data:	pointer to new data
> + * @new_size:	size of new data
> + * @gfp_flags:	GFP mask for allocation
> + * @lock:	take klp_shadow_lock during klp_shadow_hash operations

I am not sure about lock argument. Do we need it? Common practice is to 
have function foo() which takes a lock, and function __foo() which does 
not.

In klp_shadow_get_or_attach(), you use it as I'd expect. You take the 
spinlock, call this function and release the spinlock. Is it possible 
to do the same in klp_shadow_attach() and have __klp_shadow_attach() 
without lock argument?

> + *
> + * Note: allocates @new_size space for shadow variable data and copies
> + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> + * space.  If @new_data is NULL, @new_size is still allocated, but no
> + * copy is performed.

I must say I'm not entirely happy with this. I don't know if this is what 
Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
memcpy instead of a simple assignment in v1 seems worse. 

I'll take another look tomorrow and will think about it.

Miroslav

> + * Return: the shadow variable new_data element, NULL on failure.
> + */
> +static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> +				size_t new_size, gfp_t gfp_flags,
> +				bool lock)
> +{
> +	struct klp_shadow *shadow;
> +	unsigned long flags;
> +
> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> +	if (!shadow)
> +		return NULL;
> +
> +	shadow->obj = obj;
> +	shadow->num = num;
> +	if (new_data)
> +		memcpy(shadow->new_data, new_data, new_size);
> +
> +	if (lock)
> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> +	if (lock)
> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return shadow->new_data;
> +}
> +
> +/**
> + * klp_shadow_attach() - allocate and add a new shadow variable
> + * @obj:	pointer to original data
> + * @num:	numerical description of new num
> + * @new_data:	pointer to new data
> + * @new_size:	size of new data
> + * @gfp_flags:	GFP mask for allocation
> + *
> + * Return: the shadow variable new_data element, NULL on failure.
> + */
> +void *klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> +			size_t new_size, gfp_t gfp_flags)
> +{
> +	return _klp_shadow_attach(obj, num, new_data, new_size,
> +				  gfp_flags, true);
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> +
> +/**
> + * klp_shadow_get_or_attach() - get existing or attach a new shadow variable
> + * @obj:	pointer to original data
> + * @num:	numerical description of new data
> + * @new_data:	pointer to new data
> + * @new_size:   size of new data
> + * @gfp_flags:	GFP mask used to allocate shadow variable metadata
> + *
> + * Note: if memory allocation is necessary, it will do so under a spinlock,
> + * so @gfp_flags should include GFP_NOWAIT, or GFP_ATOMIC, etc.
> + *
> + * Return: the shadow variable new_data element, NULL on failure.
> + */
> +void *klp_shadow_get_or_attach(void *obj, unsigned long num, void *new_data,
> +			       size_t new_size, gfp_t gfp_flags)
> +{
> +	void *nd;
> +	unsigned long flags;
> +
> +	nd = klp_shadow_get(obj, num);
> +
> +	if (!nd) {
> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> +		nd = klp_shadow_get(obj, num);
> +		if (!nd)
> +			nd = _klp_shadow_attach(obj, num, new_data, new_size,
> +						gfp_flags, false);
> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +	}
> +
> +	return nd;
> +
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_get_or_attach);

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Joe Lawrence]

On Mon, Jul 17, 2017 at 05:29:41PM +0200, Miroslav Benes wrote:
>
> On Wed, 28 Jun 2017, Joe Lawrence wrote:
> 
> > +Brief API summary
> > +-----------------
> > + [ ... snip ...]
> > +* klp_shadow_detach() - detach and free all <*, num> shadow variables
> > +  - find and remove any <*, num> references from hashtable
> > +    - if found, release shadow variable
> 
> I think that the second one should be klp_shadow_detach_all(), shouldn't 
> it?

Good catch, I'll fixup in v3.

> > +static DEFINE_HASHTABLE(klp_shadow_hash, 12);
> 
> Is there a reason, why you pick 12? I'm just curious.

The hashtable bit-size was inherited from the kpatch implementation.
Perhaps Josh knows why this value was picked?

Aside: we could have per-livepatch hashtables if that was desired, this
value could be then adjusted accordingly.  We haven't needed them for
kpatch, so I didn't see good reason to complicate things.

> > +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 original data
> > + * @num:	numerical description of new data
> 
> Josh proposed better description. Could we also have a note somewhere in 
> the documentation what this member is practically for? I mean versioning 
> and ability to attach new members to a data structure if live patches are 
> stacked.

That's a good idea and I posted a sample doc-blurb in my other reply to
Petr about terminology.

> > + * @new_data:	new data area
> > + */
> > +struct klp_shadow {
> > +	struct hlist_node node;
> > +	struct rcu_head rcu_head;
> > +	void *obj;
> > +	unsigned long num;
> > +	char new_data[];
> > +};
> 
> What is the reason to change 'void *new_data' to 'char new_data[]'? I 
> assume it is related to API changes below...
> 
> [...]
> 
> > +/**
> > + * _klp_shadow_attach() - allocate and add a new shadow variable
> > + * @obj:	pointer to original data
> > + * @num:	numerical description of new data
> > + * @new_data:	pointer to new data
> > + * @new_size:	size of new data
> > + * @gfp_flags:	GFP mask for allocation
> > + * @lock:	take klp_shadow_lock during klp_shadow_hash operations
> 
> I am not sure about lock argument. Do we need it? Common practice is to 
> have function foo() which takes a lock, and function __foo() which does 
> not.
> 
> In klp_shadow_get_or_attach(), you use it as I'd expect. You take the 
> spinlock, call this function and release the spinlock. Is it possible 
> to do the same in klp_shadow_attach() and have __klp_shadow_attach() 
> without lock argument?

Yes, this would be possible, though it would restrict
klp_shadow_attach() from accepting gfp_flags that might allow for
sleeping.  More on that below ...
 
> > + *
> > + * Note: allocates @new_size space for shadow variable data and copies
> > + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> > + * space.  If @new_data is NULL, @new_size is still allocated, but no
> > + * copy is performed.
> 
> I must say I'm not entirely happy with this. I don't know if this is what 
> Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> memcpy instead of a simple assignment in v1 seems worse. 

This change was a bit of a experiment on my part in reaction to
adding klp_shadow_get_or_attach().

I like the simplicity of v1's pointer assignment -- in fact, moving all
allocation responsiblity (klp_shadow meta-data and data[] area) out to
the caller is doable, though implementing klp_shadow_get_or_attach() and
and klp_shadow_detach_all() complicates matters, for example, adding an
alloc/release callback.  I originally attempted this for v2, but turned
back when the API and implementation grew complicated.  If the memcpy
and gfp_flag restrictions are too ugly, I can try revisting that
approach.  Ideas welcome :)

Regards,

-- Joe

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Josh Poimboeuf]

On Tue, Jul 18, 2017 at 04:21:07PM -0400, Joe Lawrence wrote:
> On Mon, Jul 17, 2017 at 05:29:41PM +0200, Miroslav Benes wrote:
> >
> > On Wed, 28 Jun 2017, Joe Lawrence wrote:
> > 
> > > +Brief API summary
> > > +-----------------
> > > + [ ... snip ...]
> > > +* klp_shadow_detach() - detach and free all <*, num> shadow variables
> > > +  - find and remove any <*, num> references from hashtable
> > > +    - if found, release shadow variable
> > 
> > I think that the second one should be klp_shadow_detach_all(), shouldn't 
> > it?
> 
> Good catch, I'll fixup in v3.
> 
> > > +static DEFINE_HASHTABLE(klp_shadow_hash, 12);
> > 
> > Is there a reason, why you pick 12? I'm just curious.
>
> The hashtable bit-size was inherited from the kpatch implementation.
> Perhaps Josh knows why this value was picked?

My thinking was that it gives you about 4096 unique hash table entries
for 32k of RAM.  It was a rough guess.  It's hard to really predict what
size you need.

> Aside: we could have per-livepatch hashtables if that was desired, this
> value could be then adjusted accordingly.  We haven't needed them for
> kpatch, so I didn't see good reason to complicate things.

I think a global hash table is much better because it allows you to deal
more gracefully with patch upgrades.

> > > + *
> > > + * Note: allocates @new_size space for shadow variable data and copies
> > > + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> > > + * space.  If @new_data is NULL, @new_size is still allocated, but no
> > > + * copy is performed.
> > 
> > I must say I'm not entirely happy with this. I don't know if this is what 
> > Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> > memcpy instead of a simple assignment in v1 seems worse. 
> 
> This change was a bit of a experiment on my part in reaction to
> adding klp_shadow_get_or_attach().
> 
> I like the simplicity of v1's pointer assignment -- in fact, moving all
> allocation responsiblity (klp_shadow meta-data and data[] area) out to
> the caller is doable, though implementing klp_shadow_get_or_attach() and
> and klp_shadow_detach_all() complicates matters, for example, adding an
> alloc/release callback.  I originally attempted this for v2, but turned
> back when the API and implementation grew complicated.  If the memcpy
> and gfp_flag restrictions are too ugly, I can try revisting that
> approach.  Ideas welcome :)

Personally I'm not a fan of the callbacks, I like the v2 API.

-- 
Josh

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Miroslav Benes]

> > > +/**
> > > + * _klp_shadow_attach() - allocate and add a new shadow variable
> > > + * @obj:	pointer to original data
> > > + * @num:	numerical description of new data
> > > + * @new_data:	pointer to new data
> > > + * @new_size:	size of new data
> > > + * @gfp_flags:	GFP mask for allocation
> > > + * @lock:	take klp_shadow_lock during klp_shadow_hash operations
> > 
> > I am not sure about lock argument. Do we need it? Common practice is to 
> > have function foo() which takes a lock, and function __foo() which does 
> > not.
> > 
> > In klp_shadow_get_or_attach(), you use it as I'd expect. You take the 
> > spinlock, call this function and release the spinlock. Is it possible 
> > to do the same in klp_shadow_attach() and have __klp_shadow_attach() 
> > without lock argument?
> 
> Yes, this would be possible, though it would restrict
> klp_shadow_attach() from accepting gfp_flags that might allow for
> sleeping.  More on that below ...

Ok, that is a good remark. The problem is that it applies to 
klp_shadow_get_or_attach() too. There you acquire a spin_lock and call 
_klp_shadow_attach() with gfp_flags, which are then used for kzalloc.

I might misread the code. It is getting late here.

> > > + *
> > > + * Note: allocates @new_size space for shadow variable data and copies
> > > + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> > > + * space.  If @new_data is NULL, @new_size is still allocated, but no
> > > + * copy is performed.
> > 
> > I must say I'm not entirely happy with this. I don't know if this is what 
> > Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> > memcpy instead of a simple assignment in v1 seems worse. 
> 
> This change was a bit of a experiment on my part in reaction to
> adding klp_shadow_get_or_attach().
> 
> I like the simplicity of v1's pointer assignment -- in fact, moving all
> allocation responsiblity (klp_shadow meta-data and data[] area) out to
> the caller is doable, though implementing klp_shadow_get_or_attach() and
> and klp_shadow_detach_all() complicates matters, for example, adding an
> alloc/release callback.  I originally attempted this for v2, but turned
> back when the API and implementation grew complicated.  If the memcpy
> and gfp_flag restrictions are too ugly, I can try revisting that
> approach.  Ideas welcome :)

Well, I didn't like callbacks either :). And no, I do not have a better 
idea. I still need to think about it.

Miroslav

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Miroslav Benes]

> > > > + *
> > > > + * Note: allocates @new_size space for shadow variable data and copies
> > > > + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> > > > + * space.  If @new_data is NULL, @new_size is still allocated, but no
> > > > + * copy is performed.
> > > 
> > > I must say I'm not entirely happy with this. I don't know if this is what 
> > > Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> > > memcpy instead of a simple assignment in v1 seems worse. 
> > 
> > This change was a bit of a experiment on my part in reaction to
> > adding klp_shadow_get_or_attach().
> > 
> > I like the simplicity of v1's pointer assignment -- in fact, moving all
> > allocation responsiblity (klp_shadow meta-data and data[] area) out to
> > the caller is doable, though implementing klp_shadow_get_or_attach() and
> > and klp_shadow_detach_all() complicates matters, for example, adding an
> > alloc/release callback.  I originally attempted this for v2, but turned
> > back when the API and implementation grew complicated.  If the memcpy
> > and gfp_flag restrictions are too ugly, I can try revisting that
> > approach.  Ideas welcome :)
> 
> Well, I didn't like callbacks either :). And no, I do not have a better 
> idea. I still need to think about it.

Done and I agree that memcpy approach is not so bad after all :). So I'm 
fine with it.

Miroslav

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Joe Lawrence]

On 07/20/2017 10:45 AM, Miroslav Benes wrote:
> 
>>>>> + *
>>>>> + * Note: allocates @new_size space for shadow variable data and copies
>>>>> + * @new_size bytes from @new_data into the shadow varaible's own @new_data
>>>>> + * space.  If @new_data is NULL, @new_size is still allocated, but no
>>>>> + * copy is performed.
>>>>
>>>> I must say I'm not entirely happy with this. I don't know if this is what 
>>>> Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
>>>> memcpy instead of a simple assignment in v1 seems worse. 
>>>
>>> This change was a bit of a experiment on my part in reaction to
>>> adding klp_shadow_get_or_attach().
>>>
>>> I like the simplicity of v1's pointer assignment -- in fact, moving all
>>> allocation responsiblity (klp_shadow meta-data and data[] area) out to
>>> the caller is doable, though implementing klp_shadow_get_or_attach() and
>>> and klp_shadow_detach_all() complicates matters, for example, adding an
>>> alloc/release callback.  I originally attempted this for v2, but turned
>>> back when the API and implementation grew complicated.  If the memcpy
>>> and gfp_flag restrictions are too ugly, I can try revisting that
>>> approach.  Ideas welcome :)
>>
>> Well, I didn't like callbacks either :). And no, I do not have a better 
>> idea. I still need to think about it.
> 
> Done and I agree that memcpy approach is not so bad after all :). So I'm 
> fine with it.

I looked at it again this morning and a "pass-your-own" allocation API
always comes back to adding callbacks and other complications :(  In the
end, most callers will be shadowing pointers and not entire structures,
so I think the copy isn't too bad.

On a related note, if we keep the allocations and memcpy, how about I
shift around the attach/get calls like so:

  __klp_shadow_attach
    set shadow variable member values
    memcpy
    add to hash

  klp_shadow_attach
    alloc new shadow var
    lock
    call __klp_shadow_attach with new alloc
    unlock

  klp_shadow_get_or_attach
    be optimistic, call klp_shadow_get (if found, return it)
    be pessimistic, alloc new shadow var
    lock
      call klp_shadow_get again
      if unlikely found
        kfree unneeded alloc
      else
        call __klp_shadow_attach with new alloc
    unlock
    return whichever shadow var we used

This way both calls can accept gfp_flags that may sleep, with the only
downside that klp_shadow_get_or_attach may allocate an unnecessary
shadow variable in the unlikely case that it's found on the second
klp_shadow_get attempt (under the lock).  No more clunky "bool lock"
flag either. :)

-- Joe

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Josh Poimboeuf]

On Thu, Jul 20, 2017 at 11:48:41AM -0400, Joe Lawrence wrote:
> On a related note, if we keep the allocations and memcpy, how about I
> shift around the attach/get calls like so:
> 
>   __klp_shadow_attach
>     set shadow variable member values
>     memcpy
>     add to hash
> 
>   klp_shadow_attach
>     alloc new shadow var
>     lock
>     call __klp_shadow_attach with new alloc
>     unlock
> 
>   klp_shadow_get_or_attach
>     be optimistic, call klp_shadow_get (if found, return it)
>     be pessimistic, alloc new shadow var
>     lock
>       call klp_shadow_get again
>       if unlikely found
>         kfree unneeded alloc
>       else
>         call __klp_shadow_attach with new alloc
>     unlock
>     return whichever shadow var we used
> 
> This way both calls can accept gfp_flags that may sleep, with the only
> downside that klp_shadow_get_or_attach may allocate an unnecessary
> shadow variable in the unlikely case that it's found on the second
> klp_shadow_get attempt (under the lock).  No more clunky "bool lock"
> flag either. :)

Sounds good to me!

-- 
Josh

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Petr Mladek]

On Thu 2017-07-20 11:48:41, Joe Lawrence wrote:
> On 07/20/2017 10:45 AM, Miroslav Benes wrote:
> > 
> >>>>> + *
> >>>>> + * Note: allocates @new_size space for shadow variable data and copies
> >>>>> + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> >>>>> + * space.  If @new_data is NULL, @new_size is still allocated, but no
> >>>>> + * copy is performed.
> >>>>
> >>>> I must say I'm not entirely happy with this. I don't know if this is what 
> >>>> Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> >>>> memcpy instead of a simple assignment in v1 seems worse. 
> >>>
> >>> This change was a bit of a experiment on my part in reaction to
> >>> adding klp_shadow_get_or_attach().
> >>>
> >>> I like the simplicity of v1's pointer assignment -- in fact, moving all
> >>> allocation responsiblity (klp_shadow meta-data and data[] area) out to
> >>> the caller is doable, though implementing klp_shadow_get_or_attach() and
> >>> and klp_shadow_detach_all() complicates matters, for example, adding an
> >>> alloc/release callback.  I originally attempted this for v2, but turned
> >>> back when the API and implementation grew complicated.  If the memcpy
> >>> and gfp_flag restrictions are too ugly, I can try revisting that
> >>> approach.  Ideas welcome :)
> >>
> >> Well, I didn't like callbacks either :). And no, I do not have a better 
> >> idea. I still need to think about it.
> > 
> > Done and I agree that memcpy approach is not so bad after all :). So I'm 
> > fine with it.
> 
> I looked at it again this morning and a "pass-your-own" allocation API
> always comes back to adding callbacks and other complications :(  In the
> end, most callers will be shadowing pointers and not entire structures,
> so I think the copy isn't too bad.

I agree.

> On a related note, if we keep the allocations and memcpy, how about I
> shift around the attach/get calls like so:
> 
>   __klp_shadow_attach
>     set shadow variable member values
>     memcpy
>     add to hash
> 
>   klp_shadow_attach
>     alloc new shadow var
>     lock
>     call __klp_shadow_attach with new alloc
>     unlock
>
>   klp_shadow_get_or_attach
>     be optimistic, call klp_shadow_get (if found, return it)
>     be pessimistic, alloc new shadow var
>     lock
>       call klp_shadow_get again
>       if unlikely found
>         kfree unneeded alloc
>       else
>         call __klp_shadow_attach with new alloc
>     unlock
>     return whichever shadow var we used

I would really suggest that klp_shadow_attach() prevents adding
duplicates. We should make the API as safe as possible.
Catching unexpected duplicate could safe people a lot of
headaches.

Please read more on this in my review
https://lkml.kernel.org/r/20170718124500.GF3393@pathway.suse.cz

Best Regards,
Petr

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Miroslav Benes]

On Thu, 20 Jul 2017, Joe Lawrence wrote:

> On 07/20/2017 10:45 AM, Miroslav Benes wrote:
> > 
> >>>>> + *
> >>>>> + * Note: allocates @new_size space for shadow variable data and copies
> >>>>> + * @new_size bytes from @new_data into the shadow varaible's own @new_data
> >>>>> + * space.  If @new_data is NULL, @new_size is still allocated, but no
> >>>>> + * copy is performed.
> >>>>
> >>>> I must say I'm not entirely happy with this. I don't know if this is what 
> >>>> Petr had in mind (I'm sure he'll get to the patch set soon). Calling 
> >>>> memcpy instead of a simple assignment in v1 seems worse. 
> >>>
> >>> This change was a bit of a experiment on my part in reaction to
> >>> adding klp_shadow_get_or_attach().
> >>>
> >>> I like the simplicity of v1's pointer assignment -- in fact, moving all
> >>> allocation responsiblity (klp_shadow meta-data and data[] area) out to
> >>> the caller is doable, though implementing klp_shadow_get_or_attach() and
> >>> and klp_shadow_detach_all() complicates matters, for example, adding an
> >>> alloc/release callback.  I originally attempted this for v2, but turned
> >>> back when the API and implementation grew complicated.  If the memcpy
> >>> and gfp_flag restrictions are too ugly, I can try revisting that
> >>> approach.  Ideas welcome :)
> >>
> >> Well, I didn't like callbacks either :). And no, I do not have a better 
> >> idea. I still need to think about it.
> > 
> > Done and I agree that memcpy approach is not so bad after all :). So I'm 
> > fine with it.
> 
> I looked at it again this morning and a "pass-your-own" allocation API
> always comes back to adding callbacks and other complications :(  In the
> end, most callers will be shadowing pointers and not entire structures,
> so I think the copy isn't too bad.

I agree.
 
> On a related note, if we keep the allocations and memcpy, how about I
> shift around the attach/get calls like so:
> 
>   __klp_shadow_attach
>     set shadow variable member values
>     memcpy
>     add to hash
> 
>   klp_shadow_attach
>     alloc new shadow var
>     lock
>     call __klp_shadow_attach with new alloc
>     unlock
> 
>   klp_shadow_get_or_attach
>     be optimistic, call klp_shadow_get (if found, return it)
>     be pessimistic, alloc new shadow var
>     lock
>       call klp_shadow_get again
>       if unlikely found
>         kfree unneeded alloc
>       else
>         call __klp_shadow_attach with new alloc
>     unlock
>     return whichever shadow var we used
> 
> This way both calls can accept gfp_flags that may sleep, with the only
> downside that klp_shadow_get_or_attach may allocate an unnecessary
> shadow variable in the unlikely case that it's found on the second
> klp_shadow_get attempt (under the lock).  No more clunky "bool lock"
> flag either. :)

Yes, this looks good.

Thanks,
Miroslav

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Petr Mladek]

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

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

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

s/"ps_lock"/PS_LOCK/

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

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

So, the code might look like:

  void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
  {
	DEFINE_SPINLOCK(ps_lock_fallback)
	spinlock_t *ps_lock;
	...
	/* sync with ieee80211_tx_h_unicast_ps_buf */
	ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
			&ps_lock_fallback, sizeof(ps_lock_fallback),
			GFP_ATOMIC);

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


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

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

> +	kfree(sta);
> +
> +


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

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


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

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

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

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


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

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

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

Best Regards,
Petr

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Joe Lawrence]

On 07/18/2017 08:45 AM, Petr Mladek wrote:
> On Wed 2017-06-28 11:37:26, Joe Lawrence wrote:
>> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
>> new file mode 100644
>> index 000000000000..7f28982e6b1c
>> --- /dev/null
>> +++ b/Documentation/livepatch/shadow-vars.txt
>> +Use cases
>> +---------
>> +
>> +See the example shadow variable livepatch modules in samples/livepatch
>> +for full working demonstrations.
>> +
>> +Example 1: Commit 1d147bfa6429 ("mac80211: fix AP powersave TX vs.
>> +wakeup race") added a spinlock to net/mac80211/sta_info.h :: struct
>> +sta_info.  Implementing this change with a shadow variable is
>> +straightforward.
>> +
>> +Allocation - when a host sta_info structure is allocated, attach a
>> +shadow variable copy of the ps_lock:
>> +
>> +#define PS_LOCK 1
>> +struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>> +				const u8 *addr, gfp_t gfp)
>> +{
>> +	struct sta_info *sta;
>> +	spinlock_t *ps_lock;
>> +	...
>> +	sta = kzalloc(sizeof(*sta) + hw->sta_data_size, gfp);
> 
> klp_shadow_attach() does the allocation as well now.
> Note that we could pass already initialized spin_lock.
> 
>> +	...
>> +	ps_lock = klp_shadow_attach(sta, PS_LOCK, NULL, sizeof(*ps_lock), gfp);
>> +	if (!ps_lock)
>> +		goto shadow_fail;
>> +	spin_lock_init(ps_lock);
>> +	...
>> +
>> +Usage - when using the shadow spinlock, query the shadow variable API to
>> +retrieve it:
>> +
>> +void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
>> +{
>> +	spinlock_t *ps_lock;
>> +	...
>> +	/* sync with ieee80211_tx_h_unicast_ps_buf */
>> +	ps_lock = klp_shadow_get(sta, "ps_lock");
> 
> s/"ps_lock"/PS_LOCK/
> 
> The same problem is repeated many times below (also in the 2nd
> example).
> 
> Also this is a nice example, where klp_shadow_get_or_attach()
> would be useful. It would fix even already existing instances.
> 
> So, the code might look like:
> 
>   void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
>   {
> 	DEFINE_SPINLOCK(ps_lock_fallback)
> 	spinlock_t *ps_lock;
> 	...
> 	/* sync with ieee80211_tx_h_unicast_ps_buf */
> 	ps_lock = klp_shadow_get_or_attach(sta, PS_LOCK,
> 			&ps_lock_fallback, sizeof(ps_lock_fallback),
> 			GFP_ATOMIC);
> 
> It is a bit ugly that we always initialize ps_lock_fallback
> even when it is not used. But it helps to avoid a custom
> callback that would create the fallback variable. I think
> that it is an acceptable deal.

Yup, it's a tradeoff for the caller.  If they want a shadow variable
added and considered "live", they better have previously initialized it :)

> 
>> +	if (ps_lock)
>> +		spin_lock(ps_lock);
>> +	...
>> +	if (ps_lock)
>> +		spin_unlock(ps_lock);
>> +	...
>> +
>> +Release - when the host sta_info structure is freed, first detach the
>> +shadow variable and then free the shadow spinlock:
>> +
>> +void sta_info_free(struct ieee80211_local *local, struct sta_info *sta)
>> +{
>> +	spinlock_t *ps_lock;
>> +	...
>> +	ps_lock = klp_shadow_get(sta, "ps_lock");
>> +	if (ps_lock)
>> +		klp_shadow_detach(sta, "ps_lock");
> 
> Isn't klp_shadow_detach() enough? If it an optimization, 
> klp_shadow_detach() might get optimized the same way.
> But I am not sure if it is worth it.

Let me go back and review these inline examples for v3... I didn't
update them carefully enough when drafting v2.

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

Noted for v3.

>> +{
>> +	struct klp_shadow *shadow;
>> +	unsigned long flags;
>> +
>> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
>> +	if (!shadow)
>> +		return NULL;
>> +
>> +	shadow->obj = obj;
>> +	shadow->num = num;
>> +	if (new_data)
>> +		memcpy(shadow->new_data, new_data, new_size);
>> +
>> +	if (lock)
>> +		spin_lock_irqsave(&klp_shadow_lock, flags);
>> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> 
> We should check if the shadow variable already existed. Otherwise,
> it would be possible to silently create many duplicates.
> 
> It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> to behave the same.

They would be almost exactly the same, except one version would bounce a
redundant entry while the other would return the existing one.  I could
envision callers wanting any of the following behavior:

If a shadow <obj, id> already exists:
  0 - add a second shadow variable (??? why)
  1 - return NULL, WARN
  2 - return the existing one
  3 - update the existing one with the new data and return it

* v2 klp_shadow_attach() currently implements #0, can be made to do #1
* v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
makes more sense

Going back to existing kpatch use-cases, since we paired shadow variable
creation to their parent object creation, -EEXIST was never an issue.  I
think we concocted one proof-of-concept kpatch where we created shadow
variables "in-flight", that is, we patched a routine that operated on
the parent object and created a shadow variable if one did not already
exist.  The in-flight patch was for single function and we knew that it
would never be called concurrently for the same parent object.  tl;dr =
kpatch never worried about existing shadow <obj, id>.

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

What do you think of expanding the API to include each the cases
outlined above?   Something like:

  1 - klp_attach = allocate and add a unique <obj, id> to the hash,
                   duplicates return NULL and a WARN

  2 - klp_get_or_attach = return <obj, id> if it already exists,
                          otherwise allocate a new one

  3 - klp_get_or_update = update and return <obj, id> if it already
                          exists, otherwise allocate a new one

IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
be dropped.  Since you suggested adding klp_get_or_attach(), what do you
think?

> 
>> +	if (lock)
>> +		spin_unlock_irqrestore(&klp_shadow_lock, flags);
>> +
>> +	return shadow->new_data;
>> +}
> 
> Otherwise, I rather like the API. Thanks a lot for adding
> klp_shadow_get_or_attach().
> 
> I did not comment things that were already discussed in
> other threads.

klp_shadow_get_or_attach() looks to be really useful in concurrent
situations, especially cases where we'd like to do in-flight shadow
variable creation.

Appreciate the comments as always.

-- Joe

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Miroslav Benes]

> >> +{
> >> +	struct klp_shadow *shadow;
> >> +	unsigned long flags;
> >> +
> >> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> >> +	if (!shadow)
> >> +		return NULL;
> >> +
> >> +	shadow->obj = obj;
> >> +	shadow->num = num;
> >> +	if (new_data)
> >> +		memcpy(shadow->new_data, new_data, new_size);
> >> +
> >> +	if (lock)
> >> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> >> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> > 
> > We should check if the shadow variable already existed. Otherwise,
> > it would be possible to silently create many duplicates.
> > 
> > It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> > to behave the same.
> 
> They would be almost exactly the same, except one version would bounce a
> redundant entry while the other would return the existing one.  I could
> envision callers wanting any of the following behavior:
> 
> If a shadow <obj, id> already exists:
>   0 - add a second shadow variable (??? why)
>   1 - return NULL, WARN
>   2 - return the existing one
>   3 - update the existing one with the new data and return it
> 
> * v2 klp_shadow_attach() currently implements #0, can be made to do #1
> * v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
> makes more sense

I have a feeling that we're becoming overprotective here again. I think 
that klp_shadow_attach() adding a new entry makes sense. 
Although I can imagine #1. I think it is a responsibility of the user to 
know what to call. And that is what klp_shadow_get_or_attach() is for.

klp_shadow_get() and klp_shadow_attach() are two main API functions. 
klp_shadow_get_or_attach() is there to make things safe if needed 
(concurrency).
 
> Going back to existing kpatch use-cases, since we paired shadow variable
> creation to their parent object creation, -EEXIST was never an issue.  I
> think we concocted one proof-of-concept kpatch where we created shadow
> variables "in-flight", that is, we patched a routine that operated on
> the parent object and created a shadow variable if one did not already
> exist.  The in-flight patch was for single function and we knew that it
> would never be called concurrently for the same parent object.  tl;dr =
> kpatch never worried about existing shadow <obj, id>.

And this makes sense to me too.

> > I would do WARN() in klp_shadow_attach() when the variable
> > already existed are return NULL. Of course it might be inoncent
> > duplication. But it might mean that someone else is using another
> > variable of the same name but with different content. klp_shadow_get()
> > would then return the same variable for two different purposes.
> > Then the whole system might end like a glass on a stony floor.
> 
> What do you think of expanding the API to include each the cases
> outlined above?   Something like:
> 
>   1 - klp_attach = allocate and add a unique <obj, id> to the hash,
>                    duplicates return NULL and a WARN
> 
>   2 - klp_get_or_attach = return <obj, id> if it already exists,
>                           otherwise allocate a new one
> 
>   3 - klp_get_or_update = update and return <obj, id> if it already
>                           exists, otherwise allocate a new one
> 
> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
> be dropped.  Since you suggested adding klp_get_or_attach(), what do you
> think?

I don't know. I'd be prudent now. We can always add it later...

Miroslav

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Petr Mladek]

On Fri 2017-07-21 11:12:18, Miroslav Benes wrote:
> 
> > >> +{
> > >> +	struct klp_shadow *shadow;
> > >> +	unsigned long flags;
> > >> +
> > >> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> > >> +	if (!shadow)
> > >> +		return NULL;
> > >> +
> > >> +	shadow->obj = obj;
> > >> +	shadow->num = num;
> > >> +	if (new_data)
> > >> +		memcpy(shadow->new_data, new_data, new_size);
> > >> +
> > >> +	if (lock)
> > >> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> > >> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> > > 
> > > We should check if the shadow variable already existed. Otherwise,
> > > it would be possible to silently create many duplicates.
> > > 
> > > It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> > > to behave the same.
> > 
> > They would be almost exactly the same, except one version would bounce a
> > redundant entry while the other would return the existing one.  I could
> > envision callers wanting any of the following behavior:
> > 
> > If a shadow <obj, id> already exists:
> >   0 - add a second shadow variable (??? why)
> >   1 - return NULL, WARN
> >   2 - return the existing one
> >   3 - update the existing one with the new data and return it
> > 
> > * v2 klp_shadow_attach() currently implements #0, can be made to do #1
> > * v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
> > makes more sense
> 
> I have a feeling that we're becoming overprotective here again. I think 
> that klp_shadow_attach() adding a new entry makes sense. 
> Although I can imagine #1. I think it is a responsibility of the user to 
> know what to call. And that is what klp_shadow_get_or_attach() is for.

The shadow id is an integer. This prevents also from using the same
id by two patches for a different purpose. The two livepatches might
be crated months after each other. There might be many fixes
accumulated in the livepatch. Is it really almost impossible
to make mistakes so this rather small change is not worth it?

Another motivation is that the author of the livepatch usually
is not familiar with the patched code. It makes it more prone
to mistakes.

Best Regards,
Petr

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Petr Mladek]

On Thu 2017-07-20 16:30:37, Joe Lawrence wrote:
> On 07/18/2017 08:45 AM, Petr Mladek wrote:
> > On Wed 2017-06-28 11:37:26, Joe Lawrence wrote:
> >> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> >> new file mode 100644
> >> index 000000000000..d37a61c57e72
> >> --- /dev/null
> >> +++ b/kernel/livepatch/shadow.c
> >> +static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> >> +				size_t new_size, gfp_t gfp_flags,
> >> +				bool lock)
> > 
> > Nested implementation is usually prefixed by two underlines __.
> > It is more visible and helps to distinguish it from the normal function.
> 
> Noted for v3.
> 
> >> +{
> >> +	struct klp_shadow *shadow;
> >> +	unsigned long flags;
> >> +
> >> +	shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> >> +	if (!shadow)
> >> +		return NULL;
> >> +
> >> +	shadow->obj = obj;
> >> +	shadow->num = num;
> >> +	if (new_data)
> >> +		memcpy(shadow->new_data, new_data, new_size);
> >> +
> >> +	if (lock)
> >> +		spin_lock_irqsave(&klp_shadow_lock, flags);
> >> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> > 
> > We should check if the shadow variable already existed. Otherwise,
> > it would be possible to silently create many duplicates.
> > 
> > It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> > to behave the same.
> 
> They would be almost exactly the same, except one version would bounce a
> redundant entry while the other would return the existing one.  I could
> envision callers wanting any of the following behavior:
> 
> If a shadow <obj, id> already exists:
>   0 - add a second shadow variable (??? why)
>   1 - return NULL, WARN
>   2 - return the existing one
>   3 - update the existing one with the new data and return it
> 
> * v2 klp_shadow_attach() currently implements #0, can be made to do #1
> * v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
> makes more sense
> 
> Going back to existing kpatch use-cases, since we paired shadow variable
> creation to their parent object creation, -EEXIST was never an issue.  I
> think we concocted one proof-of-concept kpatch where we created shadow
> variables "in-flight", that is, we patched a routine that operated on
> the parent object and created a shadow variable if one did not already
> exist.  The in-flight patch was for single function and we knew that it
> would never be called concurrently for the same parent object.  tl;dr =
> kpatch never worried about existing shadow <obj, id>.

I am not sure if you want to explain why you did not care. Or if
you want to suggest that we should not care :-)

I agree that if the API is used in simple/clear situations then
this might look like an overkill. But I am afraid that the API users
do not have this in hands. They usually have to create a livepatch
based on an upstream secutity fix. The fix need not be always simple.
Then it is handy to have an API that helps to catch mistakes
and keeps the patched system in a sane state.


> > I would do WARN() in klp_shadow_attach() when the variable
> > already existed are return NULL. Of course it might be inoncent
> > duplication. But it might mean that someone else is using another
> > variable of the same name but with different content. klp_shadow_get()
> > would then return the same variable for two different purposes.
> > Then the whole system might end like a glass on a stony floor.
> 
> What do you think of expanding the API to include each the cases
> outlined above?   Something like:
> 
>   1 - klp_attach = allocate and add a unique <obj, id> to the hash,
>                    duplicates return NULL and a WARN

Sounds good.

>   2 - klp_get_or_attach = return <obj, id> if it already exists,
>                           otherwise allocate a new one

Sounds good.

>   3 - klp_get_or_update = update and return <obj, id> if it already
>                           exists, otherwise allocate a new one

I am not sure where this behavior would make sense. See below.


> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
> be dropped.  Since you suggested adding klp_get_or_attach(), what do you
> think?

I do not agree. Let's look at the example with the missing lock.
The patch adds the lock if it did not exist. Then the lock can
be used to synchronize all further operations.

klp_get_or_update() would always replace the existing lock
with a freshly initialized one. We would loss the information
if it was locked or not.


> klp_shadow_get_or_attach() looks to be really useful in concurrent
> situations, especially cases where we'd like to do in-flight shadow
> variable creation.

Thanks a lot for working in the API. It will be handy.

Best Regards,
Petr

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Joe Lawrence]

On 07/21/2017 05:13 AM, Petr Mladek wrote:
> On Thu 2017-07-20 16:30:37, Joe Lawrence wrote:
>> Going back to existing kpatch use-cases, since we paired shadow variable
>> creation to their parent object creation, -EEXIST was never an issue.  I
>> think we concocted one proof-of-concept kpatch where we created shadow
>> variables "in-flight", that is, we patched a routine that operated on
>> the parent object and created a shadow variable if one did not already
>> exist.  The in-flight patch was for single function and we knew that it
>> would never be called concurrently for the same parent object.  tl;dr =
>> kpatch never worried about existing shadow <obj, id>.
> 
> I am not sure if you want to explain why you did not care. Or if
> you want to suggest that we should not care :-)

We knew that in our limited use-cases for in-flight shadow variables,
concurrency was not an issue.  Josh has a better historical perspective,
but I think this particular use-case appeared way after the initial
kpatch implementation of shadow variables.  Now that we know we can use
them in this way, I agree that it's important to hash out the
implications while designing the livepatch counterpart.

> I agree that if the API is used in simple/clear situations then
> this might look like an overkill. But I am afraid that the API users
> do not have this in hands. They usually have to create a livepatch
> based on an upstream secutity fix. The fix need not be always simple.
> Then it is handy to have an API that helps to catch mistakes
> and keeps the patched system in a sane state.
Very true, though I wonder what interesting state that will be when
running patched code and partially shadowed variables. :)  At the very
least, I think this protection would be valuable during patch sanity
testing.

>>> I would do WARN() in klp_shadow_attach() when the variable
>>> already existed are return NULL. Of course it might be inoncent
>>> duplication. But it might mean that someone else is using another
>>> variable of the same name but with different content. klp_shadow_get()
>>> would then return the same variable for two different purposes.
>>> Then the whole system might end like a glass on a stony floor.
>>
>> What do you think of expanding the API to include each the cases
>> outlined above?   Something like:
>>
>>   1 - klp_attach = allocate and add a unique <obj, id> to the hash,
>>                    duplicates return NULL and a WARN
> 
> Sounds good.
> 
>>   2 - klp_get_or_attach = return <obj, id> if it already exists,
>>                           otherwise allocate a new one
> 
> Sounds good.
> 
>>   3 - klp_get_or_update = update and return <obj, id> if it already
>>                           exists, otherwise allocate a new one
> 
> I am not sure where this behavior would make sense. See below.
> 
> 
>> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
>> be dropped.  Since you suggested adding klp_get_or_attach(), what do you
>> think?
> 
> I do not agree. Let's look at the example with the missing lock.
> The patch adds the lock if it did not exist. Then the lock can
> be used to synchronize all further operations.
> 
> klp_get_or_update() would always replace the existing lock
> with a freshly initialized one. We would loss the information
> if it was locked or not.

Ah good point, perhaps we have two situations here:

  A - A shadow variable that's pointing to some object, like a lock,
      where the original object is required.  (Your example above.)

  B - A shadow variable that's storing the data itself.  In other words,
      instead of attaching a pointer, the whole object was attached:

        void patched_function()
        {
           ...
           klp_get_or_attach(obj, id, &jiffies, sizeof(jiffies), ...)
           ...

      in which case the caller is only interested in pushing in the
      latest version of jiffies.

For these I suggest klp_get_or_attach() for case A and
klp_get_or_update() for case B.

-- Joe

Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Josh Poimboeuf]

On Fri, Jul 21, 2017 at 09:55:59AM -0400, Joe Lawrence wrote:
> >>> I would do WARN() in klp_shadow_attach() when the variable
> >>> already existed are return NULL. Of course it might be inoncent
> >>> duplication. But it might mean that someone else is using another
> >>> variable of the same name but with different content. klp_shadow_get()
> >>> would then return the same variable for two different purposes.
> >>> Then the whole system might end like a glass on a stony floor.
> >>
> >> What do you think of expanding the API to include each the cases
> >> outlined above?   Something like:
> >>
> >>   1 - klp_attach = allocate and add a unique <obj, id> to the hash,
> >>                    duplicates return NULL and a WARN
> > 
> > Sounds good.
> > 
> >>   2 - klp_get_or_attach = return <obj, id> if it already exists,
> >>                           otherwise allocate a new one
> > 
> > Sounds good.
> > 
> >>   3 - klp_get_or_update = update and return <obj, id> if it already
> >>                           exists, otherwise allocate a new one
> > 
> > I am not sure where this behavior would make sense. See below.
> > 
> > 
> >> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
> >> be dropped.  Since you suggested adding klp_get_or_attach(), what do you
> >> think?
> > 
> > I do not agree. Let's look at the example with the missing lock.
> > The patch adds the lock if it did not exist. Then the lock can
> > be used to synchronize all further operations.
> > 
> > klp_get_or_update() would always replace the existing lock
> > with a freshly initialized one. We would loss the information
> > if it was locked or not.
> 
> Ah good point, perhaps we have two situations here:
> 
>   A - A shadow variable that's pointing to some object, like a lock,
>       where the original object is required.  (Your example above.)
> 
>   B - A shadow variable that's storing the data itself.  In other words,
>       instead of attaching a pointer, the whole object was attached:
> 
>         void patched_function()
>         {
>            ...
>            klp_get_or_attach(obj, id, &jiffies, sizeof(jiffies), ...)
>            ...
> 
>       in which case the caller is only interested in pushing in the
>       latest version of jiffies.
> 
> For these I suggest klp_get_or_attach() for case A and
> klp_get_or_update() for case B.

klp_get_or_update() doesn't actually 'get', because even if it does, it
gets updated first.  I think a more precise name would be
klp_update_or_attach().

-- 
Josh

[PATCH v2 2/2] livepatch: add shadow variable sample programs

[Joe Lawrence]

Add sample livepatch modules to demonstrate the shadow variable API.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---

Reviewers -- it's probably easier to grok reading livepatch-shadow-mod.c
             *before* the two livepatch modules, livepatch-shadow-fix1.c
             and livepatch-shadow-fix2.c.

 samples/Kconfig                           |   5 +-
 samples/livepatch/Makefile                |   3 +
 samples/livepatch/livepatch-shadow-fix1.c | 160 ++++++++++++++
 samples/livepatch/livepatch-shadow-fix2.c | 157 +++++++++++++
 samples/livepatch/livepatch-shadow-mod.c  | 353 ++++++++++++++++++++++++++++++
 5 files changed, 675 insertions(+), 3 deletions(-)
 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/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..2e1d9cb89fad
--- /dev/null
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -0,0 +1,160 @@
+/*
+ * livepatch-shadow-fix1.c - Shadow variables, livepatch demo
+ *
+ * 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/>.
+ */
+
+/*
+ * 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.
+ */
+
+#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 T1_PERIOD 1			/* allocator thread */
+#define T2_PERIOD (3 * T1_PERIOD)	/* cleanup thread */
+
+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;
+
+	/* Dummies live long enough to see a few t2 instances */
+	d->jiffies_expire = jiffies + 1000 * 4 * T2_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..ad6346da4aa3
--- /dev/null
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -0,0 +1,157 @@
+/*
+ * livepatch-shadow-fix2.c - Shadow variables, livepatch demo
+ *
+ * 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/>.
+ */
+
+/*
+ * 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.
+ */
+
+#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..423f4b7b0adb
--- /dev/null
+++ b/samples/livepatch/livepatch-shadow-mod.c
@@ -0,0 +1,353 @@
+/*
+ * livepatch-shadow-mod.c - Shadow variables, buggy module demo
+ *
+ * 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/>.
+ */
+
+/* Creates two running threads:
+ *
+ *   T1 - allocs a new dummy structure, sets a jiffie expiration time
+ *        in the future, adds the new structure to a list
+ *
+ *   T2 - cleans up expired dummies on the list
+ *
+ * For the purposes of demonstrating a livepatch shadow variable fix,
+ * the creation thread also allocates additional memory, but doesn't
+ * save a pointer to it in the dummy structure.  The cleanup thread
+ * then leaks the extra memory when it frees (only) the dummy
+ * structure.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * Load the buggy demonstration module:
+ * $ insmod samples/livepatch/livepatch-shadow-mod.ko
+ *
+ * T1 allocator thread periodically wakes up and creates new dummy
+ * structures allocating extra memory and set to expire some jiffie time
+ * in the future:
+ *
+ * [  529.503048] dummy_alloc: dummy @ ffff880112921e38, expires @ 10003af60
+ * [  530.527051] dummy_alloc: dummy @ ffff8801129219e8, expires @ 10003b360
+ * [  531.551049] dummy_alloc: dummy @ ffff880112920458, expires @ 10003b760
+ *
+ * T2 cleanup thread wakes up, but doesn't find any expired dummies to
+ * cleanup yet:
+ *
+ * [  531.551212] cleanup_thread:  jiffies = 100038880
+ * [  532.575045] dummy_alloc: dummy @ ffff880112921708, expires @ 10003bb60
+ * [  533.599051] dummy_alloc: dummy @ ffff880112920a18, expires @ 10003bf60
+ * [  534.623052] dummy_alloc: dummy @ ffff8801129205c8, expires @ 10003c360
+ * [  535.647056] dummy_alloc: dummy @ ffff880112921598, expires @ 10003c760
+ * [  536.671032] dummy_alloc: dummy @ ffff880112920fd8, expires @ 10003cb60
+ * [  537.567089] cleanup_thread:  jiffies = 10003a000
+ * [  537.695043] dummy_alloc: dummy @ ffff880112920008, expires @ 10003cf60
+ * [  538.719047] dummy_alloc: dummy @ ffff880112921878, expires @ 10003d360
+ * [  539.743061] dummy_alloc: dummy @ ffff880112920cf8, expires @ 10003d760
+ * [  540.767041] dummy_alloc: dummy @ ffff880116567148, expires @ 10003db60
+ * [  541.791045] dummy_alloc: dummy @ ffff880116567598, expires @ 10003df60
+ * [  542.815046] dummy_alloc: dummy @ ffff880116567cc8, expires @ 10003e360
+ *
+ * T2 cleanup thread eventually finds a few expired dummies, frees them,
+ * and in the process leaks memory!
+ *
+ * [  543.711058] cleanup_thread:  jiffies = 10003b800
+ * [  543.711366] dummy_free: dummy @ ffff880112920458, expired = 10003b760
+ * [  543.711853] dummy_free: dummy @ ffff8801129219e8, expired = 10003b360
+ * [  543.712294] dummy_free: dummy @ ffff880112921e38, expired = 10003af60
+ * [  543.839046] dummy_alloc: dummy @ ffff8801165672b8, expires @ 10003e760
+ * [  544.863054] dummy_alloc: dummy @ ffff8801165665c8, expires @ 10003eb60
+ * [  545.887066] dummy_alloc: dummy @ ffff880119c42008, expires @ 10003ef60
+ * [  546.911046] dummy_alloc: dummy @ ffff88011aa05b58, expires @ 10003f360
+ * [  547.935048] dummy_alloc: dummy @ ffff8801150005c8, expires @ 10003f760
+ * [  548.959062] dummy_alloc: dummy @ ffff880113ee9e38, expires @ 10003fb60
+ *
+ *
+ * Fix the memory leak
+ * -------------------
+ *
+ * One way to fix this memory leak is to attach a shadow variable
+ * pointer to each dummy structure at its allocation point.  This
+ * use-case demonstrates a livepatch/shadow variable fix for short-lived
+ * data structures.
+ *
+ * In this example, existing dummy structures will unfortunately
+ * continue to leak memory, however once all of the dummies that were
+ * allocated before the live patch are retired, the memory leak will be
+ * closed.
+ *
+ * Load the livepatch fix1:
+ * $ insmod samples/livepatch/livepatch-shadow-fix1.ko
+ *
+ * T1 alloc thread wakes up and now calls a patched dummy_alloc() which
+ * saves the extra memory into a shadow variable:
+ *
+ * [  564.147027] livepatch: enabling patch 'livepatch_shadow_fix1'
+ * [  564.153922] livepatch: 'livepatch_shadow_fix1': patching...
+ * [  564.319052] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff880112af88a8, expires @ 100043760
+ * [  565.343060] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff880112af9708, expires @ 100043b60
+ * [  565.727039] livepatch: 'livepatch_shadow_fix1': patching complete
+ * [  566.367050] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff880112af8fd8, expires @ 100043f60
+ * [  567.391047] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff880112af9878, expires @ 100044360
+ *
+ * T2 cleanup thread calls a patched dummy_free() routine which retrives
+ * the shadow variable that saved the memory pointer.
+ *
+ * Note: Initially, memory will still be leaked as no shadow variables
+ * are found for dummy structures already created:
+ *
+ * [  568.287070] cleanup_thread:  jiffies = 100041800
+ * [  568.287492] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880113ee8738 leaked!
+ * [  568.288062] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880113ee9598 leaked!
+ * [  568.288644] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880113ee8178 leaked!
+ * [  568.289178] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880113ee8b88 leaked!
+ * [  568.289724] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880113ee9b58 leaked!
+ * [  568.290258] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880113ee82e8 leaked!
+ * [  568.415046] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff880112af8a18, expires @ 100044760
+ * [  569.439050] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff880112af85c8, expires @ 100044b60
+ * [  570.463049] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff880112af9148, expires @ 100044f60
+ * [  571.487022] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff880112af8458, expires @ 100045360
+ * [  572.511049] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff880112af99e8, expires @ 100045760
+ * [  573.535036] livepatch_shadow_fix1: livepatch_fix1_dummy_alloc: dummy @ ffff88011650e008, expires @ 100045b60
+ *
+ * T2 cleanup thread will eventually begin to reap dummy structures that
+ * do have an associated shadow variable.  Over time, this memory leak
+ * will be closed completely as all dummy structures will have a
+ * corresponding shadow variable tracking the extra allocated memory:
+ *
+ * [  580.575028] cleanup_thread:  jiffies = 100044800
+ * [  580.575675] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880112af8a18, prevented leak @ ffff880112b7c568
+ * [  580.576607] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880112af9878, prevented leak @ ffff880112b7d990
+ * [  580.577545] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880112af8fd8, prevented leak @ ffff880112b7c410
+ * [  580.578483] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880112af9708, prevented leak @ ffff880112b7dae8
+ * [  580.579327] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880112af88a8, prevented leak @ ffff880112b7c2b8
+ * [  580.580324] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff8801151bf148 leaked!
+ * ...
+ * [  586.719022] cleanup_thread:  jiffies = 100046000
+ * [  586.719648] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff88011650e738, prevented leak @ ffff880112b7c970
+ * [  586.720948] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff88011650e008, prevented leak @ ffff880112b7d588
+ * [  586.722215] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880112af99e8, prevented leak @ ffff880112b7c818
+ * [  586.723496] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880112af8458, prevented leak @ ffff880112b7d6e0
+ * [  586.724824] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880112af9148, prevented leak @ ffff880112b7c6c0
+ * [  586.726146] livepatch_shadow_fix1: livepatch_fix1_dummy_free: dummy @ ffff880112af85c8, prevented leak @ ffff880112b7d838
+ *
+ *
+ * Extend functionality
+ * --------------------
+ *
+ * Shadow variables can also be attached to in-flight dummy structures.
+ * In the second livepatch, use a shadow variable counter to keep track
+ * of the number of times a given dummy structure is inspected for
+ * expiration.
+ *
+ * Load the livepatch fix2 (on top of fix1):
+ * $ insmod samples/livepatch/livepatch-shadow-fix2.ko
+ *
+ * [  592.303611] livepatch: enabling patch 'livepatch_shadow_fix2'
+ * [  592.307458] livepatch: 'livepatch_shadow_fix2': patching...
+ *
+ * T2 cleanup thread calls a patched dummy_check(), which keeps a shadow
+ * variable counter of the number times it inspects a given dummy
+ * structure.  The final count is reported by a patched dummy_free().
+ *
+ * Note: initially the final count will be 1, as soon-to-expire dummies
+ * will only have had a shadow variable counter for a single pass
+ * through the alloc - check - cleanup cycle.  Overtime, newer dummies
+ * will have increased count values:
+ *
+ * [  592.863035] cleanup_thread:  jiffies = 100047800
+ * [  592.863355] livepatch_shadow_fix2: livepatch_fix2_dummy_free: dummy @ ffff880129d89b58, prevented leak @ ffff880112b7c2b8
+ * [  592.864100] livepatch_shadow_fix2: livepatch_fix2_dummy_free: dummy @ ffff880129d89b58, check counter = 1
+ * ...
+ * [  599.007047] cleanup_thread:  jiffies = 100049000
+ * [  599.007368] livepatch_shadow_fix2: livepatch_fix2_dummy_free: dummy @ ffff880112920cf8, prevented leak @ ffff880112b7d838
+ * [  599.008167] livepatch_shadow_fix2: livepatch_fix2_dummy_free: dummy @ ffff880112920cf8, check counter = 2
+ * ...
+ * [  605.151049] cleanup_thread:  jiffies = 10004a800
+ * [  605.151464] livepatch_shadow_fix2: livepatch_fix2_dummy_free: dummy @ ffff880113ee9b58, prevented leak @ ffff88011643c818
+ * [  605.152146] livepatch_shadow_fix2: livepatch_fix2_dummy_free: dummy @ ffff880113ee9b58, check counter = 2
+ * [  605.152783] livepatch_shadow_fix2: livepatch_fix2_dummy_free: dummy @ ffff880112920fd8, prevented leak @ ffff880112b7c970
+ * [  605.153490] livepatch_shadow_fix2: livepatch_fix2_dummy_free: dummy @ ffff880112920fd8, check counter = 3
+ * ...
+ *
+ *
+ * Cleanup
+ * -------
+ *
+ * $ 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");
+
+#define T1_PERIOD 1			/* allocator thread */
+#define T2_PERIOD (3 * T1_PERIOD)	/* cleanup thread */
+
+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;
+
+	/* Dummies live long enough to see a few t2 instances */
+	d->jiffies_expire = jiffies + 1000 * 4 * T2_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);
+}
+
+/*
+ * T1: alloc_thread allocates new dummy structures, allocates additional
+ *     memory, aptly named "leak", but doesn't keep permanent record of it.
+ */
+struct workqueue_struct *alloc_wq;
+struct delayed_work alloc_dwork;
+static void alloc_thread(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);
+
+	queue_delayed_work(alloc_wq, &alloc_dwork,
+		msecs_to_jiffies(1000 * T1_PERIOD));
+}
+
+/*
+ * T2: cleanup_thread frees dummy structures.  Without knownledge of "leak",
+ *     it leaks the additional memory that alloc_thread created.
+ */
+struct workqueue_struct *cleanup_wq;
+struct delayed_work cleanup_dwork;
+static void cleanup_thread(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);
+
+	queue_delayed_work(cleanup_wq, &cleanup_dwork,
+		msecs_to_jiffies(1000 * 2 * T2_PERIOD));
+}
+
+static int livepatch_shadow_mod_init(void)
+{
+	alloc_wq = create_singlethread_workqueue("klp_demo_alloc_wq");
+	if (!alloc_wq)
+		return -1;
+
+	cleanup_wq = create_singlethread_workqueue("klp_demo_cleanup_wq");
+	if (!cleanup_wq)
+		goto exit_free_alloc;
+
+	INIT_DELAYED_WORK(&alloc_dwork, alloc_thread);
+	queue_delayed_work(alloc_wq, &alloc_dwork, 1000 * T1_PERIOD);
+
+	INIT_DELAYED_WORK(&cleanup_dwork, cleanup_thread);
+	queue_delayed_work(cleanup_wq, &cleanup_dwork,
+		msecs_to_jiffies(1000 * T2_PERIOD));
+
+	return 0;
+
+exit_free_alloc:
+	destroy_workqueue(alloc_wq);
+
+	return -1;
+}
+
+static void livepatch_shadow_mod_exit(void)
+{
+	struct dummy *d, *tmp;
+
+	/* Cleanup T1 */
+	if (!cancel_delayed_work(&alloc_dwork))
+		flush_workqueue(alloc_wq);
+	destroy_workqueue(alloc_wq);
+
+	/* Cleanup T2 */
+	if (!cancel_delayed_work(&cleanup_dwork))
+		flush_workqueue(cleanup_wq);
+	destroy_workqueue(cleanup_wq);
+
+	/* 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 v2 2/2] livepatch: add shadow variable sample programs

[Petr Mladek]

On Wed 2017-06-28 11:37:27, Joe Lawrence wrote:
> diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
> new file mode 100644
> index 000000000000..423f4b7b0adb
> --- /dev/null
> +++ b/samples/livepatch/livepatch-shadow-mod.c
> + * Usage
> + * -----
> + *
> + * Fix the memory leak
> + * -------------------
> + *
> + * Extend functionality
> + * --------------------

When I made first quick look, I had troubles to understand that
these two sections were still part of the Usage section. Also
the commands how to enable the modules were hidden deep in the
expected output analyze.

I wonder if it would make sense to move most of the information
into the respective module sources. I actually missed some
more info in that modules. The few lines were hard to spot
after the long copyright/license blob.

> +
> +#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");
> +
> +#define T1_PERIOD 1			/* allocator thread */
> +#define T2_PERIOD (3 * T1_PERIOD)	/* cleanup thread */
> +
> +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;
> +
> +	/* Dummies live long enough to see a few t2 instances */
> +	d->jiffies_expire = jiffies + 1000 * 4 * T2_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);
> +}
> +
> +/*
> + * T1: alloc_thread allocates new dummy structures, allocates additional
> + *     memory, aptly named "leak", but doesn't keep permanent record of it.
> + */
> +struct workqueue_struct *alloc_wq;

A custom workqueue is needed only for special purposes.
IMHO, the standard system workqueue is perfectly fine
in our case. AFAIK, it is able to spawn/run about
256 worker threads and process this number of different
works in parallel.

> +struct delayed_work alloc_dwork;
> +static void alloc_thread(struct work_struct *work)

The suffix "_thread" is misleading. I think that alloc_work_func()
is the most descriptive name.


> +{
> +	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);
> +
> +	queue_delayed_work(alloc_wq, &alloc_dwork,
> +		msecs_to_jiffies(1000 * T1_PERIOD));
> +}
> +
> +static int livepatch_shadow_mod_init(void)
> +{
> +	alloc_wq = create_singlethread_workqueue("klp_demo_alloc_wq");
> +	if (!alloc_wq)
> +		return -1;
> +
> +	cleanup_wq = create_singlethread_workqueue("klp_demo_cleanup_wq");
> +	if (!cleanup_wq)
> +		goto exit_free_alloc;
> +
> +	INIT_DELAYED_WORK(&alloc_dwork, alloc_thread);
> +	queue_delayed_work(alloc_wq, &alloc_dwork, 1000 * T1_PERIOD);
> +
> +	INIT_DELAYED_WORK(&cleanup_dwork, cleanup_thread);
> +	queue_delayed_work(cleanup_wq, &cleanup_dwork,
> +		msecs_to_jiffies(1000 * T2_PERIOD));

If you use the system workqueue and DECLARE_DELAYED_WORK, you might
reduce this to:

       schedule_delayed_work(&alloc_dwork, 1000 * T1_PERIOD);
       schedule_delayed_work(&cleanup_dwork, msecs_to_jiffies(1000 * T2_PERIOD));


> +	return 0;
> +
> +exit_free_alloc:
> +	destroy_workqueue(alloc_wq);
> +
> +	return -1;
> +}
> +
> +static void livepatch_shadow_mod_exit(void)
> +{
> +	struct dummy *d, *tmp;
> +
> +	/* Cleanup T1 */
> +	if (!cancel_delayed_work(&alloc_dwork))
> +		flush_workqueue(alloc_wq);

You will get the same with

	cancel_delayed_work_sync(&alloc_dwork);

I am sorry, I spent too much time working on kthread worker API
and was not able to help myself. Also Tejun would put a shame
on me if I did not suggest this ;-)

Otherwise I like the examples.

Best Regards,
Petr

Re: [PATCH v2 2/2] livepatch: add shadow variable sample programs

[Joe Lawrence]

On Tue, Jul 18, 2017 at 04:47:45PM +0200, Petr Mladek wrote:
> Date:   Tue, 18 Jul 2017 16:47:45 +0200
> From: Petr Mladek <pmladek@suse.com>
> To: Joe Lawrence <joe.lawrence@redhat.com>
> Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh
>  Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@redhat.com>, Jiri Kosina
>  <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>
> Subject: Re: [PATCH v2 2/2] livepatch: add shadow variable sample programs
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Wed 2017-06-28 11:37:27, Joe Lawrence wrote:
> > diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
> > new file mode 100644
> > index 000000000000..423f4b7b0adb
> > --- /dev/null
> > +++ b/samples/livepatch/livepatch-shadow-mod.c
> > + * Usage
> > + * -----
> > + *
> > + * Fix the memory leak
> > + * -------------------
> > + *
> > + * Extend functionality
> > + * --------------------
> 
> When I made first quick look, I had troubles to understand that
> these two sections were still part of the Usage section.

I could double underline "Usage" and keep the single underlines for "Fix
the memory leak' and "Extend functionality" sections if that helps.

>                                                          Also
> the commands how to enable the modules were hidden deep in the
> expected output analyze.

Yeah, v2's examples were hard to summarize and describe through
comments.  The code is pretty simple, but the devil is in the timing
details and log output seemed the best way to illustrate the fixes.

Do you think the log entries are worth copying into these comments?  I
could simply describe the effect and let the reader generate their own
if they so desired.

> I wonder if it would make sense to move most of the information
> into the respective module sources. 

Are you suggesting that it would be clearer to distribute the
log + comments to each individual livepatch-shadow-*.c module file?

>                                     I actually missed some
> more info in that modules. The few lines were hard to spot
> after the long copyright/license blob.

Would a heading like this help separate the legalese from the patch
description?

/* [ ... GPL, copyrights, etc ... ]
 *
 * Module Description
 * ==================
 * [...]
 */
 
> > +
> > +#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");
> > +
> > +#define T1_PERIOD 1			/* allocator thread */
> > +#define T2_PERIOD (3 * T1_PERIOD)	/* cleanup thread */
> > +
> > +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;
> > +
> > +	/* Dummies live long enough to see a few t2 instances */
> > +	d->jiffies_expire = jiffies + 1000 * 4 * T2_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);
> > +}
> > +
> > +/*
> > + * T1: alloc_thread allocates new dummy structures, allocates additional
> > + *     memory, aptly named "leak", but doesn't keep permanent record of it.
> > + */
> > +struct workqueue_struct *alloc_wq;
> 
> A custom workqueue is needed only for special purposes.
> IMHO, the standard system workqueue is perfectly fine
> in our case. AFAIK, it is able to spawn/run about
> 256 worker threads and process this number of different
> works in parallel.

No problem.  This example executes infrequently and obviously isn't high
priority or anything.

> > +struct delayed_work alloc_dwork;
> > +static void alloc_thread(struct work_struct *work)
> 
> The suffix "_thread" is misleading. I think that alloc_work_func()
> is the most descriptive name.
> 

Noted for v3.

> > +{
> > +	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);
> > +
> > +	queue_delayed_work(alloc_wq, &alloc_dwork,
> > +		msecs_to_jiffies(1000 * T1_PERIOD));
> > +}
> > +
> > +static int livepatch_shadow_mod_init(void)
> > +{
> > +	alloc_wq = create_singlethread_workqueue("klp_demo_alloc_wq");
> > +	if (!alloc_wq)
> > +		return -1;
> > +
> > +	cleanup_wq = create_singlethread_workqueue("klp_demo_cleanup_wq");
> > +	if (!cleanup_wq)
> > +		goto exit_free_alloc;
> > +
> > +	INIT_DELAYED_WORK(&alloc_dwork, alloc_thread);
> > +	queue_delayed_work(alloc_wq, &alloc_dwork, 1000 * T1_PERIOD);
> > +
> > +	INIT_DELAYED_WORK(&cleanup_dwork, cleanup_thread);
> > +	queue_delayed_work(cleanup_wq, &cleanup_dwork,
> > +		msecs_to_jiffies(1000 * T2_PERIOD));
> 
> If you use the system workqueue and DECLARE_DELAYED_WORK, you might
> reduce this to:
> 
>        schedule_delayed_work(&alloc_dwork, 1000 * T1_PERIOD);
>        schedule_delayed_work(&cleanup_dwork, msecs_to_jiffies(1000 * T2_PERIOD));

Good cleanup for v3.

> > +	return 0;
> > +
> > +exit_free_alloc:
> > +	destroy_workqueue(alloc_wq);
> > +
> > +	return -1;
> > +}
> > +
> > +static void livepatch_shadow_mod_exit(void)
> > +{
> > +	struct dummy *d, *tmp;
> > +
> > +	/* Cleanup T1 */
> > +	if (!cancel_delayed_work(&alloc_dwork))
> > +		flush_workqueue(alloc_wq);
> 
> You will get the same with
> 
> 	cancel_delayed_work_sync(&alloc_dwork);
> 
> I am sorry, I spent too much time working on kthread worker API
> and was not able to help myself. Also Tejun would put a shame
> on me if I did not suggest this ;-)

No worries, thanks for the kthread worker tips, especially this last
one.  I'll incorporate them all in the next version.
 
> Otherwise I like the examples.

Funny that I had anticipated head-scratching over the convoluted example
instead of my workqueue usage :)

-- Joe

Re: [PATCH v2 2/2] livepatch: add shadow variable sample programs

[Petr Mladek]

On Tue 2017-07-18 15:15:00, Joe Lawrence wrote:
> On Tue, Jul 18, 2017 at 04:47:45PM +0200, Petr Mladek wrote:
> > Date:   Tue, 18 Jul 2017 16:47:45 +0200
> > From: Petr Mladek <pmladek@suse.com>
> > To: Joe Lawrence <joe.lawrence@redhat.com>
> > Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh
> >  Poimboeuf <jpoimboe@redhat.com>, Jessica Yu <jeyu@redhat.com>, Jiri Kosina
> >  <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>
> > Subject: Re: [PATCH v2 2/2] livepatch: add shadow variable sample programs
> > User-Agent: Mutt/1.5.21 (2010-09-15)
> > 
> > On Wed 2017-06-28 11:37:27, Joe Lawrence wrote:
> > > diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
> > > new file mode 100644
> > > index 000000000000..423f4b7b0adb
> > > --- /dev/null
> > > +++ b/samples/livepatch/livepatch-shadow-mod.c
> > > + * Usage
> > > + * -----
> > > + *
> > > + * Fix the memory leak
> > > + * -------------------
> > > + *
> > > + * Extend functionality
> > > + * --------------------
> > 
> > When I made first quick look, I had troubles to understand that
> > these two sections were still part of the Usage section.
> 
> I could double underline "Usage" and keep the single underlines for "Fix
> the memory leak' and "Extend functionality" sections if that helps.

I am not sure that it would make any big difference.


> > the commands how to enable the modules were hidden deep in the
> > expected output analyze.
> 
> Yeah, v2's examples were hard to summarize and describe through
> comments.  The code is pretty simple, but the devil is in the timing
> details and log output seemed the best way to illustrate the fixes.
> 
> Do you think the log entries are worth copying into these comments?  I
> could simply describe the effect and let the reader generate their own
> if they so desired.

The logs are fine but I would put them into separate sections/files.
I mean that I would redude the "Usage" section to

 * Load the buggy demonstration module:
 * $> insmod samples/livepatch/livepatch-shadow-mod.ko
 *
 * After xx seconds/minutes watch log messages
 * $> dmesg
 * And load the livepatch fix1:
 * $> insmod samples/livepatch/livepatch-shadow-fix1.ko
 ...

Then I would describe the expected effect and output in separate section.

> > I wonder if it would make sense to move most of the information
> > into the respective module sources. 
> 
> Are you suggesting that it would be clearer to distribute the
> log + comments to each individual livepatch-shadow-*.c module file?

Exactly. I am not 100% sure that it will be better but my guess
is that it might help.

> >                                     I actually missed some
> > more info in that modules. The few lines were hard to spot
> > after the long copyright/license blob.
> 
> Would a heading like this help separate the legalese from the patch
> description?

> /* [ ... GPL, copyrights, etc ... ]
>  *
>  * Module Description
>  * ==================
>  * [...]

This won't be needed if the description is longer and the expected
logs are part of it. IMHO, people want to compare the code and
the output anyway.

Anyway, this is a minor issue. I was not only able to descibe
it shortly. Do not worry much about it.

>  
> > Otherwise I like the examples.
> 
> Funny that I had anticipated head-scratching over the convoluted example
> instead of my workqueue usage :)

Heh, I can't think of anything better. I like that that it shows
usage for both klp_shadow_get() and klp_shadow_get_or_attach().

Best Regards,
Petr

Re: [PATCH v2 2/2] livepatch: add shadow variable sample programs

[Petr Mladek]

On Wed 2017-06-28 11:37:27, Joe Lawrence wrote:
> Add sample livepatch modules to demonstrate the shadow variable API.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
> new file mode 100644
> index 000000000000..423f4b7b0adb
> --- /dev/null
> +++ b/samples/livepatch/livepatch-shadow-mod.c
> +
> +#define T1_PERIOD 1			/* allocator thread */
> +#define T2_PERIOD (3 * T1_PERIOD)	/* cleanup thread */
> +
> +
> +	/* Dummies live long enough to see a few t2 instances */
> +	d->jiffies_expire = jiffies + 1000 * 4 * T2_PERIOD;

One more thing that I realized later. This will depend on HZ.
Also above defined periods are later multiplied by 4 or 2
so that it is not easy to understand and tune.

I would suggest to define the exact periods in ms on one
location:

#define ALLOC_PERIOD_MSEC  10
#define FREE_PERIOD_MSEC   (6 * ALLOC_PERIOD_MSEC)
#define EXPIRE_PERIOD_MSEC (24 * ALLOC_PERION_MSEC)

and then use msec_to_jiffies(XXX_PERIOD_MSEC) on
the respective locations.

Huh, I should reduce my alarm level. This is just
an example code after all.

Best Regards,
Petr