LinuxLists.cc - PATCH v6 0/6] livepatch: Atomic replace feature

2018-01-25 16:04:12

Subject: PATCH v6 0/6] livepatch: Atomic replace feature

Hi,

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 is my rework based on Jason's v5 patchset[1]. My intention was:

+ reduce code duplication and nop-specific shortcuts
+ split the huge patch for an easier review
+ simplify the logic where possible
+ add/update/clear comments
+ better fit into the existing code

I am not supper happy with the result. But it took me much longer
time than expected. It is high time, I shared the current state
and get some feedback.

Jason,

I used your code in most of the patches and kept you as the author.
Please, let me know if you would prefer another solution.

Also I am sorry that I have done it this way. I was not able to
suggest it out of head. I needed many iterations to end up with
the current state. I needed to play with the code. Therefore
it made sense to send what I got.

[1] https://lkml.kernel.org/r/[email protected]

Jason Baron (5):
livepatch: Use lists to manage patches, objects and functions
livepatch: Initial support for dynamic structures
livepatch: Allow to unpatch only functions of the given type
livepatch: Support separate list for replaced patches.
livepatch: Add atomic replace

Petr Mladek (1):
livepatch: Free only structures with initialized kobject

include/linux/livepatch.h | 59 ++++++-
kernel/livepatch/core.c | 378 ++++++++++++++++++++++++++++++++++++++----
kernel/livepatch/core.h | 4 +
kernel/livepatch/patch.c | 29 +++-
kernel/livepatch/patch.h | 4 +-
kernel/livepatch/transition.c | 38 ++++-
6 files changed, 464 insertions(+), 48 deletions(-)

--
2.13.6

2018-01-25 16:03:38

by Petr Mladek

[permalink] [raw]

Subject: PATCH v6 6/6] livepatch: Add atomic replace

From: 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 not longer be
modified by the new patch. They are temporarily used as a new target
during the patch transition.

There are used several simplifications:

+ nops' structures are generated already when the patch is registered.
All registered patches are taken into account, even the disabled ones.
As a result, there might be more nops than are really needed when
the patch is enabled and some disabled patches were removed before.
But we are on the safe side and it simplifies the implementation.
Especially we could reuse the existing init() functions. Also freeing
is easier because the same nops are created and removed only once.

Alternative solution would be to create nops when the patch is enabled.
But then we would need to complicated to reuse the init() functions
and error paths. It would increase the risk of errors because of
late kobject initialization. It would need tricky waiting for
freed kobjects when finalizing a reverted enable transaction.

+ The replaced patches are removed from the stack and cannot longer
be enabled directly. Otherwise, we would need to implement a more
complex logic of handling the stack of patches. It might be hard
to come with a reasonable semantic.

A fallback is to remove (rmmod) the replaced patches and register
(insmod) them again.

+ Nops are handled like normal function patches. It reduces changes
in the existing code.

It would be possible to copy internal values when they are allocated
and make short cuts in init() functions. It would be possible to use
the fact that old_func and new_func point to the same function and
do not init new_func and new_size at all. It would be possible to
detect nop func in ftrace handler and just leave. But all these would
just complicate the code and maintenance.

+ The callbacks from the replaced patches are not called. 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.
No to say that the old scripts even would not expect to be called
in this situation.

Signed-off-by: Jason Baron <[email protected]>
[[email protected]: Split, reuse existing code, simplified]
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: Petr Mladek <[email protected]>
---
include/linux/livepatch.h | 3 +
kernel/livepatch/core.c | 162 +++++++++++++++++++++++++++++++++++++++++-
kernel/livepatch/core.h | 4 ++
kernel/livepatch/transition.c | 36 ++++++++++
4 files changed, 203 insertions(+), 2 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 21cad200f949..9d44d105b278 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -42,6 +42,7 @@
enum klp_func_type {
KLP_FUNC_ANY = -1, /* Substitute any type */
KLP_FUNC_ORIGINAL = 0, /* Original statically defined structure */
+ KLP_FUNC_NOP, /* Dynamically allocated NOP function patch */
};

/**
@@ -153,6 +154,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: head of list for struct klp_object
@@ -163,6 +165,7 @@ struct klp_patch {
/* external */
struct module *mod;
struct klp_object *objs;
+ bool replace;

/* internal */
struct list_head list;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6ad3195d995a..c606b291dfcd 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -142,6 +142,21 @@ 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)
{
@@ -342,6 +357,39 @@ 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 situation
+ * when these structures are not longer accessible. All functions
+ * are redirected using the klp_transition_patch. They use either
+ * a new code or they in the original code because of the special
+ * nop function patches.
+ */
+void klp_throw_away_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, KLP_FUNC_ANY);
+ old_patch->enabled = false;
+
+ /*
+ * Replaced patches could not get re-enabled to keep
+ * the code sane.
+ */
+ list_del_init(&old_patch->list);
+
+ if (!keep_module)
+ module_put(old_patch->mod);
+ }
+}
+
static int __klp_disable_patch(struct klp_patch *patch)
{
struct klp_object *obj;
@@ -537,7 +585,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
if (!klp_is_patch_usable(patch)) {
/*
* Module with the patch could either disappear meanwhile or is
- * not properly initialized yet.
+ * not properly initialized yet or the patch was just replaced.
*/
ret = -EINVAL;
goto err;
@@ -662,8 +710,16 @@ static struct attribute *klp_patch_attrs[] = {
/*
* Dynamically allocated objects and functions.
*/
+static void klp_free_func_nop(struct klp_func *func)
+{
+ kfree(func->old_name);
+ kfree(func);
+}
+
static void klp_free_func_dynamic(struct klp_func *func)
{
+ if (func->ftype == KLP_FUNC_NOP)
+ klp_free_func_nop(func);
}

static void klp_free_object_dynamic(struct klp_object *obj)
@@ -691,7 +747,7 @@ static struct klp_object *klp_alloc_object_dynamic(const char *name)
return obj;
}

-struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
+static struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
struct klp_object *old_obj)
{
struct klp_object *obj;
@@ -708,6 +764,102 @@ struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
return obj;
}

+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 ERR_PTR(-ENOMEM);
+
+ if (old_func->old_name) {
+ func->old_name = kstrdup(old_func->old_name, GFP_KERNEL);
+ if (!func->old_name) {
+ kfree(func);
+ return ERR_PTR(-ENOMEM);
+ }
+ }
+ func->old_sympos = old_func->old_sympos;
+ /* NOP func is the same as using the original implementation. */
+ func->new_func = (void *)old_func->old_addr;
+ func->ftype = KLP_FUNC_NOP;
+
+ return func;
+}
+
+static int klp_add_func_nop(struct klp_object *obj,
+ struct klp_func *old_func)
+{
+ struct klp_func *func;
+
+ func = klp_find_func(obj, old_func);
+
+ if (func)
+ return 0;
+
+ func = klp_alloc_func_nop(old_func, obj);
+ if (IS_ERR(func))
+ return PTR_ERR(func);
+
+ klp_init_func_list(obj, func);
+
+ return 0;
+}
+
+static int klp_add_object_nops(struct klp_patch *patch,
+ struct klp_object *old_obj)
+{
+ struct klp_object *obj;
+ struct klp_func *old_func;
+ int err = 0;
+
+ obj = klp_get_or_add_object(patch, old_obj);
+ if (IS_ERR(obj))
+ return PTR_ERR(obj);
+
+ klp_for_each_func(old_obj, old_func) {
+ err = klp_add_func_nop(obj, old_func);
+ if (err)
+ return err;
+ }
+
+ 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
+ *
+ * The nops are generated for all patches on the stack when
+ * the new patch is initialized. It is safe even though some
+ * older patches might get disabled and removed before the
+ * new one is enabled. In the worst case, there might be nops
+ * there will not be really needed. But it does not harm and
+ * simplifies the implementation a lot. Especially we could
+ * use the init functions as is.
+ */
+static int klp_add_nops(struct klp_patch *patch)
+{
+ struct klp_patch *old_patch;
+ struct klp_object *old_obj;
+ int err = 0;
+
+ if (WARN_ON(!patch->replace))
+ return -EINVAL;
+
+ list_for_each_entry(old_patch, &klp_patches, list) {
+ klp_for_each_object(old_patch, old_obj) {
+ err = klp_add_object_nops(patch, old_obj);
+ if (err)
+ return err;
+ }
+ }
+
+ return 0;
+}
+
/*
* Patch release framework must support the following scenarios:
*
@@ -952,6 +1104,12 @@ static int klp_init_patch(struct klp_patch *patch)
return ret;
}

+ if (patch->replace) {
+ ret = klp_add_nops(patch);
+ if (ret)
+ goto free;
+ }
+
klp_for_each_object(patch, obj) {
ret = klp_init_object(patch, obj);
if (ret)
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 48a83d4364cf..43184a5318d8 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -6,6 +6,10 @@

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);
+
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 6917100fbe79..3f6cf5b9e048 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -87,6 +87,33 @@ static void klp_complete_transition(void)
klp_transition_patch->mod->name,
klp_target_state == KLP_PATCHED ? "patching" : "unpatching");

+ /*
+ * For replace patches, we disable all previous patches, and replace
+ * the dynamic no-op functions by removing the ftrace hook.
+ */
+ if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
+ /*
+ * Make sure klp_ftrace_handler() can no longer see functions
+ * from the replaced patches. Then all functions will be
+ * redirected using klp_transition_patch. They will use
+ * either a new code or they will stay in the original code
+ * because of the nop funcs' structures.
+ */
+ if (klp_forced)
+ klp_synchronize_transition();
+
+ klp_throw_away_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
+ * gurantees that nobody will stay in the trampoline after
+ * the ftrace handler is unregistered.
+ */
+ klp_unpatch_objects(klp_transition_patch, KLP_FUNC_NOP);
+ }
+
if (klp_target_state == KLP_UNPATCHED) {
/*
* All tasks have transitioned to KLP_UNPATCHED so we can now
@@ -143,6 +170,15 @@ static void klp_complete_transition(void)
if (!klp_forced && klp_target_state == KLP_UNPATCHED)
module_put(klp_transition_patch->mod);

+ /*
+ * We do not need to wait until the objects are really freed.
+ * The patch must be on the bottom of the stack. Therefore it
+ * will never replace anything else. The only important thing
+ * is that we wait when the patch is being unregistered.
+ */
+ if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED)
+ klp_free_objects(klp_transition_patch, KLP_FUNC_NOP);
+
klp_target_state = KLP_UNDEFINED;
klp_transition_patch = NULL;
}
--
2.13.6

