livepatch: introduce atomic replace

PATCH Superseded Jason Baron 5 participants 28 messages Sep 28, 2017 – Oct 20, 2017 Live patching

lore

Share & tools

lore

Jason Baron

author

2017-09-28 03:42 UTC #0 lore raw

v3 cover livepatch: introduce atomic replace

Hi,

In testing livepatch, I found that when doing cumulative patches, if a patched function is completed reverted by a subsequent patch (back to its original state) livepatch does not revert the funtion to its original state. Specifically, if patch A introduces a change to function 1, and patch B reverts the change to function 1 and introduces changes to say function 2 and 3 as well, the change that patch A introduced to function 1 is still present. This could be addressed by first completely removing patch A (disable and then rmmod) and then inserting patch B (insmod and enable), but this leaves an unpatched window. In discussing this issue with Josh on the kpatch mailing list, he mentioned that we could get 'atomic replace working properly', and that is the direction of this patchset:
https://www.redhat.com/archives/kpatch/2017-June/msg00005.html

Patches:

1) livepatch: Add klp_object and klp_func dynamic iterators A prep patch for the 'atomic replace' feature such that dynamic objects and functions can be allocated.

2) livepatch: add atomic replace
Core feature. Note that __klp_enable_patch() calls klp_add_nops(), which necessitated moving a bunch of existing functions before __klp_enable_patch(). So there is a bit of churn in moving functions that are not modified.

Thanks,

-Jason

v2-v3:
-refactor how the dynamic nops are calculated (Petr Mladek) -move the creation of dynamic nops to enable/disable paths -add klp_replaced_patches list to indicate patches that can be re-enabled -dropped 'replaced' field
-renamed dynamic fields in klp_func, object and patch
-moved iterator implementation to kernel/livepatch/core.c -'inherit' nop immediate flag
-update kobject_put free'ing logic (Petr Mladek)

v1-v2:
-removed the func_iter and obj_iter (Petr Mladek) -initialiing kobject structure for no_op functions using:
klp_init_object() and klp_init_func()
-added a 'replace' field to klp_patch, similar to the immediate field -a 'replace' patch now disables all previous patches -tried to shorten klp_init_patch_no_ops()...
-Simplified logic klp_complete_transition (Petr Mladek)

Jason Baron (2):
livepatch: Add dynamic klp_object and klp_func iterators
livepatch: add atomic replace

 include/linux/livepatch.h     |  85 +++--
 kernel/livepatch/core.c       | 738 ++++++++++++++++++++++++++++++++----------
 kernel/livepatch/core.h       |   6 +
 kernel/livepatch/patch.c      |  22 +-
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  50 ++-
 6 files changed, 685 insertions(+), 220 deletions(-)

Signature · 1 line

2.6.1

Jason Baron

author

2017-09-28 03:42 UTC #1 lore raw

PATCH v3 1/2 livepatch: Add dynamic klp_object and klp_func iterators

In preparation to introducing atomic replace, introduce iterators for klp_func and klp_object, such that objects and functions can be dynamically allocated (needed for atomic replace). This patch is intended to effectively be a no-op until atomic replace is introduced.

Signed-off-by Jason Baron <[email protected]>

Cc Josh Poimboeuf <[email protected]>

Cc Jessica Yu <[email protected]>

Cc Jiri Kosina <[email protected]>

Cc Miroslav Benes <[email protected]>

Cc Petr Mladek <[email protected]>

---
 include/linux/livepatch.h |  81 +++++++++++++++++++++-------------
 kernel/livepatch/core.c   | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 160 insertions(+), 31 deletions(-)

include/linux/livepatch.h +50 ~~-31~~

index 194991e..e03ce11 100644
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/ftrace.h>
 #include <linux/completion.h>
+#include <linux/list.h>
 
 #if IS_ENABLED(CONFIG_LIVEPATCH)

@@ -36,18 +37,20 @@
 
 /**
  * struct klp_func - function structure for live patching
- * @old_name:	name of the function to be patched
- * @new_func:	pointer to the patched function code
- * @old_sympos: a hint indicating which symbol position the old function
- *		can be found (optional)
- * @immediate:  patch the func immediately, bypassing safety mechanisms
- * @old_addr:	the address of the function being patched
- * @kobj:	kobject for sysfs resources
- * @stack_node:	list node for klp_ops func_stack list
- * @old_size:	size of the old function
- * @new_size:	size of the new function
- * @patched:	the func has been added to the klp_ops list
- * @transition:	the func is currently being applied or reverted
+ * @old_name:	    name of the function to be patched
+ * @new_func:	    pointer to the patched function code
+ * @old_sympos:	    a hint indicating which symbol position the old function
+ *		    can be found (optional)
+ * @immediate:	    patch the func immediately, bypassing safety mechanisms
+ * @old_addr:	    the address of the function being patched
+ * @kobj:	    kobject for sysfs resources
+ * @stack_node:	    list node for klp_ops func_stack list
+ * @nop_func_entry: links dynamically allocated struct klp_func to struct
+ *		    klp_object
+ * @old_size:	    size of the old function
+ * @new_size:	    size of the new function
+ * @patched:	    the func has been added to the klp_ops list
+ * @transition:	    the func is currently being applied or reverted
  *
  * The patched and transition variables define the func's patching state.  When
  * patching, a func is always in one of the following states:

@@ -82,6 +85,7 @@ struct klp_func {
 	unsigned long old_addr;
 	struct kobject kobj;
 	struct list_head stack_node;
+	struct list_head nop_func_entry;
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;

@@ -89,12 +93,15 @@ struct klp_func {
 
 /**
  * struct klp_object - kernel object structure for live patching
- * @name:	module name (or NULL for vmlinux)
- * @funcs:	function entries for functions to be patched in the object
- * @kobj:	kobject for sysfs resources
- * @mod:	kernel module associated with the patched object
- *		(NULL for vmlinux)
- * @patched:	the object's funcs have been added to the klp_ops list
+ * @name:	   module name (or NULL for vmlinux)
+ * @funcs:	   function entries for functions to be patched in the object
+ * @kobj:	   kobject for sysfs resources
+ * @nop_func_list: head of list for dynamically allocated struct klp_func
+ * @nop_obj_entry: links dynamically allocated struct klp_object to struct
+ *		   klp_patch
+ * @mod:	   kernel module associated with the patched object
+ *		   (NULL for vmlinux)
+ * @patched:	   the object's funcs have been added to the klp_ops list
  */
 struct klp_object {
 	/* external */

@@ -103,19 +110,22 @@ struct klp_object {
 
 	/* internal */
 	struct kobject kobj;
+	struct list_head nop_func_list;
+	struct list_head nop_obj_entry;
 	struct module *mod;
 	bool patched;
 };
 
 /**
  * struct klp_patch - patch structure for live patching
- * @mod:	reference to the live patch module
- * @objs:	object entries for kernel objects to be patched
- * @immediate:  patch all funcs immediately, bypassing safety mechanisms
- * @list:	list node for global list of registered patches
- * @kobj:	kobject for sysfs resources
- * @enabled:	the patch is enabled (but operation may be incomplete)
- * @finish:	for waiting till it is safe to remove the patch module
+ * @mod:	  reference to the live patch module
+ * @objs:	  object entries for kernel objects to be patched
+ * @immediate:	  patch all funcs immediately, bypassing safety mechanisms
+ * @list:	  list node for global list of registered patches
+ * @kobj:	  kobject for sysfs resources
+ * @nop_obj_list: head of list for dynamically allocated struct klp_object
+ * @enabled:	  the patch is enabled (but operation may be incomplete)
+ * @finish:	  for waiting till it is safe to remove the patch module
  */
 struct klp_patch {
 	/* external */

@@ -126,17 +136,26 @@ struct klp_patch {
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
+	struct list_head nop_obj_list;
 	bool enabled;
 	struct completion finish;
 };
 
-#define klp_for_each_object(patch, obj) \
-	for (obj = patch->objs; obj->funcs || obj->name; obj++)
+struct klp_object *klp_obj_iter_init(struct klp_patch *patch);
+struct klp_object *klp_obj_iter_next(struct klp_patch *patch,
+				     struct klp_object *obj);
 
-#define klp_for_each_func(obj, func) \
-	for (func = obj->funcs; \
-	     func->old_name || func->new_func || func->old_sympos; \
-	     func++)
+#define klp_for_each_object(patch, obj)		  \
+	for (obj = klp_obj_iter_init(patch); obj; \
+	     obj = klp_obj_iter_next(patch, obj))
+
+struct klp_func *klp_func_iter_init(struct klp_object *obj);
+struct klp_func *klp_func_iter_next(struct klp_object *obj,
+				    struct klp_func *func);
+
+#define klp_for_each_func(obj, func)		   \
+	for (func = klp_func_iter_init(obj); func; \
+	     func = klp_func_iter_next(obj, func))
 
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);

kernel/livepatch/core.c +110 -0

128 lines not on this page · raw message

Signature · 1 line

2.6.1

Josh Poimboeuf

2017-10-06 21:22 UTC #2 lore raw

Re: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

Quoted 4 lines · Jason Baron

In preparation to introducing atomic replace, introduce iterators for klp_func and klp_object, such that objects and functions can be dynamically allocated (needed for atomic replace). This patch is intended to effectively be a no-op until atomic replace is introduced.

Hi Jason,

Sorry for coming in so late on this. This will be a nice feature. I haven't gotten to the second patch yet, but I have a few comments already.

1) Having both static *and* dynamic lists seems to add a lot of complexity and brittleness.

The livepatch API is easier to use with static data structures, so I don't think we should change the API. But I also don't think we should allow the API to overly constrain our internal representation of the data.

What I'd *really* like to do is completely separate the user-supplied data structures from our internal data . At registration time, we can convert all the arrays to dynamic lists, and then never touch the arrays again. Then managing the lists is completely straightforward.

That would also allow us to have an external public struct and an internal private struct, so the interface is more clear and we don't have to worry about any patch modules accidentally messing with our private data.

2) There seems to be a general consensus that the cumulative "replace" model is the best one. In light of that, I wonder if we can simplify our code a bit. Specifically, can we get rid of the func stack?

If the user always uses replace, then the func stack will never have more than one func. And we can reject a patch if the user doesn't use replace when they're trying to patch a function which has already been patched.

Then the func stack doesn't have to be a stack, it can just be a single func.

That would allow us to simplify the code in several places for the common case, yet still allow those who want to apply non-cumulative patches to do so most of the time.

It's not a *huge* simplification, but we've already made livepatch so flexible that it's too complex to fit everything in your head at the same time, and any simplification we can get away with is well worth it IMO. And once we have replace, I wonder if anybody would really miss the func stack.

Thoughts?

Signature · 1 line

Josh

Jason Baron

author

2017-10-10 15:15 UTC #3 lore raw

Re: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

Quoted 6 lines · Josh Poimboeuf

