support for stacked patches

PATCH In review Josh Poimboeuf 3 participants 7 messages Jan 20, 2015 Live patching

Share & tools

Josh Poimboeuf

author

2015-01-20 15:26 UTC #0 lore raw

v2 cover support for stacked patches

Add support for properly stacking patches. This includes enforcing patch stack semantics and allowing functions to be repatched.

v2 changes:
- add "enforce stacking semantics" patch
- add documentation comments for new klp_ops struct

Josh Poimboeuf (2):
livepatch: enforce patch stacking semantics
livepatch: support for repatching a function

 include/linux/livepatch.h |   4 +-
 kernel/livepatch/core.c   | 180 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 131 insertions(+), 53 deletions(-)

Signature · 1 line

2.1.0

Josh Poimboeuf

author

2015-01-20 15:26 UTC #1 lore raw

PATCH v2 1/2 livepatch: enforce patch stacking semantics

Only allow the topmost patch on the stack to be enabled or disabled, so that patches can't be removed or added in an arbitrary order.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/livepatch/core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

kernel/livepatch/core.c +10 -0

index 3d9c00b..2401e7f 100644
@@ -379,6 +379,11 @@ static int __klp_disable_patch(struct klp_patch *patch)
 	struct klp_object *obj;
 	int ret;
 
+	/* enforce stacking: only the last enabled patch can be disabled */
+	if (!list_is_last(&patch->list, &klp_patches) &&
+	    list_next_entry(patch, list)->state == KLP_ENABLED)
+		return -EBUSY;
+
 	pr_notice("disabling patch '%s'\n", patch->mod->name);
 
 	for (obj = patch->objs; obj->funcs; obj++) {

@@ -435,6 +440,11 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	if (WARN_ON(patch->state != KLP_DISABLED))
 		return -EINVAL;
 
+	/* enforce stacking: only the first disabled patch can be enabled */
+	if (patch->list.prev != &klp_patches &&
+	    list_prev_entry(patch, list)->state == KLP_DISABLED)
+		return -EBUSY;
+
 	pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
 	add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);

Signature · 1 line

2.1.0

Jiri Slaby

Reviewed-by on v2 1/2

2015-01-20 15:33 UTC #2 lore raw

Re: [PATCH v2 1/2] livepatch: enforce patch stacking semantics

Quoted 5 lines · Josh Poimboeuf

Only allow the topmost patch on the stack to be enabled or disabled, so that patches can't be removed or added in an arbitrary order.
Suggested-by Jiri Kosina <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>

Reviewed-by Jiri Slaby <[email protected]>

Quoted 23 lines

---
 kernel/livepatch/core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

kernel/livepatch/core.c +5 -0

index 3d9c00b..2401e7f 100644
@@ -379,6 +379,11 @@ static int __klp_disable_patch(struct klp_patch *patch)
 	struct klp_object *obj;
 	int ret;
 
+	/* enforce stacking: only the last enabled patch can be disabled */
+	if (!list_is_last(&patch->list, &klp_patches) &&
+	    list_next_entry(patch, list)->state == KLP_ENABLED)
+		return -EBUSY;
+
 	pr_notice("disabling patch '%s'\n", patch->mod->name);
 
 	for (obj = patch->objs; obj->funcs; obj++) {

@@ -435,6 +440,11 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	if (WARN_ON(patch->state != KLP_DISABLED))

kernel/livepatch/core.c static int __klp_enable_patch(struct klp_patch *patch) · line 450

Quoted 10 lines

kernel/livepatch/core.c +5 -0

 		return -EINVAL;
 
+	/* enforce stacking: only the first disabled patch can be enabled */
+	if (patch->list.prev != &klp_patches &&
+	    list_prev_entry(patch, list)->state == KLP_DISABLED)
+		return -EBUSY;
+
 	pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
 	add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);

thanks,

Signature · 2 lines

js
suse labs

Josh Poimboeuf

author

2015-01-20 15:26 UTC #3 lore raw

PATCH v2 2/2 livepatch: support for repatching a function

In reply to Josh Poimboeuf

Add support for patching a function multiple times. If multiple patches affect a function, the function in the most recently enabled patch "wins". This enables a cumulative patch upgrade path, where each patch is a superset of previous patches.

This requires restructuring the data a little bit. With the current design, where each klp_func struct has its own ftrace_ops, we'd have to unregister the old ops and then register the new ops, because FTRACE_OPS_FL_IPMODIFY prevents us from having two ops registered for the same function at the same time. That would leave a regression window where the function isn't patched at all (not good for a patch upgrade path).

This patch replaces the per-klp_func ftrace_ops with a global klp_ops list, with one ftrace_ops per original function. A single ftrace_ops is shared between all klp_funcs which have the same old_addr. This allows the switch between function versions to happen instantaneously by updating the klp_ops struct's func_stack list. The winner is the klp_func at the top of the func_stack (front of the list).

Signed-off-by Josh Poimboeuf <[email protected]>

---
 include/linux/livepatch.h |   4 +-
 kernel/livepatch/core.c   | 170 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 121 insertions(+), 53 deletions(-)

include/linux/livepatch.h +2 -2