2018-01-25 16:03:45

by Petr Mladek

[permalink] [raw]

Subject: PATCH v6 4/6] livepatch: Allow to unpatch only functions of the given type

2018-01-25 16:04:11

by Petr Mladek

[permalink] [raw]

Subject: PATCH v6 3/6] livepatch: Initial support for dynamic structures

From: Jason Baron <[email protected]>

We 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 not longer patched.

This patch adds basic framework to handle such dynamic structures.

It adds enum klp_func_type that allows to distinguish the dynamically
allocated funcs' structures. Note that objects' structures do not have
a clear type. Namely the static objects' structures might list both static
and dynamic funcs' structures.

The function type is then used to limit klp_free() functions. We will
want to free the dynamic structures separately when they are not
longer needed. At the same time, we also want to make our life easier,
and introduce _any_ type that will allow to process all existing
structures in one go.

We need to be careful here. First, objects' structures must be freed
only when all listed funcs' structures are freed. Also we must avoid
double free. Both problems are solved by removing the freed structures
from the list.

Also note that klp_free*() functions are called also in klp_init_patch()
error path when only some kobjects have been initialized. The other
dynamic structures must be freed immediately by calling the respective
klp_free_*_dynamic() functions.

Finally, the dynamic objects' structures are generic. The respective
klp_allocate_object_dynamic() and klp_free_object_dynamic() can
be implemented here. On the other hand, klp_free_func_dynamic()
is empty. It must be updated when a particular dynamic
klp_func_type is introduced.

Signed-off-by: Jason Baron <[email protected]>
[[email protected]: Converted into a generic API]
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]>
---
include/linux/livepatch.h | 37 +++++++++++-
kernel/livepatch/core.c | 139 ++++++++++++++++++++++++++++++++++++++++------
2 files changed, 159 insertions(+), 17 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e5db2ba7e2a5..21cad200f949 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -35,12 +35,22 @@
#define KLP_UNPATCHED 0
#define KLP_PATCHED 1

+/*
+ * Function type is used to distinguish dynamically allocated structures
+ * and limit some operations.
+ */
+enum klp_func_type {
+ KLP_FUNC_ANY = -1, /* Substitute any type */
+ KLP_FUNC_ORIGINAL = 0, /* Original statically defined structure */
+};
+
/**
* struct klp_func - function structure for live patching
* @old_name: name of the function to be patched
* @new_func: pointer to the patched function code
* @old_sympos: a hint indicating which symbol position the old function
* can be found (optional)
+ * @ftype: distinguish static and dynamic structures
* @old_addr: the address of the function being patched
* @kobj: kobject for sysfs resources
* @stack_node: list node for klp_ops func_stack list
@@ -79,6 +89,7 @@ struct klp_func {
unsigned long old_sympos;

/* internal */
+ enum klp_func_type ftype;
unsigned long old_addr;
struct kobject kobj;
struct list_head stack_node;
@@ -164,17 +175,41 @@ 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, obj_entry)
+
#define klp_for_each_object(patch, obj) \
list_for_each_entry(obj, &patch->obj_list, obj_entry)

+/* Support also dynamically allocated struct klp_object */
#define klp_for_each_func_static(obj, func) \
for (func = obj->funcs; \
- func->old_name || func->new_func || func->old_sympos; \
+ func && (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, func_entry)
+
#define klp_for_each_func(obj, func) \
list_for_each_entry(func, &obj->func_list, func_entry)

+static inline bool klp_is_object_dynamic(struct klp_object *obj)
+{
+ return !obj->funcs;
+}
+
+static inline bool klp_is_func_dynamic(struct klp_func *func)
+{
+ WARN_ON_ONCE(func->ftype == KLP_FUNC_ANY);
+ return func->ftype != KLP_FUNC_ORIGINAL;
+}
+
+static inline bool klp_is_func_type(struct klp_func *func,
+ enum klp_func_type ftype)
+{
+ return ftype == KLP_FUNC_ANY || ftype == func->ftype;
+}
+
int klp_register_patch(struct klp_patch *);
int klp_unregister_patch(struct klp_patch *);
int klp_enable_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 05d60035525d..6c06311f9ce3 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -124,6 +124,26 @@ static bool klp_initialized(void)
return !!klp_root_kobj;
}

