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

Julien Thierry
2020-01-21T11:59:01Z

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