Quoted 4 lines · Jason Baron
In preparation to introducing atomic replace, introduce iterators for klp_func and klp_object, such that objects and functions can be dynamically allocated (needed for atomic replace). This patch is intended to effectively be a no-op until atomic replace is introduced.

Quoted 6 lines

Hi Jason,
Sorry for coming in so late on this. This will be a nice feature. I haven't gotten to the second patch yet, but I have a few comments already.

Hi Josh,

Quoted 13 lines

1) Having both static *and* dynamic lists seems to add a lot of complexity and brittleness.
The livepatch API is easier to use with static data structures, so I don't think we should change the API. But I also don't think we should allow the API to overly constrain our internal representation of the data.
What I'd *really* like to do is completely separate the user-supplied data structures from our internal data . At registration time, we can convert all the arrays to dynamic lists, and then never touch the arrays again. Then managing the lists is completely straightforward.

Quoted 5 lines

That would also allow us to have an external public struct and an internal private struct, so the interface is more clear and we don't have to worry about any patch modules accidentally messing with our private data.

Separating the external/internal data structures makes sense. Another idea here would be to simply walk the static data structures at register time and then add them onto the dynamic lists. Then, all subsequent access can be done using the simple list walks. That would remove the complex iterators here. The external/internal division could still be done later but maybe doesn't couple as much to this patchset...

Quoted 16 lines

2) There seems to be a general consensus that the cumulative "replace" model is the best one. In light of that, I wonder if we can simplify our code a bit. Specifically, can we get rid of the func stack?
If the user always uses replace, then the func stack will never have more than one func. And we can reject a patch if the user doesn't use replace when they're trying to patch a function which has already been patched.
Then the func stack doesn't have to be a stack, it can just be a single func.
That would allow us to simplify the code in several places for the common case, yet still allow those who want to apply non-cumulative patches to do so most of the time.

Quoted 6 lines

It's not a *huge* simplification, but we've already made livepatch so flexible that it's too complex to fit everything in your head at the same time, and any simplification we can get away with is well worth it IMO. And once we have replace, I wonder if anybody would really miss the func stack.

I think that means instead of having a list of functions or the func stack, you have 2 pointers - one to the active patch and one to the previous patch. Maybe it would make sense to do in steps? IE have this patchset reject patches that patch a patch to existing function, leaving the func stack. And if that doesn't break any use-cases, switch to a model where there is just an active and previous patch?

Thanks,

-Jason

Josh Poimboeuf

2017-10-11 02:52 UTC #4 lore raw

Re: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

Quoted 38 lines · Jason Baron

Quoted 6 lines · Josh Poimboeuf
Quoted 4 lines · Jason Baron
In preparation to introducing atomic replace, introduce iterators for klp_func and klp_object, such that objects and functions can be dynamically allocated (needed for atomic replace). This patch is intended to effectively be a no-op until atomic replace is introduced.
Quoted 6 lines
Hi Jason,
Sorry for coming in so late on this. This will be a nice feature. I haven't gotten to the second patch yet, but I have a few comments already.
Hi Josh,
Quoted 13 lines
1) Having both static *and* dynamic lists seems to add a lot of complexity and brittleness.
The livepatch API is easier to use with static data structures, so I don't think we should change the API. But I also don't think we should allow the API to overly constrain our internal representation of the data.
What I'd *really* like to do is completely separate the user-supplied data structures from our internal data . At registration time, we can convert all the arrays to dynamic lists, and then never touch the arrays again. Then managing the lists is completely straightforward.
Quoted 5 lines
That would also allow us to have an external public struct and an internal private struct, so the interface is more clear and we don't have to worry about any patch modules accidentally messing with our private data.

Quoted 6 lines

Separating the external/internal data structures makes sense. Another idea here would be to simply walk the static data structures at register time and then add them onto the dynamic lists. Then, all subsequent access can be done using the simple list walks. That would remove the complex iterators here. The external/internal division could still be done later but maybe doesn't couple as much to this patchset...

Sounds good.

Quoted 23 lines

Quoted 16 lines
2) There seems to be a general consensus that the cumulative "replace" model is the best one. In light of that, I wonder if we can simplify our code a bit. Specifically, can we get rid of the func stack?
If the user always uses replace, then the func stack will never have more than one func. And we can reject a patch if the user doesn't use replace when they're trying to patch a function which has already been patched.
Then the func stack doesn't have to be a stack, it can just be a single func.
That would allow us to simplify the code in several places for the common case, yet still allow those who want to apply non-cumulative patches to do so most of the time.
Quoted 6 lines
It's not a *huge* simplification, but we've already made livepatch so flexible that it's too complex to fit everything in your head at the same time, and any simplification we can get away with is well worth it IMO. And once we have replace, I wonder if anybody would really miss the func stack.

Quoted 6 lines

I think that means instead of having a list of functions or the func stack, you have 2 pointers - one to the active patch and one to the previous patch. Maybe it would make sense to do in steps? IE have this patchset reject patches that patch a patch to existing function, leaving the func stack. And if that doesn't break any use-cases, switch to a model where there is just an active and previous patch?

Hm. Maybe I didn't think it through. I guess the func stack could have at most *two* funcs -- not one. Let's just forget this idea for now...

Signature · 1 line

Josh

Miroslav Benes

maintainer

2017-10-16 14:47 UTC #5 lore raw

Re: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

Quoted 39 lines · Josh Poimboeuf

Quoted 38 lines · Jason Baron
Quoted 6 lines · Josh Poimboeuf
Quoted 4 lines · Jason Baron
In preparation to introducing atomic replace, introduce iterators for klp_func and klp_object, such that objects and functions can be dynamically allocated (needed for atomic replace). This patch is intended to effectively be a no-op until atomic replace is introduced.
Quoted 6 lines
Hi Jason,
Sorry for coming in so late on this. This will be a nice feature. I haven't gotten to the second patch yet, but I have a few comments already.
Hi Josh,
Quoted 18 lines
1) Having both static *and* dynamic lists seems to add a lot of complexity and brittleness.
The livepatch API is easier to use with static data structures, so I don't think we should change the API. But I also don't think we should allow the API to overly constrain our internal representation of the data.
What I'd *really* like to do is completely separate the user-supplied data structures from our internal data . At registration time, we can convert all the arrays to dynamic lists, and then never touch the arrays again. Then managing the lists is completely straightforward.
That would also allow us to have an external public struct and an internal private struct, so the interface is more clear and we don't have to worry about any patch modules accidentally messing with our private data.

Quoted 8 lines

Quoted 6 lines
Separating the external/internal data structures makes sense. Another idea here would be to simply walk the static data structures at register time and then add them onto the dynamic lists. Then, all subsequent access can be done using the simple list walks. That would remove the complex iterators here. The external/internal division could still be done later but maybe doesn't couple as much to this patchset...
Sounds good.

I haven't seen v4 yet, but this seems to be the best approach so far.

I was the one who opposed external/internal division in the past, but given atomic replace feature we may end up there in the end. But I'd keep it separate.

Quoted 30 lines

Quoted 23 lines
Quoted 16 lines
2) There seems to be a general consensus that the cumulative "replace" model is the best one. In light of that, I wonder if we can simplify our code a bit. Specifically, can we get rid of the func stack?
If the user always uses replace, then the func stack will never have more than one func. And we can reject a patch if the user doesn't use replace when they're trying to patch a function which has already been patched.
Then the func stack doesn't have to be a stack, it can just be a single func.
That would allow us to simplify the code in several places for the common case, yet still allow those who want to apply non-cumulative patches to do so most of the time.
Quoted 6 lines
It's not a *huge* simplification, but we've already made livepatch so flexible that it's too complex to fit everything in your head at the same time, and any simplification we can get away with is well worth it IMO. And once we have replace, I wonder if anybody would really miss the func stack.
Quoted 6 lines
I think that means instead of having a list of functions or the func stack, you have 2 pointers - one to the active patch and one to the previous patch. Maybe it would make sense to do in steps? IE have this patchset reject patches that patch a patch to existing function, leaving the func stack. And if that doesn't break any use-cases, switch to a model where there is just an active and previous patch?

Quoted 2 lines

Hm. Maybe I didn't think it through. I guess the func stack could have at most *two* funcs -- not one. Let's just forget this idea for now...

Yes, there would still be some kind of stacking needed just to make atomic replace work (if I understand you correctly).

Moreover, I'd be hesitant to remove stacking. Yes, we use cumulative patches at SUSE, because they correspond well to our particular use case. Some other company or even an ordinary user of linux might do things differently. If we removed it now, it would be almost impossible to add it later, because it is so complex. So if there's way to preserve it, I'd keep. As long as possible.

Regards,
Miroslav

Jason Baron

author

2017-09-28 03:43 UTC #6 lore raw

PATCH v3 2/2 livepatch: add atomic replace

In reply to Jason Baron

Sometimes we would like to revert a particular fix. This is currently not easy because we want to keep all other fixes active and we could revert only the last applied patch.

One solution would be to apply new patch that implemented all the reverted functions like in the original code. It would work as expected but there will be unnecessary redirections. In addition, it would also require knowing which functions need to be reverted at build time.

A better solution would be to say that a new patch replaces an older one. This might be complicated if we want to replace a particular patch. But it is rather easy when a new cummulative patch replaces all others.

Add a new "replace" flag to struct klp_patch. When it is enabled, a set of nop' klp_func will be dynamically created for all functions that are already being patched but that will not longer be modified by the new patch. They are temporarily used as a new target during the patch transition.

Since 'atomic replace' has completely replaced all previous livepatch modules, it explicitly disables all previous livepatch modules. However, previous livepatch modules that have been replaced, can be re-enabled if they have the 'replace' flag set. In this case the set of 'nop' functions is re-calculated, such that it replaces all others.

For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set then:

# insmod kpatch-a.ko
# insmod kpatch-b.ko

At this point we have:

# cat /sys/kernel/livepatch/kpatch-a/enabled
0

# cat /sys/kernel/livepatch/kpatch-b/enabled
1

To revert to the kpatch-a state we can do:

echo 1 > /sys/kernel/livepatch/kpatch-a/enabled

And now we have:

# cat /sys/kernel/livepatch/kpatch-a/enabled
1

# cat /sys/kernel/livepatch/kpatch-b/enabled
0

Note that it may be possible to unload (rmmod) replaced patches in some cases based on the consistency model, when we know that all the functions that are contained in the patch may no longer be in used, however its left as future work, if this functionality is desired.

Also, __klp_enable_patch() calls klp_add_nops(), which necessitated moving a bunch of existing functions before __klp_enable_patch(). So there is a bit of churn in moving functions that are not modified.

Signed-off-by Jason Baron <[email protected]>

Cc Josh Poimboeuf <[email protected]>

Cc Jessica Yu <[email protected]>

Cc Jiri Kosina <[email protected]>

Cc Miroslav Benes <[email protected]>

Cc Petr Mladek <[email protected]>