+static struct klp_object *klp_find_object(struct klp_patch *patch,
+ struct klp_object *old_obj)
+{
+ struct klp_object *obj;
+ bool mod = klp_is_module(old_obj);
+
+ klp_for_each_object(patch, obj) {
+ if (mod) {
+ 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;
@@ -621,6 +641,66 @@ static struct attribute *klp_patch_attrs[] = {
NULL
};

+/*
+ * Dynamically allocated objects and functions.
+ */
+static void klp_free_func_dynamic(struct klp_func *func)
+{
+}
+
+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 ERR_PTR(-ENOMEM);
+
+ if (name) {
+ obj->name = kstrdup(name, GFP_KERNEL);
+ if (!obj->name) {
+ kfree(obj);
+ return ERR_PTR(-ENOMEM);
+ }
+ }
+
+ return obj;
+}
+
+struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
+ struct klp_object *old_obj)
+{
+ struct klp_object *obj;
+
+ obj = klp_find_object(patch, old_obj);
+ if (obj)
+ return obj;
+
+ obj = klp_alloc_object_dynamic(old_obj->name);
+ if (IS_ERR(obj))
+ return obj;
+
+ klp_init_object_list(patch, obj);
+ return obj;
+}
+
+/*
+ * Patch release framework must support the following scenarios:
+ *
+ * + Asynchonous release is used when kobjects are initialized.
+ *
+ * + Direct release is used in error paths for structures that
+ * have not had kobj initialized yet.
+ *
+ * + Allow to release dynamic structures of the given type when
+ * they are not longer needed.
+ */
static void klp_kobj_release_patch(struct kobject *kobj)
{
struct klp_patch *patch;
@@ -637,6 +717,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 (klp_is_object_dynamic(obj))
+ klp_free_object_dynamic(obj);
}

static struct kobj_type klp_ktype_object = {
@@ -646,6 +732,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 (klp_is_func_dynamic(func))
+ klp_free_func_dynamic(func);
}

static struct kobj_type klp_ktype_func = {
@@ -654,16 +746,25 @@ static struct kobj_type klp_ktype_func = {
};

/*
- * Free all funcs' that have the kobject initialized. Otherwise,
- * nothing is needed.
+ * Free all funcs' of the given ftype. Use the kobject when it has already
+ * been initialized. Otherwise, do it directly.
*/
-static void klp_free_funcs(struct klp_object *obj)
+static void klp_free_funcs(struct klp_object *obj,
+ enum klp_func_type ftype)
{
- struct klp_func *func;
+ struct klp_func *func, *tmp_func;
+
+ klp_for_each_func_safe(obj, func, tmp_func) {
+ if (!klp_is_func_type(func, ftype))
+ continue;
+
+ /* Avoid double free and allow to detect empty objects. */
+ list_del(&func->func_entry);

- klp_for_each_func(obj, func) {
if (func->kobj.state_initialized)
kobject_put(&func->kobj);
+ else if (klp_is_func_dynamic(func))
+ klp_free_func_dynamic(func);
}
}

@@ -679,24 +780,33 @@ static void klp_free_object_loaded(struct klp_object *obj)
}

/*
- * Free all funcs' and objects's that have the kobject initialized.
- * Otherwise, nothing is needed.
+ * Free all linked funcs' of the given ftype. Then free empty objects.
+ * Use the kobject when it has already been initialized. Otherwise,
+ * do it directly.
*/
-static void klp_free_objects(struct klp_patch *patch)
+void klp_free_objects(struct klp_patch *patch, enum klp_func_type ftype)
{
- struct klp_object *obj;
+ struct klp_object *obj, *tmp_obj;

- klp_for_each_object(patch, obj) {
- klp_free_funcs(obj);
+ klp_for_each_object_safe(patch, obj, tmp_obj) {
+ klp_free_funcs(obj, ftype);
+
+ if (!list_empty(&obj->func_list))
+ continue;
+
+ /* Avoid freeing the object twice. */
+ list_del(&obj->obj_entry);

if (obj->kobj.state_initialized)
kobject_put(&obj->kobj);
+ else if (klp_is_object_dynamic(obj))
+ klp_free_object_dynamic(obj);
}
}

static void klp_free_patch(struct klp_patch *patch)
{
- klp_free_objects(patch);
+ klp_free_objects(patch, KLP_FUNC_ANY);

if (!list_empty(&patch->list))
list_del(&patch->list);
@@ -777,9 +887,6 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
int ret;
const char *name;

- if (!obj->funcs)
- return -EINVAL;
-
obj->patched = false;
obj->mod = NULL;

@@ -840,7 +947,7 @@ static int klp_init_patch(struct klp_patch *patch)
return 0;

free:
- klp_free_objects(patch);
+ klp_free_objects(patch, KLP_FUNC_ANY);

mutex_unlock(&klp_mutex);

--
2.13.6

2018-01-25 16:04:27

by Petr Mladek

[permalink] [raw]

Subject: PATCH v6 2/6] livepatch: Free only structures with initialized kobject

We 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 not 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.

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]>
---
kernel/livepatch/core.c | 42 ++++++++++++++++++++----------------------
1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 1d525f4a270a..05d60035525d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -654,16 +654,17 @@ static struct kobj_type klp_ktype_func = {
};

/*
- * Free all functions' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
+ * Free all funcs' that have the kobject initialized. Otherwise,
+ * nothing is needed.
*/
-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) {
+ if (func->kobj.state_initialized)
+ kobject_put(&func->kobj);
+ }
}

/* Clean up when a patched object is unloaded */
@@ -678,23 +679,25 @@ static void klp_free_object_loaded(struct klp_object *obj)
}

/*
- * Free all objects' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
+ * Free all funcs' and objects's that have the kobject initialized.
+ * Otherwise, nothing is needed.
*/
-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);
+
+ 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);
}
@@ -791,21 +794,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)
@@ -842,7 +840,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);

--
2.13.6

2018-01-25 16:04:48

by Petr Mladek

[permalink] [raw]

Subject: PATCH v6 1/6] 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.

Also klp_init_*_list() functions are split because they will
be used when adding the dynamically allocated structures.

This patch does not change the existing behavior.

Signed-off-by: Jason Baron <[email protected]>
[[email protected]: Initialize lists before init calls]
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]>
---
include/linux/livepatch.h | 19 +++++++++++++++++--
kernel/livepatch/core.c | 27 +++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 4754f01c1abb..e5db2ba7e2a5 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -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)

@@ -43,6 +44,7 @@
* @old_addr: the address of the function being patched
* @kobj: kobject for sysfs resources
* @stack_node: list node for klp_ops func_stack list
+ * @func_entry: links struct klp_func to struct klp_object
* @old_size: size of the old function
* @new_size: size of the new function
* @patched: the func has been added to the klp_ops list
@@ -80,6 +82,7 @@ struct klp_func {
unsigned long old_addr;
struct kobject kobj;
struct list_head stack_node;
+ struct list_head func_entry;
unsigned long old_size, new_size;
bool patched;
bool transition;
@@ -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: head of list for struct klp_func
+ * @obj_entry: links struct klp_object to struct klp_patch
* @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 obj_entry;
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: head of list for struct klp_object
* @enabled: the patch is enabled (but operation may be incomplete)
* @finish: for waiting till it is safe to remove the patch module
*/
@@ -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, obj_entry)
+
+#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, func_entry)
+
int klp_register_patch(struct klp_patch *);
int klp_unregister_patch(struct klp_patch *);
int klp_enable_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 3a4656fb7047..1d525f4a270a 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -49,6 +49,32 @@ static LIST_HEAD(klp_patches);

static struct kobject *klp_root_kobj;

+static void klp_init_func_list(struct klp_object *obj, struct klp_func *func)
+{
+ list_add(&func->func_entry, &obj->func_list);
+}
+
+static void klp_init_object_list(struct klp_patch *patch,
+ struct klp_object *obj)
+{
+ struct klp_func *func;
+
+ list_add(&obj->obj_entry, &patch->obj_list);
+
+ INIT_LIST_HEAD(&obj->func_list);
+ klp_for_each_func_static(obj, func)
+ klp_init_func_list(obj, func);
+}
+
+static void klp_init_patch_list(struct klp_patch *patch)
+{
+ struct klp_object *obj;
+
+ INIT_LIST_HEAD(&patch->obj_list);
+ klp_for_each_object_static(patch, obj)
+ klp_init_object_list(patch, obj);
+}
+
static bool klp_is_module(struct klp_object *obj)
{
return obj->name;
@@ -794,6 +820,7 @@ static int klp_init_patch(struct klp_patch *patch)

patch->enabled = false;
init_completion(&patch->finish);
+ klp_init_patch_list(patch);

ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
klp_root_kobj, "%s", patch->mod->name);
--
2.13.6

2018-01-25 16:04:58

by Petr Mladek

[permalink] [raw]

Subject: PATCH v6 5/6] livepatch: Support separate list for replaced patches.

From: Jason Baron <[email protected]>

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 replaced patches will stay registered because they are typically
unregistered by some package uninstall scripts. But we will remove
these patches from @klp_patches list to keep the enabled patch
on the bottom of the stack. Otherwise, we would need to implement
rather complex logic for moving the patches on the stack. Also
it would complicate implementation of the atomic replace feature.
It is not worth it.

As a result, we will have patches that are registered but that
are not longer usable. Let's get prepared for this and use
a better descriptive name for klp_is_patch_registered() function.

Also create separate list for the replaced patches and allow to
unregister them. Alternative solution would be to add a flag
into struct klp_patch. Note that patch->kobj.state_initialized
is not safe because it can be cleared outside klp_mutex.

This patch does not change the existing behavior.

Signed-off-by: Jason Baron <[email protected]>
[[email protected]: Split and renamed klp_is_patch_usable()]
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]>
---
kernel/livepatch/core.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 407bbc714ced..6ad3195d995a 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -47,6 +47,13 @@ DEFINE_MUTEX(klp_mutex);

static LIST_HEAD(klp_patches);

+/*
+ * List of 'replaced' patches that have been replaced by a patch that has the
+ * 'replace' bit set. When they are added to this list, they are disabled and
+ * can not be re-enabled, but they can be unregistered().
+ */
+LIST_HEAD(klp_replaced_patches);
+
static struct kobject *klp_root_kobj;

static void klp_init_func_list(struct klp_object *obj, struct klp_func *func)
@@ -108,17 +115,28 @@ 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_in_list(struct klp_patch *patch,
+ struct list_head *head)
{
struct klp_patch *mypatch;

- list_for_each_entry(mypatch, &klp_patches, list)
+ list_for_each_entry(mypatch, head, list)
if (mypatch == patch)
return true;

return false;
}