index 950bc61..f14c6fb 100644
@@ -40,8 +40,8 @@ enum klp_state {
  * @old_addr:	a hint conveying at what address the old function
  *		can be found (optional, vmlinux patches only)
  * @kobj:	kobject for sysfs resources
- * @fops:	ftrace operations structure
  * @state:	tracks function-level patch application state
+ * @stack_node:	list node for klp_ops func_stack list
  */
 struct klp_func {
 	/* external */

@@ -59,8 +59,8 @@ struct klp_func {
 
 	/* internal */
 	struct kobject kobj;
-	struct ftrace_ops *fops;
 	enum klp_state state;
+	struct list_head stack_node;
 };
 
 /**

kernel/livepatch/core.c +119 ~~-51~~

234 lines not on this page · raw message

Signature · 1 line

2.1.0

Jiri Slaby

Reviewed-by on v2 2/2

2015-01-20 15:43 UTC #4 lore raw

Re: [PATCH v2 2/2] livepatch: support for repatching a function

Quoted 13 lines · Josh Poimboeuf

Add support for patching a function multiple times. If multiple patches affect a function, the function in the most recently enabled patch "wins". This enables a cumulative patch upgrade path, where each patch is a superset of previous patches.
This requires restructuring the data a little bit. With the current design, where each klp_func struct has its own ftrace_ops, we'd have to unregister the old ops and then register the new ops, because FTRACE_OPS_FL_IPMODIFY prevents us from having two ops registered for the same function at the same time. That would leave a regression window where the function isn't patched at all (not good for a patch upgrade path).

Quoted 8 lines

This patch replaces the per-klp_func ftrace_ops with a global klp_ops list, with one ftrace_ops per original function. A single ftrace_ops is shared between all klp_funcs which have the same old_addr. This allows the switch between function versions to happen instantaneously by updating the klp_ops struct's func_stack list. The winner is the klp_func at the top of the func_stack (front of the list).
Signed-off-by Josh Poimboeuf <[email protected]>

Reviewed-by Jiri Slaby <[email protected]>

But...

Quoted 11 lines

kernel/livepatch/core.c +3 -2

@@ -267,16 +303,28 @@ static int klp_write_object_relocations(struct module *pmod,
 
 static void notrace klp_ftrace_handler(unsigned long ip,
 				       unsigned long parent_ip,
-				       struct ftrace_ops *ops,
+				       struct ftrace_ops *fops,
 				       struct pt_regs *regs)
 {
-	struct klp_func *func = ops->private;
+	struct klp_ops *ops;
+	struct klp_func *func;

kernel/livepatch/core.c static int klp_write_object_relocations(struct module *pmod, · line 320

Quoted 10 lines

kernel/livepatch/core.c +10 -0

+
+	ops = container_of(fops, struct klp_ops, fops);
+
+	rcu_read_lock();
+	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
+				      stack_node);
+	rcu_read_unlock();
+
+	if (WARN_ON(!func))
+		return;

If it ever happens, the warn will drown the machine in the output splash.

WARN_ON_RATELIMIT?

thanks,

Signature · 2 lines

js
suse labs

Josh Poimboeuf

author

2015-01-20 15:47 UTC #5 lore raw

Re: [PATCH v2 2/2] livepatch: support for repatching a function

Quoted 21 lines · Jiri Slaby

Quoted 20 lines · Josh Poimboeuf
Add support for patching a function multiple times. If multiple patches affect a function, the function in the most recently enabled patch "wins". This enables a cumulative patch upgrade path, where each patch is a superset of previous patches.
This requires restructuring the data a little bit. With the current design, where each klp_func struct has its own ftrace_ops, we'd have to unregister the old ops and then register the new ops, because FTRACE_OPS_FL_IPMODIFY prevents us from having two ops registered for the same function at the same time. That would leave a regression window where the function isn't patched at all (not good for a patch upgrade path).
This patch replaces the per-klp_func ftrace_ops with a global klp_ops list, with one ftrace_ops per original function. A single ftrace_ops is shared between all klp_funcs which have the same old_addr. This allows the switch between function versions to happen instantaneously by updating the klp_ops struct's func_stack list. The winner is the klp_func at the top of the func_stack (front of the list).

Quoted 3 lines

Quoted 1 line
Signed-off-by Josh Poimboeuf <[email protected]>
Reviewed-by Jiri Slaby <[email protected]>

Thanks for the review!

Quoted 17 lines

But...

Quoted 15 lines

kernel/livepatch/core.c +7 -2

@@ -267,16 +303,28 @@ static int klp_write_object_relocations(struct module *pmod,
 
 static void notrace klp_ftrace_handler(unsigned long ip,
 				       unsigned long parent_ip,
-				       struct ftrace_ops *ops,
+				       struct ftrace_ops *fops,
 				       struct pt_regs *regs)
 {
-	struct klp_func *func = ops->private;
+	struct klp_ops *ops;
+	struct klp_func *func;
+
+	ops = container_of(fops, struct klp_ops, fops);
+
+	rcu_read_lock();

Quoted 8 lines

kernel/livepatch/core.c static int klp_write_object_relocations(struct module *pmod, · line 320
Quoted 6 lines
kernel/livepatch/core.c +6 -0
+	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
+				      stack_node);
+	rcu_read_unlock();
+
+	if (WARN_ON(!func))
+		return;
If it ever happens, the warn will drown the machine in the output splash.

Yeah, maybe, depending on the nature of the bug.

Quoted 1 line

WARN_ON_RATELIMIT?

Since this warning should never happen unless there's a code bug, I think WARN_ON_ONCE should be sufficient?

Signature · 1 line

Josh

Jiri Kosina

2015-01-20 19:11 UTC #6 lore raw

Re: [PATCH v2 2/2] livepatch: support for repatching a function

Quoted 6 lines · Josh Poimboeuf

Yeah, maybe, depending on the nature of the bug.
Quoted 1 line
WARN_ON_RATELIMIT?
Since this warning should never happen unless there's a code bug, I think WARN_ON_ONCE should be sufficient?

Yeah. The stacktrace is likely not to point to the source of the problem anyway, so spamming dmesg with it is rather pointless.

I've changed WARN_ON() in ftrace handler to WARN_ON_ONCE() and applied. Thanks,

Signature · 2 lines

Jiri Kosina
SUSE Labs