Message-ID: <20141120173552.GA20462@treble.redhat.com>
Date:	Thu, 20 Nov 2014 11:35:52 -0600
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Miroslav Benes <mbenes@...e.cz>
Cc:	Seth Jennings <sjenning@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Steven Rostedt <rostedt@...dmis.org>,
	Petr Mladek <pmladek@...e.cz>,
	Christoph Hellwig <hch@...radead.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Andy Lutomirski <luto@...capital.net>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	live-patching@...r.kernel.org, x86@...nel.org, kpatch@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 2/3] kernel: add support for live patching

On Thu, Nov 20, 2014 at 02:10:33PM +0100, Miroslav Benes wrote:
> 
> On Sun, 16 Nov 2014, Seth Jennings wrote:
> 
> > This commit introduces code for the live patching core.  It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.
> > 
> > It represents the greatest common functionality set between kpatch and
> > kgraft and can accept patches built using either method.
> > 
> > This first version does not implement any consistency mechanism that
> > ensures that old and new code do not run together.  In practice, ~90% of
> > CVEs are safe to apply in this way, since they simply add a conditional
> > check.  However, any function change that can not execute safely with
> > the old version of the function can _not_ be safely applied in this
> > version.
> > 
> > Signed-off-by: Seth Jennings <sjenning@...hat.com>
> 
> Hi,
> 
> below is the patch which merges the internal and external data structures 
> (so it is only one part of our original patch for version 1). Apart from 
> that I tried to make minimal changes to the code. Only unnecessary 
> kobjects were removed and I renamed lpc_create_* functions to lpc_init_* 
> as it made more sense in this approach, I think.
> 
> I hope this clearly shows our point of view stated previously. What do 
> you say?

Thanks for rebasing to v2 and splitting up the patches!  Personally I'm
ok with this patch (though I do have a few comments below).

> Next, I'll look at the three level hierarchy and sysfs directory and see 
> if we can make it simpler yet keep its advantages.
> 
> Regards,
> 
> Miroslav Benes
> SUSE Labs
> 
> -- >8 --
> From aba839eb6b3292b193843715bfce7834969c0c17 Mon Sep 17 00:00:00 2001
> From: Miroslav Benes <mbenes@...e.cz>
> Date: Wed, 19 Nov 2014 16:06:35 +0100
> Subject: [PATCH] Remove the data duplication in internal and public structures
> 
> 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@...e.cz>
> ---
>  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 */

Would it be a little clearer to list the external fields first?

> +	struct kobject *kobj;
> +	struct ftrace_ops fops;
> +	enum lpc_state state;
> +	unsigned long new_addr;

I think this internal new_addr field isn't really needed since we
already have the external new_func field.

> +
> +	/* 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;

FTRACE_OPS_FL_DYNAMIC is probably no longer appropriate here, since I
think both kGraft and kpatch allocate the func struct statically.

However, we can't know for sure how the user allocated it, so maybe we
need to change funcs->fops to be a pointer, and allocate it here?

That may also require embedding kobj into the func struct so that ops
can be freed with lpc_kobj_release_func.

> -
> -	/* 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;

Would be nice if we could detect a double call to lpc_register() and
return -EINVAL if the patch is already enabled (but I'm not sure how to
do that with this data layout).  Probably not a big deal.


> -	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

s/Allocates/Initializes/

> @@ -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)

We can probably s/userpatch/patch/g .

>  {
>  	int ret;
>  
> -	if (!userpatch || !userpatch->mod || !userpatch->objs)
> +	if (!userpatch || !userpatch->mod)

Why?

>  		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
> 

-- 
Josh
--
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