+static bool klp_is_patch_usable(struct klp_patch *patch)
+{
+ return klp_is_patch_in_list(patch, &klp_patches);
+}
+
+static bool klp_is_patch_replaced(struct klp_patch *patch)
+{
+ return klp_is_patch_in_list(patch, &klp_replaced_patches);
+}
+
static bool klp_initialized(void)
{
return !!klp_root_kobj;
@@ -375,7 +393,7 @@ int klp_disable_patch(struct klp_patch *patch)

mutex_lock(&klp_mutex);

- if (!klp_is_patch_registered(patch)) {
+ if (!klp_is_patch_usable(patch)) {
ret = -EINVAL;
goto err;
}
@@ -475,7 +493,7 @@ int klp_enable_patch(struct klp_patch *patch)

mutex_lock(&klp_mutex);

- if (!klp_is_patch_registered(patch)) {
+ if (!klp_is_patch_usable(patch)) {
ret = -EINVAL;
goto err;
}
@@ -516,7 +534,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,

mutex_lock(&klp_mutex);

- if (!klp_is_patch_registered(patch)) {
+ if (!klp_is_patch_usable(patch)) {
/*
* Module with the patch could either disappear meanwhile or is
* not properly initialized yet.
@@ -971,7 +989,7 @@ int klp_unregister_patch(struct klp_patch *patch)

mutex_lock(&klp_mutex);

- if (!klp_is_patch_registered(patch)) {
+ if (!klp_is_patch_usable(patch) && !klp_is_patch_replaced(patch)) {
ret = -EINVAL;
goto err;
}
--
2.13.6

2018-01-26 04:28:52

by Jason Baron

[permalink] [raw]

Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace

On 01/25/2018 11:02 AM, Petr Mladek wrote:
> From: 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 not longer be
> modified by the new patch. They are temporarily used as a new target
> during the patch transition.
>
> There are used several simplifications:
>
> + nops' structures are generated already when the patch is registered.
> All registered patches are taken into account, even the disabled ones.
> As a result, there might be more nops than are really needed when
> the patch is enabled and some disabled patches were removed before.
> But we are on the safe side and it simplifies the implementation.
> Especially we could reuse the existing init() functions. Also freeing
> is easier because the same nops are created and removed only once.
>
> Alternative solution would be to create nops when the patch is enabled.
> But then we would need to complicated to reuse the init() functions
> and error paths. It would increase the risk of errors because of
> late kobject initialization. It would need tricky waiting for
> freed kobjects when finalizing a reverted enable transaction.
>
> + The replaced patches are removed from the stack and cannot longer
> be enabled directly. Otherwise, we would need to implement a more
> complex logic of handling the stack of patches. It might be hard
> to come with a reasonable semantic.
>
> A fallback is to remove (rmmod) the replaced patches and register
> (insmod) them again.
>
> + Nops are handled like normal function patches. It reduces changes
> in the existing code.
>
> It would be possible to copy internal values when they are allocated
> and make short cuts in init() functions. It would be possible to use
> the fact that old_func and new_func point to the same function and
> do not init new_func and new_size at all. It would be possible to
> detect nop func in ftrace handler and just leave. But all these would
> just complicate the code and maintenance.
>
> + The callbacks from the replaced patches are not called. 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.
> No to say that the old scripts even would not expect to be called
> in this situation.
>
> Signed-off-by: Jason Baron <[email protected]>
> [[email protected]: Split, reuse existing code, simplified]

Hi Petr,

Thanks for cleaning this up - it looks good.
I just had one comment/issue below thus far.

> 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: Petr Mladek <[email protected]>
> ---
> include/linux/livepatch.h | 3 +
> kernel/livepatch/core.c | 162 +++++++++++++++++++++++++++++++++++++++++-
> kernel/livepatch/core.h | 4 ++
> kernel/livepatch/transition.c | 36 ++++++++++
> 4 files changed, 203 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 21cad200f949..9d44d105b278 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -42,6 +42,7 @@
> enum klp_func_type {
> KLP_FUNC_ANY = -1, /* Substitute any type */
> KLP_FUNC_ORIGINAL = 0, /* Original statically defined structure */
> + KLP_FUNC_NOP, /* Dynamically allocated NOP function patch */
> };
>
> /**
> @@ -153,6 +154,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: head of list for struct klp_object
> @@ -163,6 +165,7 @@ struct klp_patch {
> /* external */
> struct module *mod;
> struct klp_object *objs;
> + bool replace;
>
> /* internal */
> struct list_head list;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6ad3195d995a..c606b291dfcd 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -142,6 +142,21 @@ 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)
> {
> @@ -342,6 +357,39 @@ 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 situation
> + * when these structures are not longer accessible. All functions
> + * are redirected using the klp_transition_patch. They use either
> + * a new code or they in the original code because of the special
> + * nop function patches.
> + */
> +void klp_throw_away_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, KLP_FUNC_ANY);
> + old_patch->enabled = false;
> +
> + /*
> + * Replaced patches could not get re-enabled to keep
> + * the code sane.
> + */
> + list_del_init(&old_patch->list);

I'm wondering if this should be:

list_move(&old_patch->list, &klp_replaced_patches);

Which ensures that the only valid transition after a patch has been
'replaced' is an 'unregister'.

Otherwise, klp_replaced_patches is not used anywhere.

Thanks,

-Jason

> +
> + if (!keep_module)
> + module_put(old_patch->mod);
> + }
> +}
> +
> static int __klp_disable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
> @@ -537,7 +585,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> if (!klp_is_patch_usable(patch)) {
> /*
> * Module with the patch could either disappear meanwhile or is
> - * not properly initialized yet.
> + * not properly initialized yet or the patch was just replaced.
> */
> ret = -EINVAL;
> goto err;
> @@ -662,8 +710,16 @@ static struct attribute *klp_patch_attrs[] = {
> /*
> * Dynamically allocated objects and functions.
> */
> +static void klp_free_func_nop(struct klp_func *func)
> +{
> + kfree(func->old_name);
> + kfree(func);
> +}
> +
> static void klp_free_func_dynamic(struct klp_func *func)
> {
> + if (func->ftype == KLP_FUNC_NOP)
> + klp_free_func_nop(func);
> }
>
> static void klp_free_object_dynamic(struct klp_object *obj)
> @@ -691,7 +747,7 @@ static struct klp_object *klp_alloc_object_dynamic(const char *name)
> return obj;
> }
>
> -struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> +static struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> struct klp_object *old_obj)
> {
> struct klp_object *obj;
> @@ -708,6 +764,102 @@ struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> return obj;
> }
>
> +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 ERR_PTR(-ENOMEM);
> +
> + if (old_func->old_name) {
> + func->old_name = kstrdup(old_func->old_name, GFP_KERNEL);
> + if (!func->old_name) {
> + kfree(func);
> + return ERR_PTR(-ENOMEM);
> + }
> + }
> + func->old_sympos = old_func->old_sympos;
> + /* NOP func is the same as using the original implementation. */
> + func->new_func = (void *)old_func->old_addr;
> + func->ftype = KLP_FUNC_NOP;
> +
> + return func;
> +}
> +
> +static int klp_add_func_nop(struct klp_object *obj,
> + struct klp_func *old_func)
> +{
> + struct klp_func *func;
> +
> + func = klp_find_func(obj, old_func);
> +
> + if (func)
> + return 0;
> +
> + func = klp_alloc_func_nop(old_func, obj);
> + if (IS_ERR(func))
> + return PTR_ERR(func);
> +
> + klp_init_func_list(obj, func);
> +
> + return 0;
> +}
> +
> +static int klp_add_object_nops(struct klp_patch *patch,
> + struct klp_object *old_obj)
> +{
> + struct klp_object *obj;
> + struct klp_func *old_func;
> + int err = 0;
> +
> + obj = klp_get_or_add_object(patch, old_obj);
> + if (IS_ERR(obj))
> + return PTR_ERR(obj);
> +
> + klp_for_each_func(old_obj, old_func) {
> + err = klp_add_func_nop(obj, old_func);
> + if (err)
> + return err;
> + }
> +
> + 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
> + *
> + * The nops are generated for all patches on the stack when
> + * the new patch is initialized. It is safe even though some
> + * older patches might get disabled and removed before the
> + * new one is enabled. In the worst case, there might be nops
> + * there will not be really needed. But it does not harm and
> + * simplifies the implementation a lot. Especially we could
> + * use the init functions as is.
> + */
> +static int klp_add_nops(struct klp_patch *patch)
> +{
> + struct klp_patch *old_patch;
> + struct klp_object *old_obj;
> + int err = 0;
> +
> + if (WARN_ON(!patch->replace))
> + return -EINVAL;
> +
> + list_for_each_entry(old_patch, &klp_patches, list) {
> + klp_for_each_object(old_patch, old_obj) {
> + err = klp_add_object_nops(patch, old_obj);
> + if (err)
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> /*
> * Patch release framework must support the following scenarios:
> *
> @@ -952,6 +1104,12 @@ static int klp_init_patch(struct klp_patch *patch)
> return ret;
> }
>
> + if (patch->replace) {
> + ret = klp_add_nops(patch);
> + if (ret)
> + goto free;
> + }
> +
> klp_for_each_object(patch, obj) {
> ret = klp_init_object(patch, obj);
> if (ret)
> diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
> index 48a83d4364cf..43184a5318d8 100644
> --- a/kernel/livepatch/core.h
> +++ b/kernel/livepatch/core.h
> @@ -6,6 +6,10 @@
>
> 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);
> +
> 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 6917100fbe79..3f6cf5b9e048 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -87,6 +87,33 @@ static void klp_complete_transition(void)
> klp_transition_patch->mod->name,
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> + /*
> + * For replace patches, we disable all previous patches, and replace
> + * the dynamic no-op functions by removing the ftrace hook.
> + */
> + if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
> + /*
> + * Make sure klp_ftrace_handler() can no longer see functions
> + * from the replaced patches. Then all functions will be
> + * redirected using klp_transition_patch. They will use
> + * either a new code or they will stay in the original code
> + * because of the nop funcs' structures.
> + */
> + if (klp_forced)
> + klp_synchronize_transition();
> +
> + klp_throw_away_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
> + * gurantees that nobody will stay in the trampoline after
> + * the ftrace handler is unregistered.
> + */
> + klp_unpatch_objects(klp_transition_patch, KLP_FUNC_NOP);
> + }
> +
> if (klp_target_state == KLP_UNPATCHED) {
> /*
> * All tasks have transitioned to KLP_UNPATCHED so we can now
> @@ -143,6 +170,15 @@ static void klp_complete_transition(void)
> if (!klp_forced && klp_target_state == KLP_UNPATCHED)
> module_put(klp_transition_patch->mod);
>
> + /*
> + * We do not need to wait until the objects are really freed.
> + * The patch must be on the bottom of the stack. Therefore it
> + * will never replace anything else. The only important thing
> + * is that we wait when the patch is being unregistered.
> + */
> + if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED)
> + klp_free_objects(klp_transition_patch, KLP_FUNC_NOP);
> +
> klp_target_state = KLP_UNDEFINED;
> klp_transition_patch = NULL;
> }
>

2018-01-26 09:09:54

by Petr Mladek

[permalink] [raw]

Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace

On Thu 2018-01-25 23:27:57, Jason Baron wrote:
> On 01/25/2018 11:02 AM, Petr Mladek wrote:
> > From: Jason Baron <[email protected]>
> > A better solution would be to create cumulative patch and say that
> > it replaces all older ones.
> >
> > Signed-off-by: Jason Baron <[email protected]>
> > [[email protected]: Split, reuse existing code, simplified]
>
> Hi Petr,
>
> Thanks for cleaning this up - it looks good.

Uff, I feel relief :-)

> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 6ad3195d995a..c606b291dfcd 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > +/*
> > + * This function removes replaced patches from both func_stack
> > + * and klp_patches stack.
> > + *
> > + * We could be pretty aggressive here. It is called in situation
> > + * when these structures are not longer accessible. All functions
> > + * are redirected using the klp_transition_patch. They use either
> > + * a new code or they in the original code because of the special
> > + * nop function patches.
> > + */
> > +void klp_throw_away_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, KLP_FUNC_ANY);
> > + old_patch->enabled = false;
> > +
> > + /*
> > + * Replaced patches could not get re-enabled to keep
> > + * the code sane.
> > + */
> > + list_del_init(&old_patch->list);
>
> I'm wondering if this should be:
>
> list_move(&old_patch->list, &klp_replaced_patches);