---
 include/linux/livepatch.h     |   4 +
 kernel/livepatch/core.c       | 642 ++++++++++++++++++++++++++++++------------
 kernel/livepatch/core.h       |   6 +
 kernel/livepatch/patch.c      |  22 +-
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  50 +++-
 6 files changed, 532 insertions(+), 196 deletions(-)

include/linux/livepatch.h +4 -0

index e03ce11..93e8d44 100644
@@ -51,6 +51,7 @@
  * @new_size:	    size of the new function
  * @patched:	    the func has been added to the klp_ops list
  * @transition:	    the func is currently being applied or reverted
+ * @nop:	    used to revert a previously patched function
  *
  * The patched and transition variables define the func's patching state.  When
  * patching, a func is always in one of the following states:

@@ -89,6 +90,7 @@ struct klp_func {
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
+	bool nop;
 };
 
 /**

@@ -121,6 +123,7 @@ struct klp_object {
  * @mod:	  reference to the live patch module
  * @objs:	  object entries for kernel objects to be patched
  * @immediate:	  patch all funcs immediately, bypassing safety mechanisms
+ * @replace:	  revert previously patched functions not included in the patch
  * @list:	  list node for global list of registered patches
  * @kobj:	  kobject for sysfs resources
  * @nop_obj_list: head of list for dynamically allocated struct klp_object

@@ -132,6 +135,7 @@ struct klp_patch {
 	struct module *mod;
 	struct klp_object *objs;
 	bool immediate;
+	bool replace;
 
 	/* internal */
 	struct list_head list;

kernel/livepatch/core.c +458 ~~-184~~

731 lines not on this page · raw message

kernel/livepatch/core.h +6 -0

index c74f24c..fe45ee2 100644
@@ -1,6 +1,12 @@
 #ifndef _LIVEPATCH_CORE_H
 #define _LIVEPATCH_CORE_H
 
+#include <linux/livepatch.h>
+
 extern struct mutex klp_mutex;
+extern struct list_head klp_patches;
+extern struct list_head klp_replaced_patches;
+
+void klp_remove_nops(struct klp_patch *patch);
 
 #endif /* _LIVEPATCH_CORE_H */

kernel/livepatch/patch.c +16 -6

index 52c4e90..aae6e4d 100644
@@ -117,6 +117,9 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 		}
 	}
 
+	/* if its a 'nop', then run the original function */
+	if (func->nop)
+		goto unlock;
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	preempt_enable_notrace();

@@ -235,15 +238,21 @@ static int klp_patch_func(struct klp_func *func)
 	return ret;
 }
 
-void klp_unpatch_object(struct klp_object *obj)
+/* if nop is set, only unpatch the nop functions */
+void klp_unpatch_object(struct klp_object *obj, bool nop)
 {
 	struct klp_func *func;
 
-	klp_for_each_func(obj, func)
+	klp_for_each_func(obj, func) {
+		if (nop && !func->nop)
+			continue;
+
 		if (func->patched)
 			klp_unpatch_func(func);
+	}
 
-	obj->patched = false;
+	if (!nop)
+		obj->patched = false;
 }
 
 int klp_patch_object(struct klp_object *obj)

@@ -257,7 +266,7 @@ int klp_patch_object(struct klp_object *obj)
 	klp_for_each_func(obj, func) {
 		ret = klp_patch_func(func);
 		if (ret) {
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, false);
 			return ret;
 		}
 	}

@@ -266,11 +275,12 @@ int klp_patch_object(struct klp_object *obj)
 	return 0;
 }
 
-void klp_unpatch_objects(struct klp_patch *patch)
+/* if nop is set, only unpatch the nop functions */
+void klp_unpatch_objects(struct klp_patch *patch, bool nop)
 {
 	struct klp_object *obj;
 
 	klp_for_each_object(patch, obj)
 		if (obj->patched)
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, nop);
 }

kernel/livepatch/patch.h +2 -2

