Re: [PATCH] livepatch: Prevent to enable uninitialized patch

[Miroslav Benes <mbenes@suse.cz>]

On Mon, 11 May 2015, Minfei Huang wrote:

> From: Minfei Huang <minfei.huang@hotmail.com>
> 
> The previous patches can be applied, while the corresponding module is
> loaded. Now the code cannot handle correct behavior to deal with the
> case that the patch fail to be initialized when the module is being
> loaded.
> 
> In general, the patch will do relocation (if necessary) and
> obtain/verify function address before we start to enable patch. But we
> can still trigger to enable the patch (disable the patch firstly, then
> enable it), although the patch fail to be initialized in the function
> klp_module_notify_coming.
> 
> To fix it, we can make obj->mod to NULL, if the object fails to be
> initialized.
> 
> Signed-off-by: Minfei Huang <minfei.huang@hotmail.com>

Hi,

just to be sure, is the following what makes you worried?

The module comes and our notifier is called. We verify that it needs to be 
patched and we call klp_module_notify_coming where the object (for this 
module) is enabled. But that could fail somewhere and we print warning to 
the log (pr_warn). Now, you can disable and enable patch, during which the 
object for this very module is enabled again. And it could fail again.

Is this correct? Do you want to prevent printing of the warning again and 
again to the log? 

It could happen that the first enablement could fail because of something 
which would not be true for the second try. In such case the module would 
not be patched with your fix (it would be skipped in __klp_enable_patch 
loop).

It is possible that I do not understand the changelog and the patch 
correctly, so please shed some light on this if necessary...

Thanks,
Miroslav

> ---
>  kernel/livepatch/core.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 284e269..4bbcdda 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> -static void klp_module_notify_coming(struct klp_patch *patch,
> +static int klp_module_notify_coming(struct klp_patch *patch,
>  				     struct klp_object *obj)
>  {
>  	struct module *pmod = patch->mod;
>  	struct module *mod = obj->mod;
> -	int ret;
> +	int ret = 0;
>  
>  	ret = klp_init_object_loaded(patch, obj);
>  	if (ret)
> -		goto err;
> +		goto out;
>  
>  	if (patch->state == KLP_DISABLED)
> -		return;
> +		goto out;
>  
>  	pr_notice("applying patch '%s' to loading module '%s'\n",
>  		  pmod->name, mod->name);
>  
>  	ret = klp_enable_object(obj);
> -	if (!ret)
> -		return;
>  
> -err:
> -	pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> -		pmod->name, mod->name, ret);
> +out:
> +	if (ret)
> +		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> +				pmod->name, mod->name, ret);
> +	return ret;
>  }
>  
>  static void klp_module_notify_going(struct klp_patch *patch,
> @@ -930,6 +930,7 @@ disabled:
>  static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>  			     void *data)
>  {
> +	int ret = 0;
>  	struct module *mod = data;
>  	struct klp_patch *patch;
>  	struct klp_object *obj;
> @@ -955,7 +956,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>  
>  			if (action == MODULE_STATE_COMING) {
>  				obj->mod = mod;
> -				klp_module_notify_coming(patch, obj);
> +				ret = klp_module_notify_coming(patch, obj);
> +				if (ret)
> +					obj->mod = NULL;
>  			} else /* MODULE_STATE_GOING */
>  				klp_module_notify_going(patch, obj);
>  
> -- 
> 2.2.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
Miroslav Benes
SUSE Labs