Re: [PATCH] livepatch: clean up klp_find_object_module() usage: was: Re: [PATCHv4 2/3] kernel: add support for live patching

[Miroslav Benes]

On Fri, 28 Nov 2014, Petr Mladek wrote:

> On Fri 2014-11-28 18:07:37, Petr Mladek wrote:
> > On Tue 2014-11-25 11:15:08, 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.
> 
> [...]
> 
> > > +/* sets obj->mod if object is not vmlinux and module is found */
> > > +static bool klp_find_object_module(struct klp_object *obj)
> > > +{
> > > +	if (!obj->name)
> > > +		return 1;
> > > +
> > > +	mutex_lock(&module_mutex);
> > > +	/*
> > > +	 * We don't need to take a reference on the module here because we have
> > > +	 * the klp_mutex, which is also taken by the module notifier.  This
> > > +	 * prevents any module from unloading until we release the klp_mutex.
> > > +	 */
> > > +	obj->mod = find_module(obj->name);
> > > +	mutex_unlock(&module_mutex);
> > > +
> > > +	return !!obj->mod;
> > 
> > I know that this is effective to return boolean here because
> > it handles also patch against the kernel core (vmlinux). But
> > the logic looks tricky. I would prefer a cleaner design and
> > use this function only to set obj->mod.
> > 
> > I wanted to see how it would look like, so I will send a patch for
> > this in a separate mail.
> 
> The patch is below. Of course, merge it into the original
> patch if you agree with the idea, please.
> 
> 
> >From 93eb9f9a25ad8aa0301e246f7685d3e787037566 Mon Sep 17 00:00:00 2001
> From: Petr Mladek <pmladek@xxxxxxx>
> Date: Fri, 28 Nov 2014 15:32:27 +0100
> Subject: [PATCH 1/2] livepatch: clean up klp_find_object_module() usage
> 
> The function klp_find_object_module() looks quite tricky. It has two effects:
> sets obj->mod and decides whether the module is available or not. The second
> effect is the tricky part because it handles also the code kernel code "vmlinux"
> and is not module related. It causes returning bool, and doing the crazy double
> negation.
> 
> This patch tries to make a bit cleaner design:
> 
> 1. klp_find_object_module() handles only obj->mod. It returns
>    the pointer or NULL.
> 
> 2. It modifies klp_enable_object() to do nothing when the related
>    module has not been loaded yet.
> 
> 3. The result is that the return value klp_find_object_module() is
>    not used in the end.
> 
> We lose a check for potential klp_enable_object() misuse but it makes the code
> easier. In fact, the check for unloaded module is rather long. We might want
> to make it easier using some extra flag or another state of the object.
> Such flag might be used for the check of misuse.
> 
> Signed-off-by: Petr Mladek <pmladek@xxxxxxx>

Hi, 

I agree with the idea but actually don't like the implementation. I'll try 
to propose few changes which would hopefully preserve the effect but make 
the end result slightly better.

> ---
>  kernel/livepatch/core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 8e2e8cd242f5..9b1601729014 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -40,10 +40,10 @@ static LIST_HEAD(klp_patches);
>  static struct kobject *klp_root_kobj;
>  
>  /* sets obj->mod if object is not vmlinux and module is found */
> -static bool klp_find_object_module(struct klp_object *obj)
> +static struct module *klp_find_object_module(struct klp_object *obj)
>  {
>  	if (!obj->name)
> -		return 1;
> +		return NULL;
>  
>  	mutex_lock(&module_mutex);
>  	/*
> @@ -54,7 +54,7 @@ static bool klp_find_object_module(struct klp_object *obj)
>  	obj->mod = find_module(obj->name);
>  	mutex_unlock(&module_mutex);
>  
> -	return !!obj->mod;
> +	return obj->mod;
>  }

As we do not need the return value in the end, we could maybe drop it 
completely, leading to change in if condition

	if (!obj->name)
		return;

>  struct klp_find_arg {
> @@ -318,8 +318,9 @@ static int klp_enable_object(struct module *pmod, struct klp_object *obj)
>  	struct klp_func *func;
>  	int ret;
>  
> -	if (WARN_ON(!obj->mod && obj->name))
> -		return -EINVAL;
> +	/* nope when the patched module has not been loaded yet */
> +	if (obj->name && !obj->mod)
> +		return 0;

I have a problem with this one. In case the condition is true we do 
nothing, return back and pretend that everything is alright (return 0). 
I see that we would call klp_enable_object everytime in your proposal and 
decide here whether we want to do something or not. But I think that we 
should return some error and deal with it in the caller function. Thus 
original WARN_ON should stay here.

>  	if (obj->relocs) {
>  		ret = klp_write_object_relocations(pmod, obj);
> @@ -401,8 +402,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
>  	pr_notice("enabling patch '%s'\n", patch->mod->name);
>  
>  	for (obj = patch->objs; obj->funcs; obj++) {
> -		if (!klp_find_object_module(obj))
> -			continue;
> +		klp_find_object_module(obj);
>  		ret = klp_enable_object(patch->mod, obj);
>  		if (ret)
>  			goto unregister;

I propose this piece of code

for (obj = patch->objs; obj->funcs; obj++) {
	klp_find_object_module(obj);
	if (obj->name && !obj->mod)
		continue;
	ret = klp_enable_object(patch->mod, obj);
	...
}

What do you think? Also it could pay off to define inline function for the 
check. Somethink like klp_is_module_and_loaded...

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