index 0db2271..52cc086 100644
@@ -27,7 +27,7 @@ struct klp_ops {
 struct klp_ops *klp_find_ops(unsigned long old_addr);
 
 int klp_patch_object(struct klp_object *obj);
-void klp_unpatch_object(struct klp_object *obj);
-void klp_unpatch_objects(struct klp_patch *patch);
+void klp_unpatch_object(struct klp_object *obj, bool nop);
+void klp_unpatch_objects(struct klp_patch *patch, bool nop);
 
 #endif /* _LIVEPATCH_PATCH_H */

kernel/livepatch/transition.c +46 -4

75 lines not on this page · raw message

Signature · 1 line

2.6.1

Josh Poimboeuf

2017-10-06 22:32 UTC #7 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 24 lines · Jason Baron

Since 'atomic replace' has completely replaced all previous livepatch modules, it explicitly disables all previous livepatch modules. However, previous livepatch modules that have been replaced, can be re-enabled if they have the 'replace' flag set. In this case the set of 'nop' functions is re-calculated, such that it replaces all others.
For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set then:
# insmod kpatch-a.ko
# insmod kpatch-b.ko
At this point we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
0
# cat /sys/kernel/livepatch/kpatch-b/enabled
1
To revert to the kpatch-a state we can do:
echo 1 > /sys/kernel/livepatch/kpatch-a/enabled

Quoted 7 lines

And now we have:

# cat /sys/kernel/livepatch/kpatch-a/enabled
1

# cat /sys/kernel/livepatch/kpatch-b/enabled
0

I don't really like allowing a previously replaced patch to replace the current patch. It's just more unnecessary complexity. If the user wants to atomically revert back to kpatch-a, they should be able to:

rmmod kpatch-a
insmod kpatch-a.ko

Quoted 4 lines

Note that it may be possible to unload (rmmod) replaced patches in some cases based on the consistency model, when we know that all the functions that are contained in the patch may no longer be in used, however its left as future work, if this functionality is desired.

If you don't allow a previously replaced patch to be enabled again, I think it would be trivial to let it be unloaded.

Quoted 3 lines

Also, __klp_enable_patch() calls klp_add_nops(), which necessitated moving a bunch of existing functions before __klp_enable_patch(). So there is a bit of churn in moving functions that are not modified.

To make review easier, can you put the moving of functions into a separate patch?

Signature · 1 line

Josh

Jason Baron

author

2017-10-10 17:27 UTC #8 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 33 lines · Josh Poimboeuf

Quoted 24 lines · Jason Baron
Since 'atomic replace' has completely replaced all previous livepatch modules, it explicitly disables all previous livepatch modules. However, previous livepatch modules that have been replaced, can be re-enabled if they have the 'replace' flag set. In this case the set of 'nop' functions is re-calculated, such that it replaces all others.
For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set then:
# insmod kpatch-a.ko
# insmod kpatch-b.ko
At this point we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
0
# cat /sys/kernel/livepatch/kpatch-b/enabled
1
To revert to the kpatch-a state we can do:
echo 1 > /sys/kernel/livepatch/kpatch-a/enabled
Quoted 7 lines
And now we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
1
# cat /sys/kernel/livepatch/kpatch-b/enabled
0

Quoted 7 lines

I don't really like allowing a previously replaced patch to replace the current patch. It's just more unnecessary complexity. If the user wants to atomically revert back to kpatch-a, they should be able to:
rmmod kpatch-a
insmod kpatch-a.ko

Right - that's how I sent v1 (using rmmod/insmod to revert), but it didn't account for the fact the patch or some functions may be marked 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in some cases 'rmmod' was not feasible, I thought it would be simpler from an operational pov to just say we always revert by re-enabling a previously replaced patch as opposed to rmmod/insmod.

Quoted 8 lines

Quoted 4 lines
Note that it may be possible to unload (rmmod) replaced patches in some cases based on the consistency model, when we know that all the functions that are contained in the patch may no longer be in used, however its left as future work, if this functionality is desired.
If you don't allow a previously replaced patch to be enabled again, I think it would be trivial to let it be unloaded.

The concern is around being replaced by 'immediate' functions and thus not knowing if the code is still in use.

Quoted 7 lines

Quoted 3 lines
Also, __klp_enable_patch() calls klp_add_nops(), which necessitated moving a bunch of existing functions before __klp_enable_patch(). So there is a bit of churn in moving functions that are not modified.
To make review easier, can you put the moving of functions into a separate patch?

Agreed - I've re-posted to this thread as v3.1.

Thanks,

-Jason

Miroslav Benes

maintainer

2017-10-17 09:02 UTC #9 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 36 lines · Jason Baron

Quoted 33 lines · Josh Poimboeuf
Quoted 31 lines · Jason Baron
Since 'atomic replace' has completely replaced all previous livepatch modules, it explicitly disables all previous livepatch modules. However, previous livepatch modules that have been replaced, can be re-enabled if they have the 'replace' flag set. In this case the set of 'nop' functions is re-calculated, such that it replaces all others.
For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set then:
# insmod kpatch-a.ko
# insmod kpatch-b.ko
At this point we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
0
# cat /sys/kernel/livepatch/kpatch-b/enabled
1
To revert to the kpatch-a state we can do:
echo 1 > /sys/kernel/livepatch/kpatch-a/enabled
And now we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
1
# cat /sys/kernel/livepatch/kpatch-b/enabled
0

Quoted 7 lines

Quoted 7 lines
I don't really like allowing a previously replaced patch to replace the current patch. It's just more unnecessary complexity. If the user wants to atomically revert back to kpatch-a, they should be able to:
rmmod kpatch-a
insmod kpatch-a.ko

I agree.

Quoted 13 lines

Right - that's how I sent v1 (using rmmod/insmod to revert), but it didn't account for the fact the patch or some functions may be marked 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in some cases 'rmmod' was not feasible, I thought it would be simpler from an operational pov to just say we always revert by re-enabling a previously replaced patch as opposed to rmmod/insmod.
Quoted 5 lines
Quoted 4 lines
Note that it may be possible to unload (rmmod) replaced patches in some cases based on the consistency model, when we know that all the functions that are contained in the patch may no longer be in used, however its left as future work, if this functionality is desired.

Quoted 6 lines

Quoted 3 lines
If you don't allow a previously replaced patch to be enabled again, I think it would be trivial to let it be unloaded.
The concern is around being replaced by 'immediate' functions and thus not knowing if the code is still in use.

Hm. Would it make sense to remove immediate and rely only on the consistency model? At least for the architectures where the model is implemented (x86_64)?

If not, then I'd keep such modules there without a possibility to remove them ever. If its functionality was required again, it would of course mean to insmod a new module with it.

Miroslav

Miroslav Benes

maintainer

2017-10-17 13:50 UTC #10 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 68 lines · Miroslav Benes

Quoted 36 lines · Jason Baron
Quoted 33 lines · Josh Poimboeuf
Quoted 31 lines · Jason Baron
Since 'atomic replace' has completely replaced all previous livepatch modules, it explicitly disables all previous livepatch modules. However, previous livepatch modules that have been replaced, can be re-enabled if they have the 'replace' flag set. In this case the set of 'nop' functions is re-calculated, such that it replaces all others.
For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set then:
# insmod kpatch-a.ko
# insmod kpatch-b.ko
At this point we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
0
# cat /sys/kernel/livepatch/kpatch-b/enabled
1
To revert to the kpatch-a state we can do:
echo 1 > /sys/kernel/livepatch/kpatch-a/enabled
And now we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
1
# cat /sys/kernel/livepatch/kpatch-b/enabled
0
Quoted 7 lines
Quoted 7 lines
I don't really like allowing a previously replaced patch to replace the current patch. It's just more unnecessary complexity. If the user wants to atomically revert back to kpatch-a, they should be able to:
rmmod kpatch-a
insmod kpatch-a.ko
I agree.
Quoted 13 lines
Right - that's how I sent v1 (using rmmod/insmod to revert), but it didn't account for the fact the patch or some functions may be marked 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in some cases 'rmmod' was not feasible, I thought it would be simpler from an operational pov to just say we always revert by re-enabling a previously replaced patch as opposed to rmmod/insmod.
Quoted 5 lines
Quoted 4 lines
Note that it may be possible to unload (rmmod) replaced patches in some cases based on the consistency model, when we know that all the functions that are contained in the patch may no longer be in used, however its left as future work, if this functionality is desired.
Quoted 6 lines
Quoted 3 lines
If you don't allow a previously replaced patch to be enabled again, I think it would be trivial to let it be unloaded.
The concern is around being replaced by 'immediate' functions and thus not knowing if the code is still in use.

Quoted 7 lines

Hm. Would it make sense to remove immediate and rely only on the consistency model? At least for the architectures where the model is implemented (x86_64)?
If not, then I'd keep such modules there without a possibility to remove them ever. If its functionality was required again, it would of course mean to insmod a new module with it.

Petr pointed out (offline) that immediate could still be useful. So let me describe how I envision the whole atomic replace semantics.

Let's have three functions - a, b, c. Patch 1 is immediate and patches a().

func a b c
patches 1i

Patch 2 is not immediate and patches b()

func a b c
patches 1i

Patch 3 is atomic replace, which patches a() and c().

func a b c
patches 1i

			2
		3r		3r

With 3 applied, 3versions of a() and c() are called, original b() is called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2 cannot be reenabled.

3 can be disabled. Original a() and c() will be called in such case. If one wants to have a() from patch 1 there, he would need to apply patch 4.

func a b c
patches 1i

			2
		3r		3r
		4i

Does it make sense? This is what I would expect and I think it is easier to implement. Whole initialization of nop functions could be done in klp_init_ functions, if I am not mistaken.

Thoughts?

Miroslav

Jason Baron

author

2017-10-18 03:33 UTC #11 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 95 lines · Miroslav Benes

Quoted 68 lines · Miroslav Benes
Quoted 36 lines · Jason Baron
Quoted 33 lines · Josh Poimboeuf
Quoted 31 lines · Jason Baron
Since 'atomic replace' has completely replaced all previous livepatch modules, it explicitly disables all previous livepatch modules. However, previous livepatch modules that have been replaced, can be re-enabled if they have the 'replace' flag set. In this case the set of 'nop' functions is re-calculated, such that it replaces all others.
For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set then:
# insmod kpatch-a.ko
# insmod kpatch-b.ko
At this point we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
0
# cat /sys/kernel/livepatch/kpatch-b/enabled
1
To revert to the kpatch-a state we can do:
echo 1 > /sys/kernel/livepatch/kpatch-a/enabled
And now we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
1
# cat /sys/kernel/livepatch/kpatch-b/enabled
0
Quoted 7 lines
Quoted 7 lines
I don't really like allowing a previously replaced patch to replace the current patch. It's just more unnecessary complexity. If the user wants to atomically revert back to kpatch-a, they should be able to:
rmmod kpatch-a
insmod kpatch-a.ko
I agree.
Quoted 13 lines
Right - that's how I sent v1 (using rmmod/insmod to revert), but it didn't account for the fact the patch or some functions may be marked 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in some cases 'rmmod' was not feasible, I thought it would be simpler from an operational pov to just say we always revert by re-enabling a previously replaced patch as opposed to rmmod/insmod.
Quoted 5 lines
Quoted 4 lines
Note that it may be possible to unload (rmmod) replaced patches in some cases based on the consistency model, when we know that all the functions that are contained in the patch may no longer be in used, however its left as future work, if this functionality is desired.
Quoted 6 lines
Quoted 3 lines
If you don't allow a previously replaced patch to be enabled again, I think it would be trivial to let it be unloaded.
The concern is around being replaced by 'immediate' functions and thus not knowing if the code is still in use.
Quoted 7 lines
Hm. Would it make sense to remove immediate and rely only on the consistency model? At least for the architectures where the model is implemented (x86_64)?
If not, then I'd keep such modules there without a possibility to remove them ever. If its functionality was required again, it would of course mean to insmod a new module with it.
Petr pointed out (offline) that immediate could still be useful. So let me describe how I envision the whole atomic replace semantics.
Let's have three functions - a, b, c. Patch 1 is immediate and patches a().
func a b c
patches 1i
Patch 2 is not immediate and patches b()
func a b c
patches 1i
			2
Patch 3 is atomic replace, which patches a() and c().

Quoted 8 lines

func a b c
patches 1i
			2
		3r		3r
With 3 applied, 3versions of a() and c() are called, original b() is called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2 cannot be reenabled.

Yes, if we change back to allowing the 'atomic replace' patch to drop the module reference on previous modules when possible, then yes 2 can be rmmoded. I originally started off with that model of being able to do 'revert' via rmmod and insmod of previous modules, but then we moved to a re-enable model in v4 based on the fact that in some cases we can not do the rmmod and insmod thing. For example if patch 3 was an immediate patch, then we would replace function b 'immediately' and thus not know when it was safe to rmmod patch 2. And in fact, in your example i think its safe to rmmod patch 1 as well, since in the transition to patch 3 we would have checked the func stack for the immediately preceding function for function a which would have been 1i, and thus since we know its not running we can rmmod patch 1 as well.

I'm happy to go back to rmmod/insmod for revert which seems to be the consensus here - it certainly helps clean up and avoid large stacks of modules (in most cases), which is nice.

Quoted 7 lines

3 can be disabled. Original a() and c() will be called in such case. If one wants to have a() from patch 1 there, he would need to apply patch 4.
func a b c
patches 1i

Quoted 9 lines

			2
		3r		3r
		4i
Does it make sense? This is what I would expect and I think it is easier to implement. Whole initialization of nop functions could be done in klp_init_ functions, if I am not mistaken.
Quoted 1 line
Thoughts?

yes, that makes sense to me. It might be worth clarifying the logic around when we can drop references on previous modules when an atomic revert patch is applied -

If the atomic replace patch has any immediate functions or is marked as immediate patch, then we can not drop the reference on the previous module, since it may still be in use. If the atomic replace patch does not contain any immediates, then we can drop the reference on the immediately preceding patch only. That is because there may have been previous transitions to immediate functions in the func stack, and the transition to the atomic replace patch only checks immediately preceding transition. It would be possible to check all of the previous immediate function transitions, but this adds complexity and seems like not a common pattern. So I would suggest that we just drop the reference on the previous patch if the atomic replace patch does not contain any immediate functions.

Thanks,

-Jason

Quoted 2 lines

Miroslav

Miroslav Benes

maintainer

2017-10-18 09:10 UTC #12 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 107 lines · Jason Baron

Quoted 95 lines · Miroslav Benes
Quoted 68 lines · Miroslav Benes
Quoted 36 lines · Jason Baron
Quoted 33 lines · Josh Poimboeuf
Quoted 31 lines · Jason Baron
Since 'atomic replace' has completely replaced all previous livepatch modules, it explicitly disables all previous livepatch modules. However, previous livepatch modules that have been replaced, can be re-enabled if they have the 'replace' flag set. In this case the set of 'nop' functions is re-calculated, such that it replaces all others.
For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set then:
# insmod kpatch-a.ko
# insmod kpatch-b.ko
At this point we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
0
# cat /sys/kernel/livepatch/kpatch-b/enabled
1
To revert to the kpatch-a state we can do:
echo 1 > /sys/kernel/livepatch/kpatch-a/enabled
And now we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
1
# cat /sys/kernel/livepatch/kpatch-b/enabled
0
Quoted 7 lines
Quoted 7 lines
I don't really like allowing a previously replaced patch to replace the current patch. It's just more unnecessary complexity. If the user wants to atomically revert back to kpatch-a, they should be able to:
rmmod kpatch-a
insmod kpatch-a.ko
I agree.
Quoted 13 lines
Right - that's how I sent v1 (using rmmod/insmod to revert), but it didn't account for the fact the patch or some functions may be marked 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in some cases 'rmmod' was not feasible, I thought it would be simpler from an operational pov to just say we always revert by re-enabling a previously replaced patch as opposed to rmmod/insmod.
Quoted 5 lines
Quoted 4 lines
Note that it may be possible to unload (rmmod) replaced patches in some cases based on the consistency model, when we know that all the functions that are contained in the patch may no longer be in used, however its left as future work, if this functionality is desired.
Quoted 6 lines
Quoted 3 lines
If you don't allow a previously replaced patch to be enabled again, I think it would be trivial to let it be unloaded.
The concern is around being replaced by 'immediate' functions and thus not knowing if the code is still in use.
Quoted 7 lines
Hm. Would it make sense to remove immediate and rely only on the consistency model? At least for the architectures where the model is implemented (x86_64)?
If not, then I'd keep such modules there without a possibility to remove them ever. If its functionality was required again, it would of course mean to insmod a new module with it.
Petr pointed out (offline) that immediate could still be useful. So let me describe how I envision the whole atomic replace semantics.
Let's have three functions - a, b, c. Patch 1 is immediate and patches a().
func a b c
patches 1i
Patch 2 is not immediate and patches b()
func a b c
patches 1i
			2
Patch 3 is atomic replace, which patches a() and c().
Quoted 8 lines
func a b c
patches 1i
			2
		3r		3r
With 3 applied, 3versions of a() and c() are called, original b() is called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2 cannot be reenabled.

Quoted 8 lines

Yes, if we change back to allowing the 'atomic replace' patch to drop the module reference on previous modules when possible, then yes 2 can be rmmoded. I originally started off with that model of being able to do 'revert' via rmmod and insmod of previous modules, but then we moved to a re-enable model in v4 based on the fact that in some cases we can not do the rmmod and insmod thing. For example if patch 3 was an immediate patch, then we would replace function b 'immediately' and thus not know when it was safe to rmmod patch 2.

Correct.

Quoted 5 lines

And in fact, in your example i think
its safe to rmmod patch 1 as well, since in the transition to patch 3 we would have checked the func stack for the immediately preceding function for function a which would have been 1i, and thus since we know its not running we can rmmod patch 1 as well.

Correct as well.

Quoted 21 lines

I'm happy to go back to rmmod/insmod for revert which seems to be the consensus here - it certainly helps clean up and avoid large stacks of modules (in most cases), which is nice.
Quoted 7 lines
3 can be disabled. Original a() and c() will be called in such case. If one wants to have a() from patch 1 there, he would need to apply patch 4.
func a b c
patches 1i
Quoted 9 lines
			2
		3r		3r
		4i
Does it make sense? This is what I would expect and I think it is easier to implement. Whole initialization of nop functions could be done in klp_init_ functions, if I am not mistaken.
Quoted 1 line
Thoughts?

Quoted 7 lines

yes, that makes sense to me. It might be worth clarifying the logic around when we can drop references on previous modules when an atomic revert patch is applied -
If the atomic replace patch has any immediate functions or is marked as immediate patch, then we can not drop the reference on the previous module, since it may still be in use.

Yes, then we know nothing about tasks in the system.

Quoted 10 lines

If the atomic replace patch does
not contain any immediates, then we can drop the reference on the immediately preceding patch only. That is because there may have been previous transitions to immediate functions in the func stack, and the transition to the atomic replace patch only checks immediately preceding transition. It would be possible to check all of the previous immediate function transitions, but this adds complexity and seems like not a common pattern. So I would suggest that we just drop the reference on the previous patch if the atomic replace patch does not contain any immediate functions.

It is even more complicated and it is not connected only to atomic replace patch (I realized this while reading the first part of your email and then you confirmed it with this paragraph). The consistency model is broken with respect to immediate patches.

func a
patches 1i

		2i
		3

Now, when you're applying 3, only 2i function is checked. But there might be a task sleeping in 1i. Such task would be migrated to 3, because we do not check 1 in klp_check_stack_func() at all.

I see three solutions.

1. Say it is an user's fault. Since it is not obvious and it is easy-to-make mistake, I would not go this way.

2. We can fix klp_check_stack_func() in an exact way you're proposing. We'd go back in func stack as long as there are immediate patches there. This adds complexity and I'm not sure if all the problems would be solved because scenarios how patches are stacked and applied to different functions may be quite complex.

3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.

Miroslav

Josh Poimboeuf

2017-10-18 11:05 UTC #13 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 5 lines · Miroslav Benes

3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.

I like this idea. When can we expect v3 of the force patches? :-)

