Re: [PATCHv2 2/3] kernel: add support for live patching

[Seth Jennings]

On Thu, Nov 20, 2014 at 11:35:52AM -0600, Josh Poimboeuf wrote:
> 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@xxxxxxxxxx>
> > 
> > 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).

Thanks Josh :)

Miroslav, before you send out a revision on this patch, I'm merging it
for v3 right now.  I'll fixup any trivial fixes from this email.

I'm putting the finishing touches on v3 now.  Hopefully it will make
everyone happy, or happier, with your changes merged.  Should be getting
close...

Thanks,
Seth

> 
> > 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@xxxxxxx>
> > 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@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 */
> 
> 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 live-patching" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/