Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1655657 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] livepatch: add shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-06-01 20:30 +0200
    [PATCH 3/3] livepatch: add shadow variable sample program Joe Lawrence <joe.lawrence@redhat.com> - 2017-06-01 20:30 +0200
    [PATCH 2/3] livepatch: add shadow variable documentation Joe Lawrence <joe.lawrence@redhat.com> - 2017-06-01 20:30 +0200
    Re: [PATCH 0/3] livepatch: add shadow variable API Jiri Kosina <jikos@kernel.org> - 2017-06-01 22:10 +0200
      Re: [PATCH 0/3] livepatch: add shadow variable API Joe Lawrence <joe.lawrence@redhat.com> - 2017-06-01 22:30 +0200
        Re: [PATCH 0/3] livepatch: add shadow variable API Jiri Kosina <jikos@kernel.org> - 2017-06-01 22:40 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1655708

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

[toc] | [prev] | [next] | [standalone]

#1655734

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

[toc] | [prev] | [next] | [standalone]

#1655739

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web