Signature · 1 line

Josh

Miroslav Benes

maintainer

2017-10-18 11:29 UTC #14 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 8 lines · Josh Poimboeuf

Quoted 5 lines · Miroslav Benes
3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.
I like this idea. When can we expect v3 of the force patches? :-)

Hopefully soon. First I wanted to eliminate the amount of unreaded/unreviewed patches. Almost there. However there is OSS next week :/. But soon... :)

Miroslav

Petr Mladek

maintainer

2017-10-18 11:25 UTC #15 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

In reply to Miroslav Benes

Quoted 37 lines · Miroslav Benes

Quoted 10 lines · Jason Baron
If the atomic replace patch does
not contain any immediates, then we can drop the reference on the immediately preceding patch only. That is because there may have been previous transitions to immediate functions in the func stack, and the transition to the atomic replace patch only checks immediately preceding transition. It would be possible to check all of the previous immediate function transitions, but this adds complexity and seems like not a common pattern. So I would suggest that we just drop the reference on the previous patch if the atomic replace patch does not contain any immediate functions.
It is even more complicated and it is not connected only to atomic replace patch (I realized this while reading the first part of your email and then you confirmed it with this paragraph). The consistency model is broken with respect to immediate patches.
func a
patches 1i
		2i
		3
Now, when you're applying 3, only 2i function is checked. But there might be a task sleeping in 1i. Such task would be migrated to 3, because we do not check 1 in klp_check_stack_func() at all.
I see three solutions.
1. Say it is an user's fault. Since it is not obvious and it is easy-to-make mistake, I would not go this way.
2. We can fix klp_check_stack_func() in an exact way you're proposing. We'd go back in func stack as long as there are immediate patches there. This adds complexity and I'm not sure if all the problems would be solved because scenarios how patches are stacked and applied to different functions may be quite complex.

Quoted 5 lines

3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.

To make it clear. We currently rely on the immediate handling on architectures without a reliable stack checking. The question is if anyone uses it for another purpose in practice.

A solution would be to remove the per-func immediate flag and invert the logic of the per-patch one. We could rename it to something like "consistency_required" or "semantic_changes". A patch with this flag set then might be refused on systems without reliable stacks. Otherwise, the consistency model would be used for all patches.

As a result, all patches would be handled either using the consistency model or immediately. We would need to care about any mix of these.

Best Regards,
Petr

Jason Baron

author

2017-10-19 21:44 UTC #16 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 55 lines · Petr Mladek

Quoted 37 lines · Miroslav Benes
Quoted 10 lines · Jason Baron
If the atomic replace patch does
not contain any immediates, then we can drop the reference on the immediately preceding patch only. That is because there may have been previous transitions to immediate functions in the func stack, and the transition to the atomic replace patch only checks immediately preceding transition. It would be possible to check all of the previous immediate function transitions, but this adds complexity and seems like not a common pattern. So I would suggest that we just drop the reference on the previous patch if the atomic replace patch does not contain any immediate functions.
It is even more complicated and it is not connected only to atomic replace patch (I realized this while reading the first part of your email and then you confirmed it with this paragraph). The consistency model is broken with respect to immediate patches.
func a
patches 1i
		2i
		3
Now, when you're applying 3, only 2i function is checked. But there might be a task sleeping in 1i. Such task would be migrated to 3, because we do not check 1 in klp_check_stack_func() at all.
I see three solutions.
1. Say it is an user's fault. Since it is not obvious and it is easy-to-make mistake, I would not go this way.
2. We can fix klp_check_stack_func() in an exact way you're proposing. We'd go back in func stack as long as there are immediate patches there. This adds complexity and I'm not sure if all the problems would be solved because scenarios how patches are stacked and applied to different functions may be quite complex.
Quoted 5 lines
3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.
To make it clear. We currently rely on the immediate handling on architectures without a reliable stack checking. The question is if anyone uses it for another purpose in practice.
A solution would be to remove the per-func immediate flag and invert the logic of the per-patch one. We could rename it to something like "consistency_required" or "semantic_changes". A patch with this flag set then might be refused on systems without reliable stacks. Otherwise, the consistency model would be used for all patches.

Quoted 7 lines

As a result, all patches would be handled either using the consistency model or immediately. We would need to care about any mix of these.
Best Regards,
Petr

Hi,

So for atomic replace, it seems as if we don't want to allow replaced patches to be re-enabled but instead, allow them to rmmod/insmod'ed to allow revert.

In your above proposed model around immediate, if all patches are immediate then the atomic replace doesn't allow any of the previous patches to be removed from the kernel via rmmod, while if all patches are handled using the consistency model then all patches previous to the atomic replace patch could be removed via rmmod. So this would simplify the logic around when replaced patches could removed.

I was thinking in the current model to only allow the one preceding patch to the atomic replace patch to be rmmod'able, if the atomic replace patch was using the consistency model (to avoid complications with immediate functions/patches). And this could be extended to all *all* patches previous to the atomic replace patch to be rmmod'able when we move to the proposed model around immediates. So I don't think this should hold up the atomic replace patches?

Thanks,

-Jason

Petr Mladek

maintainer

2017-10-20 07:44 UTC #17 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 10 lines · Jason Baron

So for atomic replace, it seems as if we don't want to allow replaced patches to be re-enabled but instead, allow them to rmmod/insmod'ed to allow revert.
In your above proposed model around immediate, if all patches are immediate then the atomic replace doesn't allow any of the previous patches to be removed from the kernel via rmmod, while if all patches are handled using the consistency model then all patches previous to the atomic replace patch could be removed via rmmod. So this would simplify the logic around when replaced patches could removed.

yes

Quoted 7 lines

I was thinking in the current model to only allow the one preceding patch to the atomic replace patch to be rmmod'able, if the atomic replace patch was using the consistency model (to avoid complications with immediate functions/patches). And this could be extended to all *all* patches previous to the atomic replace patch to be rmmod'able when we move to the proposed model around immediates. So I don't think this should hold up the atomic replace patches?

Yes, this sounds reasonable from the atomic replace patch point of view.

Well, the consistency model is broken at the moment when immediate flag is used. We should fix this ASAP. IMHO, the fix might need to go upstream before the atomic replace feature. It might be even stable material.

Best Regards,
Petr

PS: One problem is that there are some conferences (Kernel Summit, Open Source Summit) next week. At least all livepatch-related SUSE people will be less reachable.

Miroslav Benes

maintainer

2017-10-20 08:59 UTC #18 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

In reply to Petr Mladek

Quoted 48 lines · Petr Mladek

Quoted 37 lines · Miroslav Benes
Quoted 10 lines · Jason Baron
If the atomic replace patch does
not contain any immediates, then we can drop the reference on the immediately preceding patch only. That is because there may have been previous transitions to immediate functions in the func stack, and the transition to the atomic replace patch only checks immediately preceding transition. It would be possible to check all of the previous immediate function transitions, but this adds complexity and seems like not a common pattern. So I would suggest that we just drop the reference on the previous patch if the atomic replace patch does not contain any immediate functions.
It is even more complicated and it is not connected only to atomic replace patch (I realized this while reading the first part of your email and then you confirmed it with this paragraph). The consistency model is broken with respect to immediate patches.
func a
patches 1i
		2i
		3
Now, when you're applying 3, only 2i function is checked. But there might be a task sleeping in 1i. Such task would be migrated to 3, because we do not check 1 in klp_check_stack_func() at all.
I see three solutions.
1. Say it is an user's fault. Since it is not obvious and it is easy-to-make mistake, I would not go this way.
2. We can fix klp_check_stack_func() in an exact way you're proposing. We'd go back in func stack as long as there are immediate patches there. This adds complexity and I'm not sure if all the problems would be solved because scenarios how patches are stacked and applied to different functions may be quite complex.
Quoted 5 lines
3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.
To make it clear. We currently rely on the immediate handling on architectures without a reliable stack checking. The question is if anyone uses it for another purpose in practice.

Quoted 6 lines

A solution would be to remove the per-func immediate flag and invert the logic of the per-patch one. We could rename it to something like "consistency_required" or "semantic_changes". A patch with this flag set then might be refused on systems without reliable stacks. Otherwise, the consistency model would be used for all patches.

I have a problem with this. I'd like to see the consistency model as a default and not something to ask for. It should be used always unless explicitly forbidden.

Just to be sure, we agreed to remove immediate, didn't we?

Miroslav

Jiri Kosina

2017-10-18 13:36 UTC #19 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

In reply to Miroslav Benes

Quoted 5 lines · Miroslav Benes

3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.

After brief off-thread discussion, I've been thinking about this a bit more and I also think that we should claim immediate "an experiment that failed", especially as the force functionality (which provides equal functionality from the userspace POV) will likely be there sonnish.

Thanks,

Signature · 2 lines

Jiri Kosina
SUSE Labs

Josh Poimboeuf

2017-10-18 16:14 UTC #20 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 8 lines · Jiri Kosina

Quoted 5 lines · Miroslav Benes
3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.

Quoted 4 lines

After brief off-thread discussion, I've been thinking about this a bit more and I also think that we should claim immediate "an experiment that failed", especially as the force functionality (which provides equal functionality from the userspace POV) will likely be there sonnish.

Agreed.

To clarify, we'll need the force patch before removing klp_patch.immediate, so we don't break non-x86 arches in the meantime.

On the other hand I think we could remove klp_func.immediate immediately.

While we're on the subject of removing code... :-)

I've mentioned this several times before, but the more features we add, the more obvious this point becomes: if we could figure out how to get rid of the "patching unloaded modules" feature, the code would be so much better, and I'd actually be able to fit the code in my brain. Then we could get rid of all these sneaky bugs that Miroslav and Petr keep finding, and I wouldn't get an uneasy feeling everytime somebody posts a new feature.

Here's one vague idea for how to achieve that. More ideas welcome.

1) Make the consistency model synchronous with respect to modules: don't allow any modules to load or unload until the patch transition is complete.

2) Instead of one big uber patch module which patches vmlinux and modules at the same time, make each patch module specific to a single object. Then bundle the patch modules together somehow into a "patch bundle" so they're treated as a single atomic unit.

3) The patch bundle, when loaded, would load some of its patch modules immediately (for those objects which are already loaded). For unloaded objects, the corresponding patch modules will be copied into memory but not loaded.

4) Then, when a to-be-patched module is loaded, the module loader loads it into memory and relocates it, but doesn't make it live. Then it loads the patch module from the memory blob, makes the patch module live, and then makes the to-be-patched module live.

(A variation would be to create a way for user space to load a module in the paused state. Then user space can handle the dependencies and do the patch juggling. I think that would mean depmod/modprobe would need to be involved.)

It could be a terrible idea, though it might be worth looking into.

Signature · 1 line

Josh

Miroslav Benes

maintainer

2017-10-19 08:30 UTC #21 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 14 lines · Josh Poimboeuf

Quoted 8 lines · Jiri Kosina
Quoted 5 lines · Miroslav Benes
3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.
Quoted 4 lines
After brief off-thread discussion, I've been thinking about this a bit more and I also think that we should claim immediate "an experiment that failed", especially as the force functionality (which provides equal functionality from the userspace POV) will likely be there sonnish.

Quoted 4 lines

Agreed.
To clarify, we'll need the force patch before removing klp_patch.immediate, so we don't break non-x86 arches in the meantime.

Yes, that is correct. I've just started to work on the force patch.

Quoted 36 lines

On the other hand I think we could remove klp_func.immediate immediately.
While we're on the subject of removing code... :-)
I've mentioned this several times before, but the more features we add, the more obvious this point becomes: if we could figure out how to get rid of the "patching unloaded modules" feature, the code would be so much better, and I'd actually be able to fit the code in my brain. Then we could get rid of all these sneaky bugs that Miroslav and Petr keep finding, and I wouldn't get an uneasy feeling everytime somebody posts a new feature.
Here's one vague idea for how to achieve that. More ideas welcome.
1) Make the consistency model synchronous with respect to modules: don't allow any modules to load or unload until the patch transition is complete.
2) Instead of one big uber patch module which patches vmlinux and modules at the same time, make each patch module specific to a single object. Then bundle the patch modules together somehow into a "patch bundle" so they're treated as a single atomic unit.
3) The patch bundle, when loaded, would load some of its patch modules immediately (for those objects which are already loaded). For unloaded objects, the corresponding patch modules will be copied into memory but not loaded.
4) Then, when a to-be-patched module is loaded, the module loader loads it into memory and relocates it, but doesn't make it live. Then it loads the patch module from the memory blob, makes the patch module live, and then makes the to-be-patched module live.

Quoted 4 lines

(A variation would be to create a way for user space to load a module in the paused state. Then user space can handle the dependencies and do the patch juggling. I think that would mean depmod/modprobe would need to be involved.)

It might be easier to do at least part of it in userspace.

Quoted 1 line

It could be a terrible idea, though it might be worth looking into.

I'm worried that we would only move the complexity elsewhere, but maybe not. We'd remove the code from kernel/livepatch/, but some non-trivial changes would go to the module loader.

Jessica, would that be possible?

We need to discuss it with packaging if that'd be possible to deliver it.

Anyway, interesting idea.

Miroslav

Josh Poimboeuf

2017-10-19 10:57 UTC #22 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 16 lines · Miroslav Benes

Quoted 14 lines · Josh Poimboeuf
Quoted 12 lines · Jiri Kosina
Quoted 5 lines · Miroslav Benes
3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.
After brief off-thread discussion, I've been thinking about this a bit more and I also think that we should claim immediate "an experiment that failed", especially as the force functionality (which provides equal functionality from the userspace POV) will likely be there sonnish.

Quoted 6 lines

Quoted 4 lines
Agreed.
To clarify, we'll need the force patch before removing klp_patch.immediate, so we don't break non-x86 arches in the meantime.
Yes, that is correct. I've just started to work on the force patch.

Good!

Quoted 41 lines

Quoted 36 lines
On the other hand I think we could remove klp_func.immediate immediately.
While we're on the subject of removing code... :-)
I've mentioned this several times before, but the more features we add, the more obvious this point becomes: if we could figure out how to get rid of the "patching unloaded modules" feature, the code would be so much better, and I'd actually be able to fit the code in my brain. Then we could get rid of all these sneaky bugs that Miroslav and Petr keep finding, and I wouldn't get an uneasy feeling everytime somebody posts a new feature.
Here's one vague idea for how to achieve that. More ideas welcome.
1) Make the consistency model synchronous with respect to modules: don't allow any modules to load or unload until the patch transition is complete.
2) Instead of one big uber patch module which patches vmlinux and modules at the same time, make each patch module specific to a single object. Then bundle the patch modules together somehow into a "patch bundle" so they're treated as a single atomic unit.
3) The patch bundle, when loaded, would load some of its patch modules immediately (for those objects which are already loaded). For unloaded objects, the corresponding patch modules will be copied into memory but not loaded.
4) Then, when a to-be-patched module is loaded, the module loader loads it into memory and relocates it, but doesn't make it live. Then it loads the patch module from the memory blob, makes the patch module live, and then makes the to-be-patched module live.
Quoted 4 lines
(A variation would be to create a way for user space to load a module in the paused state. Then user space can handle the dependencies and do the patch juggling. I think that would mean depmod/modprobe would need to be involved.)

Quoted 7 lines

It might be easier to do at least part of it in userspace.
Quoted 1 line
It could be a terrible idea, though it might be worth looking into.
I'm worried that we would only move the complexity elsewhere, but maybe not. We'd remove the code from kernel/livepatch/, but some non-trivial changes would go to the module loader.

Yes, that's a very real possibility. In order for it to be an improvement, it would have to be self-contained. We don't want to make the module code worse either.

Quoted 5 lines

Jessica, would that be possible?
We need to discuss it with packaging if that'd be possible to deliver it.
Anyway, interesting idea.

Yeah, just an idea to consider... I'd like to play around with it someday.

Signature · 1 line

Josh

Jason Baron

author

2017-10-19 21:53 UTC #23 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

In reply to Miroslav Benes

Quoted 170 lines · Miroslav Benes

Quoted 107 lines · Jason Baron
Quoted 95 lines · Miroslav Benes
Quoted 68 lines · Miroslav Benes
Quoted 36 lines · Jason Baron
Quoted 33 lines · Josh Poimboeuf
Quoted 31 lines · Jason Baron
Since 'atomic replace' has completely replaced all previous livepatch modules, it explicitly disables all previous livepatch modules. However, previous livepatch modules that have been replaced, can be re-enabled if they have the 'replace' flag set. In this case the set of 'nop' functions is re-calculated, such that it replaces all others.
For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set then:
# insmod kpatch-a.ko
# insmod kpatch-b.ko
At this point we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
0
# cat /sys/kernel/livepatch/kpatch-b/enabled
1
To revert to the kpatch-a state we can do:
echo 1 > /sys/kernel/livepatch/kpatch-a/enabled
And now we have:
# cat /sys/kernel/livepatch/kpatch-a/enabled
1
# cat /sys/kernel/livepatch/kpatch-b/enabled
0
Quoted 7 lines
Quoted 7 lines
I don't really like allowing a previously replaced patch to replace the current patch. It's just more unnecessary complexity. If the user wants to atomically revert back to kpatch-a, they should be able to:
rmmod kpatch-a
insmod kpatch-a.ko
I agree.
Quoted 13 lines
Right - that's how I sent v1 (using rmmod/insmod to revert), but it didn't account for the fact the patch or some functions may be marked 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in some cases 'rmmod' was not feasible, I thought it would be simpler from an operational pov to just say we always revert by re-enabling a previously replaced patch as opposed to rmmod/insmod.
Quoted 5 lines
Quoted 4 lines
Note that it may be possible to unload (rmmod) replaced patches in some cases based on the consistency model, when we know that all the functions that are contained in the patch may no longer be in used, however its left as future work, if this functionality is desired.
Quoted 6 lines
Quoted 3 lines
If you don't allow a previously replaced patch to be enabled again, I think it would be trivial to let it be unloaded.
The concern is around being replaced by 'immediate' functions and thus not knowing if the code is still in use.
Quoted 7 lines
Hm. Would it make sense to remove immediate and rely only on the consistency model? At least for the architectures where the model is implemented (x86_64)?
If not, then I'd keep such modules there without a possibility to remove them ever. If its functionality was required again, it would of course mean to insmod a new module with it.
Petr pointed out (offline) that immediate could still be useful. So let me describe how I envision the whole atomic replace semantics.
Let's have three functions - a, b, c. Patch 1 is immediate and patches a().
func a b c
patches 1i
Patch 2 is not immediate and patches b()
func a b c
patches 1i
			2
Patch 3 is atomic replace, which patches a() and c().
Quoted 8 lines
func a b c
patches 1i
			2
		3r		3r
With 3 applied, 3versions of a() and c() are called, original b() is called. 2 can be rmmoded. 1 cannot, because it is immediate. 1 and 2 cannot be reenabled.
Quoted 8 lines
Yes, if we change back to allowing the 'atomic replace' patch to drop the module reference on previous modules when possible, then yes 2 can be rmmoded. I originally started off with that model of being able to do 'revert' via rmmod and insmod of previous modules, but then we moved to a re-enable model in v4 based on the fact that in some cases we can not do the rmmod and insmod thing. For example if patch 3 was an immediate patch, then we would replace function b 'immediately' and thus not know when it was safe to rmmod patch 2.
Correct.
Quoted 5 lines
And in fact, in your example i think
its safe to rmmod patch 1 as well, since in the transition to patch 3 we would have checked the func stack for the immediately preceding function for function a which would have been 1i, and thus since we know its not running we can rmmod patch 1 as well.
Correct as well.
Quoted 21 lines
I'm happy to go back to rmmod/insmod for revert which seems to be the consensus here - it certainly helps clean up and avoid large stacks of modules (in most cases), which is nice.
Quoted 7 lines
3 can be disabled. Original a() and c() will be called in such case. If one wants to have a() from patch 1 there, he would need to apply patch 4.
func a b c
patches 1i
Quoted 9 lines
			2
		3r		3r
		4i
