[PATCH] Remove the data duplication in internal and public structures

From: Miroslav Benes
Date: Wed Nov 19 2014 - 10:06:35 EST

The split of internal and external structures is cleaner and makes the API more
stable. But it makes the code more complicated. It requires more space and data
copying. Also the one letter difference of the names (lp_ vs. lpc_ prefix)
causes confusion.

The API is not a real issue for live patching. We take care neither of backward
nor forward compatibility. The dependency between a patch and kernel is even
more strict than by version. They have to use the same configuration and the
same build environment.

This patch merge the external and internal structures into one. The structures
are initialized using ".item = value" syntax. Therefore the API is basically as
stable as it was before. We could later even hide it under some helper macros
if requested.

For the purpose if this patch, we used the prefix "lpc". It allows to make as
less changes as possible and show the real effect. If the patch is accepted, it
would make sense to merge it into the original patch and even use another
common prefix, for example the proposed "klp".

Signed-off-by: Miroslav Benes <mbenes@xxxxxxx>
---
include/linux/livepatch.h | 47 +++++--
kernel/livepatch/core.c | 338 ++++++++++++----------------------------------
2 files changed, 121 insertions(+), 264 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 8b68fef..f16de32 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -21,10 +21,23 @@
#define _LINUX_LIVEPATCH_H_

#include <linux/module.h>
+#include <linux/ftrace.h>

/* TODO: add kernel-doc for structures once agreed upon */