Yes, great catch!

The list_del() comes from one iteration where I got rid of the extra
list. I though that it might be enough to check
patch->kobj.state_initialized. But then I realized that this
kobject state was modified outside klp_mutex.

To be honest, I did not only minimal testing with my changes.
Mirek promised to port a battery of his kGraft-based tests and
run it.

Thanks a lot for review.

Best Regards,
Petr

2018-01-26 10:33:58

by Evgenii Shatokhin

[permalink] [raw]

Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace

On 25.01.2018 19:02, Petr Mladek wrote:
> From: 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 not longer be
> modified by the new patch. They are temporarily used as a new target
> during the patch transition.
>
> There are used several simplifications:
>
> + nops' structures are generated already when the patch is registered.
> All registered patches are taken into account, even the disabled ones.
> As a result, there might be more nops than are really needed when
> the patch is enabled and some disabled patches were removed before.
> But we are on the safe side and it simplifies the implementation.
> Especially we could reuse the existing init() functions. Also freeing
> is easier because the same nops are created and removed only once.
>
> Alternative solution would be to create nops when the patch is enabled.
> But then we would need to complicated to reuse the init() functions
> and error paths. It would increase the risk of errors because of
> late kobject initialization. It would need tricky waiting for
> freed kobjects when finalizing a reverted enable transaction.
>
> + The replaced patches are removed from the stack and cannot longer
> be enabled directly. Otherwise, we would need to implement a more
> complex logic of handling the stack of patches. It might be hard
> to come with a reasonable semantic.
>
> A fallback is to remove (rmmod) the replaced patches and register
> (insmod) them again.
>
> + Nops are handled like normal function patches. It reduces changes
> in the existing code.
>
> It would be possible to copy internal values when they are allocated
> and make short cuts in init() functions. It would be possible to use
> the fact that old_func and new_func point to the same function and
> do not init new_func and new_size at all. It would be possible to
> detect nop func in ftrace handler and just leave. But all these would
> just complicate the code and maintenance.
>
> + The callbacks from the replaced patches are not called. It would be
> pretty hard to define a reasonable semantic and implement it.

At least, it surely simplifies error handling, if these callbacks are
not called.

Anyway, I guess, this restriction should be mentioned explicitly in the
docs. I think this is not obvious for the patch developers (esp. those
familiar with RPM spec files and such ;-) ).

What concerns me is that downgrading of the cumulative patches with
callbacks becomes much more difficult this way.

I mean, suppose a user has v1 of a cumulative patch installed. Then a
newer version, v2, is released. They install it and find that it is
buggy (very unfortunate but might still happen). Now they cannot
atomically replace v2 back with v1, because the callbacks from v1 cannot
clean up after v2.

It will be needed to unload v2 explicitly and then load v1 back, which
is more fragile. The loading failures are much more unlikely with
livepatch than with the old kpatch, but they are still possible.

I have no good solution to this though.

