| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20141113171203.GA10996@cerebellum.variantweb.net>
Date: Thu, 13 Nov 2014 11:12:03 -0600
From: Seth Jennings <sjenning@...hat.com>
To: Miroslav Benes <mbenes@...e.cz>
Cc: Josh Poimboeuf <jpoimboe@...hat.com>,
Jiri Kosina <jkosina@...e.cz>,
Vojtech Pavlik <vojtech@...e.cz>,
Steven Rostedt <rostedt@...dmis.org>, jslaby@...e.cz,
pmladek@...e.cz, live-patching@...r.kernel.org, kpatch@...hat.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] kernel: add support for live patching
On Thu, Nov 13, 2014 at 11:16:00AM +0100, Miroslav Benes wrote:
>
> Hi,
>
> thank you for the first version of the united live patching core.
>
> The patch below implements some of our review objections. Changes are
> described in the commit log. It simplifies the hierarchy of data
> structures, removes data duplication (lp_ and lpc_ structures) and
> simplifies sysfs directory.
>
> I did not try to repair other stuff (races, function names, function
> prefix, api symmetry etc.). It should serve as a demonstration of our
> point of view.
>
> There are some problems with this. try_module_get and module_put may be
> called several times for each kernel module where some function is
> patched in. This should be fixed with module going notifier as suggested
> by Petr.
>
> The modified core was tested with modified testing live patch originally
> from Seth's github. It worked as expected.
>
> Please take a look at these changes, so we can discuss them in more
> detail.
Thanks Miroslav.
The functional changes are a little hard to break out from the
formatting changes like s/disable/unregister and s/lp_/lpc_/ or adding
LPC_ prefix to the enum, most (all?) of which I have included for v2.
A problem with getting rid of the object layer is that there are
operations we do that are object-level operations. For example,
module lookup and deferred module patching. Also, the dynamic
relocations need to be associated with an object, not a patch, as not
all relocations will be able to be applied at patch load time for
patches that apply to modules that aren't loaded. I understand that you
can walk the patch-level dynrela table and skip dynrela entries that
don't match the target object, but why do that when you can cleanly
express the relationship with a data structure hierarchy?
One example is the call to is_object_loaded() (renamed and reworked in
v2 btw) per function rather than per object. That is duplicate work and
information that could be more cleanly expressed through an object
layer.
I also understand that sysfs/kobject stuff adds code length. However,
the new "funcs" attribute is procfs style, not sysfs style. sysfs
attribute should convey _one_ value.
>From Documenation/filesystems/sysfs.txt:
==========
Attributes should be ASCII text files, preferably with only one value
per file. It is noted that it may not be efficient to contain only one
value per file, so it is socially acceptable to express an array of
values of the same type.
Mixing types, expressing multiple lines of data, and doing fancy
formatting of data is heavily frowned upon. Doing these things may get
you publicly humiliated and your code rewritten without notice.
==========
Also the function list would have object ambiguity. If there was a
patched function my_func() in both vmlinux and a module, it would just
appear on the list twice. You can fix this by using the mod:func syntax
like kallsyms, but it isn't as clean as expressing it in a hierarchy.
As far as the unification of the API structures with the internal
structures I have two points. First is that, IMHO, we should assume that
the structures coming from the user are const. In kpatch, for example,
we pass through some structures that are not created in the code, but by
the patch generation tool and stored in an ELF section (read-only).
Additionally, I am really against exposing the internal fields.
Commenting them as "internal" is just messy and we have to change the .h
file every time when want to add a field for internal use.
It seems that the primary purpose of this patch is to reduce the lines
of code. However, I think that the object layer of the data structure
cleanly expresses the object<->function relationship and makes code like
the deferred patching much more straightforward since you already have
the functions/dynrelas organized by object. You don't have to do the
nasty "if (strcmp(func->obj_name, objname)) continue;" business over the
entire patch every time.
Be advised, I have also done away with the new_addr/old_addr attributes
for v2 and replaced the patched module ref'ing with a combination of a
GOING notifier with lpc_mutex for protection.
Thanks,
Seth
>
> Best regards,
> --
> Miroslav Benes
> SUSE Labs
>
>
> ----
> From f659a18a630de27b47d375119d793e28ee50da04 Mon Sep 17 00:00:00 2001
> From: Miroslav Benes <mbenes@...e.cz>
> Date: Thu, 13 Nov 2014 10:25:48 +0100
> Subject: [PATCH] lpc: simplification of structure and sysfs hierarchy
>
> Original code has several issues this patch tries to remove.
>
> First, there is only lpc_func structure for patched function and lpc_patch for
> the patch as a whole. Therefore lpc_object structure as middle step of hierarchy
> is removed. Patched function is still associated with some object (vmlinux or
> module) through obj_name. Dynrelas are now in lpc_patch structure and object
> identifier (obj_name) is in the lpc_dynrela to preserve the connection.
>
> Second, sysfs structure is simplified. We do not need to propagate old_addr and
> new_addr. So, there is subdirectory for each patch (patching module) which
> includes original enabled attribute and new one funcs attribute which lists the
> patched functions.
>
> Third, data duplication (lp_ and lpc_ structures) is removed. lpc_ structures
> are now in the header file and made available for the user. This allows us to
> remove almost all the functions for structure allocation in the original code.
>
> Signed-off-by: Miroslav Benes <mbenes@...e.cz>
> ---
> include/linux/livepatch.h | 46 ++--
> kernel/livepatch/core.c | 575 +++++++++++++---------------------------------
> 2 files changed, 191 insertions(+), 430 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index c7a415b..db5ba00 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -2,10 +2,23 @@
> #define _LIVEPATCH_H_
>
> #include <linux/module.h>
> +#include <linux/ftrace.h>
>
> -struct lp_func {
> +enum lpc_state {
> + LPC_DISABLED,
> + LPC_ENABLED
> +};
> +
> +struct lpc_func {
> + /* internal */
> + struct ftrace_ops fops;
> + enum lpc_state state;
> + struct module *mod; /* module associated with patched function */
> + unsigned long new_addr; /* replacement function in patch module */
> +
> + /* external */
> const char *old_name; /* function to be patched */
> - void *new_func; /* replacement function in patch module */
> + void *new_func;
> /*
> * The old_addr field is optional and can be used to resolve
> * duplicate symbol names in the vmlinux object. If this
> @@ -15,31 +28,36 @@ struct lp_func {
> * way to resolve the ambiguity.
> */
> unsigned long old_addr;
> +
> + const char *obj_name; /* "vmlinux" or module name */
> };
>
> -struct lp_dynrela {
> +struct lpc_dynrela {
> unsigned long dest;
> unsigned long src;
> unsigned long type;
> const char *name;
> + const char *obj_name;
> int addend;
> int external;
> };
>
> -struct lp_object {
> - const char *name; /* "vmlinux" or module name */
> - struct lp_func *funcs;
> - struct lp_dynrela *dynrelas;
> -};
> +struct lpc_patch {
> + /* internal */
> + struct list_head list;
> + struct kobject kobj;
> + enum lpc_state state;
>
> -struct lp_patch {
> + /* external */
> struct module *mod; /* module containing the patch */
> - struct lp_object *objs;
> + struct lpc_dynrela *dynrelas;
> + struct lpc_func funcs[];
> };
>
> -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 *);
>
> #endif /* _LIVEPATCH_H_ */
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b32dbb5..feecc22 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -31,78 +31,32 @@
>
> #include <linux/livepatch.h>
>
> +#define lpc_for_each_patch_func(p, pf) \
> + for (pf = p->funcs; pf->old_name; pf++)
> +
> /*************************************
> * 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_SEMAPHORE(lpc_mutex);
> static LIST_HEAD(lpc_patches);
>
> -enum lpc_state {
> - DISABLED,
> - 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_dynrela *dynrelas;
> -};
> -
> -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
> *******************************************/
>
> -/* sets obj->mod if object is not vmlinux and module was found */
> -static bool is_object_loaded(struct lpc_object *obj)
> +/* sets patch_func->mod if object is not vmlinux and module was found */
> +static bool is_object_loaded(struct lpc_func *patch_func)
> {
> struct module *mod;
>
> - if (!strcmp(obj->name, "vmlinux"))
> + if (!strcmp(patch_func->obj_name, "vmlinux"))
> return 1;
>
> mutex_lock(&module_mutex);
> - mod = find_module(obj->name);
> + mod = find_module(patch_func->obj_name);
> mutex_unlock(&module_mutex);
> - obj->mod = mod;
> + patch_func->mod = mod;
>
> return !!mod;
> }
> @@ -254,18 +208,18 @@ static int lpc_find_external_symbol(struct module *pmod, const char *name,
> return lpc_find_symbol(pmod->name, name, addr);
> }
>
> -static int lpc_write_object_relocations(struct module *pmod,
> - struct lpc_object *obj)
> +static int lpc_write_relocations(struct module *pmod,
> + struct lpc_dynrela *patch_dynrelas)
> {
> int ret, size, readonly = 0, numpages;
> - struct lp_dynrela *dynrela;
> + struct lpc_dynrela *dynrela;
> u64 loc, val;
> unsigned long core = (unsigned long)pmod->module_core;
> unsigned long core_ro_size = pmod->core_ro_size;
> unsigned long core_size = pmod->core_size;
>
> - for (dynrela = obj->dynrelas; dynrela->name; dynrela++) {
> - if (!strcmp(obj->name, "vmlinux")) {
> + for (dynrela = patch_dynrelas; dynrela->name; dynrela++) {
> + if (!strcmp(dynrela->obj_name, "vmlinux")) {
> ret = lpc_verify_vmlinux_symbol(dynrela->name,
> dynrela->src);
> if (ret)
> @@ -277,7 +231,7 @@ static int lpc_write_object_relocations(struct module *pmod,
> dynrela->name,
> &dynrela->src);
> else
> - ret = lpc_find_symbol(obj->mod->name,
> + ret = lpc_find_symbol(dynrela->obj_name,
> dynrela->name,
> &dynrela->src);
> if (ret)
> @@ -357,7 +311,7 @@ static int lpc_enable_func(struct lpc_func *func)
> int ret;
>
> BUG_ON(!func->old_addr);
> - BUG_ON(func->state != DISABLED);
> + BUG_ON(func->state != LPC_DISABLED);
> ret = ftrace_set_filter_ip(&func->fops, func->old_addr, 0, 0);
> if (ret) {
> pr_err("failed to set ftrace filter for function '%s' (%d)\n",
> @@ -370,16 +324,16 @@ static int lpc_enable_func(struct lpc_func *func)
> func->old_name, ret);
> ftrace_set_filter_ip(&func->fops, func->old_addr, 1, 0);
> } else
> - func->state = ENABLED;
> + func->state = LPC_ENABLED;
>
> return ret;
> }
>
> -static int lpc_unregister_func(struct lpc_func *func)
> +static int lpc_disable_func(struct lpc_func *func)
> {
> int ret;
>
> - BUG_ON(func->state != ENABLED);
> + BUG_ON(func->state != LPC_ENABLED);
> if (!func->old_addr)
> /* parent object is not loaded */
> return 0;
> @@ -392,173 +346,131 @@ static int lpc_unregister_func(struct lpc_func *func)
> ret = ftrace_set_filter_ip(&func->fops, func->old_addr, 1, 0);
> if (ret)
> pr_warn("function unregister succeeded but failed to clear the filter\n");
> - func->state = DISABLED;
> + func->state = LPC_DISABLED;
>
> return 0;
> }
>
> -static int lpc_unregister_object(struct lpc_object *obj)
> -{
> - struct lpc_func *func;
> - int ret;
> -
> - list_for_each_entry(func, &obj->funcs, list) {
> - if (func->state != ENABLED)
> - continue;
> - ret = lpc_unregister_func(func);
> - if (ret)
> - return ret;
> - if (strcmp(obj->name, "vmlinux"))
> - func->old_addr = 0;
> - }
> - if (obj->mod)
> - module_put(obj->mod);
> - obj->state = DISABLED;
> -
> - return 0;
> -}
> -
> -/* caller must ensure that obj->mod is set if object is a module */
> -static int lpc_enable_object(struct module *pmod, struct lpc_object *obj)
> -{
> - struct lpc_func *func;
> - int ret;
> -
> - if (obj->mod && !try_module_get(obj->mod))
> - return -ENODEV;
> -
> - if (obj->dynrelas) {
> - ret = lpc_write_object_relocations(pmod, obj);
> - if (ret)
> - goto unregister;
> - }
> - list_for_each_entry(func, &obj->funcs, list) {
> - ret = lpc_find_verify_func_addr(func, obj->name);
> - if (ret)
> - goto unregister;
> -
> - ret = lpc_enable_func(func);
> - if (ret)
> - goto unregister;
> - }
> - obj->state = ENABLED;
> -
> - return 0;
> -unregister:
> - WARN_ON(lpc_unregister_object(obj));
> - return ret;
> -}
> -
> /******************************
> * enable/disable
> ******************************/
>
> /* must be called with lpc_mutex held */
> -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;
> -}
> -
> -/* must be called with lpc_mutex held */
> -static int lpc_disable_patch(struct lpc_patch *patch)
> +static int __lpc_disable_patch(struct lpc_patch *patch)
> {
> - struct lpc_object *obj;
> + struct lpc_func *patch_func;
> int ret;
>
> pr_notice("disabling patch '%s'\n", patch->mod->name);
>
> - list_for_each_entry(obj, &patch->objs, list) {
> - if (obj->state != ENABLED)
> + lpc_for_each_patch_func(patch, patch_func) {
> + if (patch_func->state != LPC_ENABLED)
> continue;
> - ret = lpc_unregister_object(obj);
> - if (ret)
> + ret = lpc_disable_func(patch_func);
> + if (ret) {
> + pr_err("lpc: cannot disable function %s\n",
> + patch_func->old_name);
> return ret;
> + }
> +
> + if (strcmp(patch_func->obj_name, "vmlinux"))
> + patch_func->old_addr = 0;
> + if (patch_func->mod)
> + module_put(patch_func->mod);
> }
> - patch->state = DISABLED;
> + patch->state = LPC_DISABLED;
>
> return 0;
> }
>
> -int lp_disable_patch(struct lp_patch *userpatch)
> +int lpc_disable_patch(struct lpc_patch *patch)
> {
> - struct lpc_patch *patch;
> int ret;
>
> down(&lpc_mutex);
> - patch = lpc_find_patch(userpatch);
> - if (!patch) {
> - ret = -ENODEV;
> - goto out;
> - }
> - ret = lpc_disable_patch(patch);
> -out:
> + ret = __lpc_disable_patch(patch);
> up(&lpc_mutex);
> +
> return ret;
> }
> -EXPORT_SYMBOL_GPL(lp_disable_patch);
> +EXPORT_SYMBOL_GPL(lpc_disable_patch);
> +
> +static int lpc_verify_enable_func(struct lpc_func *patch_func)
> +{
> + int ret;
> +
> + if (patch_func->mod && !try_module_get(patch_func->mod))
> + return -ENODEV;
> +
> + ret = lpc_find_verify_func_addr(patch_func, patch_func->obj_name);
> + if (ret)
> + return ret;
> +
> + ret = lpc_enable_func(patch_func);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
>
> /* must be called with lpc_mutex held */
> -static int lpc_enable_patch(struct lpc_patch *patch)
> +static int __lpc_enable_patch(struct lpc_patch *patch)
> {
> - struct lpc_object *obj;
> + struct lpc_func *patch_func;
> int ret;
>
> - BUG_ON(patch->state != DISABLED);
> + BUG_ON(patch->state != LPC_DISABLED);
>
> pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
> add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
>
> pr_notice("enabling patch '%s'\n", patch->mod->name);
>
> - list_for_each_entry(obj, &patch->objs, list) {
> - if (!is_object_loaded(obj))
> + if (patch->dynrelas) {
> + ret = lpc_write_relocations(patch->mod, patch->dynrelas);
> + if (ret)
> + goto err;
> + }
> +
> + lpc_for_each_patch_func(patch, patch_func) {
> + if (!is_object_loaded(patch_func))
> continue;
> - ret = lpc_enable_object(patch->mod, obj);
> +
> + ret = lpc_verify_enable_func(patch_func);
> if (ret)
> - goto unregister;
> + goto err;
> }
> - patch->state = ENABLED;
> + patch->state = LPC_ENABLED;
> +
> return 0;
>
> -unregister:
> - WARN_ON(lpc_disable_patch(patch));
> +err:
> + WARN_ON(__lpc_disable_patch(patch));
> return ret;
> }
>
> -int lp_enable_patch(struct lp_patch *userpatch)
> +int lpc_enable_patch(struct lpc_patch *patch)
> {
> - struct lpc_patch *patch;
> int ret;
>
> down(&lpc_mutex);
> - patch = lpc_find_patch(userpatch);
> - if (!patch) {
> - ret = -ENODEV;
> - goto out;
> - }
> - ret = lpc_enable_patch(patch);
> -out:
> + ret = __lpc_enable_patch(patch);
> up(&lpc_mutex);
> +
> return ret;
> }
> -EXPORT_SYMBOL_GPL(lp_enable_patch);
> +EXPORT_SYMBOL_GPL(lpc_enable_patch);
>
> /******************************
> * module notifier
> *****************************/
>
> -static int lp_module_notify(struct notifier_block *nb, unsigned long action,
> +static int lpc_module_notify(struct notifier_block *nb, unsigned long action,
> void *data)
> {
> struct module *mod = data;
> struct lpc_patch *patch;
> - struct lpc_object *obj;
> + struct lpc_func *patch_func;
> int ret = 0;
>
> if (action != MODULE_STATE_COMING)
> @@ -567,32 +479,42 @@ static int lp_module_notify(struct notifier_block *nb, unsigned long action,
> down(&lpc_mutex);
>
> list_for_each_entry(patch, &lpc_patches, list) {
> - if (patch->state == DISABLED)
> + if (patch->state == LPC_DISABLED)
> continue;
> - list_for_each_entry(obj, &patch->objs, list) {
> - if (strcmp(obj->name, mod->name))
> +
> + if (patch->dynrelas) {
> + ret = lpc_write_relocations(patch->mod,
> + patch->dynrelas);
> + if (ret)
> + goto err;
> + }
> +
> + lpc_for_each_patch_func(patch, patch_func) {
> + if (strcmp(patch_func->obj_name, mod->name))
> continue;
> +
> pr_notice("load of module '%s' detected, applying patch '%s'\n",
> mod->name, patch->mod->name);
> - obj->mod = mod;
> - ret = lpc_enable_object(patch->mod, obj);
> + patch_func->mod = mod;
> +
> + ret = lpc_verify_enable_func(patch_func);
> if (ret)
> - goto out;
> - break;
> + goto err;
> }
> }
>
> up(&lpc_mutex);
> return 0;
> -out:
> +
> +err:
> up(&lpc_mutex);
> WARN("failed to apply patch '%s' to module '%s'\n",
> patch->mod->name, mod->name);
> return 0;
> }
>
> -static struct notifier_block lp_module_nb = {
> - .notifier_call = lp_module_notify,
> +static struct notifier_block lpc_module_nb = {
> + .notifier_call = lpc_module_notify,
> .priority = INT_MIN, /* called last */
> };
>
> @@ -603,10 +525,7 @@ static struct notifier_block lp_module_nb = {
> * /sys/kernel/livepatch
> * /sys/kernel/livepatch/<patch>
> * /sys/kernel/livepatch/<patch>/enabled
> - * /sys/kernel/livepatch/<patch>/<object>
> - * /sys/kernel/livepatch/<patch>/<object>/<func>
> - * /sys/kernel/livepatch/<patch>/<object>/<func>/new_addr
> - * /sys/kernel/livepatch/<patch>/<object>/<func>/old_addr
> + * /sys/kernel/livepatch/<patch>/funcs
> */
>
> static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -620,7 +539,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> if (ret)
> return -EINVAL;
>
> - if (val != DISABLED && val != ENABLED)
> + if (val != LPC_DISABLED && val != LPC_ENABLED)
> return -EINVAL;
>
> patch = container_of(kobj, struct lpc_patch, kobj);
> @@ -632,12 +551,12 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> goto out;
> }
>
> - if (val == ENABLED) {
> - ret = lpc_enable_patch(patch);
> + if (val == LPC_ENABLED) {
> + ret = __lpc_enable_patch(patch);
> if (ret)
> goto out;
> } else {
> - ret = lpc_disable_patch(patch);
> + ret = __lpc_disable_patch(patch);
> if (ret)
> goto out;
> }
> @@ -657,40 +576,35 @@ static ssize_t enabled_show(struct kobject *kobj,
> return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->state);
> }
>
> -static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
> -static struct attribute *lpc_patch_attrs[] = {
> - &enabled_kobj_attr.attr,
> - NULL
> -};
> -
> -static ssize_t new_addr_show(struct kobject *kobj,
> +static ssize_t funcs_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> - struct lpc_func *func;
> -
> - func = container_of(kobj, struct lpc_func, kobj);
> - return snprintf(buf, PAGE_SIZE-1, "0x%016lx\n", func->new_addr);
> -}
> + struct lpc_patch *patch;
> + const struct lpc_func *patch_func;
> + ssize_t size;
>
> -static struct kobj_attribute new_addr_kobj_attr = __ATTR_RO(new_addr);
> + size = snprintf(buf, PAGE_SIZE, "Functions:\n");
>
> -static ssize_t old_addr_show(struct kobject *kobj,
> - struct kobj_attribute *attr, char *buf)
> -{
> - struct lpc_func *func;
> + patch = container_of(kobj, struct lpc_patch, kobj);
> + lpc_for_each_patch_func(patch, patch_func)
> + size += snprintf(buf + size, PAGE_SIZE - size, "%s\n",
> + patch_func->old_name);
>
> - func = container_of(kobj, struct lpc_func, kobj);
> - return snprintf(buf, PAGE_SIZE-1, "0x%016lx\n", func->old_addr);
> + return size;
> }
>
> -static struct kobj_attribute old_addr_kobj_attr = __ATTR_RO(old_addr);
> -
> -static struct attribute *lpc_func_attrs[] = {
> - &new_addr_kobj_attr.attr,
> - &old_addr_kobj_attr.attr,
> +static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
> +static struct kobj_attribute funcs_kobj_attr = __ATTR_RO(funcs);
> +static struct attribute *lpc_patch_attrs[] = {
> + &enabled_kobj_attr.attr,
> + &funcs_kobj_attr.attr,
> NULL
> };
>
> +static struct attribute_group lpc_patch_sysfs_group = {
> + .attrs = lpc_patch_attrs,
> +};
> +
> static struct kobject *lpc_root_kobj;
>
> static int lpc_create_root_kobj(void)
> @@ -720,228 +634,67 @@ static void lpc_kobj_release_patch(struct kobject *kobj)
> static struct kobj_type lpc_ktype_patch = {
> .release = lpc_kobj_release_patch,
> .sysfs_ops = &kobj_sysfs_ops,
> - .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,
> - .default_attrs = lpc_func_attrs
> };
>
> /*********************************
> - * structure allocation
> + * structure init and free
> ********************************/
>
> -static void lpc_free_funcs(struct lpc_object *obj)
> -{
> - struct lpc_func *func, *funcsafe;
> -
> - list_for_each_entry_safe(func, funcsafe, &obj->funcs, list)
> - kobject_put(&func->kobj);
> -}
> -
> -static void lpc_free_objects(struct lpc_patch *patch)
> -{
> - struct lpc_object *obj, *objsafe;
> -
> - list_for_each_entry_safe(obj, objsafe, &patch->objs, list) {
> - lpc_free_funcs(obj);
> - kobject_put(&obj->kobj);
> - }
> -}
> -
> static void lpc_free_patch(struct lpc_patch *patch)
> {
> - lpc_free_objects(patch);
> + sysfs_remove_group(&patch->kobj, &lpc_patch_sysfs_group);
> kobject_put(&patch->kobj);
> }
>
> -static struct lpc_func *lpc_create_func(struct kobject *root,
> - struct lp_func *userfunc)
> +static int lpc_init_patch(struct lpc_patch *patch)
> {
> - struct lpc_func *func;
> struct ftrace_ops *ops;
> + struct lpc_func *patch_func;
> 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 = 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)
> - goto free;
> - list_add(&func->list, &obj->funcs);
> - }
> - return 0;
> -free:
> - lpc_free_funcs(obj);
> - return -ENOMEM;
> -}
> -
> -static struct lpc_object *lpc_create_object(struct kobject *root,
> - struct lp_object *userobj)
> -{
> - struct lpc_object *obj;
> - int ret;
> -
> - /* alloc */
> - obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> - if (!obj)
> - return NULL;
> -
> - /* init */
> - INIT_LIST_HEAD(&obj->list);
> - obj->name = userobj->name;
> - obj->dynrelas = userobj->dynrelas;
> - obj->state = DISABLED;
> - /* obj->mod set by is_object_loaded() */
> - 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;
> -
> - for (userobj = userobjs; userobj->name; userobj++) {
> - obj = lpc_create_object(&patch->kobj, userobj);
> - if (!obj)
> - goto free;
> - list_add(&obj->list, &patch->objs);
> - }
> - return 0;
> -free:
> - lpc_free_objects(patch);
> - return -ENOMEM;
> -}
> -
> -static int lpc_create_patch(struct lp_patch *userpatch)
> -{
> - struct lpc_patch *patch;
> - int ret;
> -
> - /* alloc */
> - patch = kzalloc(sizeof(*patch), GFP_KERNEL);
> - if (!patch)
> - return -ENOMEM;
> -
> - /* init */
> - INIT_LIST_HEAD(&patch->list);
> - patch->userpatch = userpatch;
> - patch->mod = userpatch->mod;
> - patch->state = DISABLED;
> - INIT_LIST_HEAD(&patch->objs);
> + patch->state = LPC_DISABLED;
>
> /* sysfs */
> ret = kobject_init_and_add(&patch->kobj, &lpc_ktype_patch,
> lpc_root_kobj, patch->mod->name);
> - if (ret) {
> - kfree(patch);
> - return ret;
> - }
> + if (ret)
> + goto err_root;
>
> - /* create objects */
> - ret = lpc_create_objects(patch, userpatch->objs);
> - if (ret) {
> - kobject_put(&patch->kobj);
> - return ret;
> + /* create functions */
> + lpc_for_each_patch_func(patch, patch_func) {
> + patch_func->new_addr = (unsigned long)patch_func->new_func;
> + patch_func->state = LPC_DISABLED;
> + ops = &patch_func->fops;
> + ops->private = patch_func;
> + ops->func = lpc_ftrace_handler;
> + ops->flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_DYNAMIC;
> }
>
> + ret = sysfs_create_group(&patch->kobj, &lpc_patch_sysfs_group);
> + if (ret)
> + goto err_patch;
> +
> /* add to global list of patches */
> list_add(&patch->list, &lpc_patches);
>
> return 0;
> +
> +err_patch:
> + kobject_put(&patch->kobj);
> +err_root:
> + return ret;
> }
>
> /************************************
> * register/unregister
> ***********************************/
>
> -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 || !userpatch->funcs)
> return -EINVAL;
>
> /*
> @@ -955,36 +708,26 @@ int lp_register_patch(struct lp_patch *userpatch)
> return -ENODEV;
>
> down(&lpc_mutex);
> - ret = lpc_create_patch(userpatch);
> + ret = lpc_init_patch(userpatch);
> up(&lpc_mutex);
> if (ret)
> module_put(userpatch->mod);
>
> return ret;
> }
> -EXPORT_SYMBOL_GPL(lp_register_patch);
> +EXPORT_SYMBOL_GPL(lpc_register_patch);
>
> -int lp_unregister_patch(struct lp_patch *userpatch)
> +int lpc_unregister_patch(struct lpc_patch *userpatch)
> {
> - struct lpc_patch *patch;
> int ret = 0;
>
> down(&lpc_mutex);
> - patch = lpc_find_patch(userpatch);
> - if (!patch) {
> - ret = -ENODEV;
> - goto out;
> - }
> - if (patch->state == ENABLED) {
> - ret = -EINVAL;
> - goto out;
> - }
> - lpc_free_patch(patch);
> -out:
> + lpc_free_patch(userpatch);
> up(&lpc_mutex);
> +
> return ret;
> }
> -EXPORT_SYMBOL_GPL(lp_unregister_patch);
> +EXPORT_SYMBOL_GPL(lpc_unregister_patch);
>
> /************************************
> * entry/exit
> @@ -994,7 +737,7 @@ static int lpc_init(void)
> {
> int ret;
>
> - ret = register_module_notifier(&lp_module_nb);
> + ret = register_module_notifier(&lpc_module_nb);
> if (ret)
> return ret;
>
> @@ -1004,14 +747,14 @@ static int lpc_init(void)
>
> return 0;
> unregister:
> - unregister_module_notifier(&lp_module_nb);
> + unregister_module_notifier(&lpc_module_nb);
> return ret;
> }
>
> static void lpc_exit(void)
> {
> lpc_remove_root_kobj();
> - unregister_module_notifier(&lp_module_nb);
> + unregister_module_notifier(&lpc_module_nb);
> }
>
> module_init(lpc_init);
> --
> 2.1.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists