Re: [POC 02/23] livepatch: Split livepatch modules per livepatched object

Julien Thierry
2020-01-21T11:11:57Z

Hi Petr,

On 1/17/20 3:03 PM, Petr Mladek wrote:
> One livepatch module allows to fix vmlinux and any number of modules
> while providing some guarantees defined by the consistency model.
>
> The patched modules can be loaded at any time: before, during,
> or after the livepatch module gets loaded. They can even get
> removed and loaded again. This variety of scenarios bring some
> troubles. For example, some livepatch symbols could be relocated
> only after the related module gets loaded. These changes need
> to get cleared when the module gets unloaded so that it can
> get eventually loaded again.
>
> As a result some functionality needs to be duplicated by
> the livepatching code. Some elf sections need to be preserved
> even when they normally can be removed during the module load.
> Architecture specific code is involved which makes harder
> adding support for new architectures and the maintainace.
>
> The solution is to split the livepatch module per livepatched
> object (vmlinux or module). Then both livepatch module and
> the livepatched modules could get loaded and removed at the
> same time.
>
> This require many changes in the livepatch subsystem, module
> loader, sample livepatches and livepatches needed for selftests.
>
> The bad news is that bisection will not work by definition.
> The good news is that it allows to do the changes in smaller
> steps.
>
> The first step allows to split the existing sample and testing
> modules so that they can be later user. It is done by
> the following changes:
>
> 1. struct klp_patch:
>
> + Add "patch_name" and "obj_names" to match all the related
> livepatch modules.
>
> + Replace "objs" array with a pointer to a single struct object.
>
> + move "mod" to struct object.
>
> 2. struct klp_object:
>
> + Add "patch_name" to match all the related livepatch modules.
>
> + "mod" points to the livepatch module instead of the livepatched
> one. The pointer to the livepatched module was used only to
> detect whether it was loaded. It will be always loaded
> with related livepatch module now.
>
> 3. klp_find_object_module() and klp_is_object_loaded() are no longer
> needed. Livepatch module is loaded only when the related livepatched
> module is loaded.
>
> 4. Add klp_add_object() function that will need to initialize
> struct object, link it into the related struct klp_patch,
> and patch the functions. It will get implemented later.
>
> The livepatches for modules are put into separate source files
> that define only struct klp_object() and call the new klp_add_object()
> in the init() callback. The name of the module follows the pattern:
>
> <patch_name>__<object_name>
>

Is that a requirement? Or is it just the convention followed for the
current tests?

> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> arch/x86/kernel/livepatch.c | 5 +-
> include/linux/livepatch.h | 20 +--
> kernel/livepatch/core.c | 139 +++++++-----------
> kernel/livepatch/core.h | 5 -
> kernel/livepatch/transition.c | 14 +-
> lib/livepatch/Makefile | 2 +
> lib/livepatch/test_klp_atomic_replace.c | 18 ++-
> lib/livepatch/test_klp_callbacks_demo.c | 90 ++++++------
> lib/livepatch/test_klp_callbacks_demo.h | 11 ++
> lib/livepatch/test_klp_callbacks_demo2.c | 62 ++++++---
> lib/livepatch/test_klp_callbacks_demo2.h | 11 ++
> ...t_klp_callbacks_demo__test_klp_callbacks_busy.c | 50 +++++++
> ...st_klp_callbacks_demo__test_klp_callbacks_mod.c | 42 ++++++
> lib/livepatch/test_klp_livepatch.c | 18 ++-
> lib/livepatch/test_klp_state.c | 53 ++++---
> lib/livepatch/test_klp_state2.c | 53 ++++---
> samples/livepatch/Makefile | 4 +
> samples/livepatch/livepatch-callbacks-demo.c | 90 ++++++------
> samples/livepatch/livepatch-callbacks-demo.h | 11 ++
> ...h-callbacks-demo__livepatch-callbacks-busymod.c | 54 +++++++
> ...patch-callbacks-demo__livepatch-callbacks-mod.c | 46 ++++++
> samples/livepatch/livepatch-sample.c | 18 ++-
> samples/livepatch/livepatch-shadow-fix1.c | 120 ++--------------
> .../livepatch-shadow-fix1__livepatch-shadow-mod.c | 155 +++++++++++++++++++++
> samples/livepatch/livepatch-shadow-fix2.c | 92 ++----------
> .../livepatch-shadow-fix2__livepatch-shadow-mod.c | 127 +++++++++++++++++
> .../testing/selftests/livepatch/test-callbacks.sh | 16 +--
> 27 files changed, 841 insertions(+), 485 deletions(-)
> create mode 100644 lib/livepatch/test_klp_callbacks_demo.h
> create mode 100644 lib/livepatch/test_klp_callbacks_demo2.h
> create mode 100644 lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_busy.c
> create mode 100644 lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_mod.c
> create mode 100644 samples/livepatch/livepatch-callbacks-demo.h
> create mode 100644 samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-busymod.c
> create mode 100644 samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-mod.c
> create mode 100644 samples/livepatch/livepatch-shadow-fix1__livepatch-shadow-mod.c
> create mode 100644 samples/livepatch/livepatch-shadow-fix2__livepatch-shadow-mod.c
>
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> index 6a68e41206e7..728b44eaa168 100644
> --- a/arch/x86/kernel/livepatch.c
> +++ b/arch/x86/kernel/livepatch.c
> @@ -9,8 +9,7 @@
> #include <asm/text-patching.h>
>
> /* Apply per-object alternatives. Based on x86 module_finalize() */
> -void arch_klp_init_object_loaded(struct klp_patch *patch,
> - struct klp_object *obj)
> +void arch_klp_init_object_loaded(struct klp_object *obj)
> {
> int cnt;
> struct klp_modinfo *info;
> @@ -20,7 +19,7 @@ void arch_klp_init_object_loaded(struct klp_patch *patch,
> char sec_objname[MODULE_NAME_LEN];
> char secname[KSYM_NAME_LEN];
>
> - info = patch->mod->klp_info;
> + info = obj->mod->klp_info;
> objname = obj->name ? obj->name : "vmlinux";
>
> /* See livepatch core code for BUILD_BUG_ON() explanation */
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index e894e74905f3..a4567c17a9f2 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -105,19 +105,21 @@ struct klp_callbacks {
> /**
> * struct klp_object - kernel object structure for live patching
> * @name: module name (or NULL for vmlinux)
> + * @patch_name: module name for vmlinux
> + * @mod: reference to the live patch module for this object
> * @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)
> * @dynamic: temporary object for nop functions; dynamically allocated
> * @patched: the object's funcs have been added to the klp_ops list
> */
> struct klp_object {
> /* external */
> const char *name;
> + const char *patch_name;
> + struct module *mod;
> struct klp_func *funcs;
> struct klp_callbacks callbacks;
>
> @@ -125,7 +127,6 @@ struct klp_object {
> struct kobject kobj;
> struct list_head func_list;
> struct list_head node;
> - struct module *mod;
> bool dynamic;
> bool patched;
> };
> @@ -144,8 +145,9 @@ struct klp_state {
>
> /**
> * struct klp_patch - patch structure for live patching
> - * @mod: reference to the live patch module
> - * @objs: object entries for kernel objects to be patched
> + * @patch_name: livepatch name; same for related livepatch against other objects

You forgot to add that to the structure.

> + * @objs: object entry for vmlinux object

Nit: s/objs/obj/

> + * @obj_names: names of modules synchronously livepatched with this patch

Not sure I understand the purpose of this. Is it to check that the
klp_object that will get linked to this patch are part of a
pre-established set?

> * @states: system states that can get modified
> * @replace: replace all actively used patches
> * @list: list node for global list of actively used patches
> @@ -158,9 +160,9 @@ struct klp_state {
> */
> struct klp_patch {
> /* external */
> - struct module *mod;
> - struct klp_object *objs;
> struct klp_state *states;
> + struct klp_object *obj;
> + char **obj_names;
> bool replace;
>
> /* internal */
> @@ -194,9 +196,9 @@ struct klp_patch {
> list_for_each_entry(func, &obj->func_list, node)
>
> int klp_enable_patch(struct klp_patch *);
> +int klp_add_object(struct klp_object *);
>
> -void arch_klp_init_object_loaded(struct klp_patch *patch,
> - struct klp_object *obj);
> +void arch_klp_init_object_loaded(struct klp_object *obj);
>
> /* Called from the module loader during module coming/going states */
> int klp_module_coming(struct module *mod);
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index c3512e7e0801..bb62c5407b75 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -49,34 +49,6 @@ static bool klp_is_module(struct klp_object *obj)
> return obj->name;
> }
>
> -/* sets obj->mod if object is not vmlinux and module is found */
> -static void klp_find_object_module(struct klp_object *obj)
> -{
> - struct module *mod;
> -
> - if (!klp_is_module(obj))
> - return;
> -
> - mutex_lock(&module_mutex);
> - /*
> - * We do not want to block removal of patched modules and therefore
> - * we do not take a reference here. The patches are removed by
> - * klp_module_going() instead.
> - */
> - mod = find_module(obj->name);
> - /*
> - * Do not mess work of klp_module_coming() and klp_module_going().
> - * Note that the patch might still be needed before klp_module_going()
> - * is called. Module functions can be called even in the GOING state
> - * until mod->exit() finishes. This is especially important for
> - * patches that modify semantic of the functions.
> - */
> - if (mod && mod->klp_alive)
> - obj->mod = mod;
> -
> - mutex_unlock(&module_mutex);
> -}
> -
> static bool klp_initialized(void)
> {
> return !!klp_root_kobj;
> @@ -246,18 +218,16 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
> return 0;
> }
>
> -static int klp_write_object_relocations(struct module *pmod,
> - struct klp_object *obj)
> +static int klp_write_object_relocations(struct klp_object *obj)
> {
> int i, cnt, ret = 0;
> const char *objname, *secname;
> char sec_objname[MODULE_NAME_LEN];
> + struct module *pmod;
> Elf_Shdr *sec;
>
> - if (WARN_ON(!klp_is_object_loaded(obj)))
> - return -EINVAL;
> -
> objname = klp_is_module(obj) ? obj->name : "vmlinux";
> + pmod = obj->mod;
>
> /* For each klp relocation section */
> for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> @@ -419,8 +389,8 @@ static void klp_free_object_dynamic(struct klp_object *obj)
>
> static void klp_init_func_early(struct klp_object *obj,
> struct klp_func *func);
> -static void klp_init_object_early(struct klp_patch *patch,
> - struct klp_object *obj);
> +static int klp_init_object_early(struct klp_patch *patch,
> + struct klp_object *obj);
>
> static struct klp_object *klp_alloc_object_dynamic(const char *name,
> struct klp_patch *patch)
> @@ -662,7 +632,7 @@ static void klp_free_patch_finish(struct klp_patch *patch)
>
> /* Put the module after the last access to struct klp_patch. */
> if (!patch->forced)
> - module_put(patch->mod);
> + module_put(patch->obj->mod);
> }
>
> /*
> @@ -725,30 +695,28 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> }
>
> /* Arches may override this to finish any remaining arch-specific tasks */
> -void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
> - struct klp_object *obj)
> +void __weak arch_klp_init_object_loaded(struct klp_object *obj)
> {
> }
>
> /* parts of the initialization that is done only when the object is loaded */
> -static int klp_init_object_loaded(struct klp_patch *patch,
> - struct klp_object *obj)
> +static int klp_init_object_loaded(struct klp_object *obj)
> {
> struct klp_func *func;
> int ret;
>
> mutex_lock(&text_mutex);
>
> - module_disable_ro(patch->mod);
> - ret = klp_write_object_relocations(patch->mod, obj);
> + module_disable_ro(obj->mod);
> + ret = klp_write_object_relocations(obj);
> if (ret) {
> - module_enable_ro(patch->mod, true);
> + module_enable_ro(obj->mod, true);
> mutex_unlock(&text_mutex);
> return ret;
> }
>
> - arch_klp_init_object_loaded(patch, obj);
> - module_enable_ro(patch->mod, true);
> + arch_klp_init_object_loaded(obj);
> + module_enable_ro(obj->mod, true);
>
> mutex_unlock(&text_mutex);
>
> @@ -792,11 +760,8 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> return -EINVAL;
>
> obj->patched = false;
> - obj->mod = NULL;
>
> - klp_find_object_module(obj);
> -
> - name = klp_is_module(obj) ? obj->name : "vmlinux";
> + name = obj->name ? obj->name : "vmlinux";
> ret = kobject_add(&obj->kobj, &patch->kobj, "%s", name);
> if (ret)
> return ret;
> @@ -807,8 +772,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> return ret;
> }
>
> - if (klp_is_object_loaded(obj))
> - ret = klp_init_object_loaded(patch, obj);
> + ret = klp_init_object_loaded(obj);
>
> return ret;
> }
> @@ -820,20 +784,34 @@ static void klp_init_func_early(struct klp_object *obj,
> list_add_tail(&func->node, &obj->func_list);
> }
>
> -static void klp_init_object_early(struct klp_patch *patch,
> +static int klp_init_object_early(struct klp_patch *patch,
> struct klp_object *obj)
> {
> + struct klp_func *func;
> +
> + if (!obj->funcs)
> + return -EINVAL;
> +
> INIT_LIST_HEAD(&obj->func_list);
> kobject_init(&obj->kobj, &klp_ktype_object);
> list_add_tail(&obj->node, &patch->obj_list);
> +
> + klp_for_each_func_static(obj, func) {
> + klp_init_func_early(obj, func);
> + }
> +
> + if (obj->dynamic || try_module_get(obj->mod))
> + return 0;
> +
> + return -ENODEV;
> }
>
> static int klp_init_patch_early(struct klp_patch *patch)
> {
> - struct klp_object *obj;
> - struct klp_func *func;
> + struct klp_object *obj = patch->obj;
>
> - if (!patch->objs)
> + /* Main patch module is always for vmlinux */
> + if (obj->name)
> return -EINVAL;
>
> INIT_LIST_HEAD(&patch->list);
> @@ -844,21 +822,7 @@ static int klp_init_patch_early(struct klp_patch *patch)
> INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
> init_completion(&patch->finish);
>
> - klp_for_each_object_static(patch, obj) {

I think we can get rid of klp_for_each_object_static(), no? Now the
klp_patch is only associated to a single klp_object, so everything will
be dynamic. Is this correct?

> - if (!obj->funcs)
> - return -EINVAL;
> -
> - klp_init_object_early(patch, obj);
> -
> - klp_for_each_func_static(obj, func) {
> - klp_init_func_early(obj, func);
> - }
> - }
> -
> - if (!try_module_get(patch->mod))
> - return -ENODEV;
> -
> - return 0;
> + return klp_init_object_early(patch, obj);
> }
>
> static int klp_init_patch(struct klp_patch *patch)
> @@ -866,7 +830,7 @@ static int klp_init_patch(struct klp_patch *patch)
> struct klp_object *obj;
> int ret;
>
> - ret = kobject_add(&patch->kobj, klp_root_kobj, "%s", patch->mod->name);
> + ret = kobject_add(&patch->kobj, klp_root_kobj, "%s", patch->obj->mod->name);
> if (ret)
> return ret;
>
> @@ -887,6 +851,12 @@ static int klp_init_patch(struct klp_patch *patch)
> return 0;
> }
>
> +int klp_add_object(struct klp_object *obj)
> +{
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(klp_add_object);
> +
> static int __klp_disable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
> @@ -930,7 +900,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
> if (WARN_ON(patch->enabled))
> return -EINVAL;
>
> - pr_notice("enabling patch '%s'\n", patch->mod->name);
> + pr_notice("enabling patch '%s'\n", patch->obj->patch_name);
>
> klp_init_transition(patch, KLP_PATCHED);
>
> @@ -944,9 +914,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
> smp_wmb();
>
> klp_for_each_object(patch, obj) {
> - if (!klp_is_object_loaded(obj))
> - continue;
> -
> ret = klp_pre_patch_callback(obj);
> if (ret) {
> pr_warn("pre-patch callback failed for object '%s'\n",
> @@ -968,7 +935,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
>
> return 0;
> err:
> - pr_warn("failed to enable patch '%s'\n", patch->mod->name);
> + pr_warn("failed to enable patch '%s'\n", patch->obj->patch_name);
>
> klp_cancel_transition();
> return ret;
> @@ -991,12 +958,12 @@ int klp_enable_patch(struct klp_patch *patch)
> {
> int ret;
>
> - if (!patch || !patch->mod)
> + if (!patch || !patch->obj || !patch->obj->mod)
> return -EINVAL;
>
> - if (!is_livepatch_module(patch->mod)) {
> + if (!is_livepatch_module(patch->obj->mod)) {
> pr_err("module %s is not marked as a livepatch module\n",
> - patch->mod->name);
> + patch->obj->patch_name);

Shouldn't that be "patch->obj->mod->name" ?

> return -EINVAL;
> }
>
> @@ -1012,7 +979,7 @@ int klp_enable_patch(struct klp_patch *patch)
>
> if (!klp_is_patch_compatible(patch)) {
> pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
> - patch->mod->name);
> + patch->obj->mod->name);
> mutex_unlock(&klp_mutex);
> return -EINVAL;
> }
> @@ -1119,7 +1086,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
> klp_pre_unpatch_callback(obj);
>
> pr_notice("reverting patch '%s' on unloading module '%s'\n",
> - patch->mod->name, obj->mod->name);
> + patch->obj->patch_name, obj->name);
> klp_unpatch_object(obj);
>
> klp_post_unpatch_callback(obj);
> @@ -1154,15 +1121,15 @@ int klp_module_coming(struct module *mod)
>
> obj->mod = mod;
>
> - ret = klp_init_object_loaded(patch, obj);
> + ret = klp_init_object_loaded(obj);
> if (ret) {
> pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> - patch->mod->name, obj->mod->name, ret);
> + patch->obj->patch_name, obj->name, ret);
> goto err;
> }
>
> pr_notice("applying patch '%s' to loading module '%s'\n",
> - patch->mod->name, obj->mod->name);
> + patch->obj->patch_name, obj->name);
>
> ret = klp_pre_patch_callback(obj);
> if (ret) {
> @@ -1174,7 +1141,7 @@ int klp_module_coming(struct module *mod)
> ret = klp_patch_object(obj);
> if (ret) {
> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> - patch->mod->name, obj->mod->name, ret);
> + patch->obj->patch_name, obj->name, ret);
>
> klp_post_unpatch_callback(obj);
> goto err;
> @@ -1197,7 +1164,7 @@ int klp_module_coming(struct module *mod)
> * error to the module loader.
> */
> pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> - patch->mod->name, obj->mod->name, obj->mod->name);
> + patch->obj->patch_name, obj->name, obj->name);
> mod->klp_alive = false;
> obj->mod = NULL;
> klp_cleanup_module_patches_limited(mod, patch);
> diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
> index 38209c7361b6..01980cc0509b 100644
> --- a/kernel/livepatch/core.h
> +++ b/kernel/livepatch/core.h
> @@ -18,11 +18,6 @@ void klp_free_replaced_patches_async(struct klp_patch *new_patch);
> void klp_unpatch_replaced_patches(struct klp_patch *new_patch);
> void klp_discard_nops(struct klp_patch *new_patch);
>
> -static inline bool klp_is_object_loaded(struct klp_object *obj)
> -{
> - return !obj->name || obj->mod;
> -}
> -
> static inline int klp_pre_patch_callback(struct klp_object *obj)
> {
> int ret = 0;
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index f6310f848f34..78e3280560cd 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -74,7 +74,7 @@ static void klp_complete_transition(void)
> unsigned int cpu;
>
> pr_debug("'%s': completing %s transition\n",
> - klp_transition_patch->mod->name,
> + klp_transition_patch->obj->patch_name,
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
> @@ -120,15 +120,13 @@ static void klp_complete_transition(void)
> }
>
> klp_for_each_object(klp_transition_patch, obj) {
> - if (!klp_is_object_loaded(obj))
> - continue;
> if (klp_target_state == KLP_PATCHED)
> klp_post_patch_callback(obj);
> else if (klp_target_state == KLP_UNPATCHED)
> klp_post_unpatch_callback(obj);
> }
>
> - pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
> + pr_notice("'%s': %s complete\n", klp_transition_patch->obj->patch_name,
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> klp_target_state = KLP_UNDEFINED;
> @@ -147,7 +145,7 @@ void klp_cancel_transition(void)
> return;
>
> pr_debug("'%s': canceling patching transition, going to unpatch\n",
> - klp_transition_patch->mod->name);
> + klp_transition_patch->obj->patch_name);
>
> klp_target_state = KLP_UNPATCHED;
> klp_complete_transition();
> @@ -468,7 +466,7 @@ void klp_start_transition(void)
> WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
>
> pr_notice("'%s': starting %s transition\n",
> - klp_transition_patch->mod->name,
> + klp_transition_patch->obj->patch_name,

Isn't the transition per livepatched module rather than per-patch now?
If so, would it make more sense to display also the name of the module
being patched/unpatched?

> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> /*
> @@ -519,7 +517,7 @@ void klp_init_transition(struct klp_patch *patch, int state)
> */
> klp_target_state = state;
>
> - pr_debug("'%s': initializing %s transition\n", patch->mod->name,
> + pr_debug("'%s': initializing %s transition\n", patch->obj->patch_name,

Ditto.

> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> /*
> @@ -581,7 +579,7 @@ void klp_reverse_transition(void)
> struct task_struct *g, *task;
>
> pr_debug("'%s': reversing transition from %s\n",
> - klp_transition_patch->mod->name,
> + klp_transition_patch->obj->patch_name,

Ditto.

> klp_target_state == KLP_PATCHED ? "patching to unpatching" :
> "unpatching to patching");
>

[...]

Cheers,

--
Julien Thierry