-struct lp_func {
+enum lpc_state {
+ LPC_DISABLED,
+ LPC_ENABLED
+};
+
+struct lpc_func {
+ /* internal */
+ struct kobject *kobj;
+ struct ftrace_ops fops;
+ enum lpc_state state;
+ unsigned long new_addr;
+
+ /* external */
const char *old_name; /* function to be patched */
void *new_func; /* replacement function in patch module */
/*
@@ -38,7 +51,7 @@ struct lp_func {
unsigned long old_addr;
};

-struct lp_reloc {
+struct lpc_reloc {
unsigned long dest;
unsigned long src;
unsigned long type;
@@ -47,21 +60,33 @@ struct lp_reloc {
int external;
};

-struct lp_object {
+struct lpc_object {
+ /* internal */
+ struct kobject *kobj;
+ struct module *mod; /* module associated with object */
+ enum lpc_state state;
+
+ /* external */
const char *name; /* "vmlinux" or module name */
- struct lp_func *funcs;
- struct lp_reloc *relocs;
+ struct lpc_reloc *relocs;
+ struct lpc_func *funcs;
};

-struct lp_patch {
+struct lpc_patch {
+ /* internal */
+ struct list_head list;
+ struct kobject kobj;
+ enum lpc_state state;
+
+ /* external */
struct module *mod; /* module containing the patch */
- struct lp_object *objs;
+ struct lpc_object *objs;
};

-int lp_register_patch(struct lp_patch *);
-int lp_unregister_patch(struct lp_patch *);
-int lp_enable_patch(struct lp_patch *);
-int lp_disable_patch(struct lp_patch *);
+extern int lpc_register_patch(struct lpc_patch *);
+extern int lpc_unregister_patch(struct lpc_patch *);
+extern int lpc_enable_patch(struct lpc_patch *);
+extern int lpc_disable_patch(struct lpc_patch *);

#include <asm/livepatch.h>

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index e67c176..6586959 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -32,58 +32,9 @@
* Core structures
************************************/

-/*
- * lp_ structs vs lpc_ structs
- *
- * For each element (patch, object, func) in the live-patching code,
- * there are two types with two different prefixes: lp_ and lpc_.
- *
- * Structures used by the live-patch modules to register with this core module
- * are prefixed with lp_ (live patching). These structures are part of the
- * registration API and are defined in livepatch.h. The structures used
- * internally by this core module are prefixed with lpc_ (live patching core).
- */
-
static DEFINE_MUTEX(lpc_mutex);
static LIST_HEAD(lpc_patches);

-enum lpc_state {
- LPC_DISABLED,
- LPC_ENABLED
-};
-
-struct lpc_func {
- struct list_head list;
- struct kobject kobj;
- struct ftrace_ops fops;
- enum lpc_state state;
-
- const char *old_name;
- unsigned long new_addr;
- unsigned long old_addr;
-};
-
-struct lpc_object {
- struct list_head list;
- struct kobject kobj;
- struct module *mod; /* module associated with object */
- enum lpc_state state;
-
- const char *name;
- struct list_head funcs;
- struct lp_reloc *relocs;
-};
-
-struct lpc_patch {
- struct list_head list;
- struct kobject kobj;
- struct lp_patch *userpatch; /* for correlation during unregister */
- enum lpc_state state;
-
- struct module *mod;
- struct list_head objs;
-};
-
/*******************************************
* Helpers
*******************************************/
@@ -262,7 +213,7 @@ static int lpc_write_object_relocations(struct module *pmod,
struct lpc_object *obj)
{
int ret;
- struct lp_reloc *reloc;
+ struct lpc_reloc *reloc;

for (reloc = obj->relocs; reloc->name; reloc++) {
if (!strcmp(obj->name, "vmlinux")) {
@@ -360,7 +311,7 @@ static int lpc_disable_object(struct lpc_object *obj)
struct lpc_func *func;
int ret;

- list_for_each_entry(func, &obj->funcs, list) {
+ for (func = obj->funcs; func->old_name; func++) {
if (func->state != LPC_ENABLED)
continue;
ret = lpc_disable_func(func);
@@ -387,7 +338,8 @@ static int lpc_enable_object(struct module *pmod, struct lpc_object *obj)
if (ret)
goto unregister;
}
- list_for_each_entry(func, &obj->funcs, list) {
+
+ for (func = obj->funcs; func->old_name; func++) {
ret = lpc_find_verify_func_addr(func, obj->name);
if (ret)
goto unregister;
@@ -408,25 +360,14 @@ unregister:
* enable/disable
******************************/

-static struct lpc_patch *lpc_find_patch(struct lp_patch *userpatch)
-{
- struct lpc_patch *patch;
-
- list_for_each_entry(patch, &lpc_patches, list)
- if (patch->userpatch == userpatch)
- return patch;
-
- return NULL;
-}
-
-static int lpc_disable_patch(struct lpc_patch *patch)
+static int __lpc_disable_patch(struct lpc_patch *patch)
{
struct lpc_object *obj;
int ret;

pr_notice("disabling patch '%s'\n", patch->mod->name);

- list_for_each_entry(obj, &patch->objs, list) {
+ for (obj = patch->objs; obj->name; obj++) {
if (obj->state != LPC_ENABLED)
continue;
ret = lpc_disable_object(obj);
@@ -439,32 +380,26 @@ static int lpc_disable_patch(struct lpc_patch *patch)
}

/**
- * lp_disable_patch() - disables a registered patch
+ * lpc_disable_patch() - disables a registered patch
* @userpatch: The registered, enabled patch to be disabled
*
* Unregisters the patched functions from ftrace.
*
* Return: 0 on success, otherwise error
*/
-int lp_disable_patch(struct lp_patch *userpatch)
+int lpc_disable_patch(struct lpc_patch *patch)
{
- struct lpc_patch *patch;
int ret;

mutex_lock(&lpc_mutex);
- patch = lpc_find_patch(userpatch);
- if (!patch) {
- ret = -ENODEV;
- goto out;
- }
- ret = lpc_disable_patch(patch);
-out:
+ ret = __lpc_disable_patch(patch);
mutex_unlock(&lpc_mutex);
+
return ret;
}
-EXPORT_SYMBOL_GPL(lp_disable_patch);
+EXPORT_SYMBOL_GPL(lpc_disable_patch);

-static int lpc_enable_patch(struct lpc_patch *patch)
+static int __lpc_enable_patch(struct lpc_patch *patch)
{
struct lpc_object *obj;
int ret;
@@ -477,7 +412,7 @@ static int lpc_enable_patch(struct lpc_patch *patch)

pr_notice("enabling patch '%s'\n", patch->mod->name);

- list_for_each_entry(obj, &patch->objs, list) {
+ for (obj = patch->objs; obj->name; obj++) {
if (!lpc_find_object_module(obj))
continue;
ret = lpc_enable_object(patch->mod, obj);
@@ -493,7 +428,7 @@ unregister:
}

/**
- * lp_enable_patch() - enables a registered patch
+ * lpc_enable_patch() - enables a registered patch
* @userpatch: The registered, disabled patch to be enabled
*
* Performs the needed symbol lookups and code relocations,
@@ -501,23 +436,17 @@ unregister:
*
* Return: 0 on success, otherwise error
*/
-int lp_enable_patch(struct lp_patch *userpatch)
+int lpc_enable_patch(struct lpc_patch *patch)
{
- struct lpc_patch *patch;
int ret;

mutex_lock(&lpc_mutex);
- patch = lpc_find_patch(userpatch);
- if (!patch) {
- ret = -ENODEV;
- goto out;
- }
- ret = lpc_enable_patch(patch);
-out:
+ ret = __lpc_enable_patch(patch);
mutex_unlock(&lpc_mutex);
+
return ret;
}
-EXPORT_SYMBOL_GPL(lp_enable_patch);
+EXPORT_SYMBOL_GPL(lpc_enable_patch);

/******************************
* module notifier
@@ -568,7 +497,7 @@ static int lpc_module_notify(struct notifier_block *nb, unsigned long action,
list_for_each_entry(patch, &lpc_patches, list) {
if (patch->state == LPC_DISABLED)
continue;
- list_for_each_entry(obj, &patch->objs, list) {
+ for (obj = patch->objs; obj->name; obj++) {
if (strcmp(obj->name, mod->name))
continue;
if (action == MODULE_STATE_COMING) {
@@ -624,11 +553,11 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
}

if (val == LPC_ENABLED) {
- ret = lpc_enable_patch(patch);
+ ret = __lpc_enable_patch(patch);
if (ret)
goto err;
} else {
- ret = lpc_disable_patch(patch);
+ ret = __lpc_disable_patch(patch);
if (ret)
goto err;
}
@@ -677,7 +606,6 @@ static void lpc_kobj_release_patch(struct kobject *kobj)
patch = container_of(kobj, struct lpc_patch, kobj);
if (!list_empty(&patch->list))
list_del(&patch->list);
- kfree(patch);
}

static struct kobj_type lpc_ktype_patch = {
@@ -686,212 +614,122 @@ static struct kobj_type lpc_ktype_patch = {
.default_attrs = lpc_patch_attrs
};

-static void lpc_kobj_release_object(struct kobject *kobj)
-{
- struct lpc_object *obj;
-
- obj = container_of(kobj, struct lpc_object, kobj);
- if (!list_empty(&obj->list))
- list_del(&obj->list);
- kfree(obj);
-}
-
-static struct kobj_type lpc_ktype_object = {
- .release = lpc_kobj_release_object,
- .sysfs_ops = &kobj_sysfs_ops,
-};
-
-static void lpc_kobj_release_func(struct kobject *kobj)
-{
- struct lpc_func *func;
-
- func = container_of(kobj, struct lpc_func, kobj);
- if (!list_empty(&func->list))
- list_del(&func->list);
- kfree(func);
-}
-
-static struct kobj_type lpc_ktype_func = {
- .release = lpc_kobj_release_func,
- .sysfs_ops = &kobj_sysfs_ops,
-};
-
/*********************************
* structure allocation
********************************/

-static void lpc_free_funcs(struct lpc_object *obj)
+/*
+ * Free all functions' kobjects in the array up to some limit. When limit is
+ * NULL, all kobjects are freed.
+ */
+static void lpc_free_funcs_limited(struct lpc_object *obj,
+ struct lpc_func *limit)
{
- struct lpc_func *func, *funcsafe;
+ struct lpc_func *func;

- list_for_each_entry_safe(func, funcsafe, &obj->funcs, list)
- kobject_put(&func->kobj);
+ for (func = obj->funcs; func->old_name && func != limit; func++)
+ kobject_put(func->kobj);
}

-static void lpc_free_objects(struct lpc_patch *patch)
+/*
+ * Free all objects' kobjects in the array up to some limit. When limit is
+ * NULL, all kobjects are freed.
+ */
+static void lpc_free_objects_limited(struct lpc_patch *patch,
+ struct lpc_object *limit)
{
- struct lpc_object *obj, *objsafe;
+ struct lpc_object *obj;

- list_for_each_entry_safe(obj, objsafe, &patch->objs, list) {
- lpc_free_funcs(obj);
- kobject_put(&obj->kobj);
+ for (obj = patch->objs; obj->name && obj != limit; obj++) {
+ lpc_free_funcs_limited(obj, NULL);
+ kobject_put(obj->kobj);
}
}

static void lpc_free_patch(struct lpc_patch *patch)
{
- lpc_free_objects(patch);
+ lpc_free_objects_limited(patch, NULL);
kobject_put(&patch->kobj);
}

-static struct lpc_func *lpc_create_func(struct kobject *root,
- struct lp_func *userfunc)
+static int lpc_init_funcs(struct lpc_object *obj)
{
struct lpc_func *func;
struct ftrace_ops *ops;
- int ret;
-
- /* alloc */
- func = kzalloc(sizeof(*func), GFP_KERNEL);
- if (!func)
- return NULL;
-
- /* init */
- INIT_LIST_HEAD(&func->list);
- func->old_name = userfunc->old_name;
- func->new_addr = (unsigned long)userfunc->new_func;
- func->old_addr = userfunc->old_addr;
- func->state = LPC_DISABLED;
- ops = &func->fops;
- ops->private = func;
- ops->func = lpc_ftrace_handler;
- ops->flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_DYNAMIC;
-
- /* sysfs */
- ret = kobject_init_and_add(&func->kobj, &lpc_ktype_func,
- root, func->old_name);
- if (ret) {
- kfree(func);
- return NULL;
- }
-
- return func;
-}
-
-static int lpc_create_funcs(struct lpc_object *obj,
- struct lp_func *userfuncs)
-{
- struct lp_func *userfunc;
- struct lpc_func *func;

- if (!userfuncs)
- return -EINVAL;
-
- for (userfunc = userfuncs; userfunc->old_name; userfunc++) {
- func = lpc_create_func(&obj->kobj, userfunc);
- if (!func)
+ for (func = obj->funcs; func->old_name; func++) {
+ func->new_addr = (unsigned long)func->new_func;
+ func->state = LPC_DISABLED;
+ ops = &func->fops;
+ ops->private = func;
+ ops->func = lpc_ftrace_handler;
+ ops->flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_DYNAMIC;
+
+ /* sysfs */
+ func->kobj = kobject_create_and_add(func->old_name, obj->kobj);
+ if (!func->kobj)
goto free;
- list_add(&func->list, &obj->funcs);
}
+
return 0;
free:
- lpc_free_funcs(obj);
+ lpc_free_funcs_limited(obj, func);
return -ENOMEM;
}

-static struct lpc_object *lpc_create_object(struct kobject *root,
- struct lp_object *userobj)
+static int lpc_init_objects(struct lpc_patch *patch)
{
struct lpc_object *obj;
int ret;

- /* alloc */
- obj = kzalloc(sizeof(*obj), GFP_KERNEL);
- if (!obj)
- return NULL;
+ for (obj = patch->objs; obj->name; obj++) {
+ /* obj->mod set by lpc_object_module_get() */
+ obj->state = LPC_DISABLED;

- /* init */
- INIT_LIST_HEAD(&obj->list);
- obj->name = userobj->name;
- obj->relocs = userobj->relocs;
- obj->state = LPC_DISABLED;
- /* obj->mod set by lpc_object_module_get() */
- INIT_LIST_HEAD(&obj->funcs);
-
- /* sysfs */
- ret = kobject_init_and_add(&obj->kobj, &lpc_ktype_object,
- root, obj->name);
- if (ret) {
- kfree(obj);
- return NULL;
- }
-
- /* create functions */
- ret = lpc_create_funcs(obj, userobj->funcs);
- if (ret) {
- kobject_put(&obj->kobj);
- return NULL;
- }
-
- return obj;
-}
-
-static int lpc_create_objects(struct lpc_patch *patch,
- struct lp_object *userobjs)
-{
- struct lp_object *userobj;
- struct lpc_object *obj;
-
- if (!userobjs)
- return -EINVAL;
+ /* sysfs */
+ obj->kobj = kobject_create_and_add(obj->name, &patch->kobj);
+ if (!obj->kobj)
+ goto free;

- for (userobj = userobjs; userobj->name; userobj++) {
- obj = lpc_create_object(&patch->kobj, userobj);
- if (!obj)
+ /* init functions */
+ ret = lpc_init_funcs(obj);
+ if (ret) {
+ kobject_put(obj->kobj);
goto free;
- list_add(&obj->list, &patch->objs);
+ }
}
+
return 0;
free:
- lpc_free_objects(patch);
+ lpc_free_objects_limited(patch, obj);
return -ENOMEM;
}

-static int lpc_create_patch(struct lp_patch *userpatch)
+static int lpc_init_patch(struct lpc_patch *patch)
{
- struct lpc_patch *patch;
int ret;

- /* alloc */
- patch = kzalloc(sizeof(*patch), GFP_KERNEL);
- if (!patch)
- return -ENOMEM;
+ mutex_lock(&lpc_mutex);

/* init */
- INIT_LIST_HEAD(&patch->list);
- patch->userpatch = userpatch;
- patch->mod = userpatch->mod;
patch->state = LPC_DISABLED;
- INIT_LIST_HEAD(&patch->objs);

/* sysfs */
ret = kobject_init_and_add(&patch->kobj, &lpc_ktype_patch,
lpc_root_kobj, patch->mod->name);
- if (ret) {
- kfree(patch);
+ if (ret)
return ret;
- }

/* create objects */
- ret = lpc_create_objects(patch, userpatch->objs);
+ ret = lpc_init_objects(patch);
if (ret) {
kobject_put(&patch->kobj);
return ret;
}

/* add to global list of patches */
- mutex_lock(&lpc_mutex);
list_add(&patch->list, &lpc_patches);
+
mutex_unlock(&lpc_mutex);

return 0;
@@ -902,7 +740,7 @@ static int lpc_create_patch(struct lp_patch *userpatch)
***********************************/

/**
- * lp_register_patch() - registers a patch
+ * lpc_register_patch() - registers a patch
* @userpatch: Patch to be registered
*
* Allocates the data structure associated with the patch and
@@ -910,11 +748,11 @@ static int lpc_create_patch(struct lp_patch *userpatch)
*
* Return: 0 on success, otherwise error
*/
-int lp_register_patch(struct lp_patch *userpatch)
+int lpc_register_patch(struct lpc_patch *userpatch)
{
int ret;

- if (!userpatch || !userpatch->mod || !userpatch->objs)
+ if (!userpatch || !userpatch->mod)
return -EINVAL;

/*
@@ -927,43 +765,37 @@ int lp_register_patch(struct lp_patch *userpatch)
if (!try_module_get(userpatch->mod))
return -ENODEV;

- ret = lpc_create_patch(userpatch);
+ ret = lpc_init_patch(userpatch);
if (ret)
module_put(userpatch->mod);

return ret;
}
-EXPORT_SYMBOL_GPL(lp_register_patch);
+EXPORT_SYMBOL_GPL(lpc_register_patch);

/**
- * lp_unregister_patch() - unregisters a patch
+ * lpc_unregister_patch() - unregisters a patch
* @userpatch: Disabled patch to be unregistered
*
* Frees the data structures and removes the sysfs interface.
*
* Return: 0 on success, otherwise error
*/
-int lp_unregister_patch(struct lp_patch *userpatch)
+int lpc_unregister_patch(struct lpc_patch *userpatch)
{
- struct lpc_patch *patch;
int ret = 0;

mutex_lock(&lpc_mutex);
- patch = lpc_find_patch(userpatch);
- if (!patch) {
- ret = -ENODEV;
- goto out;
- }
- if (patch->state == LPC_ENABLED) {
+ if (userpatch->state == LPC_ENABLED) {
ret = -EINVAL;
goto out;
}
- lpc_free_patch(patch);
+ lpc_free_patch(userpatch);
out:
mutex_unlock(&lpc_mutex);
return ret;
}
-EXPORT_SYMBOL_GPL(lp_unregister_patch);
+EXPORT_SYMBOL_GPL(lpc_unregister_patch);

/************************************
* entry/exit
--
2.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/