livepatch: Atomic replace feature
Subsystems (2)
Petr Mladek
maintainercover livepatch: Atomic replace feature
The atomic replace allows to create cumulative patches. They are useful when you maintain many livepatches and want to remove one that is lower on the stack. In addition it is very useful when more patches touch the same function and there are dependencies between them.
This version is heavily refactored and cleaned based on feedback from Josh. There are actually only three functional changes.
It still passes the first draft of the selfttest from Joe that can
be found at https://lkml.kernel.org/r/[email protected]
Changes against v10:
+ Bug fixes and functional changes:
+ Handle Nops in klp_ftrace_handled() to avoid infinite loop [Mirek]
+ Really add dynamically allocated klp_object into the list [Petr]
+ Clear patch->replace when transition finishes [Josh]
+ Refactoring and clean up [Josh]:
+ Replace enum types with bools
+ Avoid using ERR_PTR
+ Remove too paranoid warnings
+ Distinguish registered patches by a flag instead of a list
+ Squash some functions
+ Update comments, documentation, and commit messages
+ Squashed and split patches to do more controversial changes later
Changes against v9:
+ Fixed check of valid NOPs for already loaded objects, regression introduced in v9 [Joe, Mirek]
+ Allow to replace even disabled patches [Evgenii]
Changes against v8:
+ Fixed handling of statically defined struct klp_object with empty array of functions [Joe, Mirek]
+ Removed redundant func->new_func assignment for NOPs [Mirek]
+ Improved some wording [Mirek]
Changes against v7:
+ Fixed handling of NOPs for not-yet-loaded modules
+ Made klp_replaced_patches list static [Mirek]
+ Made klp_free_object() public later [Mirek]
+ Fixed several reported typos [Mirek, Joe]
+ Updated documentation according to the feedback [Joe]
+ Added some Acks [Mirek]
Changes against v6:
+ used list_move when disabling replaced patches [Jason]
+ renamed KLP_FUNC_ORIGINAL -> KLP_FUNC_STATIC [Mirek]
+ used klp_is_func_type() in klp_unpatch_object() [Mirek]
+ moved static definition of klp_get_or_add_object() [Mirek]
+ updated comment about synchronization in forced mode [Mirek]
+ added user documentation
+ fixed several typos
Jason Baron (3):
livepatch: Use lists to manage patches, objects and functions
livepatch: Add atomic replace
livepatch: Remove replaced patches from the stack
Petr Mladek (5):
livepatch: Free only structures with initialized kobject
livepatch: Add an extra flag to distinguish registered patches
livepatch: Remove Nop structures when unused
livepatch: Allow to replace even disabled patches
livepatch: Atomic replace and cumulative patches documentation
Documentation/livepatch/cumulative-patches.txt | 105 ++++++++ include/linux/livepatch.h | 33 ++- kernel/livepatch/core.c | 355 ++++++++++++++++++++++--- kernel/livepatch/core.h | 4 + kernel/livepatch/patch.c | 39 ++- kernel/livepatch/patch.h | 1 + kernel/livepatch/transition.c | 27 ++ 7 files changed, 519 insertions(+), 45 deletions(-) create mode 100644 Documentation/livepatch/cumulative-patches.txt
Signature · 1 line
2.13.6
Petr Mladek
maintainerPATCH 1/8 livepatch: Use lists to manage patches, objects and functions
From: Jason Baron <[email protected]>
Currently klp_patch contains a pointer to a statically allocated array of struct klp_object and struct klp_objects contains a pointer to a statically allocated array of klp_func. In order to allow for the dynamic allocation of objects and functions, link klp_patch, klp_object, and klp_func together via linked lists. This allows us to more easily allocate new objects and functions, while having the iterator be a simple linked list walk.
The static structures are added to the lists early. It allows to add the dynamically allocated objects before klp_init_object() and klp_init_func() calls. Therefore it reduces the further changes to the code.
This patch does not change the existing behavior.
--- include/linux/livepatch.h | 19 +++++++++++++++++-- kernel/livepatch/core.c | 16 ++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-)
include/linux/livepatch.h +17 -2
@@ -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 )
@@ -42,6 +43,7 @@* can be found (optional) * @old_addr: the address of the function being patched * @kobj: kobject for sysfs resources +* @node: list node for klp_object func_list * @stack_node: list node for klp_ops func_stack list * @old_size: size of the old function * @new_size: size of the new function
@@ -79,6 +81,7 @@ struct klp_func {/* internal */ unsigned long old_addr;struct kobject kobj; +struct list_head node;struct list_head stack_node;unsigned long old_size, new_size;bool patched;
@@ -117,6 +120,8 @@ struct klp_callbacks {* @kobj: kobject for sysfs resources * @mod: kernel module associated with the patched object * (NULL for vmlinux) +* @func_list: dynamic list of the function entries +* @node: list node for klp_patch obj_list * @patched: the object's funcs have been added to the klp_ops list */ struct klp_object {
@@ -127,6 +132,8 @@ struct klp_object {/* internal */ struct kobject kobj; +struct list_head func_list; +struct list_head node;struct module *mod;bool patched; };
@@ -137,6 +144,7 @@ struct klp_object {* @objs: object entries for kernel objects to be patched * @list: list node for global list of registered patches * @kobj: kobject for sysfs resources +* @obj_list: dynamic list of the object entries * @enabled: the patch is enabled (but operation may be incomplete) * @finish: for waiting till it is safe to remove the patch module */
@@ -148,18 +156,25 @@ struct klp_patch {/* internal */ struct list_head list;struct kobject kobj; +struct list_head obj_list;bool enabled;struct completion finish; }; -#define klp_for_each_object (patch, obj) \ +#define klp_for_each_object_static (patch, obj) \ for (obj = patch->objs; obj->funcs || obj->name; obj++) -#define klp_for_each_func (obj, func) \ +#define klp_for_each_object (patch, obj) \ + list_for_each_entry(obj, &patch->obj_list, node) + +#define klp_for_each_func_static (obj, func) \ for (func = obj->funcs; \ func->old_name || func->new_func || func->old_sympos; \ func++) +#define klp_for_each_func (obj, func) \ + list_for_each_entry(func, &obj->func_list, node) +int klp_register_patch (struct klp_patch *);int klp_unregister_patch (struct klp_patch *);int klp_enable_patch (struct klp_patch *);
kernel/livepatch/core.c +16 -0
@@ -49,6 +49,21 @@ static LIST_HEAD(klp_patches);static struct kobject *klp_root_kobj; +static void klp_init_lists (struct klp_patch *patch) +{ +struct klp_object *obj; +struct klp_func *func; + +INIT_LIST_HEAD (&patch->obj_list); +klp_for_each_object_static (patch, obj) { +list_add (&obj->node, &patch->obj_list); + +INIT_LIST_HEAD (&obj->func_list); +klp_for_each_func_static (obj, func) +list_add (&func->node, &obj->func_list); + } +} +static bool klp_is_module (struct klp_object *obj) {return obj->name;
@@ -794,6 +809,7 @@ static int klp_init_patch(struct klp_patch *patch) patch->enabled = false;init_completion (&patch->finish); +klp_init_lists (patch); ret =kobject_init_and_add (&patch->kobj, &klp_ktype_patch, klp_root_kobj,"%s" , patch->mod->name);
Signature · 1 line
2.13.6
Petr Mladek
maintainerPATCH 2/8 livepatch: Free only structures with initialized kobject
In reply to Petr MladekWe are going to add a feature called atomic replace. It will allow to create a patch that would replace all already registered patches. For this, we will need to dynamically create funcs and objects for functions that are no longer patched.
We will want to reuse the existing init() and free() functions. Up to now, the free() functions checked a limit and were called only for structures with initialized kobject. But we will want to call them also for structures that were allocated but where the kobject was not initialized yet.
This patch removes the limit. It calls klp_free*() functions for all structures. But only the ones with initialized kobject are freed. The handling of un-initialized structures will be added later with the support for dynamic structures.
This patch does not change the existing behavior.
--- kernel/livepatch/core.c | 44 ++++++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 26 deletions(-)
kernel/livepatch/core.c +18 -26
@@ -642,17 +642,15 @@ static struct kobj_type klp_ktype_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) +static void klp_free_funcs (struct klp_object *obj) {struct klp_func *func; -for (func = obj->funcs; func->old_name && func != limit; func++) -kobject_put (&func->kobj); +klp_for_each_func (obj, func) { +/* Might be called from klp_init_patch() error path. */ +if (func->kobj.state_initialized) +kobject_put (&func->kobj); + } }/* Clean up when a patched object is unloaded */
@@ -666,24 +664,23 @@ static void klp_free_object_loaded(struct klp_object *obj) 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) +static void klp_free_objects (struct klp_patch *patch) {struct klp_object *obj; -for (obj = patch->objs; obj->funcs && obj != limit; obj++) { -klp_free_funcs_limited (obj,NULL ); -kobject_put (&obj->kobj); +klp_for_each_object (patch, obj) { +klp_free_funcs (obj); + +/* Might be called from klp_init_patch() error path. */ +if (obj->kobj.state_initialized) +kobject_put (&obj->kobj); } }static void klp_free_patch (struct klp_patch *patch) { -klp_free_objects_limited (patch,NULL ); +klp_free_objects (patch); +if (!list_empty (&patch->list))list_del (&patch->list); }
@@ -780,21 +777,16 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)klp_for_each_func (obj, func) { ret =klp_init_func (obj, func);if (ret) - goto free; +return ret; }if (klp_is_object_loaded (obj)) { ret =klp_init_object_loaded (patch, obj);if (ret) - goto free; +return ret; }return 0 ; - -free: -klp_free_funcs_limited (obj, func); -kobject_put (&obj->kobj); -return ret; }static int klp_init_patch (struct klp_patch *patch)
@@ -831,7 +823,7 @@ static int klp_init_patch(struct klp_patch *patch)return 0 ; free: -klp_free_objects_limited (patch, obj); +klp_free_objects (patch);mutex_unlock (&klp_mutex);
Signature · 1 line
2.13.6
Petr Mladek
maintainerPATCH 3/8 livepatch: Add atomic replace
In reply to Petr MladekFrom: Jason Baron <[email protected]>
Sometimes we would like to revert a particular fix. Currently, this is 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.
Another problem is when there are many patches that touch the same functions. There might be dependencies between patches that are not enforced on the kernel side. Also it might be pretty hard to actually prepare the patch and ensure compatibility with the other patches.
A better solution would be to create cumulative patch and say that it replaces all older ones.
This patch adds 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 no longer be modified by the new patch. They used as a new target during the patch transition.
The idea is to handle Nops' structures like the static ones. When the dynamic structures are allocated, we initialize all values that are normally statically defined. The only exception is "new_func" in struct klp_func. It has to point to the original function. But the address is known only when the object (module) is loaded.
Nevertheless we still need to distinguish the dynamically allocated structures in some operations. For this, we add "nop" flag into struct klp_func and "dynamic" flag into struct klp_object. They need special handling in the following situations:
+ The structures are added into the lists of objects and functions immediately. In fact, the lists were created for this purpose.
+ The address of the original function is known only when the patched object (module) is loaded. Therefore it is copied later in klp_init_object_loaded().
+ The ftrace handler must not set PC to func->new_addr. It would cause infinite loop because the address points back to the beginning of the original function.
+ The various free() functions must free the structure itself.
Note that other ways to detect the dynamic structures are not considered safe. For example, even the statically defined struct klp_object might include empty funcs array. It might be there just to run some callbacks.
Special callbacks handling:
The callbacks from the replaced patches are _not_ called by intention. It would be pretty hard to define a reasonable semantic and implement it.
It might even be counter-productive. The new patch is cumulative. It is supposed to include most of the changes from older patches. In most cases, it will not want to call pre_unpatch() post_unpatch() callbacks from the replaced patches. It would disable/break things for no good reasons. Also it should be easier to handle various scenarios in a single script in the new patch than think about interactions caused by running many scripts from older patches. Not to say that the old scripts even would not expect to be called in this situation.
--- include/linux/livepatch.h | 6 ++ kernel/livepatch/core.c | 193 +++++++++++++++++++++++++++++++++++++++++++++- kernel/livepatch/patch.c | 8 ++ 3 files changed, 204 insertions(+), 3 deletions(-)
include/linux/livepatch.h +6 -0
@@ -47,6 +47,7 @@* @stack_node: list node for klp_ops func_stack list * @old_size: size of the old function * @new_size: size of the new function +* @nop: temporary patch to use the original code again; dyn. allocated * @patched: the func has been added to the klp_ops list * @transition: the func is currently being applied or reverted *
@@ -84,6 +85,7 @@ struct klp_func {struct list_head node;struct list_head stack_node;unsigned long old_size, new_size; +bool nop;bool patched;bool transition; };
@@ -122,6 +124,7 @@ struct klp_callbacks {* (NULL for vmlinux) * @func_list: dynamic list of the function entries * @node: list node for klp_patch obj_list +* @dynamic: temporary object for nop functions; dynamically allocated * @patched: the object's funcs have been added to the klp_ops list */ struct klp_object {
@@ -135,6 +138,7 @@ struct klp_object {struct list_head func_list;struct list_head node;struct module *mod; +bool dynamic;bool patched; };
@@ -142,6 +146,7 @@ struct klp_object {* struct klp_patch - patch structure for live patching * @mod: reference to the live patch module * @objs: object entries for kernel objects to be patched +* @replace: replace all already registered patches * @list: list node for global list of registered patches * @kobj: kobject for sysfs resources * @obj_list: dynamic list of the object entries
@@ -152,6 +157,7 @@ struct klp_patch {/* external */ struct module *mod;struct klp_object *objs; +bool replace;/* internal */ struct list_head list;
kernel/livepatch/core.c +190 -3
260 lines not on this page · raw message
kernel/livepatch/patch.c +8 -0
@@ -118,7 +118,15 @@ static void notrace klp_ftrace_handler(unsigned long ip, } } +/* +* NOPs are used to replace existing patches with original code. +* Do nothing! Setting pc would cause an infinite loop. +*/ +if (func->nop) + goto unlock; +klp_arch_set_pc (regs, (unsigned long )func->new_func); + unlock:preempt_enable_notrace (); }
Signature · 1 line
2.13.6
Re: [PATCH 3/8] livepatch: Add atomic replace
kernel/livepatch/core.c static void klp_free_patch(struct klp_patch *patch) · line 865
Quoted 10 lines · Petr Mladek
kernel/livepatch/core.c +5
-1@@ -687,7 +858,14 @@ static void klp_free_patch(struct klp_patch *patch)static int klp_init_func (struct klp_object *obj,struct klp_func *func) { -if (!func->old_name || !func->new_func) +if (!func->old_name) +return -EINVAL ; + +/* +* NOPs get the address later. The the patched module must be loaded,
"The the" -> "the"
Quoted 8 lines
kernel/livepatch/core.c +3
-0+* see klp_init_object_loaded(). +*/ +if (!func->new_func && !func->nop)return -EINVAL ;kernel/livepatch/core.c +0
-0INIT_LIST_HEAD (&func->stack_node);
kernel/livepatch/core.c static int klp_init_object_loaded(struct klp_patch *patch, · line 925
Quoted 6 lines
kernel/livepatch/core.c +3
-0@@ -742,6 +920,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,return -ENOENT ; } +if (func->nop) + func->new_func = (void *)func->old_addr; +
These changes make it more obvious that 'new_func' isn't quite the right name. It should really be 'new_addr' IMO.
Signature · 1 line
Josh
Miroslav Benes
maintainerRe: [PATCH 3/8] livepatch: Add atomic replace
Quoted 11 lines
Quoted 11 lines
kernel/livepatch/core.c +3
-0+* see klp_init_object_loaded(). +*/ +if (!func->new_func && !func->nop)return -EINVAL ;INIT_LIST_HEAD (&func->stack_node);@@ -742,6 +920,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,return -ENOENT ; }
Quoted 6 lines
kernel/livepatch/core.c static int klp_init_object_loaded(struct klp_patch *patch, · line 925
Quoted 3 lines
kernel/livepatch/core.c +3
-0+if (func->nop) + func->new_func = (void *)func->old_addr; +These changes make it more obvious that '
new_func' isn't quite the right name. It should really be 'new_addr' IMO.
I think we wanted to point out the difference from old_addr which is initialized with the symbol name while new_func is initialized with the new function itself (function pointer). I agree though that it looks awkward in this context and I'm not against changing it to new_addr.
Petr, could you also add a note to the changelog why we need to setup new_func for nop functions, please? It's not obvious because of the hack in klp_ftrace_handler() (klp_cancel_transition()->...->klp_check_stack_func() needs it).
Miroslav
Petr Mladek
maintainerRe: [PATCH 3/8] livepatch: Add atomic replace
Quoted 18 lines · Miroslav Benes
Quoted 11 lines
Quoted 11 lines
kernel/livepatch/core.c +3
-0+* see klp_init_object_loaded(). +*/ +if (!func->new_func && !func->nop)return -EINVAL ;INIT_LIST_HEAD (&func->stack_node);@@ -742,6 +920,9 @@ static int klp_init_object_loaded(struct klp_patch *patch,return -ENOENT ; }Quoted 6 lines
kernel/livepatch/core.c static int klp_init_object_loaded(struct klp_patch *patch, · line 925
Quoted 3 lines
kernel/livepatch/core.c +3
-0+if (func->nop) + func->new_func = (void *)func->old_addr; +These changes make it more obvious that '
new_func' isn't quite the right name. It should really be 'new_addr' IMO.
Quoted 4 lines
I think we wanted to point out the difference from
old_addrwhich is initialized with the symbol name whilenew_funcis initialized with the new function itself (function pointer). I agree though that it looks awkward in this context and I'm not against changing it tonew_addr.
I am fine with the rename. I was confused by "new_func" several times in the past. "new_addr" makes it clear that we are setting an address in compare with the name in "old_name".
Quoted 4 lines
Petr, could you also add a note to the changelog why we need to setup
new_funcfor nop functions, please? It's not obvious because of the hack inklp_ftrace_handler()(klp_cancel_transition()->...->klp_check_stack_func() needs it).
Yup.
Best Regards,
Petr
Petr Mladek
maintainerPATCH 4/8 livepatch: Add an extra flag to distinguish registered patches
In reply to Petr MladekThe initial implementation of the atomic replace feature keeps the replaced patches on the stack. But people would like to remove the replaced patches from different reasons that will be described in the following patch.
This patch is just a small preparation step. We will need to keep the replaced patches registered even when they are not longer on the stack. It is because they are typically unregistered by the module exit script.
Therefore we need to detect the registered patches another way. We could not use kobj.state_initialized because it is racy. The kobject is destroyed by an asynchronous call and could not be synchronized using klp_mutex.
This patch solves the problem by adding a flag into struct klp_patch. It is manipulated under klp_mutex and therefore it is safe. It is easy to understand and it is enough in most situations.
The function klp_is_patch_registered() is not longer needed. Though it was renamed to klp_is_patch_on_stack() and used in __klp_enable_patch() as a new sanity check.
This patch does not change the existing behavior.
--- include/linux/livepatch.h | 2 ++ kernel/livepatch/core.c | 24 ++++++++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-)
include/linux/livepatch.h +2 -0
@@ -150,6 +150,7 @@ struct klp_object {* @list: list node for global list of registered patches * @kobj: kobject for sysfs resources * @obj_list: dynamic list of the object entries +* @registered: reliable way to check registration status * @enabled: the patch is enabled (but operation may be incomplete) * @finish: for waiting till it is safe to remove the patch module */
@@ -163,6 +164,7 @@ struct klp_patch {struct list_head list;struct kobject kobj;struct list_head obj_list; +bool registered;bool enabled;struct completion finish; };
kernel/livepatch/core.c +18 -6
@@ -45,6 +45,11 @@*/ DEFINE_MUTEX (klp_mutex); +/* +* Stack of patches. It defines the order in which the patches can be enabled. +* Only patches on this stack might be enabled. New patches are added when +* registered. They are removed when they are unregistered. +*/ static LIST_HEAD (klp_patches);static struct kobject *klp_root_kobj;
@@ -97,7 +102,7 @@ static void klp_find_object_module(struct klp_object *obj)mutex_unlock (&module_mutex); } -static bool klp_is_patch_registered (struct klp_patch *patch) +static bool klp_is_patch_on_stack (struct klp_patch *patch) {struct klp_patch *mypatch;
@@ -378,7 +383,7 @@ int klp_disable_patch(struct klp_patch *patch)mutex_lock (&klp_mutex); -if (!klp_is_patch_registered (patch)) { +if (!patch->registered) { ret = -EINVAL ; goto err; }
@@ -407,7 +412,11 @@ static int __klp_enable_patch(struct klp_patch *patch)if (WARN_ON (patch->enabled))return -EINVAL ; -/* enforce stacking: only the first disabled patch can be enabled */ +/* Enforce stacking. */ +if (!klp_is_patch_on_stack (patch)) +return -EINVAL ; + +/* Only the first disabled patch can be enabled. */ if (patch->list.prev != &klp_patches && !list_prev_entry (patch, list)->enabled)return -EBUSY ;
@@ -478,7 +487,7 @@ int klp_enable_patch(struct klp_patch *patch)mutex_lock (&klp_mutex); -if (!klp_is_patch_registered (patch)) { +if (!patch->registered) { ret = -EINVAL ; goto err; }
@@ -519,7 +528,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,mutex_lock (&klp_mutex); -if (!klp_is_patch_registered (patch)) { +if (!patch->registered) {/* * Module with the patch could either disappear meanwhile or is * not properly initialized yet.
@@ -528,6 +537,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, goto err; } +if (patch->enabled == enabled) {/* already in requested state */ ret = -EINVAL ;
@@ -1004,6 +1014,7 @@ static int klp_init_patch(struct klp_patch *patch) }list_add_tail (&patch->list, &klp_patches); + patch->registered = true;mutex_unlock (&klp_mutex);
@@ -1034,7 +1045,7 @@ int klp_unregister_patch(struct klp_patch *patch)mutex_lock (&klp_mutex); -if (!klp_is_patch_registered (patch)) { +if (!patch->registered) { ret = -EINVAL ; goto err; }
@@ -1045,6 +1056,7 @@ int klp_unregister_patch(struct klp_patch *patch) }klp_free_patch (patch); + patch->registered = false;mutex_unlock (&klp_mutex);
Signature · 1 line
2.13.6
Re: [PATCH 4/8] livepatch: Add an extra flag to distinguish registered patches
kernel/livepatch/core.c static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, · line 543
Quoted 8 lines · Petr Mladek
kernel/livepatch/core.c +1
-0@@ -528,6 +537,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, goto err; } +if (patch->enabled == enabled) {/* already in requested state */ ret = -EINVAL ;
Extra newline.
Signature · 1 line
Josh
Miroslav Benes
maintainerRe: [PATCH 4/8] livepatch: Add an extra flag to distinguish registered patches
In reply to Petr MladekQuoted 6 lines · Petr Mladek
The initial implementation of the atomic replace feature keeps the replaced patches on the stack. But people would like to remove the replaced patches from different reasons that will be described in the following patch.
This patch is just a small preparation step. We will need to keep the replaced patches registered even when they are not longer on the stack.
s/not longer/no longer/
Quoted 3 lines
It is because they are typically unregistered by the module exit script.
Therefore we need to detect the registered patches another way.
"in another way", "differently"?
Quoted 9 lines
We could
not use kobj.state_initialized because it is racy. The kobject is destroyed by an asynchronous call and could not be synchronized usingklp_mutex.This patch solves the problem by adding a flag into struct
klp_patch. It is manipulated underklp_mutexand therefore it is safe. It is easy to understand and it is enough in most situations.The function
klp_is_patch_registered()is not longer needed. Though
s/not longer/no longer/
s/Though/So/ ?
Quoted 4 lines
it was renamed to
klp_is_patch_on_stack()and used in__klp_enable_patch()as a new sanity check.This patch does not change the existing behavior.
In my opinion it could be easier for a review to squash the patch into the next one.
Quoted 57 lines
Signed-off-by Petr Mladek <[email protected]>Cc Josh Poimboeuf <[email protected]>Cc Jessica Yu <[email protected]>Cc Jiri Kosina <[email protected]>Cc Miroslav Benes <[email protected]>Cc Jason Baron <[email protected]>--- include/linux/livepatch.h | 2 ++ kernel/livepatch/core.c | 24 ++++++++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-)include/linux/livepatch.h +2
-0@@ -150,6 +150,7 @@ struct klp_object {* @list: list node for global list of registered patches * @kobj: kobject for sysfs resources * @obj_list: dynamic list of the object entries +* @registered: reliable way to check registration status * @enabled: the patch is enabled (but operation may be incomplete) * @finish: for waiting till it is safe to remove the patch module */ @@ -163,6 +164,7 @@ struct klp_patch {struct list_head list;struct kobject kobj;struct list_head obj_list; +bool registered;bool enabled;struct completion finish; };kernel/livepatch/core.c +6
-1@@ -45,6 +45,11 @@*/ DEFINE_MUTEX (klp_mutex); +/* +* Stack of patches. It defines the order in which the patches can be enabled. +* Only patches on this stack might be enabled. New patches are added when +* registered. They are removed when they are unregistered. +*/ static LIST_HEAD (klp_patches);static struct kobject *klp_root_kobj;@@ -97,7 +102,7 @@ static void klp_find_object_module(struct klp_object *obj)mutex_unlock (&module_mutex); } -static bool klp_is_patch_registered (struct klp_patch *patch) +static bool klp_is_patch_on_stack (struct klp_patch *patch) {struct klp_patch *mypatch;
kernel/livepatch/core.c int klp_disable_patch(struct klp_patch *patch) · line 386
Quoted 5 lines
kernel/livepatch/core.c +1
-1@@ -378,7 +383,7 @@ int klp_disable_patch(struct klp_patch *patch)mutex_lock (&klp_mutex); -if (!klp_is_patch_registered (patch)) { +if (!patch->registered) {
I don't see any actual problem, but I'd feel safer if we preserve klp_is_patch_on_stack() even somewhere in disable path.
Miroslav
Petr Mladek
maintainerRe: [PATCH 4/8] livepatch: Add an extra flag to distinguish registered patches
Quoted 11 lines · Miroslav Benes
Quoted 6 lines · Petr Mladek
The initial implementation of the atomic replace feature keeps the replaced patches on the stack. But people would like to remove the replaced patches from different reasons that will be described in the following patch.
This patch is just a small preparation step. We will need to keep the replaced patches registered even when they are not longer on the stack.
In my opinion it could be easier for a review to squash the patch into the next one.
OK, I'll squash them.
Quoted 6 lines
Quoted 6 lines
include/linux/livepatch.h +0
-0@@ -378,7 +383,7 @@ int klp_disable_patch(struct klp_patch *patch)
Quoted 7 lines
include/linux/livepatch.h int klp_disable_patch(struct klp_patch *patch) · line 386
Quoted 4 lines
include/linux/livepatch.h +1
-1mutex_lock (&klp_mutex); -if (!klp_is_patch_registered (patch)) { +if (!patch->registered) {I don't see any actual problem, but I'd feel safer if we preserve
klp_is_patch_on_stack()even somewhere in disable path.
It is strictly needed in klp_enable_patch() which will be better visible if I squash it with the next patch.
The use in klp_disable_patch() is optional. I do not have problems to add it there though. If Josh does not see it as too paranoid, I'll add it.
Best Regards,
Petr
Re: [PATCH 4/8] livepatch: Add an extra flag to distinguish registered patches
Quoted 14 lines · Petr Mladek
Quoted 6 lines
Quoted 6 lines
include/linux/livepatch.h +0
-0@@ -378,7 +383,7 @@ int klp_disable_patch(struct klp_patch *patch)Quoted 7 lines
include/linux/livepatch.h int klp_disable_patch(struct klp_patch *patch) · line 386
Quoted 4 lines
include/linux/livepatch.h +1
-1mutex_lock (&klp_mutex); -if (!klp_is_patch_registered (patch)) { +if (!patch->registered) {I don't see any actual problem, but I'd feel safer if we preserve
klp_is_patch_on_stack()even somewhere in disable path.
Quoted 6 lines
It is strictly needed in
klp_enable_patch()which will be better visible if I squash it with the next patch.The use in
klp_disable_patch()is optional. I do not have problems to add it there though. If Josh does not see it as too paranoid, I'll add it.
That does seem pointless to me, how did you know? :-)
Of course if we make the other changes we've been discussing, maybe it's no longer an issue.
Signature · 1 line
Josh
Petr Mladek
maintainerPATCH 5/8 livepatch: Remove replaced patches from the stack
In reply to Petr MladekFrom: Jason Baron <[email protected]>
The stack of patches defines an order in which the patches must be enabled. It has some drawbacks, for example:
+ People could not disable earlier patches without disabling the later ones even when they are independent.
+ The order is not clearly visible in /sys/kernel/livepatch. Users might get lost.
+ lsmod does not give a clue what patch is the last one and in use.
+ Lots of unused data and code might be locked in memory.
Some of the above problems can already be solved by using cumulative patches (replace flag) and reasonable patch names.
This is expected. The atomic replace and cumulative patches are supposed to help managing and maintaining many patches. And they can resolve even more problems mentioned above when the replaced patches get removed from the stack. Then the unused patches might be unregistered and the modules unloaded.
Removing replaced patches will actually help even more. The patch will become the first on the stack:
+ Nops' structs will not longer be necessary and might be removed. This would save memory, restore performance (no ftrace handler), allow clear view on what is really patched.
+ Disabling the patch will cause using the original code everywhere. Therefore the livepatch callbacks could handle only one scenario. Note that the complication is already complex enough when the patch gets enabled. It is currently solved by calling callbacks only from the new cumulative patch.
Some people actually expected this behavior from the beginning. After all a cumulative patch is supposed to "completely" replace an existing one. It is like when a new version of an application replaces an older one.
This patch does the first step. It removes the replaced patches from the stack. It is safe. The consistency model ensures that they are not longer used. By other words, each process works only with the structures from klp_transition_patch.
The removal is done by a special function. It combines actions done by __disable_patch() and klp_complete_transition(). But it is a fast track without all the transaction-related stuff.
--- kernel/livepatch/core.c | 37 +++++++++++++++++++++++++++++++++++++ kernel/livepatch/core.h | 3 +++ kernel/livepatch/transition.c | 3 +++ 3 files changed, 43 insertions(+)
kernel/livepatch/core.c +37 -0
@@ -332,6 +332,43 @@ static int klp_write_object_relocations(struct module *pmod,return ret; } +/* +* This function removes replaced patches from both func_stack +* and klp_patches stack. +* +* We could be pretty aggressive here. It is called in the situation where +* these structures are no longer accessible. All functions are redirected +* by the klp_transition_patch. They use either a new code or they are in +* the original code because of the special nop function patches. +* +* The only exception is when the transition was forced. In this case, +* klp_ftrace_handler() might still see the replaced patch on the stack. +* Fortunately, it is carefully designed to work with removed functions +* thanks to RCU. We only have to keep the patches on the system. This +* is handled by @keep_module parameter. +*/ +void klp_discard_replaced_patches (struct klp_patch *new_patch,bool keep_module) +{ +struct klp_patch *old_patch, *tmp_patch; + +list_for_each_entry_safe (old_patch, tmp_patch, &klp_patches, list) { +if (old_patch == new_patch) +return ; + +klp_unpatch_objects (old_patch); + old_patch->enabled = false; + +if (!keep_module) +module_put (old_patch->mod); + +/* +* Replaced patches could not get re-enabled to keep +* the code sane. +*/ +list_del_init (&old_patch->list); + } +} +static int __klp_disable_patch (struct klp_patch *patch) {struct klp_object *obj;
kernel/livepatch/core.h +3 -0
@@ -6,6 +6,9 @@extern struct mutex klp_mutex; +void klp_discard_replaced_patches (struct klp_patch *new_patch, +bool keep_module); +static inline bool klp_is_object_loaded (struct klp_object *obj) {return !obj->name || obj->mod;
kernel/livepatch/transition.c +3 -0
@@ -87,6 +87,9 @@ static void klp_complete_transition(void) klp_transition_patch->mod->name, klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); +if (klp_transition_patch->replace && klp_target_state ==KLP_PATCHED ) +klp_discard_replaced_patches (klp_transition_patch, klp_forced); +if (klp_target_state ==KLP_UNPATCHED ) {/* * All tasks have transitioned to KLP_UNPATCHED so we can now
Signature · 1 line
2.13.6
Petr Mladek
maintainerPATCH 6/8 livepatch: Remove Nop structures when unused
In reply to Petr MladekReplaced patches are removed from the stack when the transition is finished. It means that Nop structures will never be needed again and can be removed. Why should we care?
+ Nop structures make false feeling that the function is patched even though the ftrace handler has no effect.
+ Ftrace handlers are not completely for free. They cause slowdown that might be visible in some workloads. The ftrace-related slowdown might actually be the reason why the function is not longer patched in the new cumulative patch. One would expect that cumulative patch would allow to solve these problems as well.
+ Cumulative patches are supposed to replace any earlier version of the patch. The amount of NOPs depends on which version was replaced. This multiplies the amount of scenarios that might happen.
One might say that NOPs are innocent. But there are even optimized NOP instructions for different processor, for example, see arch/x86/kernel/alternative.c. And klp_ftrace_handler() is much more complicated.
+ It sounds natural to clean up a mess that is not longer needed. It could only be worse if we do not do it.
This patch allows to unpatch and free the dynamic structures independently when the transition finishes.
The free part is a bit tricky because kobject free callbacks are called asynchronously. We could not wait for them easily. Fortunately, we do not have to. Any further access can be avoided by removing them from the dynamic lists.
Finally, the patch become the first on the stack when enabled. The replace functionality will not longer be needed. Let's clear patch->replace to avoid the special handling when it is eventually disabled/enabled again.
--- include/linux/livepatch.h | 6 ++++++ kernel/livepatch/core.c | 42 +++++++++++++++++++++++++++++++++++------- kernel/livepatch/core.h | 1 + kernel/livepatch/patch.c | 31 ++++++++++++++++++++++++++----- kernel/livepatch/patch.h | 1 + kernel/livepatch/transition.c | 26 +++++++++++++++++++++++++- 6 files changed, 94 insertions(+), 13 deletions(-)
include/linux/livepatch.h +6 -0
@@ -172,6 +172,9 @@ struct klp_patch {#define klp_for_each_object_static (patch, obj) \ for (obj = patch->objs; obj->funcs || obj->name; obj++) +#define klp_for_each_object_safe (patch, obj, tmp_obj) \ + list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, node) +#define klp_for_each_object (patch, obj) \ list_for_each_entry(obj, &patch->obj_list, node)
@@ -180,6 +183,9 @@ struct klp_patch { func->old_name || func->new_func || func->old_sympos; \ func++) +#define klp_for_each_func_safe (obj, func, tmp_func) \ + list_for_each_entry_safe(func, tmp_func, &obj->func_list, node) +#define klp_for_each_func (obj, func) \ list_for_each_entry(func, &obj->func_list, node)
kernel/livepatch/core.c +35 -7
64 lines not on this page · raw message
kernel/livepatch/core.h +1 -0
@@ -8,6 +8,7 @@ extern struct mutex klp_mutex;void klp_discard_replaced_patches (struct klp_patch *new_patch,bool keep_module); +void klp_free_objects_dynamic (struct klp_patch *patch);static inline bool klp_is_object_loaded (struct klp_object *obj) {
kernel/livepatch/patch.c +26 -5
@@ -244,15 +244,26 @@ static int klp_patch_func(struct klp_func *func)return ret; } -void klp_unpatch_object (struct klp_object *obj) +static void __klp_unpatch_object (struct klp_object *obj,bool unpatch_all) {struct klp_func *func; -klp_for_each_func (obj, func) +klp_for_each_func (obj, func) { +if (!unpatch_all && !func->nop) +continue ; +if (func->patched)klp_unpatch_func (func); + } - obj->patched = false; +if (unpatch_all || obj->dynamic) + obj->patched = false; +} + + +void klp_unpatch_object (struct klp_object *obj) +{ +__klp_unpatch_object (obj, true); }int klp_patch_object (struct klp_object *obj)
@@ -275,11 +286,21 @@ int klp_patch_object(struct klp_object *obj)return 0 ; } -void klp_unpatch_objects (struct klp_patch *patch) +static void __klp_unpatch_objects (struct klp_patch *patch,bool unpatch_all) {struct klp_object *obj;klp_for_each_object (patch, obj)if (obj->patched) -klp_unpatch_object (obj); +__klp_unpatch_object (obj, unpatch_all); +} + +void klp_unpatch_objects (struct klp_patch *patch) +{ +__klp_unpatch_objects (patch, true); +} + +void klp_unpatch_objects_dynamic (struct klp_patch *patch) +{ +__klp_unpatch_objects (patch, false); }
kernel/livepatch/patch.h +1 -0
@@ -30,5 +30,6 @@ 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_objects_dynamic (struct klp_patch *patch);#endif /* _LIVEPATCH_PATCH_H */
kernel/livepatch/transition.c +25 -1
@@ -87,9 +87,18 @@ static void klp_complete_transition(void) klp_transition_patch->mod->name, klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); -if (klp_transition_patch->replace && klp_target_state ==KLP_PATCHED ) +if (klp_transition_patch->replace && klp_target_state ==KLP_PATCHED ) {klp_discard_replaced_patches (klp_transition_patch, klp_forced); +/* +* There is no need to synchronize the transition after removing +* nops. They must be the last on the func_stack. Ftrace +* guarantees that nobody will stay in the trampoline after +* the ftrace handler is unregistered. +*/ +klp_unpatch_objects_dynamic (klp_transition_patch); + } +if (klp_target_state ==KLP_UNPATCHED ) {/* * All tasks have transitioned to KLP_UNPATCHED so we can now
@@ -146,6 +155,21 @@ static void klp_complete_transition(void)if (!klp_forced && klp_target_state ==KLP_UNPATCHED )module_put (klp_transition_patch->mod); +if (klp_transition_patch->replace && klp_target_state ==KLP_PATCHED ) { +/* +* We do not need to wait until the objects are really freed. +* We will never need them again because the patch must be on +* the bottom of the stack now. +*/ +klp_free_objects_dynamic (klp_transition_patch); +/* +* Replace behavior will not longer be needed. Avoid the related +* code when disabling and enabling again. +*/ + klp_transition_patch->replace = false; + } + + klp_target_state =KLP_UNDEFINED ; klp_transition_patch =NULL ; }
Signature · 1 line
2.13.6
Re: [PATCH 6/8] livepatch: Remove Nop structures when unused
Quoted 3 lines · Petr Mladek
Replaced patches are removed from the stack when the transition is finished. It means that Nop structures will never be needed again and can be removed. Why should we care?
Warning, grammar pedantry ahead.
"Nop" isn't a proper noun, so it shouldn't be capitalized (here and in the patch subject).
Also, "NOP", used elsewhere, might be confused with x86 NOP instructions. I would vote for "nop" everywhere.
Quoted 4 lines
kernel/livepatch/transition.c +0
-0@@ -146,6 +155,21 @@ static void klp_complete_transition(void)if (!klp_forced && klp_target_state ==KLP_UNPATCHED )module_put (klp_transition_patch->mod);
kernel/livepatch/transition.c static void klp_complete_transition(void) · line 167
Quoted 10 lines
kernel/livepatch/transition.c +10
-0+if (klp_transition_patch->replace && klp_target_state ==KLP_PATCHED ) { +/* +* We do not need to wait until the objects are really freed. +* We will never need them again because the patch must be on +* the bottom of the stack now. +*/ +klp_free_objects_dynamic (klp_transition_patch); +/* +* Replace behavior will not longer be needed. Avoid the related +* code when disabling and enabling again.
"not longer" -> "no longer"
Signature · 1 line
Josh
Miroslav Benes
maintainerRe: [PATCH 6/8] livepatch: Remove Nop structures when unused
In reply to Petr MladekQuoted 62 lines · Petr Mladek
Replaced patches are removed from the stack when the transition is finished. It means that Nop structures will never be needed again and can be removed. Why should we care?
+ Nop structures make false feeling that the function is patched even though the ftrace handler has no effect.
+ Ftrace handlers are not completely for free. They cause slowdown that might be visible in some workloads. The ftrace-related slowdown might actually be the reason why the function is not longer patched in the new cumulative patch. One would expect that cumulative patch would allow to solve these problems as well.
+ Cumulative patches are supposed to replace any earlier version of the patch. The amount of NOPs depends on which version was replaced. This multiplies the amount of scenarios that might happen.
One might say that NOPs are innocent. But there are even optimized NOP instructions for different processor, for example, see
arch/x86/kernel/alternative.c. Andklp_ftrace_handler()is much more complicated.+ It sounds natural to clean up a mess that is not longer needed. It could only be worse if we do not do it.
This patch allows to unpatch and free the dynamic structures independently when the transition finishes.
The free part is a bit tricky because kobject free callbacks are called asynchronously. We could not wait for them easily. Fortunately, we do not have to. Any further access can be avoided by removing them from the dynamic lists.
Finally, the patch become the first on the stack when enabled. The replace functionality will not longer be needed. Let's clear
patch->replaceto avoid the special handling when it is eventually disabled/enabled again.Signed-off-by Petr Mladek <[email protected]>--- include/linux/livepatch.h | 6 ++++++ kernel/livepatch/core.c | 42 +++++++++++++++++++++++++++++++++++------- kernel/livepatch/core.h | 1 + kernel/livepatch/patch.c | 31 ++++++++++++++++++++++++++----- kernel/livepatch/patch.h | 1 + kernel/livepatch/transition.c | 26 +++++++++++++++++++++++++- 6 files changed, 94 insertions(+), 13 deletions(-)include/linux/livepatch.h +3
-0@@ -172,6 +172,9 @@ struct klp_patch {#define klp_for_each_object_static (patch, obj) \ for (obj = patch->objs; obj->funcs || obj->name; obj++) +#define klp_for_each_object_safe (patch, obj, tmp_obj) \ + list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, node) +#define klp_for_each_object (patch, obj) \ list_for_each_entry(obj, &patch->obj_list, node)
include/linux/livepatch.h struct klp_patch { · line 190
Quoted 8 lines
include/linux/livepatch.h +3
-0@@ -180,6 +183,9 @@ struct klp_patch { func->old_name || func->new_func || func->old_sympos; \ func++) +#define klp_for_each_func_safe (obj, func, tmp_func) \ + list_for_each_entry_safe(func, tmp_func, &obj->func_list, node) +#define klp_for_each_func (obj, func) \ list_for_each_entry(func, &obj->func_list, node)
Is there a benefit of the newly added iterators?
Miroslav
Petr Mladek
maintainerRe: [PATCH 6/8] livepatch: Remove Nop structures when unused
Quoted 31 lines · Miroslav Benes
Quoted 30 lines · Petr Mladek
This patch allows to unpatch and free the dynamic structures independently when the transition finishes.
The free part is a bit tricky because kobject free callbacks are called asynchronously. We could not wait for them easily. Fortunately, we do not have to. Any further access can be avoided by removing them from the dynamic lists.
Finally, the patch become the first on the stack when enabled. The replace functionality will not longer be needed. Let's clear
patch->replaceto avoid the special handling when it is eventually disabled/enabled again.include/linux/livepatch.h +3
-0@@ -172,6 +172,9 @@ struct klp_patch {#define klp_for_each_object_static (patch, obj) \ for (obj = patch->objs; obj->funcs || obj->name; obj++) +#define klp_for_each_object_safe (patch, obj, tmp_obj) \ + list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list, node) +#define klp_for_each_object (patch, obj) \ list_for_each_entry(obj, &patch->obj_list, node)@@ -180,6 +183,9 @@ struct klp_patch { func->old_name || func->new_func || func->old_sympos; \ func++)
Quoted 7 lines
include/linux/livepatch.h struct klp_patch { · line 190
Quoted 5 lines
include/linux/livepatch.h +3
-0+#define klp_for_each_func_safe (obj, func, tmp_func) \ + list_for_each_entry_safe(func, tmp_func, &obj->func_list, node) +#define klp_for_each_func (obj, func) \ list_for_each_entry(func, &obj->func_list, node)Is there a benefit of the newly added iterators?
You are right that there is nothing special and it is used on a single place only.
Well, it increases the chance that you will catch it when looking for the iterators. Also it makes it easier to see the difference against the non-safe iterators. I'll keep it if you are not strongly against it.
Best Regards,
Petr
Petr Mladek
maintainerPATCH 7/8 livepatch: Allow to replace even disabled patches
In reply to Petr MladekPatches without the replace flag might depend on each other. It makes sense to enforce the order in which they are enabled and disabled.
The situation is different when the patch replaces all existing ones. These patches should make the life easier for both: patch producers and users. Such a patch should be ready to replace basically any older patch. It should work well even in situations when the previous patches were not installed or when they were disabled from some reasons.
The code is almost ready for this. There are needed two changes:
+ Disable only enabled patches in klp_discard_replaced_patches().
+ Stop enforcing the stack order for the patches with the replace flag. Instead, we need to make sure that they are still usable (not replaced). This check is already there.
--- kernel/livepatch/core.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
kernel/livepatch/core.c +13 -6
@@ -355,11 +355,13 @@ void klp_discard_replaced_patches(struct klp_patch *new_patch, bool keep_module)if (old_patch == new_patch)return ; -klp_unpatch_objects (old_patch); - old_patch->enabled = false; +if (old_patch->enabled) { +klp_unpatch_objects (old_patch); + old_patch->enabled = false; -if (!keep_module) -module_put (old_patch->mod); +if (!keep_module) +module_put (old_patch->mod); + }/* * Replaced patches could not get re-enabled to keep
@@ -453,8 +455,13 @@ static int __klp_enable_patch(struct klp_patch *patch)if (!klp_is_patch_on_stack (patch))return -EINVAL ; -/* Only the first disabled patch can be enabled. */ -if (patch->list.prev != &klp_patches && +/* +* Only the first disabled patch can be enabled. This is not required +* for patches with the replace flags. They override even disabled +* patches that were registered earlier. +*/ +if (!patch->replace && + patch->list.prev != &klp_patches && !list_prev_entry (patch, list)->enabled)return -EBUSY ;
Signature · 1 line
2.13.6
Petr Mladek
maintainerPATCH 8/8 livepatch: Atomic replace and cumulative patches documentation
In reply to Petr MladekUser documentation for the atomic replace feature. It makes it easier to maintain livepatches using so-called cumulative patches.
--- Documentation/livepatch/cumulative-patches.txt | 105 +++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 Documentation/livepatch/cumulative-patches.txt
Documentation/livepatch/cumulative-patches.txt new file +105 -0
@@ -0,0 +1,105 @@ +=================================== +Atomic Replace & Cumulative Patches +=================================== + +There might be dependencies between livepatches. If multiple patches need +to do different changes to the same function(s) then we need to define +an order in which the patches will be installed. And function implementations +from any newer livepatch must be done on top of the older ones. + +This might become a maintenance nightmare. Especially if anyone would want +to remove a patch that is in the middle of the stack. + +An elegant solution comes with the feature called "Atomic Replace". It allows +to create so called "Cumulative Patches". They include all wanted changes +from all older livepatches and completely replace them in one transition. + +Usage +----- + +The atomic replace can be enabled by setting "replace" flag in struct klp_patch, +for example: + + static struct klp_patch patch = { + .mod = THIS_MODULE, + .objs = objs, + .replace = true, + }; + +Such a patch is added on top of the livepatch stack when registered. It can +be enabled even when some earlier patches have not been enabled yet. + +All processes are then migrated to use the code only from the new patch. +Once the transition is finished, all older patches are removed from the stack +of patches. Even the older not-enabled patches mentioned above. They can +even be unregistered and the related modules unloaded. + +Ftrace handlers are transparently removed from functions that are no +longer modified by the new cumulative patch. + +As a result, the livepatch authors might maintain sources only for one +cumulative patch. It helps to keep the patch consistent while adding or +removing various fixes or features. + +Users could keep only the last patch installed on the system after +the transition to has finished. It helps to clearly see what code is +actually in use. Also the livepatch might then be seen as a "normal" +module that modifies the kernel behavior. The only difference is that +it can be updated at runtime without breaking its functionality. + + +Features +-------- + +The atomic replace allows: + + + Atomically revert some functions in a previous patch while + upgrading other functions. + + + Remove eventual performance impact caused by core redirection + for functions that are no longer patched. + + + Decrease user confusion about stacking order and what patches are + currently in effect. + + +Limitations: +------------ + + + Replaced patches can no longer be enabled. But if the transition + to the cumulative patch was not forced, the kernel modules with + the older livepatches can be removed and eventually added again. + + A good practice is to set .replace flag in any released livepatch. + Then re-adding an older livepatch is equivalent to downgrading + to that patch. This is safe as long as the livepatches do _not_ do + extra modifications in (un)patching callbacks or in the module_init() + or module_exit() functions, see below. + + + + Only the (un)patching callbacks from the _new_ cumulative livepatch are + executed. Any callbacks from the replaced patches are ignored. + + By other words, the cumulative patch is responsible for doing any actions + that are necessary to properly replace any older patch. + + As a result, it might be dangerous to replace newer cumulative patches by + older ones. The old livepatches might not provide the necessary callbacks. + + This might be seen as a limitation in some scenarios. But it makes the life + easier in many others. Only the new cumulative livepatch knows what + fixes/features are added/removed and what special actions are necessary + for a smooth transition. + + In each case, it would be a nightmare to think about the order of + the various callbacks and their interactions if the callbacks from all + enabled patches were called. + + + + There is no special handling of shadow variables. Livepatch authors + must create their own rules how to pass them from one cumulative + patch to the other. Especially they should not blindly remove them + in module_exit() functions. + + A good practice might be to remove shadow variables in the post-unpatch + callback. It is called only when the livepatch is properly disabled.
Signature · 1 line
2.13.6
Petr Mladek
maintainerQuoted 20 lines · Petr Mladek
The atomic replace allows to create cumulative patches. They are useful when you maintain many livepatches and want to remove one that is lower on the stack. In addition it is very useful when more patches touch the same function and there are dependencies between them.
This version is heavily refactored and cleaned based on feedback from Josh. There are actually only three functional changes.
It still passes the first draft of the selfttest from Joe that can
be found at https://lkml.kernel.org/r/[email protected]Changes against v10:
+ Bug fixes and functional changes:
+ Handle Nops inklp_ftrace_handled()to avoid infinite loop [Mirek]
+ Really add dynamically allocatedklp_objectinto the list [Petr]
+ Clearpatch->replacewhen transition finishes [Josh]
Quoted 8 lines
+ Refactoring and clean up [Josh]:
+ Replace enum types with bools
+ Avoid usingERR_PTR
+ Remove too paranoid warnings
+ Distinguish registered patches by a flag instead of a list
+ Squash some functions
+ Update comments, documentation, and commit messages
+ Squashed and split patches to do more controversial changes later
Please, see below git diff against v10. Otherwise, the changes might be hard to find because of the refactoring.
Documentation/livepatch/cumulative-patches.txt +24 -2
@@ -31,15 +31,37 @@ be enabled even when some earlier patches have not been enabled yet. All processes are then migrated to use the code only from the new patch. Once the transition is finished, all older patches are removed from the stack -of patches. Even the older not-enabled patches mentioned above. +of patches. Even the older not-enabled patches mentioned above. They can +even be unregistered and the related modules unloaded. Ftrace handlers are transparently removed from functions that are no longer modified by the new cumulative patch. -As a result, the livepatch author might maintain sources only for one +As a result, the livepatch authors might maintain sources only for one cumulative patch. It helps to keep the patch consistent while adding or removing various fixes or features. +Users could keep only the last patch installed on the system after +the transition to has finished. It helps to clearly see what code is +actually in use. Also the livepatch might then be seen as a "normal" +module that modifies the kernel behavior. The only difference is that +it can be updated at runtime without breaking its functionality. + + +Features +-------- + +The atomic replace allows: + + + Atomically revert some functions in a previous patch while + upgrading other functions. + + + Remove eventual performance impact caused by core redirection + for functions that are no longer patched. + + + Decrease user confusion about stacking order and what patches are + currently in effect. + Limitations: ------------
include/linux/livepatch.h +17 -47
134 lines not on this page · raw message
kernel/livepatch/core.c +116 -181
567 lines not on this page · raw message
kernel/livepatch/core.h +3 -3
@@ -6,9 +6,9 @@extern struct mutex klp_mutex; -void klp_throw_away_replaced_patches (struct klp_patch *new_patch, -bool keep_module); -void klp_free_objects (struct klp_patch *patch,enum klp_func_type ftype); +void klp_discard_replaced_patches (struct klp_patch *new_patch, +bool keep_module); +void klp_free_objects_dynamic (struct klp_patch *patch);static inline bool klp_is_object_loaded (struct klp_object *obj) {
kernel/livepatch/patch.c +28 -14
81 lines not on this page · raw message
kernel/livepatch/patch.h +3 -2
@@ -28,7 +28,8 @@ 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,enum klp_func_type ftype); -void klp_unpatch_objects (struct klp_patch *patch,enum klp_func_type ftype); +void klp_unpatch_object (struct klp_object *obj); +void klp_unpatch_objects (struct klp_patch *patch); +void klp_unpatch_objects_dynamic (struct klp_patch *patch);#endif /* _LIVEPATCH_PATCH_H */
kernel/livepatch/transition.c +18 -30
73 lines not on this page · raw message
Quoted 20 lines · Petr Mladek
The atomic replace allows to create cumulative patches. They are useful when you maintain many livepatches and want to remove one that is lower on the stack. In addition it is very useful when more patches touch the same function and there are dependencies between them.
This version is heavily refactored and cleaned based on feedback from Josh. There are actually only three functional changes.
It still passes the first draft of the selfttest from Joe that can
be found at https://lkml.kernel.org/r/[email protected]Changes against v10:
+ Bug fixes and functional changes:
+ Handle Nops inklp_ftrace_handled()to avoid infinite loop [Mirek]
+ Really add dynamically allocatedklp_objectinto the list [Petr]
+ Clearpatch->replacewhen transition finishes [Josh]
Quoted 8 lines
+ Refactoring and clean up [Josh]:
+ Replace enum types with bools
+ Avoid usingERR_PTR
+ Remove too paranoid warnings
+ Distinguish registered patches by a flag instead of a list
+ Squash some functions
+ Update comments, documentation, and commit messages
+ Squashed and split patches to do more controversial changes later
Thanks again for all the changes. I think I like the general direction of the patches now, even some of the later ones ;-)
Along with the minor comments from my other emails, I still have the question about "does it make sense to enforce a stack anymore".
And of course I would really like to see the selftests in place first.
Signature · 1 line
Josh