[PATCH 0/3] livepatch: add shadow variable API

[PATCH 0/3] livepatch: add shadow variable API

[Joe Lawrence]

This patchset is a simplified livepatch port of kpatch's "shadow"
variable API [1].  The kpatch project has successfully employed such
shadow variables to implement patches that have extended data structure
elements.  This API provides livepatch a means of associating new,
shadow data fields with existing data structures. 

See the first patch for the implementation, the second for further
documentation (API, conccurency notes, use-case code snippets) and the
third patch for an update to the sample livepatch module using shadow
variables.

[1] https://github.com/dynup/kpatch/blob/master/kmod/core/shadow.c

Joe Lawrence (3):
  livepatch: introduce shadow variable API
  livepatch: add shadow variable documentation
  livepatch: add shadow variable sample program

 Documentation/livepatch/shadow-vars.txt | 175 ++++++++++++++++++++++++++++++++
 include/linux/livepatch.h               |   4 +
 kernel/livepatch/Makefile               |   2 +-
 kernel/livepatch/shadow.c               | 115 +++++++++++++++++++++
 samples/livepatch/livepatch-sample.c    |  39 ++++++-
 5 files changed, 333 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/livepatch/shadow-vars.txt
 create mode 100644 kernel/livepatch/shadow.c

-- 
1.8.3.1

[PATCH 1/3] livepatch: introduce shadow variable API

[Joe Lawrence]

Add three exported API for livepatch modules:

  void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
  void klp_shadow_detach(void *obj, char *var);
  void *klp_shadow_get(void *obj, char *var);

that implement "shadow" variables, which allow callers to associate new
shadow fields to existing data structures.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 include/linux/livepatch.h |   4 ++
 kernel/livepatch/Makefile |   2 +-
 kernel/livepatch/shadow.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 kernel/livepatch/shadow.c

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..a0d068ecf7f8 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -164,6 +164,10 @@ static inline bool klp_have_reliable_stack(void)
 	       IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
 }
 
+void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
+void klp_shadow_detach(void *obj, char *var);
+void *klp_shadow_get(void *obj, char *var);
+
 #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..72d5e567dff9
--- /dev/null
+++ b/kernel/livepatch/shadow.c
@@ -0,0 +1,115 @@
+/*
+ * 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/>.
+ */
+
+#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 {
+	struct hlist_node node;
+	struct rcu_head rcu_head;
+	void *obj;
+	char *var;
+	void *data;
+};
+
+void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
+{
+	unsigned long flags;
+	struct klp_shadow *shadow;
+
+	shadow = kmalloc(sizeof(*shadow), gfp);
+	if (!shadow)
+		return NULL;
+
+	shadow->obj = obj;
+
+	shadow->var = kstrdup(var, gfp);
+	if (!shadow->var) {
+		kfree(shadow);
+		return NULL;
+	}
+
+	shadow->data = data;
+
+	spin_lock_irqsave(&klp_shadow_lock, flags);
+	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+
+	return shadow->data;
+}
+EXPORT_SYMBOL_GPL(klp_shadow_attach);
+
+static void klp_shadow_rcu_free(struct rcu_head *head)
+{
+	struct klp_shadow *shadow;
+
+	shadow = container_of(head, struct klp_shadow, rcu_head);
+
+	kfree(shadow->var);
+	kfree(shadow);
+}
+
+void klp_shadow_detach(void *obj, char *var)
+{
+	unsigned long flags;
+	struct klp_shadow *shadow;
+
+	spin_lock_irqsave(&klp_shadow_lock, flags);
+
+	hash_for_each_possible(klp_shadow_hash, shadow, node,
+			       (unsigned long)obj) {
+		if (shadow->obj == obj && !strcmp(shadow->var, var)) {
+			hash_del_rcu(&shadow->node);
+			spin_unlock_irqrestore(&klp_shadow_lock, flags);
+			call_rcu(&shadow->rcu_head, klp_shadow_rcu_free);
+			return;
+		}
+	}
+
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+}
+EXPORT_SYMBOL_GPL(klp_shadow_detach);
+
+void *klp_shadow_get(void *obj, char *var)
+{
+	struct klp_shadow *shadow;
+
+	rcu_read_lock();
+
+	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
+				   (unsigned long)obj) {
+		if (shadow->obj == obj && !strcmp(shadow->var, var)) {
+			rcu_read_unlock();
+			return shadow->data;
+		}
+	}
+
+	rcu_read_unlock();
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(klp_shadow_get);
-- 
1.8.3.1

[PATCH 2/3] livepatch: add shadow variable documentation

[Joe Lawrence]

Document 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 | 175 ++++++++++++++++++++++++++++++++
 1 file changed, 175 insertions(+)
 create mode 100644 Documentation/livepatch/shadow-vars.txt

diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
new file mode 100644
index 000000000000..7df99ade4615
--- /dev/null
+++ b/Documentation/livepatch/shadow-vars.txt
@@ -0,0 +1,175 @@
+Shadow Variables
+================
+
+Shadow variables are a simple way for livepatch modules to associate new
+"shadow" data to existing data structures.  Original data structures
+(both definition and storage) are left unmodified and "new" data is
+allocated separately.  A shadow variable hashtable associates a string
+key and a pointer to the original data with a pointer to the new data.
+
+
+API
+---
+
+void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
+
+  Description: Allocate and attach a new shadow variable.
+  Parameters:
+
+    void *obj  - pointer to original data
+    char *var  - string key describing new data
+    gfp_t gfp  - GFP flags used to allocate shadow variable metadata
+    void *data - pointer to new data
+
+  Returns: the shadow variable data element, otherwise NULL on failure.
+
+
+void klp_shadow_detach(void *obj, char *var);
+
+  Description: Detach and free a shadow variable.
+  Parameters:
+
+      void *obj  - pointer to original data
+      char *var  - string key describing new data
+
+
+void *klp_shadow_get(void *obj, char *var);
+
+  Description: Retrieve a shadow variable data pointer.
+  Parameters:
+
+      void *obj  - pointer to original data
+      char *var  - string key describing new data
+
+  Returns: the shadow variable data element, otherwise NULL if the
+  <obj, var> combination is not found.
+
+
+Concurrency notes:
+
+* The shadow variable API simply provides a relationship between an
+<obj, var> 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 data pointer.  Therefore, initialization of
+shadow data should be completed before attaching the shadow variable.
+
+* 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.
+
+
+Use cases
+---------
+
+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 via shadow variable is
+straightforward.
+
+Allocation - when a host sta_info structure is allocated, allocate a
+corresponding spinlock_t and attach it as a new shadow variable:
+
+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 = kzalloc(sizeof(*ps_lock), gfp);
+	if (!ps_lock)
+		goto free;
+	spin_lock_init(ps_lock);
+	if (!klp_shadow_attach(sta, "ps_lock", gfp, ps_lock))
+		goto shadow_fail;
+	...
+
+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(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, with no additional storage required.
+
+This implementation diverges from the original commit, as it can attach
+the shadow variable when the code actually uses it:
+
+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, "fi", GFP_ATOMIC, fi);
+	}
+
+	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 data value the shadow
+variable holds, its existence can be keyed off of to handle the data
+structure accordingly.
+
+
+References
+==========
+
+* https://github.com/dynup/kpatch
+The livepatch implementation is based on the kpatch version of shadow
+variables.
+
+* http://files.mkgnu.net/files/dynamos/doc/papers/dynamos_eurosys_07.pdf
+Dynamic and Adaptive Updates of Non-Quiescent Subsystems in Commodity
+Operating System Kernels (Kritis Makris, Kyung Dong Ryu 2007) presented
+a datatype update technique called "shadow data structures".
-- 
1.8.3.1

[PATCH 3/3] livepatch: add shadow variable sample program

[Joe Lawrence]

Modify the sample livepatch to demonstrate the shadow variable API.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 samples/livepatch/livepatch-sample.c | 39 +++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
index 84795223f15f..e0236750cefb 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -25,26 +25,57 @@
 
 /*
  * This (dumb) live patch overrides the function that prints the
- * kernel boot cmdline when /proc/cmdline is read.
+ * kernel boot cmdline when /proc/cmdline is read.  It also
+ * demonstrates a contrived shadow-variable usage.
  *
  * Example:
  *
  * $ cat /proc/cmdline
  * <your cmdline>
+ * current=<current task pointer> count=<shadow variable counter>
  *
  * $ insmod livepatch-sample.ko
  * $ cat /proc/cmdline
  * this has been live patched
+ * current=ffff8800331c9540 count=1
+ * $ cat /proc/cmdline
+ * this has been live patched
+ * current=ffff8800331c9540 count=2
  *
  * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
  * $ cat /proc/cmdline
  * <your cmdline>
  */
 
+static LIST_HEAD(shadow_list);
+
+struct task_ctr {
+	struct list_head list;
+	int count;
+};
+
 #include <linux/seq_file.h>