Does it make sense? This is what I would expect and I think it is easier to implement. Whole initialization of nop functions could be done in klp_init_ functions, if I am not mistaken.
Quoted 1 line
Thoughts?
Quoted 7 lines
yes, that makes sense to me. It might be worth clarifying the logic around when we can drop references on previous modules when an atomic revert patch is applied -
If the atomic replace patch has any immediate functions or is marked as immediate patch, then we can not drop the reference on the previous module, since it may still be in use.
Yes, then we know nothing about tasks in the system.
Quoted 10 lines
If the atomic replace patch does
not contain any immediates, then we can drop the reference on the immediately preceding patch only. That is because there may have been previous transitions to immediate functions in the func stack, and the transition to the atomic replace patch only checks immediately preceding transition. It would be possible to check all of the previous immediate function transitions, but this adds complexity and seems like not a common pattern. So I would suggest that we just drop the reference on the previous patch if the atomic replace patch does not contain any immediate functions.

Quoted 4 lines

It is even more complicated and it is not connected only to atomic replace patch (I realized this while reading the first part of your email and then you confirmed it with this paragraph). The consistency model is broken with respect to immediate patches.

Indeed. I came to the same conclusion.

Quoted 12 lines

func a
patches 1i
		2i
		3
Now, when you're applying 3, only 2i function is checked. But there might be a task sleeping in 1i. Such task would be migrated to 3, because we do not check 1 in klp_check_stack_func() at all.
I see three solutions.

Quoted 9 lines

1. Say it is an user's fault. Since it is not obvious and it is easy-to-make mistake, I would not go this way.
2. We can fix klp_check_stack_func() in an exact way you're proposing. We'd go back in func stack as long as there are immediate patches there. This adds complexity and I'm not sure if all the problems would be solved because scenarios how patches are stacked and applied to different functions may be quite complex.

I think if its fixed in the current code (before atomic replace is introduced), then it would allow the atomic replace to drop all references to previous modules, since we would know they are no longer in use.

Thanks,

-Jason

Quoted 8 lines

3. Drop immediate. It causes problems only and its advantages on x86_64 are theoretical. You would still need to solve the interaction with atomic replace on other architecture with immediate preserved, but that may be easier. Or we can be aggressive and drop immediate completely. The force transition I proposed earlier could achieve the same.
Miroslav

Miroslav Benes

maintainer

2017-10-20 09:03 UTC #24 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

Quoted 30 lines

Quoted 5 lines
It is even more complicated and it is not connected only to atomic replace patch (I realized this while reading the first part of your email and then you confirmed it with this paragraph). The consistency model is broken with respect to immediate patches.
Indeed. I came to the same conclusion.
Quoted 12 lines
func a
patches 1i
		2i
		3
Now, when you're applying 3, only 2i function is checked. But there might be a task sleeping in 1i. Such task would be migrated to 3, because we do not check 1 in klp_check_stack_func() at all.
I see three solutions.
Quoted 9 lines
1. Say it is an user's fault. Since it is not obvious and it is easy-to-make mistake, I would not go this way.
2. We can fix klp_check_stack_func() in an exact way you're proposing. We'd go back in func stack as long as there are immediate patches there. This adds complexity and I'm not sure if all the problems would be solved because scenarios how patches are stacked and applied to different functions may be quite complex.

Quoted 4 lines

I think if its fixed in the current code (before atomic replace is introduced), then it would allow the atomic replace to drop all references to previous modules, since we would know they are no longer in use.

That's correct. Although, I'll submit my force patch soon, we can remove immediate afterwards and atomic replace should be simpler with that.

Miroslav

Petr Mladek

maintainer

2017-10-17 14:27 UTC #25 lore raw

Re: [PATCH v3 2/2] livepatch: add atomic replace

In reply to Miroslav Benes

Quoted 4 lines · Miroslav Benes

Quoted 3 lines · Jason Baron
Quoted 2 lines · Josh Poimboeuf
I don't really like allowing a previously replaced patch to replace the current patch. It's just more unnecessary complexity.

I am sorry to say but it really makes the code too complex.

Quoted 13 lines

Quoted 6 lines
Quoted 6 lines
If the user
wants to atomically revert back to kpatch-a, they should be able to:
rmmod kpatch-a
insmod kpatch-a.ko
Quoted 7 lines
Right - that's how I sent v1 (using rmmod/insmod to revert), but it didn't account for the fact the patch or some functions may be marked 'immediate' and thus its not possible to just do 'rmmod'. Thus, since in some cases 'rmmod' was not feasible, I thought it would be simpler from an operational pov to just say we always revert by re-enabling a previously replaced patch as opposed to rmmod/insmod.

Quoted 7 lines

Hm. Would it make sense to remove immediate and rely only on the consistency model? At least for the architectures where the model is implemented (x86_64)?
If not, then I'd keep such modules there without a possibility to remove them ever. If its functionality was required again, it would of course mean to insmod a new module with it.

I am fine with this compromise. It seems to be the only way to keep the livepatch code somehow sane.

Best Regards,
Petr

Jason Baron

author

2017-10-10 17:19 UTC #26 lore raw

PATCH 2/3 livepatch: shuffle core.c function order

In reply to Jason Baron

In preparation for __klp_enable_patch() to call a number of 'static' functions, in a subsequent patch, move them earlier in core.c. This patch should be a nop from a functional pov.

Signed-off-by Jason Baron <[email protected]>

Cc Josh Poimboeuf <[email protected]>

Cc Jessica Yu <[email protected]>

Cc Jiri Kosina <[email protected]>

Cc Miroslav Benes <[email protected]>

Cc Petr Mladek <[email protected]>

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

kernel/livepatch/core.c +173 ~~-173~~

index 0d92fe6..871ea88 100644
@@ -377,6 +377,179 @@ static int klp_write_object_relocations(struct module *pmod,
 	return ret;
 }
 