>
> 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.
> No to say that the old scripts even would not expect to be called
> in this situation.
>
> Signed-off-by: Jason Baron <[email protected]>
> [[email protected]: Split, reuse existing code, simplified]
> 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: Petr Mladek <[email protected]>
> ---
> include/linux/livepatch.h | 3 +
> kernel/livepatch/core.c | 162 +++++++++++++++++++++++++++++++++++++++++-
> kernel/livepatch/core.h | 4 ++
> kernel/livepatch/transition.c | 36 ++++++++++
> 4 files changed, 203 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 21cad200f949..9d44d105b278 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -42,6 +42,7 @@
> enum klp_func_type {
> KLP_FUNC_ANY = -1, /* Substitute any type */
> KLP_FUNC_ORIGINAL = 0, /* Original statically defined structure */
> + KLP_FUNC_NOP, /* Dynamically allocated NOP function patch */
> };
>
> /**
> @@ -153,6 +154,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: head of list for struct klp_object
> @@ -163,6 +165,7 @@ struct klp_patch {
> /* external */
> struct module *mod;
> struct klp_object *objs;
> + bool replace;
>
> /* internal */
> struct list_head list;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6ad3195d995a..c606b291dfcd 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -142,6 +142,21 @@ 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)
> {
> @@ -342,6 +357,39 @@ 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 situation
> + * when these structures are not longer accessible. All functions
> + * are redirected using the klp_transition_patch. They use either
> + * a new code or they in the original code because of the special
> + * nop function patches.
> + */
> +void klp_throw_away_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, KLP_FUNC_ANY);
> + old_patch->enabled = false;
> +
> + /*
> + * Replaced patches could not get re-enabled to keep
> + * the code sane.
> + */
> + list_del_init(&old_patch->list);
> +
> + if (!keep_module)
> + module_put(old_patch->mod);
> + }
> +}
> +
> static int __klp_disable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
> @@ -537,7 +585,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> if (!klp_is_patch_usable(patch)) {
> /*
> * Module with the patch could either disappear meanwhile or is
> - * not properly initialized yet.
> + * not properly initialized yet or the patch was just replaced.
> */
> ret = -EINVAL;
> goto err;
> @@ -662,8 +710,16 @@ static struct attribute *klp_patch_attrs[] = {
> /*
> * Dynamically allocated objects and functions.
> */
> +static void klp_free_func_nop(struct klp_func *func)
> +{
> + kfree(func->old_name);
> + kfree(func);
> +}
> +
> static void klp_free_func_dynamic(struct klp_func *func)
> {
> + if (func->ftype == KLP_FUNC_NOP)
> + klp_free_func_nop(func);
> }
>
> static void klp_free_object_dynamic(struct klp_object *obj)
> @@ -691,7 +747,7 @@ static struct klp_object *klp_alloc_object_dynamic(const char *name)
> return obj;
> }
>
> -struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> +static struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> struct klp_object *old_obj)
> {
> struct klp_object *obj;
> @@ -708,6 +764,102 @@ struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> return obj;
> }
>
> +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 ERR_PTR(-ENOMEM);
> +
> + if (old_func->old_name) {
> + func->old_name = kstrdup(old_func->old_name, GFP_KERNEL);
> + if (!func->old_name) {
> + kfree(func);
> + return ERR_PTR(-ENOMEM);
> + }
> + }
> + func->old_sympos = old_func->old_sympos;
> + /* NOP func is the same as using the original implementation. */
> + func->new_func = (void *)old_func->old_addr;
> + func->ftype = KLP_FUNC_NOP;
> +
> + return func;
> +}
> +
> +static int klp_add_func_nop(struct klp_object *obj,
> + struct klp_func *old_func)
> +{
> + struct klp_func *func;
> +
> + func = klp_find_func(obj, old_func);
> +
> + if (func)
> + return 0;
> +
> + func = klp_alloc_func_nop(old_func, obj);
> + if (IS_ERR(func))
> + return PTR_ERR(func);
> +
> + klp_init_func_list(obj, func);
> +
> + return 0;
> +}
> +
> +static int klp_add_object_nops(struct klp_patch *patch,
> + struct klp_object *old_obj)
> +{
> + struct klp_object *obj;
> + struct klp_func *old_func;
> + int err = 0;
> +
> + obj = klp_get_or_add_object(patch, old_obj);
> + if (IS_ERR(obj))
> + return PTR_ERR(obj);
> +
> + klp_for_each_func(old_obj, old_func) {
> + err = klp_add_func_nop(obj, old_func);
> + if (err)
> + return err;
> + }
> +
> + 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
> + *
> + * The nops are generated for all patches on the stack when
> + * the new patch is initialized. It is safe even though some
> + * older patches might get disabled and removed before the
> + * new one is enabled. In the worst case, there might be nops
> + * there will not be really needed. But it does not harm and
> + * simplifies the implementation a lot. Especially we could
> + * use the init functions as is.
> + */
> +static int klp_add_nops(struct klp_patch *patch)
> +{
> + struct klp_patch *old_patch;
> + struct klp_object *old_obj;
> + int err = 0;
> +
> + if (WARN_ON(!patch->replace))
> + return -EINVAL;
> +
> + list_for_each_entry(old_patch, &klp_patches, list) {
> + klp_for_each_object(old_patch, old_obj) {
> + err = klp_add_object_nops(patch, old_obj);
> + if (err)
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> /*
> * Patch release framework must support the following scenarios:
> *
> @@ -952,6 +1104,12 @@ static int klp_init_patch(struct klp_patch *patch)
> return ret;
> }
>
> + if (patch->replace) {
> + ret = klp_add_nops(patch);
> + if (ret)
> + goto free;
> + }
> +
> klp_for_each_object(patch, obj) {
> ret = klp_init_object(patch, obj);
> if (ret)
> diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
> index 48a83d4364cf..43184a5318d8 100644
> --- a/kernel/livepatch/core.h
> +++ b/kernel/livepatch/core.h
> @@ -6,6 +6,10 @@
>
> 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);
> +
> 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 6917100fbe79..3f6cf5b9e048 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -87,6 +87,33 @@ static void klp_complete_transition(void)
> klp_transition_patch->mod->name,
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> + /*
> + * For replace patches, we disable all previous patches, and replace
> + * the dynamic no-op functions by removing the ftrace hook.
> + */
> + if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
> + /*
> + * Make sure klp_ftrace_handler() can no longer see functions
> + * from the replaced patches. Then all functions will be
> + * redirected using klp_transition_patch. They will use
> + * either a new code or they will stay in the original code
> + * because of the nop funcs' structures.
> + */
> + if (klp_forced)
> + klp_synchronize_transition();
> +
> + klp_throw_away_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
> + * gurantees that nobody will stay in the trampoline after
> + * the ftrace handler is unregistered.
> + */
> + klp_unpatch_objects(klp_transition_patch, KLP_FUNC_NOP);
> + }
> +
> if (klp_target_state == KLP_UNPATCHED) {
> /*
> * All tasks have transitioned to KLP_UNPATCHED so we can now
> @@ -143,6 +170,15 @@ static void klp_complete_transition(void)
> if (!klp_forced && klp_target_state == KLP_UNPATCHED)
> module_put(klp_transition_patch->mod);
>
> + /*
> + * We do not need to wait until the objects are really freed.
> + * The patch must be on the bottom of the stack. Therefore it
> + * will never replace anything else. The only important thing
> + * is that we wait when the patch is being unregistered.
> + */
> + if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED)
> + klp_free_objects(klp_transition_patch, KLP_FUNC_NOP);
> +
> klp_target_state = KLP_UNDEFINED;
> klp_transition_patch = NULL;
> }
>

2018-01-31 15:39:46

by Miroslav Benes

[permalink] [raw]

Subject: Re: PATCH v6 3/6] livepatch: Initial support for dynamic structures

On Thu, 25 Jan 2018, Petr Mladek wrote:

> From: Jason Baron <[email protected]>
>
> We 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 not longer patched.
>
> This patch adds basic framework to handle such dynamic structures.
>
> It adds enum klp_func_type that allows to distinguish the dynamically
> allocated funcs' structures. Note that objects' structures do not have
> a clear type. Namely the static objects' structures might list both static
> and dynamic funcs' structures.
>
> The function type is then used to limit klp_free() functions. We will
> want to free the dynamic structures separately when they are not
> longer needed. At the same time, we also want to make our life easier,
> and introduce _any_ type that will allow to process all existing
> structures in one go.
>
> We need to be careful here. First, objects' structures must be freed
> only when all listed funcs' structures are freed. Also we must avoid
> double free. Both problems are solved by removing the freed structures
> from the list.
>
> Also note that klp_free*() functions are called also in klp_init_patch()
> error path when only some kobjects have been initialized. The other
> dynamic structures must be freed immediately by calling the respective
> klp_free_*_dynamic() functions.
>
> Finally, the dynamic objects' structures are generic. The respective
> klp_allocate_object_dynamic() and klp_free_object_dynamic() can
> be implemented here. On the other hand, klp_free_func_dynamic()
> is empty. It must be updated when a particular dynamic
> klp_func_type is introduced.
>
> Signed-off-by: Jason Baron <[email protected]>
> [[email protected]: Converted into a generic API]
> 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]>
> ---
> include/linux/livepatch.h | 37 +++++++++++-
> kernel/livepatch/core.c | 139 ++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 159 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index e5db2ba7e2a5..21cad200f949 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -35,12 +35,22 @@
> #define KLP_UNPATCHED 0
> #define KLP_PATCHED 1
>
> +/*
> + * Function type is used to distinguish dynamically allocated structures
> + * and limit some operations.
> + */
> +enum klp_func_type {
> + KLP_FUNC_ANY = -1, /* Substitute any type */
> + KLP_FUNC_ORIGINAL = 0, /* Original statically defined structure */

Wouldn't KLP_FUNC_STATIC be better? KLP_FUNC_ORIGINAL confused me couple
of times.

Miroslav

2018-01-31 15:43:12

by Miroslav Benes

[permalink] [raw]

Subject: Re: PATCH v6 4/6] livepatch: Allow to unpatch only functions of the given type

