linux-kernel - Re: [PATCH v7 2/7] livepatch: Free only structures with initialized kobject

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Feb 2018 11:06:27 +0100 (CET)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Petr Mladek <pmladek@...e.com>
cc:     Jiri Kosina <jikos@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jason Baron <jbaron@...mai.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Jessica Yu <jeyu@...nel.org>,
        Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/7] livepatch: Free only structures with initialized
 kobject

On Tue, 6 Feb 2018, Petr Mladek wrote:

> We are going to add a feature called atomic replace. It will allow to
> create a patch that would replace all already registered patches.
> For this, we will need to dynamically create funcs and objects
> for functions that are not longer patched.

s/not longer/no longer/

It applies to the other changelogs as well.
 
> We will want to reuse the existing init() and free() functions. Up to now,
> the free() functions checked a limit and were called only for structures
> with initialized kobject. But we will want to call them also for structures
> that were allocated but where the kobject was not initialized yet.
> 
> This patch removes the limit. It calls klp_free*() functions for all
> structures. But only the ones with initialized kobject are freed.
> The handling of un-initialized structures will be added later with
> the support for dynamic structures.
> 
> This patch does not change the existing behavior.
> 
> Signed-off-by: Petr Mladek <pmladek@...e.com>
> Cc: Josh Poimboeuf <jpoimboe@...hat.com>
> Cc: Jessica Yu <jeyu@...nel.org>
> Cc: Jiri Kosina <jikos@...nel.org>
> Cc: Miroslav Benes <mbenes@...e.cz>
> Cc: Jason Baron <jbaron@...mai.com>
> ---
>  kernel/livepatch/core.c | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 1d525f4a270a..ef7c3b5f561b 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -654,16 +654,17 @@ static struct kobj_type klp_ktype_func = {
>  };
>  
>  /*
> - * Free all functions' kobjects in the array up to some limit. When limit is
> - * NULL, all kobjects are freed.
> + * Free all funcs that have the kobject initialized. Otherwise,
> + * nothing is needed.

s/needed/done/ ?

I'd remove the second sentence in that case.

>   */
> -static void klp_free_funcs_limited(struct klp_object *obj,
> -				   struct klp_func *limit)
> +static void klp_free_funcs(struct klp_object *obj)
>  {
>  	struct klp_func *func;
>  
> -	for (func = obj->funcs; func->old_name && func != limit; func++)
> -		kobject_put(&func->kobj);
> +	klp_for_each_func(obj, func) {
> +		if (func->kobj.state_initialized)
> +			kobject_put(&func->kobj);
> +	}
>  }
>  
>  /* Clean up when a patched object is unloaded */
> @@ -678,23 +679,25 @@ static void klp_free_object_loaded(struct klp_object *obj)
>  }
>  
>  /*
> - * Free all objects' kobjects in the array up to some limit. When limit is
> - * NULL, all kobjects are freed.
> + * Free all funcs and objects that have the kobject initialized.
> + * Otherwise, nothing is needed.

Same here.

>   */
> -static void klp_free_objects_limited(struct klp_patch *patch,
> -				     struct klp_object *limit)
> +static void klp_free_objects(struct klp_patch *patch)
>  {
>  	struct klp_object *obj;
>  
> -	for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
> -		klp_free_funcs_limited(obj, NULL);
> -		kobject_put(&obj->kobj);
> +	klp_for_each_object(patch, obj) {
> +		klp_free_funcs(obj);
> +
> +		if (obj->kobj.state_initialized)
> +			kobject_put(&obj->kobj);
>  	}
>  }
>  
>  static void klp_free_patch(struct klp_patch *patch)
>  {
> -	klp_free_objects_limited(patch, NULL);
> +	klp_free_objects(patch);
> +
>  	if (!list_empty(&patch->list))
>  		list_del(&patch->list);
>  }
> @@ -791,21 +794,16 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  	klp_for_each_func(obj, func) {
>  		ret = klp_init_func(obj, func);
>  		if (ret)
> -			goto free;
> +			return ret;
>  	}
>  
>  	if (klp_is_object_loaded(obj)) {
>  		ret = klp_init_object_loaded(patch, obj);
>  		if (ret)
> -			goto free;
> +			return ret;
>  	}
>  
>  	return 0;
> -
> -free:
> -	klp_free_funcs_limited(obj, func);
> -	kobject_put(&obj->kobj);
> -	return ret;
>  }

I'm slightly worried about not keeping the error handling here but 
offloading it to the caller, because we may forget to update it one day. 
But yes, it keeps things simple and I can live with that.

>  static int klp_init_patch(struct klp_patch *patch)
> @@ -842,7 +840,7 @@ static int klp_init_patch(struct klp_patch *patch)
>  	return 0;
>  
>  free:
> -	klp_free_objects_limited(patch, obj);
> +	klp_free_objects(patch);
>  
>  	mutex_unlock(&klp_mutex);

With those nits fixed

Acked-by: Miroslav Benes <mbenes@...e.cz>

Miroslav