+static void klp_kobj_release_object(struct kobject *kobj)
+{
+}
+
+static struct kobj_type klp_ktype_object = {
+	.release = klp_kobj_release_object,
+	.sysfs_ops = &kobj_sysfs_ops,
+};
+
+static void klp_kobj_release_func(struct kobject *kobj)
+{
+}
+
+static struct kobj_type klp_ktype_func = {
+	.release = klp_kobj_release_func,
+	.sysfs_ops = &kobj_sysfs_ops,
+};
+
+/*
+ * Free all functions' kobjects in the array up to some limit. When limit is
+ * NULL, all kobjects are freed.
+ */
+static void klp_free_funcs_limited(struct klp_object *obj,
+				   struct klp_func *limit)
+{
+	struct klp_func *func;
+
+	for (func = obj->funcs; func->old_name && func != limit; func++)
+		kobject_put(&func->kobj);
+}
+
+/* Clean up when a patched object is unloaded */
+static void klp_free_object_loaded(struct klp_object *obj)
+{
+	struct klp_func *func;
+
+	obj->mod = NULL;
+
+	klp_for_each_func(obj, func)
+		func->old_addr = 0;
+}
+
+/*
+ * Free all objects' kobjects in the array up to some limit. When limit is
+ * NULL, all kobjects are freed.
+ */
+static void klp_free_objects_limited(struct klp_patch *patch,
+				     struct klp_object *limit)
+{
+	struct klp_object *obj;
+
+	for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
+		klp_free_funcs_limited(obj, NULL);
+		kobject_put(&obj->kobj);
+	}
+}
+
+static void klp_free_patch(struct klp_patch *patch)
+{
+	klp_free_objects_limited(patch, NULL);
+	if (!list_empty(&patch->list))
+		list_del(&patch->list);
+}
+
+static int klp_init_func(struct klp_object *obj, struct klp_func *func)
+{
+	if (!func->old_name || !func->new_func)
+		return -EINVAL;
+
+	INIT_LIST_HEAD(&func->stack_node);
+	func->patched = false;
+	func->transition = false;
+
+	/* The format for the sysfs directory is <function,sympos> where sympos
+	 * is the nth occurrence of this symbol in kallsyms for the patched
+	 * object. If the user selects 0 for old_sympos, then 1 will be used
+	 * since a unique symbol will be the first occurrence.
+	 */
+	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
+				    &obj->kobj, "%s,%lu", func->old_name,
+				    func->old_sympos ? func->old_sympos : 1);
+}
+
+/* Arches may override this to finish any remaining arch-specific tasks */
+void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
+					struct klp_object *obj)
+{
+}
+
+/* parts of the initialization that is done only when the object is loaded */
+static int klp_init_object_loaded(struct klp_patch *patch,
+				  struct klp_object *obj)
+{
+	struct klp_func *func;
+	int ret;
+
+	module_disable_ro(patch->mod);
+	ret = klp_write_object_relocations(patch->mod, obj);
+	if (ret) {
+		module_enable_ro(patch->mod, true);
+		return ret;
+	}
+
+	arch_klp_init_object_loaded(patch, obj);
+	module_enable_ro(patch->mod, true);
+
+	klp_for_each_func(obj, func) {
+		ret = klp_find_object_symbol(obj->name, func->old_name,
+					     func->old_sympos,
+					     &func->old_addr);
+		if (ret)
+			return ret;
+
+		ret = kallsyms_lookup_size_offset(func->old_addr,
+						  &func->old_size, NULL);
+		if (!ret) {
+			pr_err("kallsyms size lookup failed for '%s'\n",
+			       func->old_name);
+			return -ENOENT;
+		}
+
+		ret = kallsyms_lookup_size_offset((unsigned long)func->new_func,
+						  &func->new_size, NULL);
+		if (!ret) {
+			pr_err("kallsyms size lookup failed for '%s' replacement\n",
+			       func->old_name);
+			return -ENOENT;
+		}
+	}
+
+	return 0;
+}
+
+static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
+{
+	struct klp_func *func;
+	int ret;
+	const char *name;
+
+	if (!obj->funcs)
+		return -EINVAL;
+
+	obj->patched = false;
+	obj->mod = NULL;
+
+	klp_find_object_module(obj);
+
+	name = klp_is_module(obj) ? obj->name : "vmlinux";
+	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
+				   &patch->kobj, "%s", name);
+	if (ret)
+		return ret;
+
+	klp_for_each_func(obj, func) {
+		ret = klp_init_func(obj, func);
+		if (ret)
+			goto free;
+	}
+
+	if (klp_is_object_loaded(obj)) {
+		ret = klp_init_object_loaded(patch, obj);
+		if (ret)
+			goto free;
+	}
+
+	return 0;
+
+free:
+	klp_free_funcs_limited(obj, func);
+	kobject_put(&obj->kobj);
+	return ret;
+}
+
 static int __klp_disable_patch(struct klp_patch *patch)
 {
 	if (klp_transition_patch)

@@ -630,179 +803,6 @@ static struct kobj_type klp_ktype_patch = {
 	.default_attrs = klp_patch_attrs,
 };
 
-static void klp_kobj_release_object(struct kobject *kobj)
-{
-}
-
-static struct kobj_type klp_ktype_object = {
-	.release = klp_kobj_release_object,
-	.sysfs_ops = &kobj_sysfs_ops,
-};
-
-static void klp_kobj_release_func(struct kobject *kobj)
-{
-}
-
-static struct kobj_type klp_ktype_func = {
-	.release = klp_kobj_release_func,
-	.sysfs_ops = &kobj_sysfs_ops,
-};
-
-/*
- * Free all functions' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
- */
-static void klp_free_funcs_limited(struct klp_object *obj,
-				   struct klp_func *limit)
-{
-	struct klp_func *func;
-
-	for (func = obj->funcs; func->old_name && func != limit; func++)
-		kobject_put(&func->kobj);
-}
-
-/* Clean up when a patched object is unloaded */
-static void klp_free_object_loaded(struct klp_object *obj)
-{
-	struct klp_func *func;
-
-	obj->mod = NULL;
-
-	klp_for_each_func(obj, func)
-		func->old_addr = 0;
-}
-
-/*
- * Free all objects' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
- */
-static void klp_free_objects_limited(struct klp_patch *patch,
-				     struct klp_object *limit)
-{
-	struct klp_object *obj;
-
-	for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
-		klp_free_funcs_limited(obj, NULL);
-		kobject_put(&obj->kobj);
-	}
-}
-
-static void klp_free_patch(struct klp_patch *patch)
-{
-	klp_free_objects_limited(patch, NULL);
-	if (!list_empty(&patch->list))
-		list_del(&patch->list);
-}
-
-static int klp_init_func(struct klp_object *obj, struct klp_func *func)
-{
-	if (!func->old_name || !func->new_func)
-		return -EINVAL;
-
-	INIT_LIST_HEAD(&func->stack_node);
-	func->patched = false;
-	func->transition = false;
-
-	/* The format for the sysfs directory is <function,sympos> where sympos
-	 * is the nth occurrence of this symbol in kallsyms for the patched
-	 * object. If the user selects 0 for old_sympos, then 1 will be used
-	 * since a unique symbol will be the first occurrence.
-	 */
-	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
-				    &obj->kobj, "%s,%lu", func->old_name,
-				    func->old_sympos ? func->old_sympos : 1);
-}
-
-/* Arches may override this to finish any remaining arch-specific tasks */
-void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
-					struct klp_object *obj)
-{
-}
-
-/* parts of the initialization that is done only when the object is loaded */
-static int klp_init_object_loaded(struct klp_patch *patch,
-				  struct klp_object *obj)
-{
-	struct klp_func *func;
-	int ret;
-
-	module_disable_ro(patch->mod);
-	ret = klp_write_object_relocations(patch->mod, obj);
-	if (ret) {
-		module_enable_ro(patch->mod, true);
-		return ret;
-	}
-
-	arch_klp_init_object_loaded(patch, obj);
-	module_enable_ro(patch->mod, true);
-
-	klp_for_each_func(obj, func) {
-		ret = klp_find_object_symbol(obj->name, func->old_name,
-					     func->old_sympos,
-					     &func->old_addr);
-		if (ret)
-			return ret;
-
-		ret = kallsyms_lookup_size_offset(func->old_addr,
-						  &func->old_size, NULL);
-		if (!ret) {
-			pr_err("kallsyms size lookup failed for '%s'\n",
-			       func->old_name);
-			return -ENOENT;
-		}
-
-		ret = kallsyms_lookup_size_offset((unsigned long)func->new_func,
-						  &func->new_size, NULL);
-		if (!ret) {
-			pr_err("kallsyms size lookup failed for '%s' replacement\n",
-			       func->old_name);
-			return -ENOENT;
-		}
-	}
-
-	return 0;
-}
-
-static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
-{
-	struct klp_func *func;
-	int ret;
-	const char *name;
-
-	if (!obj->funcs)
-		return -EINVAL;
-
-	obj->patched = false;
-	obj->mod = NULL;
-
-	klp_find_object_module(obj);
-
-	name = klp_is_module(obj) ? obj->name : "vmlinux";
-	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
-				   &patch->kobj, "%s", name);
-	if (ret)
-		return ret;
-
-	klp_for_each_func(obj, func) {
-		ret = klp_init_func(obj, func);
-		if (ret)
-			goto free;
-	}
-
-	if (klp_is_object_loaded(obj)) {
-		ret = klp_init_object_loaded(patch, obj);
-		if (ret)
-			goto free;
-	}
-
-	return 0;
-
-free:
-	klp_free_funcs_limited(obj, func);
-	kobject_put(&obj->kobj);
-	return ret;
-}
-
 static void klp_init_patch_dyn(struct klp_patch *patch)
 {
 	struct klp_object *obj;

Signature · 1 line

2.6.1

Jason Baron

author

2017-10-10 17:20 UTC #27 lore raw

PATCH 3/3 livepatch: add atomic replace

In reply to Jason Baron

Sometimes we would like to revert a particular fix. This is currently This is not easy because we want to keep all other fixes active and we could revert only the last applied patch.

For example, if kpatch-a.ko and kpatch-b.ko have the 'replace' flag set then:

# insmod kpatch-a.ko
# insmod kpatch-b.ko

At this point we have:

# cat /sys/kernel/livepatch/kpatch-a/enabled
0

# cat /sys/kernel/livepatch/kpatch-b/enabled
1

To revert to the kpatch-a state we can do:

echo 1 > /sys/kernel/livepatch/kpatch-a/enabled

And now we have:

# cat /sys/kernel/livepatch/kpatch-a/enabled
1

# cat /sys/kernel/livepatch/kpatch-b/enabled
0

Signed-off-by Jason Baron <[email protected]>

Cc Josh Poimboeuf <[email protected]>

Cc Jessica Yu <[email protected]>

Cc Jiri Kosina <[email protected]>

Cc Miroslav Benes <[email protected]>

Cc Petr Mladek <[email protected]>

---
 include/linux/livepatch.h     |   4 +
 kernel/livepatch/core.c       | 306 +++++++++++++++++++++++++++++++++++++++---
 kernel/livepatch/core.h       |   6 +
 kernel/livepatch/patch.c      |  22 ++-
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  50 ++++++-
 6 files changed, 364 insertions(+), 28 deletions(-)

include/linux/livepatch.h +4 -0

index e03ce11..93e8d44 100644
@@ -51,6 +51,7 @@
  * @new_size:	    size of the new function
  * @patched:	    the func has been added to the klp_ops list
  * @transition:	    the func is currently being applied or reverted
+ * @nop:	    used to revert a previously patched function
  *
  * The patched and transition variables define the func's patching state.  When
  * patching, a func is always in one of the following states:

@@ -89,6 +90,7 @@ struct klp_func {
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
+	bool nop;
 };
 
 /**

@@ -121,6 +123,7 @@ struct klp_object {
  * @mod:	  reference to the live patch module
  * @objs:	  object entries for kernel objects to be patched
  * @immediate:	  patch all funcs immediately, bypassing safety mechanisms
+ * @replace:	  revert previously patched functions not included in the patch
  * @list:	  list node for global list of registered patches
  * @kobj:	  kobject for sysfs resources
  * @nop_obj_list: head of list for dynamically allocated struct klp_object

@@ -132,6 +135,7 @@ struct klp_patch {
 	struct module *mod;
 	struct klp_object *objs;
 	bool immediate;
+	bool replace;
 
 	/* internal */
 	struct list_head list;

kernel/livepatch/core.c +290 ~~-16~~

428 lines not on this page · raw message

kernel/livepatch/core.h +6 -0

index c74f24c..fe45ee2 100644
@@ -1,6 +1,12 @@
 #ifndef _LIVEPATCH_CORE_H
 #define _LIVEPATCH_CORE_H
 
+#include <linux/livepatch.h>
+
 extern struct mutex klp_mutex;
+extern struct list_head klp_patches;
+extern struct list_head klp_replaced_patches;
+
+void klp_remove_nops(struct klp_patch *patch);
 
 #endif /* _LIVEPATCH_CORE_H */

kernel/livepatch/patch.c +16 -6

index 52c4e90..aae6e4d 100644
@@ -117,6 +117,9 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 		}
 	}
 
+	/* if its a 'nop', then run the original function */
+	if (func->nop)
+		goto unlock;
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	preempt_enable_notrace();

@@ -235,15 +238,21 @@ static int klp_patch_func(struct klp_func *func)
 	return ret;
 }
 
-void klp_unpatch_object(struct klp_object *obj)
+/* if nop is set, only unpatch the nop functions */
+void klp_unpatch_object(struct klp_object *obj, bool nop)
 {
 	struct klp_func *func;
 
-	klp_for_each_func(obj, func)
+	klp_for_each_func(obj, func) {
+		if (nop && !func->nop)
+			continue;
+
 		if (func->patched)
 			klp_unpatch_func(func);
+	}
 
-	obj->patched = false;
+	if (!nop)
+		obj->patched = false;
 }
 
 int klp_patch_object(struct klp_object *obj)

@@ -257,7 +266,7 @@ int klp_patch_object(struct klp_object *obj)
 	klp_for_each_func(obj, func) {
 		ret = klp_patch_func(func);
 		if (ret) {
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, false);
 			return ret;
 		}
 	}

@@ -266,11 +275,12 @@ int klp_patch_object(struct klp_object *obj)
 	return 0;
 }
 
-void klp_unpatch_objects(struct klp_patch *patch)
+/* if nop is set, only unpatch the nop functions */
+void klp_unpatch_objects(struct klp_patch *patch, bool nop)
 {
 	struct klp_object *obj;
 
 	klp_for_each_object(patch, obj)
 		if (obj->patched)
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, nop);
 }

kernel/livepatch/patch.h +2 -2

index 0db2271..52cc086 100644
@@ -27,7 +27,7 @@ struct klp_ops {
 struct klp_ops *klp_find_ops(unsigned long old_addr);
 
 int klp_patch_object(struct klp_object *obj);
-void klp_unpatch_object(struct klp_object *obj);
-void klp_unpatch_objects(struct klp_patch *patch);
+void klp_unpatch_object(struct klp_object *obj, bool nop);
+void klp_unpatch_objects(struct klp_patch *patch, bool nop);
 
 #endif /* _LIVEPATCH_PATCH_H */

kernel/livepatch/transition.c +46 -4

75 lines not on this page · raw message

Signature · 1 line

2.6.1