+#include <linux/slab.h>
 static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
 {
+	struct task_ctr *nd;
+
+	nd = klp_shadow_get(current, "task_ctr");
+	if (!nd) {
+		nd = kzalloc(sizeof(*nd), GFP_KERNEL);
+		if (nd) {
+			list_add(&nd->list, &shadow_list);
+			klp_shadow_attach(current, "task_ctr", GFP_KERNEL, nd);
+		}
+	}
+
 	seq_printf(m, "%s\n", "this has been live patched");
+
+	if (nd) {
+		nd->count++;
+		seq_printf(m, "current=%p count=%d\n", current, nd->count);
+	}
+
 	return 0;
 }
 
@@ -99,6 +130,12 @@ static int livepatch_init(void)
 
 static void livepatch_exit(void)
 {
+	struct task_ctr *nd, *tmp;
+
+	list_for_each_entry_safe(nd, tmp, &shadow_list, list) {
+		list_del(&nd->list);
+		kfree(nd);
+	}
 	WARN_ON(klp_unregister_patch(&patch));
 }
 
-- 
1.8.3.1

Re: [PATCH 0/3] livepatch: add shadow variable API

[Jiri Kosina]

On Thu, 1 Jun 2017, Joe Lawrence wrote:

> This patchset is a simplified livepatch port of kpatch's "shadow"
> variable API [1].  The kpatch project has successfully employed such
> shadow variables to implement patches that have extended data structure
> elements.  This API provides livepatch a means of associating new,
> shadow data fields with existing data structures. 
> 
> See the first patch for the implementation, the second for further
> documentation (API, conccurency notes, use-case code snippets) and the
> third patch for an update to the sample livepatch module using shadow
> variables.

Thanks a lot for initiating this. 

The only issue I've spotted so far -- is there any reason, why the API 
completely ignores task_struct->patch_state, and always returns the 'new' 
value?

This basically offloads the responsibility for deciding between old/new to 
each and every caller, and that feels much more error prone compared to 
having this automatically done by klp_shadow_get().

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/3] livepatch: add shadow variable API

[Joe Lawrence]

On 06/01/2017 04:05 PM, Jiri Kosina wrote:
> On Thu, 1 Jun 2017, Joe Lawrence wrote:
> 
>> This patchset is a simplified livepatch port of kpatch's "shadow"
>> variable API [1].  The kpatch project has successfully employed such
>> shadow variables to implement patches that have extended data structure
>> elements.  This API provides livepatch a means of associating new,
>> shadow data fields with existing data structures. 
>>
>> See the first patch for the implementation, the second for further
>> documentation (API, conccurency notes, use-case code snippets) and the
>> third patch for an update to the sample livepatch module using shadow
>> variables.
> 
> Thanks a lot for initiating this. 
> 
> The only issue I've spotted so far -- is there any reason, why the API 
> completely ignores task_struct->patch_state, and always returns the 'new' 
> value?
> 
> This basically offloads the responsibility for deciding between old/new to 
> each and every caller, and that feels much more error prone compared to 
> having this automatically done by klp_shadow_get().
> 

Hi Jiri,

I'm a little confused about the question.  Maybe this clarifies a few
things:

 * klp_shadow_get() is only returning a pointer to the shadow data, the
additional storage that klp_shadow_attach() has associated with the
original data structure.  Callers will have to handle this shadow
structure accordingly, ie, not through old_struct->new_value, but rather
*new_value).

 * the intention is that only livepatched code will be calling
klp_shadow_*, so it can assume that the current task is patched

 * callers might need to verify klp_shadow_get() is returning non-NULL
if it's possible that some data-structures don't have a shadow var attached

If you are referring to stacking livepatches ... to be honest I hadn't
thought of that scenario.  In that case, we might be able to get away
with pushing something like this into the hash:

 klp #1:  klp_shadow_attach(ptr, "shadow_var", ...)
 klp #2:  klp_shadow_attach(ptr, "shadow_var_v2", ...)

... but that's just off the top of my head :)  I was hoping to handle
the easy case first.

Maybe I misunderstood the question... if so, I can update the
documentation file to better describe what's going on.

Regards,

-- Joe

Re: [PATCH 0/3] livepatch: add shadow variable API

[Jiri Kosina]

On Thu, 1 Jun 2017, Joe Lawrence wrote:

>  * the intention is that only livepatched code will be calling
> klp_shadow_*, so it can assume that the current task is patched

Ah, okay, that fully answers my question, thanks. That makes it impossible 
though to apply the same technique to a single variable twice, without 
further tweaks (either versioning of the variables, or actually stacking 
the shadowing itself).

Honestly though, I don't think that's going to be a big issue in practice.

> Maybe I misunderstood the question... if so, I can update the 
> documentation file to better describe what's going on.

I think you didn't misunderstand it. But it might be beneficial to have a 
few additional explanatory words in the documentation nevertheless :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Josh Poimboeuf]

On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
> Add three exported API for livepatch modules:
> 
>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>   void klp_shadow_detach(void *obj, char *var);
>   void *klp_shadow_get(void *obj, char *var);
> 
> that implement "shadow" variables, which allow callers to associate new
> shadow fields to existing data structures.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

Overall the patch looks good to me.  It's a simple API but we've found
it to be very useful for certain patches.

One comment below:

> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
> +{
> +	unsigned long flags;
> +	struct klp_shadow *shadow;
> +
> +	shadow = kmalloc(sizeof(*shadow), gfp);
> +	if (!shadow)
> +		return NULL;
> +
> +	shadow->obj = obj;
> +
> +	shadow->var = kstrdup(var, gfp);
> +	if (!shadow->var) {
> +		kfree(shadow);
> +		return NULL;
> +	}
> +
> +	shadow->data = data;
> +
> +	spin_lock_irqsave(&klp_shadow_lock, flags);
> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return shadow->data;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);

I wonder if we should worry about people misusing the API by calling
klp_shadow_attach() for a shadow variable that already exists.  Maybe we
should add a check and return NULL if it already exists.

-- 
Josh

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

[Joe Lawrence]

On 06/08/2017 12:49 PM, Josh Poimboeuf wrote:
> On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
>> Add three exported API for livepatch modules:
>>
>>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>>   void klp_shadow_detach(void *obj, char *var);
>>   void *klp_shadow_get(void *obj, char *var);
>>
>> that implement "shadow" variables, which allow callers to associate new
>> shadow fields to existing data structures.
>>
>> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> 
> Overall the patch looks good to me.  It's a simple API but we've found
> it to be very useful for certain patches.
> 
> One comment below:
> 
>> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
>> +{
>> +	unsigned long flags;
>> +	struct klp_shadow *shadow;
>> +
>> +	shadow = kmalloc(sizeof(*shadow), gfp);
>> +	if (!shadow)
>> +		return NULL;
>> +
>> +	shadow->obj = obj;
>> +
>> +	shadow->var = kstrdup(var, gfp);
>> +	if (!shadow->var) {
>> +		kfree(shadow);
>> +		return NULL;
>> +	}
>> +
>> +	shadow->data = data;
>> +
>> +	spin_lock_irqsave(&klp_shadow_lock, flags);
>> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
>> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
>> +
>> +	return shadow->data;
>> +}
>> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> 
> I wonder if we should worry about people misusing the API by calling
> klp_shadow_attach() for a shadow variable that already exists.  Maybe we
> should add a check and return NULL if it already exists.
> 

I don't think the API (the shadow or the underlying hash table calls)
currently protects against double-adds...  adding a check to do so would
probably need to occur with the klp_shadow_lock to protect against
concurrent detach calls.

I could implement this protection in a v2, or leave it up to the caller.
 What do you think?

-- Joe

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

[Josh Poimboeuf]

On Fri, Jun 09, 2017 at 11:36:27AM -0400, Joe Lawrence wrote:
> On 06/08/2017 12:49 PM, Josh Poimboeuf wrote:
> > On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
> >> Add three exported API for livepatch modules:
> >>
> >>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
> >>   void klp_shadow_detach(void *obj, char *var);
> >>   void *klp_shadow_get(void *obj, char *var);
> >>
> >> that implement "shadow" variables, which allow callers to associate new
> >> shadow fields to existing data structures.
> >>
> >> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > 
> > Overall the patch looks good to me.  It's a simple API but we've found
> > it to be very useful for certain patches.
> > 
> > One comment below:
> > 
> >> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
> >> +{
> >> +	unsigned long flags;
> >> +	struct klp_shadow *shadow;
> >> +
> >> +	shadow = kmalloc(sizeof(*shadow), gfp);
> >> +	if (!shadow)
> >> +		return NULL;
> >> +
> >> +	shadow->obj = obj;
> >> +
> >> +	shadow->var = kstrdup(var, gfp);
> >> +	if (!shadow->var) {
> >> +		kfree(shadow);
> >> +		return NULL;
> >> +	}
> >> +
> >> +	shadow->data = data;
> >> +
> >> +	spin_lock_irqsave(&klp_shadow_lock, flags);
> >> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> >> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> >> +
> >> +	return shadow->data;
> >> +}
> >> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> > 
> > I wonder if we should worry about people misusing the API by calling
> > klp_shadow_attach() for a shadow variable that already exists.  Maybe we
> > should add a check and return NULL if it already exists.
> > 
> 
> I don't think the API (the shadow or the underlying hash table calls)
> currently protects against double-adds...  adding a check to do so would
> probably need to occur with the klp_shadow_lock to protect against
> concurrent detach calls.
> 
> I could implement this protection in a v2, or leave it up to the caller.
>  What do you think?

