Re: [PATCH 1/2] livepatch: Implement separate coming and going module notifiers

[Miroslav Benes]

On Fri, 29 Jan 2016, Jessica Yu wrote:

> Detangle klp_module_notify() into two separate module notifiers
> klp_module_notify_{coming,going}() with the appropriate priorities,
> so that on module load, the ftrace module notifier will run *before*
> the livepatch coming module notifier but *after* the livepatch going
> module modifier.
> 
> This fixes a notifier ordering issue in which the ftrace module notifier
> (and hence ftrace_module_enable()) for COMING modules was being called
> after klp_module_notify(), which caused livepatch modules to initialize
> incorrectly.
> 
> Signed-off-by: Jessica Yu <jeyu@xxxxxxxxxx>

Hi,

I don't know what the outcome of the discussion would be, but few comments 
on the patch nevertheless.

>  kernel/livepatch/core.c | 128 +++++++++++++++++++++++++++---------------------
>  1 file changed, 73 insertions(+), 55 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bc2c85c..23f4234 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -866,60 +866,73 @@ int klp_register_patch(struct klp_patch *patch)
>  }
>  EXPORT_SYMBOL_GPL(klp_register_patch);
>  
> -static int klp_module_notify_coming(struct klp_patch *patch,
> -				     struct klp_object *obj)
> +static int klp_module_notify_coming(struct notifier_block *nb,
> +				    unsigned long action, void *data)
>  {
> -	struct module *pmod = patch->mod;
> -	struct module *mod = obj->mod;
>  	int ret;
> +	struct module *mod = data;
> +	struct klp_patch *patch;
> +	struct klp_object *obj;
>  
> -	ret = klp_init_object_loaded(patch, obj);
> -	if (ret) {
> -		pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> -			pmod->name, mod->name, ret);
> -		return ret;
> -	}
> -
> -	if (patch->state == KLP_DISABLED)
> +	if (action != MODULE_STATE_COMING)
>  		return 0;
>  
> -	pr_notice("applying patch '%s' to loading module '%s'\n",
> -		  pmod->name, mod->name);
> +	mutex_lock(&klp_mutex);
>  
> -	ret = klp_enable_object(obj);
> -	if (ret)
> -		pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> -			pmod->name, mod->name, ret);
> -	return ret;
> -}
> +	/*
> +	 * Each module has to know that the notifier has been called.
> +	 * We never know what module will get patched by a new patch.
> +	 */
> +	mod->klp_alive = true;
>  
> -static void klp_module_notify_going(struct klp_patch *patch,
> -				    struct klp_object *obj)
> -{
> -	struct module *pmod = patch->mod;
> -	struct module *mod = obj->mod;
> +	list_for_each_entry(patch, &klp_patches, list) {
> +		klp_for_each_object(patch, obj) {
> +			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
> +				continue;
> +
> +			obj->mod = mod;
>  
> -	if (patch->state == KLP_DISABLED)
> -		goto disabled;
> +			ret = klp_init_object_loaded(patch, obj);
> +			if (ret) {
> +				pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> +					patch->mod->name, obj->mod->name, ret);
> +				goto err;
> +			}
>  
> -	pr_notice("reverting patch '%s' on unloading module '%s'\n",
> -		  pmod->name, mod->name);
> +			if (patch->state == KLP_DISABLED)
> +				break;
>  
> -	klp_disable_object(obj);
> +			pr_notice("applying patch '%s' to loading module '%s'\n",
> +				  patch->mod->name, obj->mod->name);
>  
> -disabled:
> -	klp_free_object_loaded(obj);
> +			ret = klp_enable_object(obj);
> +			if (ret)
> +				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> +					patch->mod->name, obj->mod->name, ret);
> +err:
> +			if (ret) {
> +				obj->mod = NULL;
> +				pr_warn("patch '%s' is in an inconsistent state!\n",
> +					patch->mod->name);
> +			}
> +
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&klp_mutex);
> +
> +	return 0;
>  }
>  
> -static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> -			     void *data)
> +static int klp_module_notify_going(struct notifier_block *nb,
> +				   unsigned long action, void *data)
>  {
> -	int ret;
>  	struct module *mod = data;
>  	struct klp_patch *patch;
>  	struct klp_object *obj;
>  
> -	if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> +	if (action != MODULE_STATE_GOING)
>  		return 0;
>  
>  	mutex_lock(&klp_mutex);
> @@ -928,27 +941,22 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>  	 * Each module has to know that the notifier has been called.
>  	 * We never know what module will get patched by a new patch.
>  	 */
> -	if (action == MODULE_STATE_COMING)
> -		mod->klp_alive = true;
> -	else /* MODULE_STATE_GOING */
> -		mod->klp_alive = false;
> +	mod->klp_alive = false;
>  
>  	list_for_each_entry(patch, &klp_patches, list) {
>  		klp_for_each_object(patch, obj) {
>  			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
>  				continue;
>  
> -			if (action == MODULE_STATE_COMING) {
> -				obj->mod = mod;
> -				ret = klp_module_notify_coming(patch, obj);
> -				if (ret) {
> -					obj->mod = NULL;
> -					pr_warn("patch '%s' is in an inconsistent state!\n",
> -						patch->mod->name);
> -				}
> -			} else /* MODULE_STATE_GOING */
> -				klp_module_notify_going(patch, obj);
> +			if (patch->state == KLP_DISABLED)
> +				goto disabled;
> +
> +			pr_notice("reverting patch '%s' on unloading module '%s'\n",
> +				  patch->mod->name, obj->mod->name);
>  
> +			klp_disable_object(obj);
> +disabled:
> +			klp_free_object_loaded(obj);
>  			break;
>  		}
>  	}

As for the correctness both notifiers look ok, but I must admit I don't 
like the resulting code much. There is no need for 'disabled' label in the 
last hunk. I think the same could be achieved with the condition only (and 
it applies to the original klp_module_notify_going as well). I guess the 
similar could be done to klp_module_notify_coming. However it is a matter 
of taste.

> @@ -958,9 +966,14 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>  	return 0;
>  }
>  
> -static struct notifier_block klp_module_nb = {
> -	.notifier_call = klp_module_notify,
> -	.priority = INT_MIN+1, /* called late but before ftrace notifier */
> +static struct notifier_block klp_module_nb_coming = {
> +	.notifier_call = klp_module_notify_coming,
> +	.priority = INT_MIN, /* called late but after ftrace (coming) notifier */
> +};
> +
> +static struct notifier_block klp_module_nb_going = {
> +	.notifier_call = klp_module_notify_going,
> +	.priority = INT_MIN+2, /* called late but before ftrace (going) notifier */
>  };
>  
>  static int __init klp_init(void)
> @@ -973,7 +986,11 @@ static int __init klp_init(void)
>  		return -EINVAL;
>  	}
>  
> -	ret = register_module_notifier(&klp_module_nb);
> +	ret = register_module_notifier(&klp_module_nb_coming);
> +	if (ret)
> +		return ret;
> +
> +	ret = register_module_notifier(&klp_module_nb_going);
>  	if (ret)
>  		return ret;

There is klp_module_nb_coming already registered so in case of error we 
need to unregister it. Maybe goto and two different error labels below?

Otherwise than that it looks good. I agree there are advantages to split 
the notifiers. For example we can replace the coming one with the function 
call somewhere in load_module() to improve error handling if the patching 
fails while loading a module. This would be handy with a consistency model 
in the future.

Cheers,
Miroslav

>  
> @@ -986,7 +1003,8 @@ static int __init klp_init(void)
>  	return 0;
>  
>  unregister:
> -	unregister_module_notifier(&klp_module_nb);
> +	unregister_module_notifier(&klp_module_nb_coming);
> +	unregister_module_notifier(&klp_module_nb_going);
>  	return ret;
>  }