Re: [POC 05/23] livepatch: Initialize and free livepatch submodule

[Julien Thierry <jthierry@redhat.com>]

Hi Petr,

On 1/17/20 3:03 PM, Petr Mladek wrote:
> Another step when loading livepatches to livepatch modules is
> to initialize the structure, create sysfs entries, do livepatch
> specific relocations.
> 
> These operation can fail and the objects must be freed that case.
> The error message is taken from klp_module_coming() to match
> selftests.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>   kernel/livepatch/core.c | 34 +++++++++++++++++++++++++++-------
>   1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index e2c7dc6c2d5f..6c27b635e5a7 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -583,18 +583,23 @@ static void klp_free_object_loaded(struct klp_object *obj)
>   	}
>   }
>   
> +static void klp_free_object(struct klp_object *obj, bool nops_only)
> +{
> +	__klp_free_funcs(obj, nops_only);
> +
> +	if (nops_only && !obj->dynamic)
> +		return;
> +
> +	list_del(&obj->node);
> +	kobject_put(&obj->kobj);
> +}
> +
>   static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
>   {
>   	struct klp_object *obj, *tmp_obj;
>   
>   	klp_for_each_object_safe(patch, obj, tmp_obj) {
> -		__klp_free_funcs(obj, nops_only);
> -
> -		if (nops_only && !obj->dynamic)
> -			continue;
> -
> -		list_del(&obj->node);
> -		kobject_put(&obj->kobj);
> +		klp_free_object(obj, nops_only);
>   	}
>   }
>   
> @@ -812,6 +817,8 @@ static int klp_init_object_early(struct klp_patch *patch,
>   	if (obj->dynamic || try_module_get(obj->mod))
>   		return 0;
>   
> +	/* patch stays when this function fails in klp_add_object() */
> +	list_del(&obj->node);
>   	return -ENODEV;
>   }
>   
> @@ -993,9 +1000,22 @@ int klp_add_object(struct klp_object *obj)
>   		goto err;
>   	}
>   
> +	ret = klp_init_object_early(patch, obj);

klp_init_object_early() can fail after adding obj to patch->obj_list. 
This wouldn't get cleaned up by the "err" path.

It probably would keep things simple to only add obj to patch->obj_list 
if early initialization is successful in patch 2 (ofc I'm talking about 
the actual patch of this patch series ;) ).

> +	if (ret)
> +		goto err;
> +
> +	ret = klp_init_object(patch, obj);
> +	if (ret) {
> +		pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> +			patch->obj->patch_name, obj->name, ret);
> +		goto err_free;
> +	}
> +
>   	mutex_unlock(&klp_mutex);
>   	return 0;
>   
> +err_free:
> +	klp_free_object(obj, false);
>   err:
>   	/*
>   	 * If a patch is unsuccessfully applied, return
> 

Cheers,

-- 
Julien Thierry