Yeah, I don't have a strong opinion either way.  It's fine with me to
leave it as it is and trust the patch author not to mess it up.

-- 
Josh

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Josh Poimboeuf]

On Thu, Jun 01, 2017 at 02:25:26PM -0400, Joe Lawrence wrote:
>  #include <linux/seq_file.h>
> +#include <linux/slab.h>
>  static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
>  {
> +	struct task_ctr *nd;
> +
> +	nd = klp_shadow_get(current, "task_ctr");
> +	if (!nd) {
> +		nd = kzalloc(sizeof(*nd), GFP_KERNEL);
> +		if (nd) {
> +			list_add(&nd->list, &shadow_list);
> +			klp_shadow_attach(current, "task_ctr", GFP_KERNEL, nd);
> +		}
> +	}
> +
>  	seq_printf(m, "%s\n", "this has been live patched");
> +
> +	if (nd) {
> +		nd->count++;
> +		seq_printf(m, "current=%p count=%d\n", current, nd->count);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -99,6 +130,12 @@ static int livepatch_init(void)
>  
>  static void livepatch_exit(void)
>  {
> +	struct task_ctr *nd, *tmp;
> +
> +	list_for_each_entry_safe(nd, tmp, &shadow_list, list) {
> +		list_del(&nd->list);
> +		kfree(nd);
> +	}
>  	WARN_ON(klp_unregister_patch(&patch));
>  }

What does 'nd' stand for?  I think a name like 'ctr' would be clearer.

-- 
Josh

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Joe Lawrence]

On 06/09/2017 02:38 PM, Josh Poimboeuf wrote:
> On Thu, Jun 01, 2017 at 02:25:26PM -0400, Joe Lawrence wrote:
>> [ ... snip ... ]  
>> @@ -99,6 +130,12 @@ static int livepatch_init(void)
>>  
>>  static void livepatch_exit(void)
>>  {
>> +	struct task_ctr *nd, *tmp;
>> +
>> +	list_for_each_entry_safe(nd, tmp, &shadow_list, list) {
>> +		list_del(&nd->list);
>> +		kfree(nd);
>> +	}
>>  	WARN_ON(klp_unregister_patch(&patch));
>>  }
> 
> What does 'nd' stand for?  I think a name like 'ctr' would be clearer.
> 

Ah, in an earlier draft (pre-post) that data structure was called
"new_data", so "nd" was simply a short hand abbreviation.  I'll update
for v2, thanks.

-- Joe

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

[Joe Lawrence]

On 06/09/2017 02:34 PM, Josh Poimboeuf wrote:
> On Fri, Jun 09, 2017 at 11:36:27AM -0400, Joe Lawrence wrote:
>> On 06/08/2017 12:49 PM, Josh Poimboeuf wrote:
>>> On Thu, Jun 01, 2017 at 02:25:24PM -0400, Joe Lawrence wrote:
>>>> Add three exported API for livepatch modules:
>>>>
>>>>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>>>>   void klp_shadow_detach(void *obj, char *var);
>>>>   void *klp_shadow_get(void *obj, char *var);
>>>>
>>>> that implement "shadow" variables, which allow callers to associate new
>>>> shadow fields to existing data structures.
>>>>
>>>> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
>>>
>>> Overall the patch looks good to me.  It's a simple API but we've found
>>> it to be very useful for certain patches.
>>>
>>> One comment below:
>>>
>>>> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
>>>> +{
>>>> +	unsigned long flags;
>>>> +	struct klp_shadow *shadow;
>>>> +
>>>> +	shadow = kmalloc(sizeof(*shadow), gfp);
>>>> +	if (!shadow)
>>>> +		return NULL;
>>>> +
>>>> +	shadow->obj = obj;
>>>> +
>>>> +	shadow->var = kstrdup(var, gfp);
>>>> +	if (!shadow->var) {
>>>> +		kfree(shadow);
>>>> +		return NULL;
>>>> +	}
>>>> +
>>>> +	shadow->data = data;
>>>> +
>>>> +	spin_lock_irqsave(&klp_shadow_lock, flags);
>>>> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
>>>> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
>>>> +
>>>> +	return shadow->data;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
>>>
>>> I wonder if we should worry about people misusing the API by calling
>>> klp_shadow_attach() for a shadow variable that already exists.  Maybe we
>>> should add a check and return NULL if it already exists.
>>>
>>
>> I don't think the API (the shadow or the underlying hash table calls)
>> currently protects against double-adds...  adding a check to do so would
>> probably need to occur with the klp_shadow_lock to protect against
>> concurrent detach calls.
>>
>> I could implement this protection in a v2, or leave it up to the caller.
>>  What do you think?
> 
> Yeah, I don't have a strong opinion either way.  It's fine with me to
> leave it as it is and trust the patch author not to mess it up.
> 

Without having encountered this in our kpatch shadow variable
experiences, I'm not sure which is better:

  1 - leave it as is, caller beware
  2 - return the existing shadow var
  3 - fail the double-add, differentiate failure with ERR_PTR

I'm leaning (1) to keep it simple, but since this an exported interface,
if any of the other options are any more "future-proof", I'd take that
into consideration.

-- Joe

Re: [PATCH 0/3] livepatch: add shadow variable API

[Miroslav Benes]

> If you are referring to stacking livepatches ... to be honest I hadn't
> thought of that scenario.  In that case, we might be able to get away
> with pushing something like this into the hash:
> 
>  klp #1:  klp_shadow_attach(ptr, "shadow_var", ...)
>  klp #2:  klp_shadow_attach(ptr, "shadow_var_v2", ...)

I thought this was the reason to have a string there. Otherwise, a 
pointer to original data would be enough, wouldn't it?

Miroslav

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Miroslav Benes]

On Thu, 1 Jun 2017, Joe Lawrence wrote:

> Modify the sample livepatch to demonstrate the shadow variable API.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  samples/livepatch/livepatch-sample.c | 39 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)

Would it make sense to make this our second sample module? I think we 
should keep one as simple as possible, and illustrate new features in 
separate sample modules.

I'd like to have another one for klp-convert patch set too.

What do you think?

Miroslav

Re: [PATCH 0/3] livepatch: add shadow variable API

[Joe Lawrence]

On 06/13/2017 06:19 AM, Miroslav Benes wrote:
>> If you are referring to stacking livepatches ... to be honest I hadn't
>> thought of that scenario.  In that case, we might be able to get away
>> with pushing something like this into the hash:
>>
>>  klp #1:  klp_shadow_attach(ptr, "shadow_var", ...)
>>  klp #2:  klp_shadow_attach(ptr, "shadow_var_v2", ...)
> 
> I thought this was the reason to have a string there. Otherwise, a 
> pointer to original data would be enough, wouldn't it?

Well, one could attach multiple shadow variables to the same data
structure, ie, one for each new data element.  In the stacking case, you
might add a spinlock in patch 1, then a linked-list in patch 2.  Patched
codepaths would then use klp_shadow_get(obj, "spinlock") or
klp_shadow_get(obj, "list") as needed.

Versioning shadow variables would be a bit more involved.  You'd have to
figure out if you A) convert existing shadow variables to the new format
on livepatch module load, or B) convert on the fly, or C) handle none,
v1, and v2 instances of the shadow variables.  /head spins

To be honest, I don't think we've never needed anything beyond basic
shadow variables in kpatch, so I'm only speculating about their
potential (ab)uses :)  That said, since this patchset is introducing the
API, it would be good to be reasonably flexible.

-- Joe

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Joe Lawrence]

On 06/13/2017 07:00 AM, Miroslav Benes wrote:
> On Thu, 1 Jun 2017, Joe Lawrence wrote:
> 
>> Modify the sample livepatch to demonstrate the shadow variable API.
>>
>> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
>> ---
>>  samples/livepatch/livepatch-sample.c | 39 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> Would it make sense to make this our second sample module? I think we 
> should keep one as simple as possible, and illustrate new features in 
> separate sample modules.
> 
> I'd like to have another one for klp-convert patch set too.
> 
> What do you think?
> 
> Miroslav
> 

Yeah, adding this as a new sample module is a good idea.  I'll split
that out in v2.

I'm not familiar with klp-convert, so I'll assume that will be handled
as its own patch(set).

Thanks,

-- Joe

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Miroslav Benes]

> I'm not familiar with klp-convert, so I'll assume that will be handled
> as its own patch(set).

Correct.

Miroslav

Re: [PATCH 0/3] livepatch: add shadow variable API

[Miroslav Benes]

On Tue, 13 Jun 2017, Joe Lawrence wrote:

