Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [POC 05/23] livepatch: Initialize and free livepatch submodule
To:     Petr Mladek <pmladek@suse.com>, Jiri Kosina <jikos@kernel.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>
Cc:     Joe Lawrence <joe.lawrence@redhat.com>,
        Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>,
        Nicolai Stange <nstange@suse.de>,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20200117150323.21801-1-pmladek@suse.com>
 <20200117150323.21801-6-pmladek@suse.com>
From:   Julien Thierry <jthierry@redhat.com>
Message-ID: <0cc056a0-99c7-68d6-9f22-28c043254ab2@redhat.com>
Date:   Tue, 21 Jan 2020 11:58:53 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.3.0
MIME-Version: 1.0
In-Reply-To: <20200117150323.21801-6-pmladek@suse.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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