> +void klp_unpatch_object(struct klp_object *obj, enum klp_func_type ftype)
> {
> struct klp_func *func;
> + bool patched = false;
>
> - klp_for_each_func(obj, func)
> - if (func->patched)
> + klp_for_each_func(obj, func) {
> + if (!func->patched)
> + continue;
> +
> + if (ftype == KLP_FUNC_ANY || ftype == func->ftype)

You defined klp_is_func_type() exactly for this purpose.

Otherwise, it looks good. There is no functional change.

Miroslav

2018-01-31 16:48:58

by Miroslav Benes

[permalink] [raw]

Subject: Re: PATCH v6 5/6] livepatch: Support separate list for replaced patches.

On Thu, 25 Jan 2018, Petr Mladek wrote:

> From: Jason Baron <[email protected]>
>
> 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 replaced patches will stay registered because they are typically
> unregistered by some package uninstall scripts. But we will remove
> these patches from @klp_patches list to keep the enabled patch
> on the bottom of the stack. Otherwise, we would need to implement
> rather complex logic for moving the patches on the stack. Also
> it would complicate implementation of the atomic replace feature.
> It is not worth it.
>
> As a result, we will have patches that are registered but that
> are not longer usable. Let's get prepared for this and use
> a better descriptive name for klp_is_patch_registered() function.
>
> Also create separate list for the replaced patches and allow to
> unregister them. Alternative solution would be to add a flag
> into struct klp_patch. Note that patch->kobj.state_initialized
> is not safe because it can be cleared outside klp_mutex.
>
> This patch does not change the existing behavior.

Ok, why not. We could also unregister the patches right away and amend the
check in klp_unregister_patch() not to return error. Ever.

However, this seems better in the end.

Miroslav

2018-01-31 21:57:42

by Josh Poimboeuf

[permalink] [raw]

Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace

On Fri, Jan 26, 2018 at 01:33:04PM +0300, Evgenii Shatokhin wrote:
> > + The callbacks from the replaced patches are not called. It would be
> > pretty hard to define a reasonable semantic and implement it.
>
> At least, it surely simplifies error handling, if these callbacks are not
> called.
>
> Anyway, I guess, this restriction should be mentioned explicitly in the
> docs. I think this is not obvious for the patch developers (esp. those
> familiar with RPM spec files and such ;-) ).
>
> What concerns me is that downgrading of the cumulative patches with
> callbacks becomes much more difficult this way.
>
> I mean, suppose a user has v1 of a cumulative patch installed. Then a newer
> version, v2, is released. They install it and find that it is buggy (very
> unfortunate but might still happen). Now they cannot atomically replace v2
> back with v1, because the callbacks from v1 cannot clean up after v2.
>
> It will be needed to unload v2 explicitly and then load v1 back, which is
> more fragile. The loading failures are much more unlikely with livepatch
> than with the old kpatch, but they are still possible.
>
> I have no good solution to this though.

I think the solution is to build a v3, which is basically identical to
v1, except it also has callbacks for cleaning up after v2, if necessary.

It should also be smart enough to deal with the case that v2 was not
installed beforehand.

--
Josh

2018-02-01 08:18:19

by Evgenii Shatokhin

[permalink] [raw]

Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace

On 01.02.2018 00:55, Josh Poimboeuf wrote:
> On Fri, Jan 26, 2018 at 01:33:04PM +0300, Evgenii Shatokhin wrote:
>>> + The callbacks from the replaced patches are not called. It would be
>>> pretty hard to define a reasonable semantic and implement it.
>>
>> At least, it surely simplifies error handling, if these callbacks are not
>> called.
>>
>> Anyway, I guess, this restriction should be mentioned explicitly in the
>> docs. I think this is not obvious for the patch developers (esp. those
>> familiar with RPM spec files and such ;-) ).
>>
>> What concerns me is that downgrading of the cumulative patches with
>> callbacks becomes much more difficult this way.
>>
>> I mean, suppose a user has v1 of a cumulative patch installed. Then a newer
>> version, v2, is released. They install it and find that it is buggy (very
>> unfortunate but might still happen). Now they cannot atomically replace v2
>> back with v1, because the callbacks from v1 cannot clean up after v2.
>>
>> It will be needed to unload v2 explicitly and then load v1 back, which is
>> more fragile. The loading failures are much more unlikely with livepatch
>> than with the old kpatch, but they are still possible.
>>
>> I have no good solution to this though.
>
> I think the solution is to build a v3, which is basically identical to
> v1, except it also has callbacks for cleaning up after v2, if necessary.
>
> It should also be smart enough to deal with the case that v2 was not
> installed beforehand.
>

I thought about this, but such patches would be very difficult to
maintain. It seems better to explicitly unload v2 and then load v1 back
in such cases.

In addition, releasing v3 takes some time (build + appropriate QA
procedures) while the users may want to do something about the faulty
patch "right here, right now". This is what "downgrade" option is for.

Anyway, livepatch is much more likely to apply the patches successfully
than the old kpatch core. So it should be OK to simply unload v2 and
then load v1 in such (hopefully rare) scenarios.

As I said, the current behaviour is acceptable, esp. when it is
documented. Implementation of livepatch is already complex enough.

Regards,
Evgenii

2018-02-01 13:50:20

by Miroslav Benes

[permalink] [raw]

Subject: Re: PATCH v6 0/6] livepatch: Atomic replace feature

On Thu, 25 Jan 2018, Petr Mladek wrote:

> Hi,
>
> 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 is my rework based on Jason's v5 patchset[1]. My intention was:
>
> + reduce code duplication and nop-specific shortcuts
> + split the huge patch for an easier review
> + simplify the logic where possible
> + add/update/clear comments
> + better fit into the existing code
>
> I am not supper happy with the result. But it took me much longer
> time than expected. It is high time, I shared the current state
> and get some feedback.
>
>
> Jason,
>
> I used your code in most of the patches and kept you as the author.
> Please, let me know if you would prefer another solution.
>
> Also I am sorry that I have done it this way. I was not able to
> suggest it out of head. I needed many iterations to end up with
> the current state. I needed to play with the code. Therefore
> it made sense to send what I got.
>
>
> [1] https://lkml.kernel.org/r/[email protected]

Hi,

the series looks good to me (apart from the mentioned bugs and a couple
of nits). I'll port my old kGraft testing modules and hopefully it will
survive.

Well, one more thing. I think there is a problem with shadow variables.
Similar to callbacks situation. Shadow variables cannot be destroyed the
way it is shown in our samples. Cumulative patches want to preserve
everything as much as possible. If I'm right, it should be mentioned in
the documentation.

Miroslav

2018-02-01 13:50:23

by Miroslav Benes

[permalink] [raw]

Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace

> -struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> +static struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> struct klp_object *old_obj)

A nit, but this change belongs to 3/6, doesn't it?

> {
> struct klp_object *obj;
> @@ -708,6 +764,102 @@ struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> return obj;
> }

[...]

> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 6917100fbe79..3f6cf5b9e048 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -87,6 +87,33 @@ static void klp_complete_transition(void)
> klp_transition_patch->mod->name,
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> + /*
> + * For replace patches, we disable all previous patches, and replace
> + * the dynamic no-op functions by removing the ftrace hook.
> + */
> + if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
> + /*
> + * Make sure klp_ftrace_handler() can no longer see functions
> + * from the replaced patches. Then all functions will be
> + * redirected using klp_transition_patch. They will use
> + * either a new code or they will stay in the original code
> + * because of the nop funcs' structures.
> + */
> + if (klp_forced)
> + klp_synchronize_transition();

I'm not sure why this is here. klp_forced should not be so special that it
would warrant a synchronization. However, I'm also not sure that it would
be safe to remove it from the patch.

Is this because "force" is the only one who can clear TIF_PATCH_PENDING of
other tasks? So, there could be a task running in klp_ftrace_handler(),
its TIF_PATCH_PENDING is cleared by force and we could have a problem
because of that since we're throwing away replaced patches.

Ok, that sounds viable. I'd welcome a comment on that in the code.

Thanks,
Miroslav

> +
> + klp_throw_away_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
> + * gurantees that nobody will stay in the trampoline after
> + * the ftrace handler is unregistered.
> + */
> + klp_unpatch_objects(klp_transition_patch, KLP_FUNC_NOP);
> + }
> +
> if (klp_target_state == KLP_UNPATCHED) {
> /*
> * All tasks have transitioned to KLP_UNPATCHED so we can now

2018-02-01 14:27:01

by Joe Lawrence

[permalink] [raw]

Subject: Re: PATCH v6 2/6] livepatch: Free only structures with initialized kobject

On 01/25/2018 11:01 AM, Petr Mladek wrote:
> We 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 not longer patched.

Super small nit: funcs', objects' ? More on this in the next inline
comment.

> [ ... snip ... ]
> /*
> - * Free all functions' kobjects in the array up to some limit. When limit is
> - * NULL, all kobjects are freed.
> + * Free all funcs' that have the kobject initialized. Otherwise,
> + * nothing is needed.

In new comment, what is the apostrophe in funcs' for? The code (still)
deals with the kobject embedded in the klp_func, but the comment text
moved "kobject" such that I think the apostrophe can now be removed.

> [ ... snip ... ]
> /*
> - * Free all objects' kobjects in the array up to some limit. When limit is
> - * NULL, all kobjects are freed.
> + * Free all funcs' and objects's that have the kobject initialized.
> + * Otherwise, nothing is needed.
> */

Ditto.

-- Joe

2018-02-01 14:30:44

by Joe Lawrence

[permalink] [raw]

Subject: Re: PATCH v6 0/6] livepatch: Atomic replace feature