> On 06/13/2017 06:19 AM, Miroslav Benes wrote:
> >> If you are referring to stacking livepatches ... to be honest I hadn't
> >> thought of that scenario.  In that case, we might be able to get away
> >> with pushing something like this into the hash:
> >>
> >>  klp #1:  klp_shadow_attach(ptr, "shadow_var", ...)
> >>  klp #2:  klp_shadow_attach(ptr, "shadow_var_v2", ...)
> > 
> > I thought this was the reason to have a string there. Otherwise, a 
> > pointer to original data would be enough, wouldn't it?
> 
> Well, one could attach multiple shadow variables to the same data
> structure, ie, one for each new data element.  In the stacking case, you
> might add a spinlock in patch 1, then a linked-list in patch 2.  Patched
> codepaths would then use klp_shadow_get(obj, "spinlock") or
> klp_shadow_get(obj, "list") as needed.

Ok, I mixed two different things into one. Yes, this is a valid use case.

> Versioning shadow variables would be a bit more involved.  You'd have to
> figure out if you A) convert existing shadow variables to the new format
> on livepatch module load, or B) convert on the fly, or C) handle none,
> v1, and v2 instances of the shadow variables.  /head spins

I'm gonna pretend I didn't read this.

> To be honest, I don't think we've never needed anything beyond basic
> shadow variables in kpatch, so I'm only speculating about their
> potential (ab)uses :)  That said, since this patchset is introducing the
> API, it would be good to be reasonably flexible.

I'd worry about that later. If we ever come upon that.

Thanks,
Miroslav

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

[Petr Mladek]

On Thu 2017-06-01 14:25:24, Joe Lawrence wrote:
> Add three exported API for livepatch modules:
> 
>   void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
>   void klp_shadow_detach(void *obj, char *var);
>   void *klp_shadow_get(void *obj, char *var);
> 
> that implement "shadow" variables, which allow callers to associate new
> shadow fields to existing data structures.

It would be great to explain what shadow variables mean
here. Alternatively I would sqash the 2nd patch into this one.

People might use git blame to search what this API/code is for
and this commit not give much hints.

> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> new file mode 100644
> index 000000000000..72d5e567dff9
> --- /dev/null
> +++ b/kernel/livepatch/shadow.c
> @@ -0,0 +1,115 @@
> +/*
> + * 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/>.
> + */
> +
> +#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);
> +

Please, add comment that will explain the structure members.
See include/linux/livepatch.h for explanation.

> +struct klp_shadow {
> +	struct hlist_node node;
> +	struct rcu_head rcu_head;
> +	void *obj;
> +	char *var;
> +	void *data;

I would make the meaning more obvious. What about renaming?

  var -> key or id
  data -> shadow_obj or new_obj

> +};
> +

Also the function would deserve a comment explaining the meaning
and parameters.

> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data)
> +{
> +	unsigned long flags;
> +	struct klp_shadow *shadow;
> +
> +	shadow = kmalloc(sizeof(*shadow), gfp);
> +	if (!shadow)
> +		return NULL;
> +
> +	shadow->obj = obj;
> +
> +	shadow->var = kstrdup(var, gfp);

I would use int or long instead of "char *". You do not know
in which context the API will be used. strdup() might be
unnecessarily expensive. You could always use enum or #define
to get readable names for the key.


> +	if (!shadow->var) {
> +		kfree(shadow);
> +		return NULL;
> +	}
> +
> +	shadow->data = data;
> +
> +	spin_lock_irqsave(&klp_shadow_lock, flags);
> +	hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> +	spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +
> +	return shadow->data;
> +}
> +EXPORT_SYMBOL_GPL(klp_shadow_attach);
> +
> +static void klp_shadow_rcu_free(struct rcu_head *head)
> +{
> +	struct klp_shadow *shadow;
> +
> +	shadow = container_of(head, struct klp_shadow, rcu_head);
> +
> +	kfree(shadow->var);
> +	kfree(shadow);

If we use int/long instead of the string, we will do not
need a custom free function. free_rcu() will work then.

> +}
> +
> +void klp_shadow_detach(void *obj, char *var)
> +{
> +	unsigned long flags;
> +	struct klp_shadow *shadow;
> +
> +	spin_lock_irqsave(&klp_shadow_lock, flags);
> +
> +	hash_for_each_possible(klp_shadow_hash, shadow, node,
> +			       (unsigned long)obj) {
> +		if (shadow->obj == obj && !strcmp(shadow->var, var)) {

Do we need to test "shadow->obj == obj" here? If it is not true,
there would be a bug in the hashtable implementation or in
klp_shadow_attach().

Well, it might make sense to add a consistency check:

      WARN_ON(shadow->obj != obj);


> +			hash_del_rcu(&shadow->node);
> +			spin_unlock_irqrestore(&klp_shadow_lock, flags);
> +			call_rcu(&shadow->rcu_head, klp_shadow_rcu_free);

call_rcu() just queues the request. It does not wait for it.
It can be called inside the lock and the code might
be easier:

			hash_del_rcu(&shadow->node);
			call_rcu(&shadow->rcu_head,
				  klp_shadow_rcu_free);
			break;
		}
	}
	spin_unlock_irqrestore(&klp_shadow_lock, flags);
}


Otherwise, I like that the API rather trivial and still useful.

Best Regards,
Petr

Re: [PATCH 2/3] livepatch: add shadow variable documentation

[Petr Mladek]

On Thu 2017-06-01 14:25:25, Joe Lawrence wrote:
> Document 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 | 175 ++++++++++++++++++++++++++++++++
>  1 file changed, 175 insertions(+)
>  create mode 100644 Documentation/livepatch/shadow-vars.txt
> 
> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
> new file mode 100644
> index 000000000000..7df99ade4615
> --- /dev/null
> +++ b/Documentation/livepatch/shadow-vars.txt
> +API
> +---
> +
> +void *klp_shadow_attach(void *obj, char *var, gfp_t gfp, void *data);
> +
> +  Description: Allocate and attach a new shadow variable.
> +  Parameters:
> +
> +    void *obj  - pointer to original data
> +    char *var  - string key describing new data
> +    gfp_t gfp  - GFP flags used to allocate shadow variable metadata
> +    void *data - pointer to new data
> +
> +  Returns: the shadow variable data element, otherwise NULL on failure.
> +
> +
> +void klp_shadow_detach(void *obj, char *var);
> +
> +  Description: Detach and free a shadow variable.
> +  Parameters:
> +
> +      void *obj  - pointer to original data
> +      char *var  - string key describing new data
> +
> +
> +void *klp_shadow_get(void *obj, char *var);
> +
> +  Description: Retrieve a shadow variable data pointer.
> +  Parameters:
> +
> +      void *obj  - pointer to original data
> +      char *var  - string key describing new data
> +
> +  Returns: the shadow variable data element, otherwise NULL if the
> +  <obj, var> combination is not found.

I would convert all the above into comments of the function definitions
in shadow.c. It is really helpful when you try to undestand the code
using cscope. Also it is much easier to keep the documentation
and code in sync when they are in the same file.

> +
> +Concurrency notes:
> +
> +* The shadow variable API simply provides a relationship between an
> +<obj, var> 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 data pointer.  Therefore, initialization of
> +shadow data should be completed before attaching the shadow variable.
> +
> +* 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.

I would prefer to have this in the source file as well.

Otherwise, I like the descriptions and the examples.

Best Regards,
Petr

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

[Josh Poimboeuf]

