Re: [PATCH 5/6] livepatch v5: split init and free code that is done only for loaded modules

[Petr Mladek]

On Tue 2014-12-09 12:51:41, Josh Poimboeuf wrote:
> On Tue, Dec 09, 2014 at 07:05:06PM +0100, Petr Mladek wrote:
> > This patch makes it clear what initialization and freeing steps need to be done
> > when an object (module) is being loaded or removed. It will help to maintain
> > the module coming and going handlers. Also it will remove duplicated
> > code from these handlers.
> > 
> > Signed-off-by: Petr Mladek <pmladek@xxxxxxx>
> > ---
> >  kernel/livepatch/core.c | 92 ++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 61 insertions(+), 31 deletions(-)
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 97a8d4a3d6d8..fe312b9ada78 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -590,6 +590,12 @@ static struct kobj_type klp_ktype_func = {
> >  	.sysfs_ops = &kobj_sysfs_ops,
> >  };
> >  
> > +/* Clean up when a patched object is unloaded */
> > +static void klp_free_func_loaded(struct klp_func *func)
> > +{
> > +	func->old_addr = 0;
> > +}
> > +
> >  /*
> >   * Free all functions' kobjects in the array up to some limit. When limit is
> >   * NULL, all kobjects are freed.
> > @@ -603,6 +609,17 @@ static void klp_free_funcs_limited(struct klp_object *obj,
> >  		kobject_put(&func->kobj);
> >  }
> >  
> > +/* Clean up when a patched object is unloaded */
> > +static void klp_free_object_loaded(struct klp_object *obj)
> > +{
> > +	struct klp_func *func;
> > +
> > +	obj->mod = NULL;
> > +
> > +	for (func = obj->funcs; func->old_name; func++)
> > +		klp_free_func_loaded(func);
> > +}
> > +
> >  /*
> >   * Free all objects' kobjects in the array up to some limit. When limit is
> >   * NULL, all kobjects are freed.
> > @@ -626,6 +643,12 @@ static void klp_free_patch(struct klp_patch *patch)
> >  	kobject_put(&patch->kobj);
> >  }
> >  
> > +/* parts of the initialization that is done only when the object is loaded */
> > +static int klp_init_func_loaded(struct klp_object *obj, struct klp_func *func)
> > +{
> > +	return klp_find_verify_func_addr(obj, func);
> > +}
> > +
> 
> Creating a new function here for one line of code, which is only called
> once, seems excessive, and makes the code harder to understand IMO.

I see your point. Well, note that the split code is code is called
twice from klp_init_object() and klp_module_coming(), so it helps
to remove the code duplicity.

Also it clearly separates the operations that are always done
and that are done only when the object is loaded.

If you suggest to call klp_find_verify_func_addr() directly
from klp_init_object_loaded(), I am fine with it. We could always
create it later if there are more operations needed for
struct func.


> Ditto for klp_free_func_loaded.

Note that there will be two lines if we agree to add old_ip into
struct klp_func.

If it handles only old_addr, I agree with moving the one line to
klp_free_object_loaded().

Best Regards,
Petr

> >  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >  {
> >  	struct ftrace_ops *ops;
> > @@ -633,10 +656,6 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >  
> >  	func->state = KLP_DISABLED;
> >  
> > -	ret = klp_find_verify_func_addr(obj, func);
> > -	if (ret)
> > -		return ret;
> > -
> >  	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> >  	if (!ops)
> >  		ret = -ENOMEM;
> > @@ -656,6 +675,28 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >  	return 0;
> >  }
> >  
> > +/* parts of the initialization that is done only when the object is loaded */
> > +static int klp_init_object_loaded(struct klp_patch *patch,
> > +				  struct klp_object *obj)
> > +{
> > +	struct klp_func *func;
> > +	int ret;
> > +
> > +	if (obj->relocs) {
> > +		ret = klp_write_object_relocations(patch->mod, obj);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	for (func = obj->funcs; func->old_name; func++) {
> > +		ret = klp_init_func_loaded(obj, func);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> >  {
> >  	struct klp_func *func;
> > @@ -669,12 +710,6 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> >  
> >  	klp_find_object_module(obj);
> >  
> > -	if (obj->relocs && klp_is_object_loaded(obj)) {
> > -		ret = klp_write_object_relocations(patch->mod, obj);
> > -		if (ret)
> > -			return ret;
> > -	}
> > -
> >  	name = klp_is_module(obj) ? obj->name : "vmlinux";
> >  	obj->kobj = kobject_create_and_add(name, &patch->kobj);
> >  	if (!obj->kobj)
> > @@ -686,6 +721,12 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> >  			goto free;
> >  	}
> >  
> > +	if (klp_is_object_loaded(obj)) {
> > +		ret = klp_init_object_loaded(patch, obj);
> > +		if (ret)
> > +			goto free;
> > +	}
> > +
> >  	return 0;
> >  
> >  free:
> > @@ -802,27 +843,19 @@ int klp_register_patch(struct klp_patch *patch)
> >  }
> >  EXPORT_SYMBOL_GPL(klp_register_patch);
> >  
> > -static void klp_module_notify_coming(struct module *pmod,
> > +static void klp_module_notify_coming(struct klp_patch *patch,
> >  				     struct klp_object *obj)
> >  {
> > -	struct klp_func *func;
> > +	struct module *pmod = patch->mod;
> >  	struct module *mod = obj->mod;
> >  	int ret;
> >  
> >  	pr_notice("applying patch '%s' to loading module '%s'\n",
> >  		  pmod->name, mod->name);
> >  
> > -	if (obj->relocs) {
> > -		ret = klp_write_object_relocations(pmod, obj);
> > -		if (ret)
> > -			goto err;
> > -	}
> > -
> > -	for (func = obj->funcs; func->old_name; func++) {
> > -		ret = klp_find_verify_func_addr(obj, func);
> > -		if (ret)
> > -			goto err;
> > -	}
> > +	ret = klp_init_object_loaded(patch, obj);
> > +	if (ret)
> > +		goto err;
> >  
> >  	ret = klp_enable_object(obj);
> >  	if (!ret)
> > @@ -833,10 +866,10 @@ err:
> >  		pmod->name, mod->name, ret);
> >  }
> >  
> > -static void klp_module_notify_going(struct module *pmod,
> > +static void klp_module_notify_going(struct klp_patch *patch,
> >  				    struct klp_object *obj)
> >  {
> > -	struct klp_func *func;
> > +	struct module *pmod = patch->mod;
> >  	struct module *mod = obj->mod;
> >  	int ret;
> >  
> > @@ -848,10 +881,7 @@ static void klp_module_notify_going(struct module *pmod,
> >  		pr_warn("failed to revert patch '%s' on module '%s' (%d)\n",
> >  			pmod->name, mod->name, ret);
> >  
> > -	for (func = obj->funcs; func->old_name; func++)
> > -		func->old_addr = 0;
> > -
> > -	obj->mod = NULL;
> > +	klp_free_object_loaded(obj);
> >  }
> >  
> >  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > @@ -876,9 +906,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> >  
> >  			if (action == MODULE_STATE_COMING) {
> >  				obj->mod = mod;
> > -				klp_module_notify_coming(patch->mod, obj);
> > +				klp_module_notify_coming(patch, obj);
> >  			} else /* MODULE_STATE_GOING */
> > -				klp_module_notify_going(patch->mod, obj);
> > +				klp_module_notify_going(patch, obj);
> >  
> >  			break;
> >  		}
> > -- 
> > 1.8.5.2
> > 
> 
> -- 
> Josh
--
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/