From 19514910d021c93c7823ec32067e6b7dea224f0f Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 9 Jan 2019 13:43:19 +0100 Subject: livepatch: Change unsigned long old_addr -> void *old_func in struct klp_func The address of the to be patched function and new function is stored in struct klp_func as: void *new_func; unsigned long old_addr; The different naming scheme and type are derived from the way the addresses are set. @old_addr is assigned at runtime using kallsyms-based search. @new_func is statically initialized, for example: static struct klp_func funcs[] = { { .old_name = "cmdline_proc_show", .new_func = livepatch_cmdline_proc_show, }, { } }; This patch changes unsigned long old_addr -> void *old_func. It removes some confusion when these address are later used in the code. It is motivated by a followup patch that adds special NOP struct klp_func where we want to assign func->new_func = func->old_addr respectively func->new_func = func->old_func. This patch does not modify the existing behavior. Suggested-by: Josh Poimboeuf Signed-off-by: Petr Mladek Acked-by: Miroslav Benes Acked-by: Joe Lawrence Acked-by: Alice Ferrazzi Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- include/linux/livepatch.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include/linux') diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index aec44b1d9582..634e13876380 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -40,7 +40,7 @@ * @new_func: pointer to the patched function code * @old_sympos: a hint indicating which symbol position the old function * can be found (optional) - * @old_addr: the address of the function being patched + * @old_func: pointer to 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 @@ -77,7 +77,7 @@ struct klp_func { unsigned long old_sympos; /* internal */ - unsigned long old_addr; + void *old_func; struct kobject kobj; struct list_head stack_node; unsigned long old_size, new_size; -- cgit v1.2.3-71-gd317 From 0430f78bf38f9972f0cf0522709cc63d49fa164c Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 9 Jan 2019 13:43:21 +0100 Subject: livepatch: Consolidate klp_free functions The code for freeing livepatch structures is a bit scattered and tricky: + direct calls to klp_free_*_limited() and kobject_put() are used to release partially initialized objects + klp_free_patch() removes the patch from the public list and releases all objects except for patch->kobj + object_put(&patch->kobj) and the related wait_for_completion() are called directly outside klp_mutex; this code is duplicated; Now, we are going to remove the registration stage to simplify the API and the code. This would require handling more situations in klp_enable_patch() error paths. More importantly, we are going to add a feature called atomic replace. It will need to dynamically create func and object structures. We will want to reuse the existing init() and free() functions. This would create even more error path scenarios. This patch implements more straightforward free functions: + checks kobj_added flag instead of @limit[*] + initializes patch->list early so that the check for empty list always works + The action(s) that has to be done outside klp_mutex are done in separate klp_free_patch_finish() function. It waits only when patch->kobj was really released via the _start() part. The patch does not change the existing behavior. [*] We need our own flag to track that the kobject was successfully added to the hierarchy. Note that kobj.state_initialized only indicates that kobject has been initialized, not whether is has been added (and needs to be removed on cleanup). Signed-off-by: Petr Mladek Cc: Josh Poimboeuf Cc: Miroslav Benes Cc: Jessica Yu Cc: Jiri Kosina Cc: Jason Baron Acked-by: Miroslav Benes Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- include/linux/livepatch.h | 6 ++ kernel/livepatch/core.c | 137 +++++++++++++++++++++++++++++++--------------- 2 files changed, 98 insertions(+), 45 deletions(-) (limited to 'include/linux') diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index 634e13876380..6978785bc059 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -45,6 +45,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 + * @kobj_added: @kobj has been added and needs freeing * @patched: the func has been added to the klp_ops list * @transition: the func is currently being applied or reverted * @@ -81,6 +82,7 @@ struct klp_func { struct kobject kobj; struct list_head stack_node; unsigned long old_size, new_size; + bool kobj_added; bool patched; bool transition; }; @@ -117,6 +119,7 @@ struct klp_callbacks { * @kobj: kobject for sysfs resources * @mod: kernel module associated with the patched object * (NULL for vmlinux) + * @kobj_added: @kobj has been added and needs freeing * @patched: the object's funcs have been added to the klp_ops list */ struct klp_object { @@ -128,6 +131,7 @@ struct klp_object { /* internal */ struct kobject kobj; struct module *mod; + bool kobj_added; bool patched; }; @@ -137,6 +141,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 + * @kobj_added: @kobj has been added and needs freeing * @enabled: the patch is enabled (but operation may be incomplete) * @finish: for waiting till it is safe to remove the patch module */ @@ -148,6 +153,7 @@ struct klp_patch { /* internal */ struct list_head list; struct kobject kobj; + bool kobj_added; bool enabled; struct completion finish; }; diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 20589da35194..6f0d9095f662 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -465,17 +465,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_added) + kobject_put(&func->kobj); + } } /* Clean up when a patched object is unloaded */ @@ -489,30 +487,60 @@ static void klp_free_object_loaded(struct klp_object *obj) func->old_func = NULL; } -/* - * 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_added) + kobject_put(&obj->kobj); } } -static void klp_free_patch(struct klp_patch *patch) +/* + * This function implements the free operations that can be called safely + * under klp_mutex. + * + * The operation must be completed by calling klp_free_patch_finish() + * outside klp_mutex. + */ +static void klp_free_patch_start(struct klp_patch *patch) { - klp_free_objects_limited(patch, NULL); if (!list_empty(&patch->list)) list_del(&patch->list); + + klp_free_objects(patch); +} + +/* + * This function implements the free part that must be called outside + * klp_mutex. + * + * It must be called after klp_free_patch_start(). And it has to be + * the last function accessing the livepatch structures when the patch + * gets disabled. + */ +static void klp_free_patch_finish(struct klp_patch *patch) +{ + /* + * Avoid deadlock with enabled_store() sysfs callback by + * calling this outside klp_mutex. It is safe because + * this is called when the patch gets disabled and it + * cannot get enabled again. + */ + if (patch->kobj_added) { + kobject_put(&patch->kobj); + wait_for_completion(&patch->finish); + } } static int klp_init_func(struct klp_object *obj, struct klp_func *func) { + int ret; + if (!func->old_name || !func->new_func) return -EINVAL; @@ -528,9 +556,13 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func) * 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); + ret = kobject_init_and_add(&func->kobj, &klp_ktype_func, + &obj->kobj, "%s,%lu", func->old_name, + func->old_sympos ? func->old_sympos : 1); + if (!ret) + func->kobj_added = true; + + return ret; } /* Arches may override this to finish any remaining arch-specific tasks */ @@ -589,9 +621,6 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) int ret; const char *name; - if (!obj->funcs) - return -EINVAL; - if (klp_is_module(obj) && strlen(obj->name) >= MODULE_NAME_LEN) return -EINVAL; @@ -605,46 +634,66 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj) &patch->kobj, "%s", name); if (ret) return ret; + obj->kobj_added = true; klp_for_each_func(obj, func) { ret = klp_init_func(obj, func); if (ret) - goto free; + return ret; } - if (klp_is_object_loaded(obj)) { + 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_init_patch(struct klp_patch *patch) +static int klp_init_patch_early(struct klp_patch *patch) { struct klp_object *obj; - int ret; + struct klp_func *func; if (!patch->objs) return -EINVAL; - mutex_lock(&klp_mutex); - + INIT_LIST_HEAD(&patch->list); + patch->kobj_added = false; patch->enabled = false; init_completion(&patch->finish); + klp_for_each_object(patch, obj) { + if (!obj->funcs) + return -EINVAL; + + obj->kobj_added = false; + + klp_for_each_func(obj, func) + func->kobj_added = false; + } + + return 0; +} + +static int klp_init_patch(struct klp_patch *patch) +{ + struct klp_object *obj; + int ret; + + mutex_lock(&klp_mutex); + + ret = klp_init_patch_early(patch); + if (ret) { + mutex_unlock(&klp_mutex); + return ret; + } + ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch, klp_root_kobj, "%s", patch->mod->name); if (ret) { mutex_unlock(&klp_mutex); return ret; } + patch->kobj_added = true; klp_for_each_object(patch, obj) { ret = klp_init_object(patch, obj); @@ -659,12 +708,11 @@ static int klp_init_patch(struct klp_patch *patch) return 0; free: - klp_free_objects_limited(patch, obj); + klp_free_patch_start(patch); mutex_unlock(&klp_mutex); - kobject_put(&patch->kobj); - wait_for_completion(&patch->finish); + klp_free_patch_finish(patch); return ret; } @@ -693,12 +741,11 @@ int klp_unregister_patch(struct klp_patch *patch) goto err; } - klp_free_patch(patch); + klp_free_patch_start(patch); mutex_unlock(&klp_mutex); - kobject_put(&patch->kobj); - wait_for_completion(&patch->finish); + klp_free_patch_finish(patch); return 0; err: -- cgit v1.2.3-71-gd317 From 68007289bf3cd937a5b8fc4987d2787167bd06ca Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 9 Jan 2019 13:43:22 +0100 Subject: livepatch: Don't block the removal of patches loaded after a forced transition module_put() is currently never called in klp_complete_transition() when klp_force is set. As a result, we might keep the reference count even when klp_enable_patch() fails and klp_cancel_transition() is called. This might give the impression that a module might get blocked in some strange init state. Fortunately, it is not the case. The reference count is ignored when mod->init fails and erroneous modules are always removed. Anyway, this might be confusing. Instead, this patch moves the global klp_forced flag into struct klp_patch. As a result, we block only modules that might still be in use after a forced transition. Newly loaded livepatches might be eventually completely removed later. It is not a big deal. But the code is at least consistent with the reality. Signed-off-by: Petr Mladek Acked-by: Joe Lawrence Acked-by: Miroslav Benes Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- include/linux/livepatch.h | 2 ++ kernel/livepatch/core.c | 4 +++- kernel/livepatch/core.h | 1 + kernel/livepatch/transition.c | 10 +++++----- 4 files changed, 11 insertions(+), 6 deletions(-) (limited to 'include/linux') diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index 6978785bc059..6a9165d9b090 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -143,6 +143,7 @@ struct klp_object { * @kobj: kobject for sysfs resources * @kobj_added: @kobj has been added and needs freeing * @enabled: the patch is enabled (but operation may be incomplete) + * @forced: was involved in a forced transition * @finish: for waiting till it is safe to remove the patch module */ struct klp_patch { @@ -155,6 +156,7 @@ struct klp_patch { struct kobject kobj; bool kobj_added; bool enabled; + bool forced; struct completion finish; }; diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 6f0d9095f662..e77c5017ae0c 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -45,7 +45,8 @@ */ DEFINE_MUTEX(klp_mutex); -static LIST_HEAD(klp_patches); +/* Registered patches */ +LIST_HEAD(klp_patches); static struct kobject *klp_root_kobj; @@ -659,6 +660,7 @@ static int klp_init_patch_early(struct klp_patch *patch) INIT_LIST_HEAD(&patch->list); patch->kobj_added = false; patch->enabled = false; + patch->forced = false; init_completion(&patch->finish); klp_for_each_object(patch, obj) { diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h index 48a83d4364cf..d0cb5390e247 100644 --- a/kernel/livepatch/core.h +++ b/kernel/livepatch/core.h @@ -5,6 +5,7 @@ #include extern struct mutex klp_mutex; +extern struct list_head klp_patches; static inline bool klp_is_object_loaded(struct klp_object *obj) { diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index f27a378ad5e1..a4c921364003 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -33,8 +33,6 @@ struct klp_patch *klp_transition_patch; static int klp_target_state = KLP_UNDEFINED; -static bool klp_forced = false; - /* * This work can be performed periodically to finish patching or unpatching any * "straggler" tasks which failed to transition in the first attempt. @@ -137,10 +135,10 @@ static void klp_complete_transition(void) klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); /* - * klp_forced set implies unbounded increase of module's ref count if + * patch->forced set implies unbounded increase of module's ref count if * the module is disabled/enabled in a loop. */ - if (!klp_forced && klp_target_state == KLP_UNPATCHED) + if (!klp_transition_patch->forced && klp_target_state == KLP_UNPATCHED) module_put(klp_transition_patch->mod); klp_target_state = KLP_UNDEFINED; @@ -620,6 +618,7 @@ void klp_send_signals(void) */ void klp_force_transition(void) { + struct klp_patch *patch; struct task_struct *g, *task; unsigned int cpu; @@ -633,5 +632,6 @@ void klp_force_transition(void) for_each_possible_cpu(cpu) klp_update_patch_state(idle_task(cpu)); - klp_forced = true; + list_for_each_entry(patch, &klp_patches, list) + patch->forced = true; } -- cgit v1.2.3-71-gd317 From 958ef1e39d24d6cb8bf2a7406130a98c9564230f Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 9 Jan 2019 13:43:23 +0100 Subject: livepatch: Simplify API by removing registration step The possibility to re-enable a registered patch was useful for immediate patches where the livepatch module had to stay until the system reboot. The improved consistency model allows to achieve the same result by unloading and loading the livepatch module again. Also we are going to add a feature called atomic replace. It will allow to create a patch that would replace all already registered patches. The aim is to handle dependent patches more securely. It will obsolete the stack of patches that helped to handle the dependencies so far. Then it might be unclear when a cumulative patch re-enabling is safe. It would be complicated to support the many modes. Instead we could actually make the API and code easier to understand. Therefore, remove the two step public API. All the checks and init calls are moved from klp_register_patch() to klp_enabled_patch(). Also the patch is automatically freed, including the sysfs interface when the transition to the disabled state is completed. As a result, there is never a disabled patch on the top of the stack. Therefore we do not need to check the stack in __klp_enable_patch(). And we could simplify the check in __klp_disable_patch(). Also the API and logic is much easier. It is enough to call klp_enable_patch() in module_init() call. The patch can be disabled by writing '0' into /sys/kernel/livepatch//enabled. Then the module can be removed once the transition finishes and sysfs interface is freed. The only problem is how to free the structures and kobjects safely. The operation is triggered from the sysfs interface. We could not put the related kobject from there because it would cause lock inversion between klp_mutex and kernfs locks, see kn->count lockdep map. Therefore, offload the free task to a workqueue. It is perfectly fine: + The patch can no longer be used in the livepatch operations. + The module could not be removed until the free operation finishes and module_put() is called. + The operation is asynchronous already when the first klp_try_complete_transition() fails and another call is queued with a delay. Suggested-by: Josh Poimboeuf Signed-off-by: Petr Mladek Acked-by: Miroslav Benes Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- Documentation/livepatch/livepatch.txt | 135 +++++-------- include/linux/livepatch.h | 7 +- kernel/livepatch/core.c | 280 +++++++++------------------ kernel/livepatch/core.h | 2 + kernel/livepatch/transition.c | 19 +- samples/livepatch/livepatch-callbacks-demo.c | 13 +- samples/livepatch/livepatch-sample.c | 13 +- samples/livepatch/livepatch-shadow-fix1.c | 14 +- samples/livepatch/livepatch-shadow-fix2.c | 14 +- 9 files changed, 168 insertions(+), 329 deletions(-) (limited to 'include/linux') diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt index 2d7ed09dbd59..8f56490a4bb6 100644 --- a/Documentation/livepatch/livepatch.txt +++ b/Documentation/livepatch/livepatch.txt @@ -12,12 +12,11 @@ Table of Contents: 4. Livepatch module 4.1. New functions 4.2. Metadata - 4.3. Livepatch module handling 5. Livepatch life-cycle - 5.1. Registration + 5.1. Loading 5.2. Enabling 5.3. Disabling - 5.4. Unregistration + 5.4. Removing 6. Sysfs 7. Limitations @@ -298,117 +297,89 @@ into three levels: see the "Consistency model" section. -4.3. Livepatch module handling ------------------------------- - -The usual behavior is that the new functions will get used when -the livepatch module is loaded. For this, the module init() function -has to register the patch (struct klp_patch) and enable it. See the -section "Livepatch life-cycle" below for more details about these -two operations. - -Module removal is only safe when there are no users of the underlying -functions. This is the reason why the force feature permanently disables -the removal. The forced tasks entered the functions but we cannot say -that they returned back. Therefore it cannot be decided when the -livepatch module can be safely removed. When the system is successfully -transitioned to a new patch state (patched/unpatched) without being -forced it is guaranteed that no task sleeps or runs in the old code. - - 5. Livepatch life-cycle ======================= -Livepatching defines four basic operations that define the life cycle of each -live patch: registration, enabling, disabling and unregistration. There are -several reasons why it is done this way. - -First, the patch is applied only when all patched symbols for already -loaded objects are found. The error handling is much easier if this -check is done before particular functions get redirected. +Livepatching can be described by four basic operations: +loading, enabling, disabling, removing. -Second, it might take some time until the entire system is migrated with -the hybrid consistency model being used. The patch revert might block -the livepatch module removal for too long. Therefore it is useful to -revert the patch using a separate operation that might be called -explicitly. But it does not make sense to remove all information until -the livepatch module is really removed. +5.1. Loading +------------ -5.1. Registration ------------------ +The only reasonable way is to enable the patch when the livepatch kernel +module is being loaded. For this, klp_enable_patch() has to be called +in the module_init() callback. There are two main reasons: -Each patch first has to be registered using klp_register_patch(). This makes -the patch known to the livepatch framework. Also it does some preliminary -computing and checks. +First, only the module has an easy access to the related struct klp_patch. -In particular, the patch is added into the list of known patches. The -addresses of the patched functions are found according to their names. -The special relocations, mentioned in the section "New functions", are -applied. The relevant entries are created under -/sys/kernel/livepatch/. The patch is rejected when any operation -fails. +Second, the error code might be used to refuse loading the module when +the patch cannot get enabled. 5.2. Enabling ------------- -Registered patches might be enabled either by calling klp_enable_patch() or -by writing '1' to /sys/kernel/livepatch//enabled. The system will -start using the new implementation of the patched functions at this stage. +The livepatch gets enabled by calling klp_enable_patch() from +the module_init() callback. The system will start using the new +implementation of the patched functions at this stage. -When a patch is enabled, livepatch enters into a transition state where -tasks are converging to the patched state. This is indicated by a value -of '1' in /sys/kernel/livepatch//transition. Once all tasks have -been patched, the 'transition' value changes to '0'. For more -information about this process, see the "Consistency model" section. +First, the addresses of the patched functions are found according to their +names. The special relocations, mentioned in the section "New functions", +are applied. The relevant entries are created under +/sys/kernel/livepatch/. The patch is rejected when any above +operation fails. -If an original function is patched for the first time, a function -specific struct klp_ops is created and an universal ftrace handler is -registered. +Second, livepatch enters into a transition state where tasks are converging +to the patched state. If an original function is patched for the first +time, a function specific struct klp_ops is created and an universal +ftrace handler is registered[*]. This stage is indicated by a value of '1' +in /sys/kernel/livepatch//transition. For more information about +this process, see the "Consistency model" section. -Functions might be patched multiple times. The ftrace handler is registered -only once for the given function. Further patches just add an entry to the -list (see field `func_stack`) of the struct klp_ops. The last added -entry is chosen by the ftrace handler and becomes the active function -replacement. +Finally, once all tasks have been patched, the 'transition' value changes +to '0'. -Note that the patches might be enabled in a different order than they were -registered. +[*] Note that functions might be patched multiple times. The ftrace handler + is registered only once for a given function. Further patches just add + an entry to the list (see field `func_stack`) of the struct klp_ops. + The right implementation is selected by the ftrace handler, see + the "Consistency model" section. 5.3. Disabling -------------- -Enabled patches might get disabled either by calling klp_disable_patch() or -by writing '0' to /sys/kernel/livepatch//enabled. At this stage -either the code from the previously enabled patch or even the original -code gets used. +Enabled patches might get disabled by writing '0' to +/sys/kernel/livepatch//enabled. -When a patch is disabled, livepatch enters into a transition state where -tasks are converging to the unpatched state. This is indicated by a -value of '1' in /sys/kernel/livepatch//transition. Once all tasks -have been unpatched, the 'transition' value changes to '0'. For more -information about this process, see the "Consistency model" section. +First, livepatch enters into a transition state where tasks are converging +to the unpatched state. The system starts using either the code from +the previously enabled patch or even the original one. This stage is +indicated by a value of '1' in /sys/kernel/livepatch//transition. +For more information about this process, see the "Consistency model" +section. -Here all the functions (struct klp_func) associated with the to-be-disabled +Second, once all tasks have been unpatched, the 'transition' value changes +to '0'. All the functions (struct klp_func) associated with the to-be-disabled patch are removed from the corresponding struct klp_ops. The ftrace handler is unregistered and the struct klp_ops is freed when the func_stack list becomes empty. -Patches must be disabled in exactly the reverse order in which they were -enabled. It makes the problem and the implementation much easier. +Third, the sysfs interface is destroyed. +Note that patches must be disabled in exactly the reverse order in which +they were enabled. It makes the problem and the implementation much easier. -5.4. Unregistration -------------------- -Disabled patches might be unregistered by calling klp_unregister_patch(). -This can be done only when the patch is disabled and the code is no longer -used. It must be called before the livepatch module gets unloaded. +5.4. Removing +------------- -At this stage, all the relevant sys-fs entries are removed and the patch -is removed from the list of known patches. +Module removal is only safe when there are no users of functions provided +by the module. This is the reason why the force feature permanently +disables the removal. Only when the system is successfully transitioned +to a new patch state (patched/unpatched) without being forced it is +guaranteed that no task sleeps or runs in the old code. 6. Sysfs diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index 6a9165d9b090..8f9c19c69744 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -139,11 +139,12 @@ 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 - * @list: list node for global list of registered patches + * @list: list node for global list of actively used patches * @kobj: kobject for sysfs resources * @kobj_added: @kobj has been added and needs freeing * @enabled: the patch is enabled (but operation may be incomplete) * @forced: was involved in a forced transition + * @free_work: patch cleanup from workqueue-context * @finish: for waiting till it is safe to remove the patch module */ struct klp_patch { @@ -157,6 +158,7 @@ struct klp_patch { bool kobj_added; bool enabled; bool forced; + struct work_struct free_work; struct completion finish; }; @@ -168,10 +170,7 @@ struct klp_patch { func->old_name || func->new_func || func->old_sympos; \ func++) -int klp_register_patch(struct klp_patch *); -int klp_unregister_patch(struct klp_patch *); int klp_enable_patch(struct klp_patch *); -int klp_disable_patch(struct klp_patch *); void arch_klp_init_object_loaded(struct klp_patch *patch, struct klp_object *obj); diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index e77c5017ae0c..bd41b03a72d5 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -45,7 +45,11 @@ */ DEFINE_MUTEX(klp_mutex); -/* Registered patches */ +/* + * Actively used patches: enabled or in transition. Note that replaced + * or disabled patches are not listed even though the related kernel + * module still can be loaded. + */ LIST_HEAD(klp_patches); static struct kobject *klp_root_kobj; @@ -83,17 +87,6 @@ static void klp_find_object_module(struct klp_object *obj) mutex_unlock(&module_mutex); } -static bool klp_is_patch_registered(struct klp_patch *patch) -{ - struct klp_patch *mypatch; - - list_for_each_entry(mypatch, &klp_patches, list) - if (mypatch == patch) - return true; - - return false; -} - static bool klp_initialized(void) { return !!klp_root_kobj; @@ -292,7 +285,6 @@ static int klp_write_object_relocations(struct module *pmod, * /sys/kernel/livepatch/// */ static int __klp_disable_patch(struct klp_patch *patch); -static int __klp_enable_patch(struct klp_patch *patch); static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) @@ -309,40 +301,32 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, mutex_lock(&klp_mutex); - if (!klp_is_patch_registered(patch)) { - /* - * Module with the patch could either disappear meanwhile or is - * not properly initialized yet. - */ - ret = -EINVAL; - goto err; - } - if (patch->enabled == enabled) { /* already in requested state */ ret = -EINVAL; - goto err; + goto out; } - if (patch == klp_transition_patch) { + /* + * Allow to reverse a pending transition in both ways. It might be + * necessary to complete the transition without forcing and breaking + * the system integrity. + * + * Do not allow to re-enable a disabled patch. + */ + if (patch == klp_transition_patch) klp_reverse_transition(); - } else if (enabled) { - ret = __klp_enable_patch(patch); - if (ret) - goto err; - } else { + else if (!enabled) ret = __klp_disable_patch(patch); - if (ret) - goto err; - } + else + ret = -EINVAL; +out: mutex_unlock(&klp_mutex); + if (ret) + return ret; return count; - -err: - mutex_unlock(&klp_mutex); - return ret; } static ssize_t enabled_show(struct kobject *kobj, @@ -508,7 +492,7 @@ static void klp_free_objects(struct klp_patch *patch) * The operation must be completed by calling klp_free_patch_finish() * outside klp_mutex. */ -static void klp_free_patch_start(struct klp_patch *patch) +void klp_free_patch_start(struct klp_patch *patch) { if (!list_empty(&patch->list)) list_del(&patch->list); @@ -536,6 +520,23 @@ static void klp_free_patch_finish(struct klp_patch *patch) kobject_put(&patch->kobj); wait_for_completion(&patch->finish); } + + /* Put the module after the last access to struct klp_patch. */ + if (!patch->forced) + module_put(patch->mod); +} + +/* + * The livepatch might be freed from sysfs interface created by the patch. + * This work allows to wait until the interface is destroyed in a separate + * context. + */ +static void klp_free_patch_work_fn(struct work_struct *work) +{ + struct klp_patch *patch = + container_of(work, struct klp_patch, free_work); + + klp_free_patch_finish(patch); } static int klp_init_func(struct klp_object *obj, struct klp_func *func) @@ -661,6 +662,7 @@ static int klp_init_patch_early(struct klp_patch *patch) patch->kobj_added = false; patch->enabled = false; patch->forced = false; + INIT_WORK(&patch->free_work, klp_free_patch_work_fn); init_completion(&patch->finish); klp_for_each_object(patch, obj) { @@ -673,6 +675,9 @@ static int klp_init_patch_early(struct klp_patch *patch) func->kobj_added = false; } + if (!try_module_get(patch->mod)) + return -ENODEV; + return 0; } @@ -681,115 +686,22 @@ static int klp_init_patch(struct klp_patch *patch) struct klp_object *obj; int ret; - mutex_lock(&klp_mutex); - - ret = klp_init_patch_early(patch); - if (ret) { - mutex_unlock(&klp_mutex); - return ret; - } - ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch, klp_root_kobj, "%s", patch->mod->name); - if (ret) { - mutex_unlock(&klp_mutex); + if (ret) return ret; - } patch->kobj_added = true; klp_for_each_object(patch, obj) { ret = klp_init_object(patch, obj); if (ret) - goto free; + return ret; } list_add_tail(&patch->list, &klp_patches); - mutex_unlock(&klp_mutex); - - return 0; - -free: - klp_free_patch_start(patch); - - mutex_unlock(&klp_mutex); - - klp_free_patch_finish(patch); - - return ret; -} - -/** - * klp_unregister_patch() - unregisters a patch - * @patch: Disabled patch to be unregistered - * - * Frees the data structures and removes the sysfs interface. - * - * Return: 0 on success, otherwise error - */ -int klp_unregister_patch(struct klp_patch *patch) -{ - int ret; - - mutex_lock(&klp_mutex); - - if (!klp_is_patch_registered(patch)) { - ret = -EINVAL; - goto err; - } - - if (patch->enabled) { - ret = -EBUSY; - goto err; - } - - klp_free_patch_start(patch); - - mutex_unlock(&klp_mutex); - - klp_free_patch_finish(patch); - return 0; -err: - mutex_unlock(&klp_mutex); - return ret; -} -EXPORT_SYMBOL_GPL(klp_unregister_patch); - -/** - * klp_register_patch() - registers a patch - * @patch: Patch to be registered - * - * Initializes the data structure associated with the patch and - * creates the sysfs interface. - * - * There is no need to take the reference on the patch module here. It is done - * later when the patch is enabled. - * - * Return: 0 on success, otherwise error - */ -int klp_register_patch(struct klp_patch *patch) -{ - if (!patch || !patch->mod) - return -EINVAL; - - if (!is_livepatch_module(patch->mod)) { - pr_err("module %s is not marked as a livepatch module\n", - patch->mod->name); - return -EINVAL; - } - - if (!klp_initialized()) - return -ENODEV; - - if (!klp_have_reliable_stack()) { - pr_err("This architecture doesn't have support for the livepatch consistency model.\n"); - return -ENOSYS; - } - - return klp_init_patch(patch); } -EXPORT_SYMBOL_GPL(klp_register_patch); static int __klp_disable_patch(struct klp_patch *patch) { @@ -802,8 +714,7 @@ static int __klp_disable_patch(struct klp_patch *patch) return -EBUSY; /* enforce stacking: only the last enabled patch can be disabled */ - if (!list_is_last(&patch->list, &klp_patches) && - list_next_entry(patch, list)->enabled) + if (!list_is_last(&patch->list, &klp_patches)) return -EBUSY; klp_init_transition(patch, KLP_UNPATCHED); @@ -822,44 +733,12 @@ static int __klp_disable_patch(struct klp_patch *patch) smp_wmb(); klp_start_transition(); - klp_try_complete_transition(); patch->enabled = false; + klp_try_complete_transition(); return 0; } -/** - * klp_disable_patch() - disables a registered patch - * @patch: The registered, enabled patch to be disabled - * - * Unregisters the patched functions from ftrace. - * - * Return: 0 on success, otherwise error - */ -int klp_disable_patch(struct klp_patch *patch) -{ - int ret; - - mutex_lock(&klp_mutex); - - if (!klp_is_patch_registered(patch)) { - ret = -EINVAL; - goto err; - } - - if (!patch->enabled) { - ret = -EINVAL; - goto err; - } - - ret = __klp_disable_patch(patch); - -err: - mutex_unlock(&klp_mutex); - return ret; -} -EXPORT_SYMBOL_GPL(klp_disable_patch); - static int __klp_enable_patch(struct klp_patch *patch) { struct klp_object *obj; @@ -871,17 +750,8 @@ 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 */ - if (patch->list.prev != &klp_patches && - !list_prev_entry(patch, list)->enabled) - return -EBUSY; - - /* - * A reference is taken on the patch module to prevent it from being - * unloaded. - */ - if (!try_module_get(patch->mod)) - return -ENODEV; + if (!patch->kobj_added) + return -EINVAL; pr_notice("enabling patch '%s'\n", patch->mod->name); @@ -916,8 +786,8 @@ static int __klp_enable_patch(struct klp_patch *patch) } klp_start_transition(); - klp_try_complete_transition(); patch->enabled = true; + klp_try_complete_transition(); return 0; err: @@ -928,11 +798,15 @@ err: } /** - * klp_enable_patch() - enables a registered patch - * @patch: The registered, disabled patch to be enabled + * klp_enable_patch() - enable the livepatch + * @patch: patch to be enabled * - * Performs the needed symbol lookups and code relocations, - * then registers the patched functions with ftrace. + * Initializes the data structure associated with the patch, creates the sysfs + * interface, performs the needed symbol lookups and code relocations, + * registers the patched functions with ftrace. + * + * This function is supposed to be called from the livepatch module_init() + * callback. * * Return: 0 on success, otherwise error */ @@ -940,17 +814,51 @@ int klp_enable_patch(struct klp_patch *patch) { int ret; + if (!patch || !patch->mod) + return -EINVAL; + + if (!is_livepatch_module(patch->mod)) { + pr_err("module %s is not marked as a livepatch module\n", + patch->mod->name); + return -EINVAL; + } + + if (!klp_initialized()) + return -ENODEV; + + if (!klp_have_reliable_stack()) { + pr_err("This architecture doesn't have support for the livepatch consistency model.\n"); + return -ENOSYS; + } + + mutex_lock(&klp_mutex); - if (!klp_is_patch_registered(patch)) { - ret = -EINVAL; - goto err; + ret = klp_init_patch_early(patch); + if (ret) { + mutex_unlock(&klp_mutex); + return ret; } + ret = klp_init_patch(patch); + if (ret) + goto err; + ret = __klp_enable_patch(patch); + if (ret) + goto err; + + mutex_unlock(&klp_mutex); + + return 0; err: + klp_free_patch_start(patch); + mutex_unlock(&klp_mutex); + + klp_free_patch_finish(patch); + return ret; } EXPORT_SYMBOL_GPL(klp_enable_patch); diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h index d0cb5390e247..d4eefc520c08 100644 --- a/kernel/livepatch/core.h +++ b/kernel/livepatch/core.h @@ -7,6 +7,8 @@ extern struct mutex klp_mutex; extern struct list_head klp_patches; +void klp_free_patch_start(struct klp_patch *patch); + static inline bool klp_is_object_loaded(struct klp_object *obj) { return !obj->name || obj->mod; diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index a4c921364003..c9917a24b3a4 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -134,13 +134,6 @@ static void klp_complete_transition(void) pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name, klp_target_state == KLP_PATCHED ? "patching" : "unpatching"); - /* - * patch->forced set implies unbounded increase of module's ref count if - * the module is disabled/enabled in a loop. - */ - if (!klp_transition_patch->forced && klp_target_state == KLP_UNPATCHED) - module_put(klp_transition_patch->mod); - klp_target_state = KLP_UNDEFINED; klp_transition_patch = NULL; } @@ -357,6 +350,7 @@ void klp_try_complete_transition(void) { unsigned int cpu; struct task_struct *g, *task; + struct klp_patch *patch; bool complete = true; WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED); @@ -405,7 +399,18 @@ void klp_try_complete_transition(void) } /* we're done, now cleanup the data structures */ + patch = klp_transition_patch; klp_complete_transition(); + + /* + * It would make more sense to free the patch in + * klp_complete_transition() but it is called also + * from klp_cancel_transition(). + */ + if (!patch->enabled) { + klp_free_patch_start(patch); + schedule_work(&patch->free_work); + } } /* diff --git a/samples/livepatch/livepatch-callbacks-demo.c b/samples/livepatch/livepatch-callbacks-demo.c index 72f9e6d1387b..62d97953ad02 100644 --- a/samples/livepatch/livepatch-callbacks-demo.c +++ b/samples/livepatch/livepatch-callbacks-demo.c @@ -195,22 +195,11 @@ static struct klp_patch patch = { static int livepatch_callbacks_demo_init(void) { - int ret; - - ret = klp_register_patch(&patch); - if (ret) - return ret; - ret = klp_enable_patch(&patch); - if (ret) { - WARN_ON(klp_unregister_patch(&patch)); - return ret; - } - return 0; + return klp_enable_patch(&patch); } static void livepatch_callbacks_demo_exit(void) { - WARN_ON(klp_unregister_patch(&patch)); } module_init(livepatch_callbacks_demo_init); diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c index 2d554dd930e2..01c9cf003ca2 100644 --- a/samples/livepatch/livepatch-sample.c +++ b/samples/livepatch/livepatch-sample.c @@ -69,22 +69,11 @@ static struct klp_patch patch = { static int livepatch_init(void) { - int ret; - - ret = klp_register_patch(&patch); - if (ret) - return ret; - ret = klp_enable_patch(&patch); - if (ret) { - WARN_ON(klp_unregister_patch(&patch)); - return ret; - } - return 0; + return klp_enable_patch(&patch); } static void livepatch_exit(void) { - WARN_ON(klp_unregister_patch(&patch)); } module_init(livepatch_init); diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c index e8f1bd6b29b1..a5a5cac21d4d 100644 --- a/samples/livepatch/livepatch-shadow-fix1.c +++ b/samples/livepatch/livepatch-shadow-fix1.c @@ -157,25 +157,13 @@ static struct klp_patch patch = { static int livepatch_shadow_fix1_init(void) { - int ret; - - ret = klp_register_patch(&patch); - if (ret) - return ret; - ret = klp_enable_patch(&patch); - if (ret) { - WARN_ON(klp_unregister_patch(&patch)); - return ret; - } - return 0; + return klp_enable_patch(&patch); } static void livepatch_shadow_fix1_exit(void) { /* Cleanup any existing SV_LEAK shadow variables */ klp_shadow_free_all(SV_LEAK, livepatch_fix1_dummy_leak_dtor); - - WARN_ON(klp_unregister_patch(&patch)); } module_init(livepatch_shadow_fix1_init); diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c index b34c7bf83356..52de947b5526 100644 --- a/samples/livepatch/livepatch-shadow-fix2.c +++ b/samples/livepatch/livepatch-shadow-fix2.c @@ -129,25 +129,13 @@ static struct klp_patch patch = { static int livepatch_shadow_fix2_init(void) { - int ret; - - ret = klp_register_patch(&patch); - if (ret) - return ret; - ret = klp_enable_patch(&patch); - if (ret) { - WARN_ON(klp_unregister_patch(&patch)); - return ret; - } - return 0; + return klp_enable_patch(&patch); } static void livepatch_shadow_fix2_exit(void) { /* Cleanup any existing SV_COUNTER shadow variables */ klp_shadow_free_all(SV_COUNTER, NULL); - - WARN_ON(klp_unregister_patch(&patch)); } module_init(livepatch_shadow_fix2_init); -- cgit v1.2.3-71-gd317 From 20e55025958e18e671d92c7adea00c301ac93c43 Mon Sep 17 00:00:00 2001 From: Jason Baron Date: Wed, 9 Jan 2019 13:43:24 +0100 Subject: livepatch: Use lists to manage patches, objects and functions 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. Signed-off-by: Jason Baron [pmladek@suse.com: Initialize lists before init calls] Signed-off-by: Petr Mladek Acked-by: Miroslav Benes Acked-by: Joe Lawrence Cc: Josh Poimboeuf Cc: Jiri Kosina Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- include/linux/livepatch.h | 19 +++++++++++++++++-- kernel/livepatch/core.c | 9 +++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) (limited to 'include/linux') diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index 8f9c19c69744..e117e20ff771 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -24,6 +24,7 @@ #include #include #include +#include #if IS_ENABLED(CONFIG_LIVEPATCH) @@ -42,6 +43,7 @@ * can be found (optional) * @old_func: pointer to 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 @@ -80,6 +82,7 @@ struct klp_func { /* internal */ void *old_func; struct kobject kobj; + struct list_head node; struct list_head stack_node; unsigned long old_size, new_size; bool kobj_added; @@ -117,6 +120,8 @@ struct klp_callbacks { * @funcs: function entries for functions to be patched in the object * @callbacks: functions to be executed pre/post (un)patching * @kobj: kobject for sysfs resources + * @func_list: dynamic list of the function entries + * @node: list node for klp_patch obj_list * @mod: kernel module associated with the patched object * (NULL for vmlinux) * @kobj_added: @kobj has been added and needs freeing @@ -130,6 +135,8 @@ struct klp_object { /* internal */ struct kobject kobj; + struct list_head func_list; + struct list_head node; struct module *mod; bool kobj_added; bool patched; @@ -141,6 +148,7 @@ struct klp_object { * @objs: object entries for kernel objects to be patched * @list: list node for global list of actively used patches * @kobj: kobject for sysfs resources + * @obj_list: dynamic list of the object entries * @kobj_added: @kobj has been added and needs freeing * @enabled: the patch is enabled (but operation may be incomplete) * @forced: was involved in a forced transition @@ -155,6 +163,7 @@ struct klp_patch { /* internal */ struct list_head list; struct kobject kobj; + struct list_head obj_list; bool kobj_added; bool enabled; bool forced; @@ -162,14 +171,20 @@ struct klp_patch { 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_enable_patch(struct klp_patch *); void arch_klp_init_object_loaded(struct klp_patch *patch, diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index bd41b03a72d5..37d0d3645fa6 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -659,20 +659,25 @@ static int klp_init_patch_early(struct klp_patch *patch) return -EINVAL; INIT_LIST_HEAD(&patch->list); + INIT_LIST_HEAD(&patch->obj_list); patch->kobj_added = false; patch->enabled = false; patch->forced = false; INIT_WORK(&patch->free_work, klp_free_patch_work_fn); init_completion(&patch->finish); - klp_for_each_object(patch, obj) { + klp_for_each_object_static(patch, obj) { if (!obj->funcs) return -EINVAL; + INIT_LIST_HEAD(&obj->func_list); obj->kobj_added = false; + list_add_tail(&obj->node, &patch->obj_list); - klp_for_each_func(obj, func) + klp_for_each_func_static(obj, func) { func->kobj_added = false; + list_add_tail(&func->node, &obj->func_list); + } } if (!try_module_get(patch->mod)) -- cgit v1.2.3-71-gd317 From e1452b607c48c642caf57299f4da83aa002f8533 Mon Sep 17 00:00:00 2001 From: Jason Baron Date: Wed, 9 Jan 2019 13:43:25 +0100 Subject: livepatch: Add atomic replace 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. Atomic replace && cumulative 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 are 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 and the address is known only when the object (module) is loaded. Note that we really need to set it. The address is used, for example, in klp_check_stack_func(). 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_func. 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. Also note that the safe iterator must be used in the free() functions. Otherwise already freed structures might get accessed. 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. Removing replaced patches: One nice effect of the cumulative patches is that the code from the older patches is no longer used. Therefore the replaced patches can be removed. It has several advantages: + Nops' structs will no 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. + The state is clean in both the sysfs interface and lsmod. The modules with the replaced livepatches might even get removed from the system. 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 list of patches. It is safe. The consistency model ensures that they are no 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. Signed-off-by: Jason Baron [pmladek@suse.com: Split, reuse existing code, simplified] Signed-off-by: Petr Mladek Cc: Josh Poimboeuf Cc: Jessica Yu Cc: Jiri Kosina Cc: Miroslav Benes Acked-by: Miroslav Benes Acked-by: Josh Poimboeuf Signed-off-by: Jiri Kosina --- Documentation/livepatch/livepatch.txt | 31 ++++- include/linux/livepatch.h | 12 ++ kernel/livepatch/core.c | 232 ++++++++++++++++++++++++++++++++-- kernel/livepatch/core.h | 1 + kernel/livepatch/patch.c | 8 ++ kernel/livepatch/transition.c | 3 + 6 files changed, 273 insertions(+), 14 deletions(-) (limited to 'include/linux') diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt index 8f56490a4bb6..2a70f43166f6 100644 --- a/Documentation/livepatch/livepatch.txt +++ b/Documentation/livepatch/livepatch.txt @@ -15,8 +15,9 @@ Table of Contents: 5. Livepatch life-cycle 5.1. Loading 5.2. Enabling - 5.3. Disabling - 5.4. Removing + 5.3. Replacing + 5.4. Disabling + 5.5. Removing 6. Sysfs 7. Limitations @@ -300,8 +301,12 @@ into three levels: 5. Livepatch life-cycle ======================= -Livepatching can be described by four basic operations: -loading, enabling, disabling, removing. +Livepatching can be described by five basic operations: +loading, enabling, replacing, disabling, removing. + +Where the replacing and the disabling operations are mutually +exclusive. They have the same result for the given patch but +not for the system. 5.1. Loading @@ -347,7 +352,21 @@ to '0'. the "Consistency model" section. -5.3. Disabling +5.3. Replacing +-------------- + +All enabled patches might get replaced by a cumulative patch that +has the .replace flag set. + +Once the new patch is enabled and the 'transition' finishes then +all the functions (struct klp_func) associated with the replaced +patches are removed from the corresponding struct klp_ops. Also +the ftrace handler is unregistered and the struct klp_ops is +freed when the related function is not modified by the new patch +and func_stack list becomes empty. + + +5.4. Disabling -------------- Enabled patches might get disabled by writing '0' to @@ -372,7 +391,7 @@ Note that patches must be disabled in exactly the reverse order in which they were enabled. It makes the problem and the implementation much easier. -5.4. Removing +5.5. Removing ------------- Module removal is only safe when there are no users of functions provided diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h index e117e20ff771..53551f470722 100644 --- a/include/linux/livepatch.h +++ b/include/linux/livepatch.h @@ -48,6 +48,7 @@ * @old_size: size of the old function * @new_size: size of the new function * @kobj_added: @kobj has been added and needs freeing + * @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 * @@ -86,6 +87,7 @@ struct klp_func { struct list_head stack_node; unsigned long old_size, new_size; bool kobj_added; + bool nop; bool patched; bool transition; }; @@ -125,6 +127,7 @@ struct klp_callbacks { * @mod: kernel module associated with the patched object * (NULL for vmlinux) * @kobj_added: @kobj has been added and needs freeing + * @dynamic: temporary object for nop functions; dynamically allocated * @patched: the object's funcs have been added to the klp_ops list */ struct klp_object { @@ -139,6 +142,7 @@ struct klp_object { struct list_head node; struct module *mod; bool kobj_added; + bool dynamic; bool patched; }; @@ -146,6 +150,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 actively used patches * @list: list node for global list of actively used patches * @kobj: kobject for sysfs resources * @obj_list: dynamic list of the object entries @@ -159,6 +164,7 @@ struct klp_patch { /* external */ struct module *mod; struct klp_object *objs; + bool replace; /* internal */ struct list_head list; @@ -174,6 +180,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) @@ -182,6 +191,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) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 37d0d3645fa6..ecb7660f1d8b 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -92,6 +92,40 @@ static bool klp_initialized(void) return !!klp_root_kobj; } +static struct klp_func *klp_find_func(struct klp_object *obj, + struct klp_func *old_func) +{ + struct klp_func *func; + + klp_for_each_func(obj, func) { + if ((strcmp(old_func->old_name, func->old_name) == 0) && + (old_func->old_sympos == func->old_sympos)) { + return func; + } + } + + return NULL; +} + +static struct klp_object *klp_find_object(struct klp_patch *patch, + struct klp_object *old_obj) +{ + struct klp_object *obj; + + klp_for_each_object(patch, obj) { + if (klp_is_module(old_obj)) { + if (klp_is_module(obj) && + strcmp(old_obj->name, obj->name) == 0) { + return obj; + } + } else if (!klp_is_module(obj)) { + return obj; + } + } + + return NULL; +} + struct klp_find_arg { const char *objname; const char *name; @@ -418,6 +452,121 @@ static struct attribute *klp_patch_attrs[] = { NULL }; +static void klp_free_object_dynamic(struct klp_object *obj) +{ + kfree(obj->name); + kfree(obj); +} + +static struct klp_object *klp_alloc_object_dynamic(const char *name) +{ + struct klp_object *obj; + + obj = kzalloc(sizeof(*obj), GFP_KERNEL); + if (!obj) + return NULL; + + if (name) { + obj->name = kstrdup(name, GFP_KERNEL); + if (!obj->name) { + kfree(obj); + return NULL; + } + } + + INIT_LIST_HEAD(&obj->func_list); + obj->dynamic = true; + + return obj; +} + +static void klp_free_func_nop(struct klp_func *func) +{ + kfree(func->old_name); + kfree(func); +} + +static struct klp_func *klp_alloc_func_nop(struct klp_func *old_func, + struct klp_object *obj) +{ + struct klp_func *func; + + func = kzalloc(sizeof(*func), GFP_KERNEL); + if (!func) + return NULL; + + if (old_func->old_name) { + func->old_name = kstrdup(old_func->old_name, GFP_KERNEL); + if (!func->old_name) { + kfree(func); + return NULL; + } + } + + /* + * func->new_func is same as func->old_func. These addresses are + * set when the object is loaded, see klp_init_object_loaded(). + */ + func->old_sympos = old_func->old_sympos; + func->nop = true; + + return func; +} + +static int klp_add_object_nops(struct klp_patch *patch, + struct klp_object *old_obj) +{ + struct klp_object *obj; + struct klp_func *func, *old_func; + + obj = klp_find_object(patch, old_obj); + + if (!obj) { + obj = klp_alloc_object_dynamic(old_obj->name); + if (!obj) + return -ENOMEM; + + list_add_tail(&obj->node, &patch->obj_list); + } + + klp_for_each_func(old_obj, old_func) { + func = klp_find_func(obj, old_func); + if (func) + continue; + + func = klp_alloc_func_nop(old_func, obj); + if (!func) + return -ENOMEM; + + list_add_tail(&func->node, &obj->func_list); + } + + return 0; +} + +/* + * Add 'nop' functions which simply return to the caller to run + * the original function. The 'nop' functions are added to a + * patch to facilitate a 'replace' mode. + */ +static int klp_add_nops(struct klp_patch *patch) +{ + struct klp_patch *old_patch; + struct klp_object *old_obj; + + list_for_each_entry(old_patch, &klp_patches, list) { + klp_for_each_object(old_patch, old_obj) { + int err; + + err = klp_add_object_nops(patch, old_obj); + if (err) + return err; + } + } + + return 0; +} + static void klp_kobj_release_patch(struct kobject *kobj) { struct klp_patch *patch; @@ -434,6 +583,12 @@ static struct kobj_type klp_ktype_patch = { static void klp_kobj_release_object(struct kobject *kobj) { + struct klp_object *obj; + + obj = container_of(kobj, struct klp_object, kobj); + + if (obj->dynamic) + klp_free_object_dynamic(obj); } static struct kobj_type klp_ktype_object = { @@ -443,6 +598,12 @@ static struct kobj_type klp_ktype_object = { static void klp_kobj_release_func(struct kobject *kobj) { + struct klp_func *func; + + func = container_of(kobj, struct klp_func, kobj); + + if (func->nop) + klp_free_func_nop(func); } static struct kobj_type klp_ktype_func = { @@ -452,12 +613,15 @@ static struct kobj_type klp_ktype_func = { static void klp_free_funcs(struct klp_object *obj) { - struct klp_func *func; + struct klp_func *func, *tmp_func; - klp_for_each_func(obj, func) { + klp_for_each_func_safe(obj, func, tmp_func) { /* Might be called from klp_init_patch() error path. */ - if (func->kobj_added) + if (func->kobj_added) { kobject_put(&func->kobj); + } else if (func->nop) { + klp_free_func_nop(func); + } } } @@ -468,20 +632,27 @@ static void klp_free_object_loaded(struct klp_object *obj) obj->mod = NULL; - klp_for_each_func(obj, func) + klp_for_each_func(obj, func) { func->old_func = NULL; + + if (func->nop) + func->new_func = NULL; + } } static void klp_free_objects(struct klp_patch *patch) { - struct klp_object *obj; + struct klp_object *obj, *tmp_obj; - klp_for_each_object(patch, obj) { + klp_for_each_object_safe(patch, obj, tmp_obj) { klp_free_funcs(obj); /* Might be called from klp_init_patch() error path. */ - if (obj->kobj_added) + if (obj->kobj_added) { kobject_put(&obj->kobj); + } else if (obj->dynamic) { + klp_free_object_dynamic(obj); + } } } @@ -543,7 +714,14 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func) { int ret; - if (!func->old_name || !func->new_func) + if (!func->old_name) + return -EINVAL; + + /* + * NOPs get the address later. The patched module must be loaded, + * see klp_init_object_loaded(). + */ + if (!func->new_func && !func->nop) return -EINVAL; if (strlen(func->old_name) >= KSYM_NAME_LEN) @@ -605,6 +783,9 @@ static int klp_init_object_loaded(struct klp_patch *patch, return -ENOENT; } + if (func->nop) + func->new_func = func->old_func; + ret = kallsyms_lookup_size_offset((unsigned long)func->new_func, &func->new_size, NULL); if (!ret) { @@ -697,6 +878,12 @@ static int klp_init_patch(struct klp_patch *patch) return ret; patch->kobj_added = true; + if (patch->replace) { + ret = klp_add_nops(patch); + if (ret) + return ret; + } + klp_for_each_object(patch, obj) { ret = klp_init_object(patch, obj); if (ret) @@ -868,6 +1055,35 @@ err: } EXPORT_SYMBOL_GPL(klp_enable_patch); +/* + * This function removes replaced patches. + * + * 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. Also + * this is handled transparently by patch->module_put. + */ +void klp_discard_replaced_patches(struct klp_patch *new_patch) +{ + 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; + + old_patch->enabled = false; + klp_unpatch_objects(old_patch); + klp_free_patch_start(old_patch); + schedule_work(&old_patch->free_work); + } +} + /* * Remove parts of patches that touch a given kernel module. The list of * patches processed might be limited. When limit is NULL, all patches diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h index d4eefc520c08..f6a853adcc00 100644 --- a/kernel/livepatch/core.h +++ b/kernel/livepatch/core.h @@ -8,6 +8,7 @@ extern struct mutex klp_mutex; extern struct list_head klp_patches; void klp_free_patch_start(struct klp_patch *patch); +void klp_discard_replaced_patches(struct klp_patch *new_patch); static inline bool klp_is_object_loaded(struct klp_object *obj) { diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c index 825022d70912..0ff466ab4b5a 100644 --- a/kernel/livepatch/patch.c +++ b/kernel/livepatch/patch.c @@ -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(); } diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index c9917a24b3a4..f4c5908a9731 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -85,6 +85,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); + if (klp_target_state == KLP_UNPATCHED) { /* * All tasks have transitioned to KLP_UNPATCHED so we can now -- cgit v1.2.3-71-gd317