On Wed, Jun 14, 2017 at 02:59:13PM +0200, Petr Mladek wrote:
> > +}
> > +
> > +void klp_shadow_detach(void *obj, char *var)
> > +{
> > +	unsigned long flags;
> > +	struct klp_shadow *shadow;
> > +
> > +	spin_lock_irqsave(&klp_shadow_lock, flags);
> > +
> > +	hash_for_each_possible(klp_shadow_hash, shadow, node,
> > +			       (unsigned long)obj) {
> > +		if (shadow->obj == obj && !strcmp(shadow->var, var)) {
> 
> Do we need to test "shadow->obj == obj" here? If it is not true,
> there would be a bug in the hashtable implementation or in
> klp_shadow_attach().
> 
> Well, it might make sense to add a consistency check:
> 
>       WARN_ON(shadow->obj != obj);
> 

It would make sense if hash_for_each_possible() worked that way, but for
some reason it doesn't. :-/  It gives you all the hash collisions.

-- 
Josh

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Petr Mladek]

On Thu 2017-06-01 14:25:26, Joe Lawrence wrote:
> Modify the sample livepatch to demonstrate the shadow variable API.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  samples/livepatch/livepatch-sample.c | 39 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> index 84795223f15f..e0236750cefb 100644
> --- a/samples/livepatch/livepatch-sample.c
> +++ b/samples/livepatch/livepatch-sample.c
> @@ -25,26 +25,57 @@
>  
>  /*
>   * This (dumb) live patch overrides the function that prints the
> - * kernel boot cmdline when /proc/cmdline is read.
> + * kernel boot cmdline when /proc/cmdline is read.  It also
> + * demonstrates a contrived shadow-variable usage.
>   *
>   * Example:
>   *
>   * $ cat /proc/cmdline
>   * <your cmdline>
> + * current=<current task pointer> count=<shadow variable counter>
>   *
>   * $ insmod livepatch-sample.ko
>   * $ cat /proc/cmdline
>   * this has been live patched
> + * current=ffff8800331c9540 count=1
> + * $ cat /proc/cmdline
> + * this has been live patched
> + * current=ffff8800331c9540 count=2
>   *
>   * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
>   * $ cat /proc/cmdline
>   * <your cmdline>
>   */
>  
> +static LIST_HEAD(shadow_list);
> +
> +struct task_ctr {
> +	struct list_head list;
> +	int count;
> +};
> +
>  #include <linux/seq_file.h>
> +#include <linux/slab.h>
>  static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
>  {
> +	struct task_ctr *nd;
> +
> +	nd = klp_shadow_get(current, "task_ctr");
> +	if (!nd) {
> +		nd = kzalloc(sizeof(*nd), GFP_KERNEL);
> +		if (nd) {
> +			list_add(&nd->list, &shadow_list);
> +			klp_shadow_attach(current, "task_ctr", GFP_KERNEL, nd);
> +		}
> +	}

Hmm, this looks racy:

CPU0				CPU1

klp_shadow_get()		klp_shadow_get()
# returns NULL			# returns NULL
if (!nd) {			if (!nd) {
  nd = kzalloc();		  nd = kzalloc();
  list_add(&nd->list,		  list_add(&nd->list,
	   &shadow_list);		&shadow_list);

BANG: This is one race. We add items into the shared shadow_list
in parallel. The question is if we need it. IMHO, the API
could help here, see the comments for livepatch_exit() below.

  klp_shadow_attach();		klp_shadow_attach();

WARNING: This is fine because the shadow objects are for
different objects. I mean that "current" must point
to different processes on the two CPUs.

But it is racy in general. The question is if the API
could help here. A possibility might be to allow to
define a callback function that would create the shadow
structure when it does not exist. I mean something like

typedef void (*klp_shadow_create_obj_func_t)(void * obj);

void *klp_shadow_get_or_create(void *obj, int key, gfp_t gfp,
				klp_shadow_create_obj_fun_t *create)
{
	struct klp_shadow *shadow;

	shadow = klp_shadow_get(obj, key);

	if (!shadow && create) {
		void *shadow_obj;

		spin_lock_irqsave(&klp_shadow_lock, flags);
		shadow = klp_shadow_get(obj, key);
		if (shadow)
			goto out;

		shadow_obj = create(obj);
		shadow = __klp_shadow_attach(obj, key, gfp,
					shadow_obj);
out:
		spin_unlock_irqrestore(&klp_shadow_lock, flags);
	}

	return shadow;
}

I do not know. Maybe it is too ugly. Or will it safe a duplicated code
in many cases?

>  	seq_printf(m, "%s\n", "this has been live patched");
> +
> +	if (nd) {
> +		nd->count++;
> +		seq_printf(m, "current=%p count=%d\n", current, nd->count);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -99,6 +130,12 @@ static int livepatch_init(void)
>  
>  static void livepatch_exit(void)
>  {
> +	struct task_ctr *nd, *tmp;
> +
> +	list_for_each_entry_safe(nd, tmp, &shadow_list, list) {
> +		list_del(&nd->list);
> +		kfree(nd);

I think that this will be a rather common operation. Do we need
the extra list? It might be enough to delete all entries
in the hash table with a given key. Well, we might need
a callback again:

typedef void (*klp_shadow_free_obj_func_t)(void *shadow_obj);

void klp_shadow_free_all(int key, klp_shadow_free_obj_fun_t *shadow_free)
{
	int i;
	struct klp_shadow *shadow;

	spin_lock_irqsave(&klp_shadow_lock, flags);

	hash_for_each(klp_shadow_hash, i, shadow, node) {
			hash_del_rcu(&shadow->node);
			/* patch and shadow data are not longer used. */
			shadow_free(shadow->shadow_obj);
			/*
			 * shadow structure might still be traversed
			 * by someone
			 */
			kfree_rcu(shadow, rcu_head);
		}
	}

	spin_unlocklock_irqrestore(&klp_shadow_lock, flags);
}

Best Regards,
Petr

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

[Petr Mladek]

On Wed 2017-06-14 08:17:44, Josh Poimboeuf wrote:
> On Wed, Jun 14, 2017 at 02:59:13PM +0200, Petr Mladek wrote:
> > > +}
> > > +
> > > +void klp_shadow_detach(void *obj, char *var)
> > > +{
> > > +	unsigned long flags;
> > > +	struct klp_shadow *shadow;
> > > +
> > > +	spin_lock_irqsave(&klp_shadow_lock, flags);
> > > +
> > > +	hash_for_each_possible(klp_shadow_hash, shadow, node,
> > > +			       (unsigned long)obj) {
> > > +		if (shadow->obj == obj && !strcmp(shadow->var, var)) {
> > 
> > Do we need to test "shadow->obj == obj" here? If it is not true,
> > there would be a bug in the hashtable implementation or in
> > klp_shadow_attach().
> > 
> > Well, it might make sense to add a consistency check:
> > 
> >       WARN_ON(shadow->obj != obj);
> > 
> 
> It would make sense if hash_for_each_possible() worked that way, but for
> some reason it doesn't. :-/  It gives you all the hash collisions.

I see. Shame on me. The original code makes perfect sense then.

Best Regards,
Petr

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Josh Poimboeuf]

On Wed, Jun 14, 2017 at 04:21:02PM +0200, Petr Mladek wrote:
> On Thu 2017-06-01 14:25:26, Joe Lawrence wrote:
> > Modify the sample livepatch to demonstrate the shadow variable API.
> > 
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> >  samples/livepatch/livepatch-sample.c | 39 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> > index 84795223f15f..e0236750cefb 100644
> > --- a/samples/livepatch/livepatch-sample.c
> > +++ b/samples/livepatch/livepatch-sample.c
> > @@ -25,26 +25,57 @@
> >  
> >  /*
> >   * This (dumb) live patch overrides the function that prints the
> > - * kernel boot cmdline when /proc/cmdline is read.
> > + * kernel boot cmdline when /proc/cmdline is read.  It also
> > + * demonstrates a contrived shadow-variable usage.
> >   *
> >   * Example:
> >   *
> >   * $ cat /proc/cmdline
> >   * <your cmdline>
> > + * current=<current task pointer> count=<shadow variable counter>
> >   *
> >   * $ insmod livepatch-sample.ko
> >   * $ cat /proc/cmdline
> >   * this has been live patched
> > + * current=ffff8800331c9540 count=1
> > + * $ cat /proc/cmdline
> > + * this has been live patched
> > + * current=ffff8800331c9540 count=2
> >   *
> >   * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
> >   * $ cat /proc/cmdline
> >   * <your cmdline>
> >   */
> >  
> > +static LIST_HEAD(shadow_list);
> > +
> > +struct task_ctr {
> > +	struct list_head list;
> > +	int count;
> > +};
> > +
> >  #include <linux/seq_file.h>
> > +#include <linux/slab.h>
> >  static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
> >  {
> > +	struct task_ctr *nd;
> > +
> > +	nd = klp_shadow_get(current, "task_ctr");
> > +	if (!nd) {
> > +		nd = kzalloc(sizeof(*nd), GFP_KERNEL);
> > +		if (nd) {
> > +			list_add(&nd->list, &shadow_list);
> > +			klp_shadow_attach(current, "task_ctr", GFP_KERNEL, nd);
> > +		}
> > +	}
> 
> Hmm, this looks racy:
> 
> CPU0				CPU1
> 
> klp_shadow_get()		klp_shadow_get()
> # returns NULL			# returns NULL
> if (!nd) {			if (!nd) {
>   nd = kzalloc();		  nd = kzalloc();
>   list_add(&nd->list,		  list_add(&nd->list,
> 	   &shadow_list);		&shadow_list);
> 
> BANG: This is one race. We add items into the shared shadow_list
> in parallel. The question is if we need it. IMHO, the API
> could help here, see the comments for livepatch_exit() below.
> 
>   klp_shadow_attach();		klp_shadow_attach();
> 
> WARNING: This is fine because the shadow objects are for
> different objects. I mean that "current" must point
> to different processes on the two CPUs.
> 
> But it is racy in general. The question is if the API
> could help here. A possibility might be to allow to
> define a callback function that would create the shadow
> structure when it does not exist. I mean something like
> 
> typedef void (*klp_shadow_create_obj_func_t)(void * obj);
> 
> void *klp_shadow_get_or_create(void *obj, int key, gfp_t gfp,
> 				klp_shadow_create_obj_fun_t *create)
> {
> 	struct klp_shadow *shadow;
> 
> 	shadow = klp_shadow_get(obj, key);
> 
> 	if (!shadow && create) {
> 		void *shadow_obj;
> 
> 		spin_lock_irqsave(&klp_shadow_lock, flags);
> 		shadow = klp_shadow_get(obj, key);
> 		if (shadow)
> 			goto out;
> 
> 		shadow_obj = create(obj);
> 		shadow = __klp_shadow_attach(obj, key, gfp,
> 					shadow_obj);
> out:
> 		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> 	}
> 
> 	return shadow;
> }
> 
> I do not know. Maybe it is too ugly. Or will it safe a duplicated code
> in many cases?

I think this sample module is confusing because it uses the API in a
contrived way.  In reality, we use it more like the API documentation
describes: klp_shadow_attach() is called right after the parent struct
is allocated and klp_shadow_detach() is called right before the parent
struct is freed.  So the above race wouldn't normally exist.

I think Joe implemented it this way in order to keep it simple, so it
wouldn't have to use kallsyms to do manual relocations, etc.  But maybe
a more realistic example would be better since it represents how things
should really be done in the absence of out-of-tree tooling like
kpatch-build or klp-convert.

> >  	seq_printf(m, "%s\n", "this has been live patched");
> > +
> > +	if (nd) {
> > +		nd->count++;
> > +		seq_printf(m, "current=%p count=%d\n", current, nd->count);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -99,6 +130,12 @@ static int livepatch_init(void)
> >  
> >  static void livepatch_exit(void)
> >  {
> > +	struct task_ctr *nd, *tmp;
> > +
> > +	list_for_each_entry_safe(nd, tmp, &shadow_list, list) {
> > +		list_del(&nd->list);
> > +		kfree(nd);
> 
> I think that this will be a rather common operation. Do we need
> the extra list? It might be enough to delete all entries
> in the hash table with a given key. Well, we might need
> a callback again:
> 
> typedef void (*klp_shadow_free_obj_func_t)(void *shadow_obj);
> 
> void klp_shadow_free_all(int key, klp_shadow_free_obj_fun_t *shadow_free)
> {
> 	int i;
> 	struct klp_shadow *shadow;
> 
> 	spin_lock_irqsave(&klp_shadow_lock, flags);
> 
> 	hash_for_each(klp_shadow_hash, i, shadow, node) {
> 			hash_del_rcu(&shadow->node);
> 			/* patch and shadow data are not longer used. */
> 			shadow_free(shadow->shadow_obj);
> 			/*
> 			 * shadow structure might still be traversed
> 			 * by someone
> 			 */
> 			kfree_rcu(shadow, rcu_head);
> 		}
> 	}
> 
> 	spin_unlocklock_irqrestore(&klp_shadow_lock, flags);
> }

I often wonder whether it's really a good idea to even allow the
unloading of patch modules at all.  It adds complexity to the livepatch
code.  Is it worth it?  I don't have an answer but I'd be interested in
other people's opinion.

-- 
Josh

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Joe Lawrence]

On 06/14/2017 10:21 AM, Petr Mladek wrote:
> On Thu 2017-06-01 14:25:26, Joe Lawrence wrote:
>> Modify the sample livepatch to demonstrate the shadow variable API.
>>
>> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
>> ---
>>  samples/livepatch/livepatch-sample.c | 39 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
>> index 84795223f15f..e0236750cefb 100644
>> --- a/samples/livepatch/livepatch-sample.c
>> +++ b/samples/livepatch/livepatch-sample.c
>> @@ -25,26 +25,57 @@
>>  
>>  /*
>>   * This (dumb) live patch overrides the function that prints the
>> - * kernel boot cmdline when /proc/cmdline is read.
>> + * kernel boot cmdline when /proc/cmdline is read.  It also
>> + * demonstrates a contrived shadow-variable usage.
>>   *
>>   * Example:
>>   *
>>   * $ cat /proc/cmdline
>>   * <your cmdline>
>> + * current=<current task pointer> count=<shadow variable counter>
>>   *
>>   * $ insmod livepatch-sample.ko
>>   * $ cat /proc/cmdline
>>   * this has been live patched
>> + * current=ffff8800331c9540 count=1
>> + * $ cat /proc/cmdline
>> + * this has been live patched
>> + * current=ffff8800331c9540 count=2
>>   *
>>   * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
>>   * $ cat /proc/cmdline
>>   * <your cmdline>
>>   */
>>  
>> +static LIST_HEAD(shadow_list);
>> +
>> +struct task_ctr {
>> +	struct list_head list;
>> +	int count;
>> +};
>> +
>>  #include <linux/seq_file.h>
>> +#include <linux/slab.h>
>>  static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
>>  {
>> +	struct task_ctr *nd;
>> +
>> +	nd = klp_shadow_get(current, "task_ctr");
>> +	if (!nd) {
>> +		nd = kzalloc(sizeof(*nd), GFP_KERNEL);
>> +		if (nd) {
>> +			list_add(&nd->list, &shadow_list);
>> +			klp_shadow_attach(current, "task_ctr", GFP_KERNEL, nd);
>> +		}
>> +	}
> 
> Hmm, this looks racy:
> 
> CPU0				CPU1
> 
> klp_shadow_get()		klp_shadow_get()
> # returns NULL			# returns NULL
> if (!nd) {			if (!nd) {
>   nd = kzalloc();		  nd = kzalloc();
>   list_add(&nd->list,		  list_add(&nd->list,
> 	   &shadow_list);		&shadow_list);
> 
> BANG: This is one race. We add items into the shared shadow_list
> in parallel. The question is if we need it. IMHO, the API
> could help here, see the comments for livepatch_exit() below.
> 
>   klp_shadow_attach();		klp_shadow_attach();
> 
> WARNING: This is fine because the shadow objects are for
> different objects. I mean that "current" must point
> to different processes on the two CPUs.
> 
> But it is racy in general.

Good catch.  Let me add a disclaimer here and state that this example is
definitely contrived as I was trying to minimize its size.  Applying
shadow variables to a more real life use case would drag in a bunch of
"changed" function dependencies.  I didn't want to work around those
with a pile of kallsyms workarounds.  It did lead you to ask an
interesting question though:

>                            The question is if the API
> could help here. A possibility might be to allow to
> define a callback function that would create the shadow
> structure when it does not exist. I mean something like
> 
> typedef void (*klp_shadow_create_obj_func_t)(void * obj);
> 
> void *klp_shadow_get_or_create(void *obj, int key, gfp_t gfp,
> 				klp_shadow_create_obj_fun_t *create)
> {
> 	struct klp_shadow *shadow;
> 
> 	shadow = klp_shadow_get(obj, key);
> 
> 	if (!shadow && create) {
> 		void *shadow_obj;
> 
> 		spin_lock_irqsave(&klp_shadow_lock, flags);
> 		shadow = klp_shadow_get(obj, key);
> 		if (shadow)
> 			goto out;
> 
> 		shadow_obj = create(obj);
> 		shadow = __klp_shadow_attach(obj, key, gfp,
> 					shadow_obj);
> out:
> 		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> 	}
> 
> 	return shadow;
> }
> 
> I do not know. Maybe it is too ugly. Or will it safe a duplicated code
> in many cases?

In the kpatch experience, we've typically created shadow variables when
new host variables are allocated.  That means that patched code must
handle instances of host variables with and without their shadow
counterparts.  It also avoids the race scenario above since we're not
calling klp_shadow_attach for the same <obj,key> concurrently.

That said, I think we may have written a few test patches that create
new shadow vars if klp_shadow_get fails.

A callback would eliminate a potentially racy klp_shadow_get +
klp_shadow_attach combination.  One wrinkle would be initialization.
Once the callback creates the shadow variable, any subsequent
klp_shadow_get may return it...

Perhaps it would be sufficient to add additional argument to
klp_shadow_get_or_create that gets passed to the create() function?

>>  	seq_printf(m, "%s\n", "this has been live patched");
>> +
>> +	if (nd) {
>> +		nd->count++;
>> +		seq_printf(m, "current=%p count=%d\n", current, nd->count);
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -99,6 +130,12 @@ static int livepatch_init(void)
>>  
>>  static void livepatch_exit(void)
>>  {
>> +	struct task_ctr *nd, *tmp;
>> +
>> +	list_for_each_entry_safe(nd, tmp, &shadow_list, list) {
>> +		list_del(&nd->list);
>> +		kfree(nd);
> 
> I think that this will be a rather common operation. Do we need
> the extra list? It might be enough to delete all entries
> in the hash table with a given key. Well, we might need
> a callback again:

Sorry to drop the disclaimer here again, but this was required by the
contrived example... in kpatch experience, just like we usually spot the
klp_shadow_attach to the host allocation code, we do the same for when
releasing both.  If I had done that here, I would have needed to drag in
a lot more code in order to resolve all the symbols (maybe needing
kallsym lookups :(

> typedef void (*klp_shadow_free_obj_func_t)(void *shadow_obj);
> 
> void klp_shadow_free_all(int key, klp_shadow_free_obj_fun_t *shadow_free)
> {
> 	int i;
> 	struct klp_shadow *shadow;
> 
> 	spin_lock_irqsave(&klp_shadow_lock, flags);
> 
> 	hash_for_each(klp_shadow_hash, i, shadow, node) {
> 			hash_del_rcu(&shadow->node);
> 			/* patch and shadow data are not longer used. */
> 			shadow_free(shadow->shadow_obj);
> 			/*
> 			 * shadow structure might still be traversed
> 			 * by someone
> 			 */
> 			kfree_rcu(shadow, rcu_head);
> 		}
> 	}
> 
> 	spin_unlocklock_irqrestore(&klp_shadow_lock, flags);
> }
> 
> Best Regards,
> Petr

Thanks for the review, questions and suggestions!  Let me stew on the
attach/free callback ideas for a bit -- they address a use-case that
kpatch doesn't really utilize, but may be otherwise valid.  My choice
for the sample program was definitely confusing, so I may see how
complicated a more expected implementation turns out.

-- Joe

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Petr Mladek]

On Wed 2017-06-14 09:57:56, Josh Poimboeuf wrote:
> On Wed, Jun 14, 2017 at 04:21:02PM +0200, Petr Mladek wrote:
> > But it is racy in general. The question is if the API
> > could help here. A possibility might be to allow to
> > define a callback function that would create the shadow
> > structure when it does not exist. I mean something like
> > 
> > typedef void (*klp_shadow_create_obj_func_t)(void * obj);
> > 
> > void *klp_shadow_get_or_create(void *obj, int key, gfp_t gfp,
> > 				klp_shadow_create_obj_fun_t *create)
> > {
> > 	struct klp_shadow *shadow;
> > 
> > 	shadow = klp_shadow_get(obj, key);
> > 
> > 	if (!shadow && create) {
> > 		void *shadow_obj;
> > 
> > 		spin_lock_irqsave(&klp_shadow_lock, flags);
> > 		shadow = klp_shadow_get(obj, key);
> > 		if (shadow)
> > 			goto out;
> > 
> > 		shadow_obj = create(obj);
> > 		shadow = __klp_shadow_attach(obj, key, gfp,
> > 					shadow_obj);
> > out:
> > 		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> > 	}
> > 
> > 	return shadow;
> > }
> > 
> > I do not know. Maybe it is too ugly. Or will it safe a duplicated code
> > in many cases?
> 
> I think this sample module is confusing because it uses the API in a
> contrived way.  In reality, we use it more like the API documentation
> describes: klp_shadow_attach() is called right after the parent struct
> is allocated and klp_shadow_detach() is called right before the parent
> struct is freed.  So the above race wouldn't normally exist.

But it kind of limits the usage only for short-living objects.
I mean that it does not help much to patch only the
allocation()/destroy() path when many affected objects
are created during boot or right after boot.

Well, I admit that my opinion is rather theoretical. You have more
experience with real life scenarios.

 
> I think Joe implemented it this way in order to keep it simple, so it
> wouldn't have to use kallsyms to do manual relocations, etc.  But maybe
> a more realistic example would be better since it represents how things
> should really be done in the absence of out-of-tree tooling like
> kpatch-build or klp-convert.

BTW: It seems that the example works only by chance. I test it by

   cat /proc/cmdline

It always forks a new process to run /usr/bin/cat. I guess that
there is a cache (in the memory management) and a high chance
that new process gets the last freed task_struct. But I got
different pointers for the process when I tried it many times.


> I often wonder whether it's really a good idea to even allow the
> unloading of patch modules at all.  It adds complexity to the livepatch
> code.  Is it worth it?  I don't have an answer but I'd be interested in
> other people's opinion.

I could imagine a situation when a livepatch causes, for example,
performance, problems on a server because of the redirection
to the new code. Then it might be handy to disable the patch
and ftrace handlers completely.

I know that disabling and removing patch are two different
things. Well, removing the patch is kind of test that the code
works as expected. If nothing else, this feature forced me
to understand various nasty things that help to be more
confident about the rest of the code ;-)

Best Regards,
Petr

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Josh Poimboeuf]

On Thu, Jun 15, 2017 at 12:59:43PM +0200, Petr Mladek wrote:
> On Wed 2017-06-14 09:57:56, Josh Poimboeuf wrote:
> > On Wed, Jun 14, 2017 at 04:21:02PM +0200, Petr Mladek wrote:
> > > But it is racy in general. The question is if the API
> > > could help here. A possibility might be to allow to
> > > define a callback function that would create the shadow
> > > structure when it does not exist. I mean something like
> > > 
> > > typedef void (*klp_shadow_create_obj_func_t)(void * obj);
> > > 
> > > void *klp_shadow_get_or_create(void *obj, int key, gfp_t gfp,
> > > 				klp_shadow_create_obj_fun_t *create)
> > > {
> > > 	struct klp_shadow *shadow;
> > > 
> > > 	shadow = klp_shadow_get(obj, key);
> > > 
> > > 	if (!shadow && create) {
> > > 		void *shadow_obj;
> > > 
> > > 		spin_lock_irqsave(&klp_shadow_lock, flags);
> > > 		shadow = klp_shadow_get(obj, key);
> > > 		if (shadow)
> > > 			goto out;
> > > 
> > > 		shadow_obj = create(obj);
> > > 		shadow = __klp_shadow_attach(obj, key, gfp,
> > > 					shadow_obj);
> > > out:
> > > 		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> > > 	}
> > > 
> > > 	return shadow;
> > > }
> > > 
> > > I do not know. Maybe it is too ugly. Or will it safe a duplicated code
> > > in many cases?
> > 
> > I think this sample module is confusing because it uses the API in a
> > contrived way.  In reality, we use it more like the API documentation
> > describes: klp_shadow_attach() is called right after the parent struct
> > is allocated and klp_shadow_detach() is called right before the parent
> > struct is freed.  So the above race wouldn't normally exist.
> 
> But it kind of limits the usage only for short-living objects.
> I mean that it does not help much to patch only the
> allocation()/destroy() path when many affected objects
> are created during boot or right after boot.
> 
> Well, I admit that my opinion is rather theoretical. You have more
> experience with real life scenarios.

Yes, maybe something like the above (create shadow var on read) would be
useful in some cases.  You'd have to be careful about allocating memory;
maybe GFP_NOWAIT would be needed.

> > I think Joe implemented it this way in order to keep it simple, so it
> > wouldn't have to use kallsyms to do manual relocations, etc.  But maybe
> > a more realistic example would be better since it represents how things
> > should really be done in the absence of out-of-tree tooling like
> > kpatch-build or klp-convert.
> 
> BTW: It seems that the example works only by chance. I test it by
> 
>    cat /proc/cmdline
> 
> It always forks a new process to run /usr/bin/cat. I guess that
> there is a cache (in the memory management) and a high chance
> that new process gets the last freed task_struct. But I got
> different pointers for the process when I tried it many times.
> 
> 
> > I often wonder whether it's really a good idea to even allow the
> > unloading of patch modules at all.  It adds complexity to the livepatch
> > code.  Is it worth it?  I don't have an answer but I'd be interested in
> > other people's opinion.
> 
> I could imagine a situation when a livepatch causes, for example,
> performance, problems on a server because of the redirection
> to the new code. Then it might be handy to disable the patch
> and ftrace handlers completely.

Fair enough, though it sounds theoretical.  It would be good to know
we're supporting actual real world use cases.

Unloading a patch module which created shadow variables will cause
memory leaks.  So either the shadow code or the patch module will need
to keep track of all the module's shadow variables so they can be freed
when the patch module gets unloaded.

-- 
Josh

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Josh Poimboeuf]

On Thu, Jun 15, 2017 at 10:38:00AM -0400, Joe Lawrence wrote:
> On Thu, Jun 15, 2017 at 08:49:27AM -0500, Josh Poimboeuf wrote:
> > Date: Thu, 15 Jun 2017 08:49:27 -0500
> > From: Josh Poimboeuf <jpoimboe@redhat.com>
> > To: Petr Mladek <pmladek@suse.com>
> > Cc: 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>, Miroslav Benes <mbenes@suse.cz>
> > Subject: Re: [PATCH 3/3] livepatch: add shadow variable sample program
> > User-Agent: Mutt/1.6.0.1 (2016-04-01)
> > 
> > On Thu, Jun 15, 2017 at 12:59:43PM +0200, Petr Mladek wrote:
> > > On Wed 2017-06-14 09:57:56, Josh Poimboeuf wrote:
> > > > On Wed, Jun 14, 2017 at 04:21:02PM +0200, Petr Mladek wrote:
> > > > > But it is racy in general. The question is if the API
> > > > > could help here. A possibility might be to allow to
> > > > > define a callback function that would create the shadow
> > > > > structure when it does not exist. I mean something like
> > > > > 
> > > > > typedef void (*klp_shadow_create_obj_func_t)(void * obj);
> > > > > 
> > > > > void *klp_shadow_get_or_create(void *obj, int key, gfp_t gfp,
> > > > > 				klp_shadow_create_obj_fun_t *create)
> > > > > {
> > > > > 	struct klp_shadow *shadow;
> > > > > 
> > > > > 	shadow = klp_shadow_get(obj, key);
> > > > > 
> > > > > 	if (!shadow && create) {
> > > > > 		void *shadow_obj;
> > > > > 
> > > > > 		spin_lock_irqsave(&klp_shadow_lock, flags);
> > > > > 		shadow = klp_shadow_get(obj, key);
> > > > > 		if (shadow)
> > > > > 			goto out;
> > > > > 
> > > > > 		shadow_obj = create(obj);
> > > > > 		shadow = __klp_shadow_attach(obj, key, gfp,
> > > > > 					shadow_obj);
> > > > > out:
> > > > > 		spin_unlock_irqrestore(&klp_shadow_lock, flags);
> > > > > 	}
> > > > > 
> > > > > 	return shadow;
> > > > > }
> > > > > 
> > > > > I do not know. Maybe it is too ugly. Or will it safe a duplicated code
> > > > > in many cases?
> > > > 
> > > > I think this sample module is confusing because it uses the API in a
> > > > contrived way.  In reality, we use it more like the API documentation
> > > > describes: klp_shadow_attach() is called right after the parent struct
> > > > is allocated and klp_shadow_detach() is called right before the parent
> > > > struct is freed.  So the above race wouldn't normally exist.
> > > 
> > > But it kind of limits the usage only for short-living objects.
> > > I mean that it does not help much to patch only the
> > > allocation()/destroy() path when many affected objects
> > > are created during boot or right after boot.
> > > 
> > > Well, I admit that my opinion is rather theoretical. You have more
> > > experience with real life scenarios.
> > 
> > Yes, maybe something like the above (create shadow var on read) would be
> > useful in some cases.  You'd have to be careful about allocating memory;
> > maybe GFP_NOWAIT would be needed.
> 
> I think we tossed around an idea like this, sans callbacks, for one our
> CVE-research patches.  As for the GFP flags, isn't that going to depend
> on the context of the patched code?

Right, the caller would need to specify the GFP flags for the klp_shadow
struct allocation, just like with klp_shadow_attach().

> Also, the caller can choose to allocate the new shadow data, but not
> the tracking struct klp_shadow, however it sees fit... pre-allocate a
> pool, whatever.  That is one distinction between this implementation
> and the kpatch counterpart.
> 
> > > > I often wonder whether it's really a good idea to even allow the
> > > > unloading of patch modules at all.  It adds complexity to the livepatch
> > > > code.  Is it worth it?  I don't have an answer but I'd be interested in
> > > > other people's opinion.
> > > 
> > > I could imagine a situation when a livepatch causes, for example,
> > > performance, problems on a server because of the redirection
> > > to the new code. Then it might be handy to disable the patch
> > > and ftrace handlers completely.
> > 
> > Fair enough, though it sounds theoretical.  It would be good to know
> > we're supporting actual real world use cases.
> > 
> > Unloading a patch module which created shadow variables will cause
> > memory leaks.  So either the shadow code or the patch module will need
> > to keep track of all the module's shadow variables so they can be freed
> > when the patch module gets unloaded.
> 
> As Petr suggested earlier in the thread, maybe a klp_shadow_detach_all
> function that rips through the klp_shadow_hash cleaning everything up.
> This could be called on patch module exit.

Yeah, though it'd be nice if it didn't need a callback to allow the user
to free the shadow data.  Maybe we should just move the shadow data
allocation into the klp_shadow code, like kpatch.

-- 
Josh

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

[Miroslav Benes]

> > +struct klp_shadow {
> > +	struct hlist_node node;
> > +	struct rcu_head rcu_head;
> > +	void *obj;
> > +	char *var;
> > +	void *data;
> 
> I would make the meaning more obvious. What about renaming?
> 
>   var -> key or id
>   data -> shadow_obj or new_obj

But var is not a key to a hash table. obj is. Renaming obj to key would be 
misleading in my opinion, because it IS a pointer to an object. And data 
is ok too, as far as I'm concerned. Just saying.

But yes, I'd welcome a description too.

Miroslav

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Miroslav Benes]

> Good catch.  Let me add a disclaimer here and state that this example is
> definitely contrived as I was trying to minimize its size.  Applying
> shadow variables to a more real life use case would drag in a bunch of
> "changed" function dependencies.  I didn't want to work around those
> with a pile of kallsyms workarounds.  It did lead you to ask an
> interesting question though:

You can always create a completely artificial sample based on a real life 
use case. I mean, you can create a module with functions and data 
structures you need and then patch one of those functions. You control 
everything and you can limit dependencies and kallsyms workarounds (or 
avoid them completely).

Miroslav

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Miroslav Benes]

> > > I often wonder whether it's really a good idea to even allow the
> > > unloading of patch modules at all.  It adds complexity to the livepatch
> > > code.  Is it worth it?  I don't have an answer but I'd be interested in
> > > other people's opinion.
> > 
> > I could imagine a situation when a livepatch causes, for example,
> > performance, problems on a server because of the redirection
> > to the new code. Then it might be handy to disable the patch
> > and ftrace handlers completely.
> 
> Fair enough, though it sounds theoretical.  It would be good to know
> we're supporting actual real world use cases.

We distribute cumulative "replace_all" patches at SUSE. replace_all means 
that all previous patches are reverted in the process of application. All 
livepatch modules with zero refcount are removed. This keeps a number of 
loaded modules low and system's state well defined, which is always a good 
thing, because a customer might run into problems and we'd have to debug 
it.

It is true that it is a limitation too. Especially for state changes and 
data structure modifications. Sometimes it is easy to patch a system, but 
impossible to unpatch it. Because we don't have a consistency on a state 
level, only on a task/process level. But I perceive this also as an 
advantage. I have to always know what a livepatch does exactly and I 
discovered couple of problems just because I had to think about unloading 
of modules.

Miroslav

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Josh Poimboeuf]

On Mon, Jun 19, 2017 at 06:56:37PM +0200, Miroslav Benes wrote:
> 
> > > > I often wonder whether it's really a good idea to even allow the
> > > > unloading of patch modules at all.  It adds complexity to the livepatch
> > > > code.  Is it worth it?  I don't have an answer but I'd be interested in
> > > > other people's opinion.
> > > 
> > > I could imagine a situation when a livepatch causes, for example,
> > > performance, problems on a server because of the redirection
> > > to the new code. Then it might be handy to disable the patch
> > > and ftrace handlers completely.
> > 
> > Fair enough, though it sounds theoretical.  It would be good to know
> > we're supporting actual real world use cases.
> 
> We distribute cumulative "replace_all" patches at SUSE. replace_all means 
> that all previous patches are reverted in the process of application. All 
> livepatch modules with zero refcount are removed. This keeps a number of 
> loaded modules low and system's state well defined, which is always a good 
> thing, because a customer might run into problems and we'd have to debug 
> it.

We used to have something similar in kpatch.  And we recently discovered
that this "replace_all" feature would also be nice to have in livepatch.

We had a patch B which needed to partially revert patch A.  We had to
manually do the revert at a function level, which basically means
repatching the function so that it resembles its original state.

It would be much more straightforward to be able to tell klp to revert
everything in patch A while applying patch B.  Then the func stack would
never have more than one entry.  And that would be good for performance
as well.

-- 
Josh

Re: [PATCH 3/3] livepatch: add shadow variable sample program

[Miroslav Benes]

On Fri, 30 Jun 2017, Josh Poimboeuf wrote:

> On Mon, Jun 19, 2017 at 06:56:37PM +0200, Miroslav Benes wrote:
> > 
> > > > > I often wonder whether it's really a good idea to even allow the
> > > > > unloading of patch modules at all.  It adds complexity to the livepatch
> > > > > code.  Is it worth it?  I don't have an answer but I'd be interested in
> > > > > other people's opinion.
> > > > 
> > > > I could imagine a situation when a livepatch causes, for example,
> > > > performance, problems on a server because of the redirection
> > > > to the new code. Then it might be handy to disable the patch
> > > > and ftrace handlers completely.
> > > 
> > > Fair enough, though it sounds theoretical.  It would be good to know
> > > we're supporting actual real world use cases.
> > 
> > We distribute cumulative "replace_all" patches at SUSE. replace_all means 
> > that all previous patches are reverted in the process of application. All 
> > livepatch modules with zero refcount are removed. This keeps a number of 
> > loaded modules low and system's state well defined, which is always a good 
> > thing, because a customer might run into problems and we'd have to debug 
> > it.
> 
> We used to have something similar in kpatch.  And we recently discovered
> that this "replace_all" feature would also be nice to have in livepatch.
> 
> We had a patch B which needed to partially revert patch A.  We had to
> manually do the revert at a function level, which basically means
> repatching the function so that it resembles its original state.
> 
> It would be much more straightforward to be able to tell klp to revert
> everything in patch A while applying patch B.  Then the func stack would
> never have more than one entry.  And that would be good for performance
> as well.

Exactly.

It is on my TODO list right after the fake signal. I've been occupied by 
different things recently but I'll definitely return to it soon.

Regards,
Miroslav