On 02/01/2018 08:49 AM, Miroslav Benes wrote:
>
> Well, one more thing. I think there is a problem with shadow variables.
> Similar to callbacks situation. Shadow variables cannot be destroyed the
> way it is shown in our samples. Cumulative patches want to preserve
> everything as much as possible. If I'm right, it should be mentioned in
> the documentation.

Are you talking about using klp_shadow_free_all() call in a module_exit
routine? Yeah, I think in this case, that responsibility would be
passed to the newly loaded cumulative patch, right?

-- Joe

2018-02-01 15:09:31

by Miroslav Benes

[permalink] [raw]

Subject: Re: PATCH v6 0/6] livepatch: Atomic replace feature

On Thu, 1 Feb 2018, Joe Lawrence wrote:

> On 02/01/2018 08:49 AM, Miroslav Benes wrote:
> >
> > Well, one more thing. I think there is a problem with shadow variables.
> > Similar to callbacks situation. Shadow variables cannot be destroyed the
> > way it is shown in our samples. Cumulative patches want to preserve
> > everything as much as possible. If I'm right, it should be mentioned in
> > the documentation.
>
> Are you talking about using klp_shadow_free_all() call in a module_exit
> routine? Yeah, I think in this case, that responsibility would be
> passed to the newly loaded cumulative patch, right?

Yes, but we haven't got an option not to call it here (as with callbacks,
where we can omit callbacks completely with atomic replace patches). A
live patch author must be aware of this and use shadow variables
appropriately.

Miroslav

2018-02-01 15:19:25

by Josh Poimboeuf

[permalink] [raw]

Subject: Re: PATCH v6 0/6] livepatch: Atomic replace feature

On Thu, Feb 01, 2018 at 04:08:14PM +0100, Miroslav Benes wrote:
> On Thu, 1 Feb 2018, Joe Lawrence wrote:
>
> > On 02/01/2018 08:49 AM, Miroslav Benes wrote:
> > >
> > > Well, one more thing. I think there is a problem with shadow variables.
> > > Similar to callbacks situation. Shadow variables cannot be destroyed the
> > > way it is shown in our samples. Cumulative patches want to preserve
> > > everything as much as possible. If I'm right, it should be mentioned in
> > > the documentation.
> >
> > Are you talking about using klp_shadow_free_all() call in a module_exit
> > routine? Yeah, I think in this case, that responsibility would be
> > passed to the newly loaded cumulative patch, right?
>
> Yes, but we haven't got an option not to call it here (as with callbacks,
> where we can omit callbacks completely with atomic replace patches). A
> live patch author must be aware of this and use shadow variables
> appropriately.

So maybe we should recommend that shadow variables generally be freed
from a post-unpatch callback.

--
Josh

2018-02-01 15:52:32

by Miroslav Benes

[permalink] [raw]

Subject: Re: PATCH v6 0/6] livepatch: Atomic replace feature

On Thu, 1 Feb 2018, Josh Poimboeuf wrote:

> On Thu, Feb 01, 2018 at 04:08:14PM +0100, Miroslav Benes wrote:
> > On Thu, 1 Feb 2018, Joe Lawrence wrote:
> >
> > > On 02/01/2018 08:49 AM, Miroslav Benes wrote:
> > > >
> > > > Well, one more thing. I think there is a problem with shadow variables.
> > > > Similar to callbacks situation. Shadow variables cannot be destroyed the
> > > > way it is shown in our samples. Cumulative patches want to preserve
> > > > everything as much as possible. If I'm right, it should be mentioned in
> > > > the documentation.
> > >
> > > Are you talking about using klp_shadow_free_all() call in a module_exit
> > > routine? Yeah, I think in this case, that responsibility would be
> > > passed to the newly loaded cumulative patch, right?
> >
> > Yes, but we haven't got an option not to call it here (as with callbacks,
> > where we can omit callbacks completely with atomic replace patches). A
> > live patch author must be aware of this and use shadow variables
> > appropriately.
>
> So maybe we should recommend that shadow variables generally be freed
> from a post-unpatch callback.

Yes, that's a possibility. In other words, if there is a need to call
klp_shadow_free_all() somewhere, it should be in a post-unpatch callback.

Miroslav

2018-02-05 10:34:30

by Petr Mladek

[permalink] [raw]

Subject: Re: PATCH v6 3/6] livepatch: Initial support for dynamic structures

On Wed 2018-01-31 16:39:04, Miroslav Benes wrote:
> On Thu, 25 Jan 2018, Petr Mladek wrote:
>
> > From: Jason Baron <[email protected]>
> >
> > We 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 not longer patched.
> >
> > This patch adds basic framework to handle such dynamic structures.
> >
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index e5db2ba7e2a5..21cad200f949 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -35,12 +35,22 @@
> > #define KLP_UNPATCHED 0
> > #define KLP_PATCHED 1
> >
> > +/*
> > + * Function type is used to distinguish dynamically allocated structures
> > + * and limit some operations.
> > + */
> > +enum klp_func_type {
> > + KLP_FUNC_ANY = -1, /* Substitute any type */
> > + KLP_FUNC_ORIGINAL = 0, /* Original statically defined structure */
>
> Wouldn't KLP_FUNC_STATIC be better? KLP_FUNC_ORIGINAL confused me couple
> of times.

OK, I am going to use KLP_FUNC_STATIC in v7.

Best Regards,
Petr

2018-02-05 14:16:36

by Petr Mladek

[permalink] [raw]

Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace

On Thu 2018-02-01 14:49:24, Miroslav Benes wrote:
>
> > -struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> > +static struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> > struct klp_object *old_obj)
>
> A nit, but this change belongs to 3/6, doesn't it?
>
> > {
> > struct klp_object *obj;
> > @@ -708,6 +764,102 @@ struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> > return obj;
> > }
>
> [...]
>
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index 6917100fbe79..3f6cf5b9e048 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -87,6 +87,33 @@ static void klp_complete_transition(void)
> > klp_transition_patch->mod->name,
> > klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
> >
> > + /*
> > + * For replace patches, we disable all previous patches, and replace
> > + * the dynamic no-op functions by removing the ftrace hook.
> > + */
> > + if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
> > + /*
> > + * Make sure klp_ftrace_handler() can no longer see functions
> > + * from the replaced patches. Then all functions will be
> > + * redirected using klp_transition_patch. They will use
> > + * either a new code or they will stay in the original code
> > + * because of the nop funcs' structures.
> > + */
> > + if (klp_forced)
> > + klp_synchronize_transition();
>
> I'm not sure why this is here. klp_forced should not be so special that it
> would warrant a synchronization. However, I'm also not sure that it would
> be safe to remove it from the patch.
>
> Is this because "force" is the only one who can clear TIF_PATCH_PENDING of
> other tasks? So, there could be a task running in klp_ftrace_handler(),
> its TIF_PATCH_PENDING is cleared by force and we could have a problem
> because of that since we're throwing away replaced patches.

Yes. I was deep in the paranoid mode when I added this synchronization ;-)

A reckless user might force the transaction anytime. There might
be running tasks that are still falling back to an older patch.
In fact, there might even be ftrace handlers that have not seen
the new patch on the stack yet.

It still should be safe because of the rcu list access. But
better be safe than sorry. Then we do not to take care
about this and simply remove the old patches in
klp_throw_away_replaced_patches().

> Ok, that sounds viable. I'd welcome a comment on that in the code.

I am going to use the following comment in v7:

/*
* Make sure that no ftrace handler accesses any older patch
* on the stack. This might happen when the user forced the
* transaction while some running tasks were still falling
* back to the old code. There might even still be ftrace
* handlers that have not seen the last patch on the stack yet.
*
* It probably is not necessary because of the rcu-safe access.
* But better be safe than sorry.
*/
if (klp_forced)
klp_synchronize_transition();
> > +
> > + klp_throw_away_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
> > + * gurantees that nobody will stay in the trampoline after
> > + * the ftrace handler is unregistered.
> > + */
> > + klp_unpatch_objects(klp_transition_patch, KLP_FUNC_NOP);
> > + }
> > +

Best Regards,
Petr