[PATCH 0/2] Fix ordering of ftrace + livepatch module notifier callbacks

[PATCH 0/2] Fix ordering of ftrace + livepatch module notifier callbacks

[Jessica Yu]

As explained here [1], livepatch modules are failing to initialize properly
because the ftrace coming module notifier (which calls
ftrace_module_enable()) runs after the livepatch module notifier (which
enables the patch(es)). Thus livepatch attempts to apply patches to modules
before ftrace_module_enable() is even called for the corresponding
module(s). Separate klp_module_notify() into coming and going notifiers
and tweak the priorities to fix the order in which the ftrace and livepatch
notifiers are called.

Tested the changes with a test livepatch module that patches 9p and nilfs2,
and verified that the issue is fixed.

Patch 1/2 based on the 'for-next' branch in livepatching -
git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git

Patch 2/2 based on the 'ftrace/core' branch in linux-trace -
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git

[1] http://lkml.kernel.org/g/20160128204033.GA32131@packer-debian-8-amd64.digitalocean.com

Jessica Yu (2):
  livepatch: Implement separate coming and going module notifiers
  ftrace: Adjust priority of ftrace module notifier

 kernel/livepatch/core.c | 128 +++++++++++++++++++++++++++---------------------
 kernel/trace/ftrace.c   |   7 ++-
 2 files changed, 79 insertions(+), 56 deletions(-)

-- 
2.4.3

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

[Jessica Yu]

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@redhat.com>
---
 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;
 		}
 	}
@@ -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;
 
@@ -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;
 }
 
-- 
2.4.3

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@redhat.com>

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;
>  }

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

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
> 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.

Yeah, we'll need something like that eventually.  Though we'll need to
make sure that ftrace_module_enable() is still called beforehand, after
setting MODULE_STATE_COMING state, due to the race described in 5156dca.

Something like:

[note: klp_module_notify_coming() is replaced with klp_module_enable()]

diff --git a/kernel/module.c b/kernel/module.c
index 8358f46..aeabd81 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	mod->state = MODULE_STATE_COMING;
 	mutex_unlock(&module_mutex);
 
+	ftrace_module_enable(mod);
+	err = klp_module_enable(mod);
+	if (err) {
+		ftrace_release_mod(mod);
+		return err;
+	}
+
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_COMING, mod);
 	return 0;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index eca592f..c42cf37 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self,
 	struct module *mod = data;
 
 	switch (val) {
-	case MODULE_STATE_COMING:
-		ftrace_module_enable(mod);
-		break;
 	case MODULE_STATE_GOING:
 		ftrace_release_mod(mod);
 		break;

-- 
Josh

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

[Steven Rostedt]

[ Added Rusty, as he's still maintainer of the module code ]

On Fri, 29 Jan 2016 11:30:10 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
> > 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.  
> 
> Yeah, we'll need something like that eventually.  Though we'll need to
> make sure that ftrace_module_enable() is still called beforehand, after
> setting MODULE_STATE_COMING state, due to the race described in 5156dca.
> 
> Something like:
> 
> [note: klp_module_notify_coming() is replaced with klp_module_enable()]
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 8358f46..aeabd81 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	mod->state = MODULE_STATE_COMING;
>  	mutex_unlock(&module_mutex);
>  
> +	ftrace_module_enable(mod);
> +	err = klp_module_enable(mod);
> +	if (err) {
> +		ftrace_release_mod(mod);
> +		return err;
> +	}
> +
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_COMING, mod);
>  	return 0;
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index eca592f..c42cf37 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self,
>  	struct module *mod = data;
>  
>  	switch (val) {
> -	case MODULE_STATE_COMING:
> -		ftrace_module_enable(mod);
> -		break;
>  	case MODULE_STATE_GOING:
>  		ftrace_release_mod(mod);
>  		break;

If we end up doing something like this, I would just say punt and have
the ftrace code be hardcoded into the module code and remove the
notifiers completely. ftrace (and live kernel patching for that matter)
are rather special. They are not a filesystem or driver. They are core
utilities and having them called directly from the module code may be
prudent and better to understand and control.

Note, you still need to have prototypes for ftrace_module_enable() and
a stub when ftrace is not configured. Same goes for klp_module_enable().

-- Steve

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

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 12:40:14PM -0500, Steven Rostedt wrote:
> [ Added Rusty, as he's still maintainer of the module code ]
> 
> On Fri, 29 Jan 2016 11:30:10 -0600
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
> > > 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.  
> > 
> > Yeah, we'll need something like that eventually.  Though we'll need to
> > make sure that ftrace_module_enable() is still called beforehand, after
> > setting MODULE_STATE_COMING state, due to the race described in 5156dca.
> > 
> > Something like:
> > 
> > [note: klp_module_notify_coming() is replaced with klp_module_enable()]
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 8358f46..aeabd81 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
> >  	mod->state = MODULE_STATE_COMING;
> >  	mutex_unlock(&module_mutex);
> >  
> > +	ftrace_module_enable(mod);
> > +	err = klp_module_enable(mod);
> > +	if (err) {
> > +		ftrace_release_mod(mod);
> > +		return err;
> > +	}
> > +
> >  	blocking_notifier_call_chain(&module_notify_list,
> >  				     MODULE_STATE_COMING, mod);
> >  	return 0;
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index eca592f..c42cf37 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self,
> >  	struct module *mod = data;
> >  
> >  	switch (val) {
> > -	case MODULE_STATE_COMING:
> > -		ftrace_module_enable(mod);
> > -		break;
> >  	case MODULE_STATE_GOING:
> >  		ftrace_release_mod(mod);
> >  		break;
> 
> If we end up doing something like this, I would just say punt and have
> the ftrace code be hardcoded into the module code and remove the
> notifiers completely. ftrace (and live kernel patching for that matter)
> are rather special. They are not a filesystem or driver. They are core
> utilities and having them called directly from the module code may be
> prudent and better to understand and control.

Agreed, and we might as well make this change now to avoid more churn
later.

> 
> Note, you still need to have prototypes for ftrace_module_enable() and
> a stub when ftrace is not configured. Same goes for klp_module_enable().
> 
> -- Steve
> 

-- 
Josh

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

[Miroslav Benes]

On Fri, 29 Jan 2016, Josh Poimboeuf wrote:

> On Fri, Jan 29, 2016 at 12:40:14PM -0500, Steven Rostedt wrote:
> > [ Added Rusty, as he's still maintainer of the module code ]
> > 
> > On Fri, 29 Jan 2016 11:30:10 -0600
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > > On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
> > > > 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.  
> > > 
> > > Yeah, we'll need something like that eventually.  Though we'll need to
> > > make sure that ftrace_module_enable() is still called beforehand, after
> > > setting MODULE_STATE_COMING state, due to the race described in 5156dca.
> > > 
> > > Something like:
> > > 
> > > [note: klp_module_notify_coming() is replaced with klp_module_enable()]
> > > 
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 8358f46..aeabd81 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
> > >  	mod->state = MODULE_STATE_COMING;
> > >  	mutex_unlock(&module_mutex);
> > >  
> > > +	ftrace_module_enable(mod);
> > > +	err = klp_module_enable(mod);
> > > +	if (err) {
> > > +		ftrace_release_mod(mod);
> > > +		return err;
> > > +	}
> > > +
> > >  	blocking_notifier_call_chain(&module_notify_list,
> > >  				     MODULE_STATE_COMING, mod);
> > >  	return 0;
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index eca592f..c42cf37 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self,
> > >  	struct module *mod = data;
> > >  
> > >  	switch (val) {
> > > -	case MODULE_STATE_COMING:
> > > -		ftrace_module_enable(mod);
> > > -		break;
> > >  	case MODULE_STATE_GOING:
> > >  		ftrace_release_mod(mod);
> > >  		break;
> > 
> > If we end up doing something like this, I would just say punt and have
> > the ftrace code be hardcoded into the module code and remove the
> > notifiers completely. ftrace (and live kernel patching for that matter)
> > are rather special. They are not a filesystem or driver. They are core
> > utilities and having them called directly from the module code may be
> > prudent and better to understand and control.
> 
> Agreed, and we might as well make this change now to avoid more churn
> later.

It is possible to achieve the same goal even with the notifiers. They are 
processed synchronously in complete_formation(). So we can put our klp 
hook after that, right? Or better, put it to load_module() after 
complete_formation() call. There is an error handling code even today 
(that is, parse_args() or mod_sysfs_setup() can fail). Moreover, we'll 
have a hook there with Jessica's relocation rework patch set.

But Steven's reasoning is convincing, so I'm all up for it.

Regards,
Miroslav

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

[Steven Rostedt]

On Fri, 29 Jan 2016 20:25:15 +0100 (CET)
Miroslav Benes <mbenes@suse.cz> wrote:

> It is possible to achieve the same goal even with the notifiers. They are 
> processed synchronously in complete_formation(). So we can put our klp 
> hook after that, right? Or better, put it to load_module() after 
> complete_formation() call. There is an error handling code even today 
> (that is, parse_args() or mod_sysfs_setup() can fail). Moreover, we'll 
> have a hook there with Jessica's relocation rework patch set.

The problem with notifiers is that you don't know what is being called.
A function call directly in the code, where it will always be needed if
configured in, is a reasonable need to not use a notifier.

Although, I have to admit, if live kernel patching is configured in,
it's not always needed to be called here, does it? With ftrace, the
call has to be done when ftrace is configured in regardless if tracing
is used or not.

> 
> But Steven's reasoning is convincing, so I'm all up for it.

Great!

-- Steve

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

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 02:29:50PM -0500, Steven Rostedt wrote:
> On Fri, 29 Jan 2016 20:25:15 +0100 (CET)
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
> > It is possible to achieve the same goal even with the notifiers. They are 
> > processed synchronously in complete_formation(). So we can put our klp 
> > hook after that, right? Or better, put it to load_module() after 
> > complete_formation() call. There is an error handling code even today 
> > (that is, parse_args() or mod_sysfs_setup() can fail). Moreover, we'll 
> > have a hook there with Jessica's relocation rework patch set.
> 
> The problem with notifiers is that you don't know what is being called.
> A function call directly in the code, where it will always be needed if
> configured in, is a reasonable need to not use a notifier.
> 
> Although, I have to admit, if live kernel patching is configured in,
> it's not always needed to be called here, does it? With ftrace, the
> call has to be done when ftrace is configured in regardless if tracing
> is used or not.

For live patching it actually does need to be called for every module.
We need to check if any previously loaded patches have any modifications
which affect the module.

> 
> > 
> > But Steven's reasoning is convincing, so I'm all up for it.
> 
> Great!
> 
> -- Steve

-- 
Josh

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

[Steven Rostedt]

On Fri, 29 Jan 2016 13:47:15 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> > Although, I have to admit, if live kernel patching is configured in,
> > it's not always needed to be called here, does it? With ftrace, the
> > call has to be done when ftrace is configured in regardless if tracing
> > is used or not.  
> 
> For live patching it actually does need to be called for every module.
> We need to check if any previously loaded patches have any modifications
> which affect the module.
> 

But only if you are using the live kernel patching facility. My point
is, if I never use live kernel patching (which 99.9% of Linux users do
no use), then the call will basically be a nop.

With ftrace, that's not the story. Even if you don't ever use the
facility (like 99.8% of Linux users do not use ;-) the function is
still not a nop. There's three calls needed for each module.

1) convert all the calls to mcount/fentry into a nop, and save
those locations in a table. They are all marked as disabled (not to be
used)

2) After module setup (where the notifiers are called), the locations
need to be enabled, otherwise ftrace would never work for that module.

3) On module exit, the locations must be removed, otherwise ftrace may
still try to write to non-existing code which could brick specific
network cards.

These are done if ftrace is configured regardless if it is every
actually used by a user.

Thus, my question is, what does live kernel patching need to do if I
never add a patch?

-- Steve

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

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 03:08:23PM -0500, Steven Rostedt wrote:
> On Fri, 29 Jan 2016 13:47:15 -0600
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> 
> > > Although, I have to admit, if live kernel patching is configured in,
> > > it's not always needed to be called here, does it? With ftrace, the
> > > call has to be done when ftrace is configured in regardless if tracing
> > > is used or not.  
> > 
> > For live patching it actually does need to be called for every module.
> > We need to check if any previously loaded patches have any modifications
> > which affect the module.
> > 
> 
> But only if you are using the live kernel patching facility. My point
> is, if I never use live kernel patching (which 99.9% of Linux users do
> no use), then the call will basically be a nop.
> 
> With ftrace, that's not the story. Even if you don't ever use the
> facility (like 99.8% of Linux users do not use ;-) the function is
> still not a nop. There's three calls needed for each module.
> 
> 1) convert all the calls to mcount/fentry into a nop, and save
> those locations in a table. They are all marked as disabled (not to be
> used)
> 
> 2) After module setup (where the notifiers are called), the locations
> need to be enabled, otherwise ftrace would never work for that module.
> 
> 3) On module exit, the locations must be removed, otherwise ftrace may
> still try to write to non-existing code which could brick specific
> network cards.
> 
> These are done if ftrace is configured regardless if it is every
> actually used by a user.
> 
> Thus, my question is, what does live kernel patching need to do if I
> never add a patch?

Right, as you say it's basically a nop 99.99% of the time.  But we still
need to do the "are any patches loaded" check, so we still need the call
into a livepatch function to do that.

-- 
Josh

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

[Jiri Kosina]

On Fri, 29 Jan 2016, Josh Poimboeuf wrote:

> Right, as you say it's basically a nop 99.99% of the time.  But we still 
> need to do the "are any patches loaded" check, so we still need the call 
> into a livepatch function to do that.

We might create a static key for that (some might call it 
over-optimization though, given it's happening on kind of a slow path 
anyway).

-- 
Jiri Kosina
SUSE Labs

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

[Josh Poimboeuf]

On Mon, Feb 01, 2016 at 01:27:57PM +0100, Jiri Kosina wrote:
> On Fri, 29 Jan 2016, Josh Poimboeuf wrote:
> 
> > Right, as you say it's basically a nop 99.99% of the time.  But we still 
> > need to do the "are any patches loaded" check, so we still need the call 
> > into a livepatch function to do that.
> 
> We might create a static key for that (some might call it 
> over-optimization though, given it's happening on kind of a slow path 
> anyway).

I really don't think that's needed.  Nobody will notice an extra
function call in the module load path.  (And BTW it'll be slightly
faster than today since it doesn't go through the notifier first.)

-- 
Josh

Re: livepatch: Implement separate coming and going module notifiers

[Jessica Yu]

+++ Steven Rostedt [29/01/16 14:29 -0500]:
>On Fri, 29 Jan 2016 20:25:15 +0100 (CET)
>Miroslav Benes <mbenes@suse.cz> wrote:
>
>> It is possible to achieve the same goal even with the notifiers. They are
>> processed synchronously in complete_formation(). So we can put our klp
>> hook after that, right? Or better, put it to load_module() after
>> complete_formation() call. There is an error handling code even today
>> (that is, parse_args() or mod_sysfs_setup() can fail). Moreover, we'll
>> have a hook there with Jessica's relocation rework patch set.
>
>The problem with notifiers is that you don't know what is being called.
>A function call directly in the code, where it will always be needed if
>configured in, is a reasonable need to not use a notifier.

That's a pretty good reason. This bug popped up in the first place
due to a lack of clarity on which notifier gets called when.

>Although, I have to admit, if live kernel patching is configured in,
>it's not always needed to be called here, does it? With ftrace, the
>call has to be done when ftrace is configured in regardless if tracing
>is used or not.

Livepatch has to check if any of its patches affect the coming module,
so hardcoded or not it will still need to loop through the klp_patches
list like it does in the current notifiers.

Jessica

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

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 08:25:15PM +0100, Miroslav Benes wrote:
> On Fri, 29 Jan 2016, Josh Poimboeuf wrote:
> 
> > On Fri, Jan 29, 2016 at 12:40:14PM -0500, Steven Rostedt wrote:
> > > [ Added Rusty, as he's still maintainer of the module code ]
> > > 
> > > On Fri, 29 Jan 2016 11:30:10 -0600
> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > 
> > > > On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
> > > > > 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.  
> > > > 
> > > > Yeah, we'll need something like that eventually.  Though we'll need to
> > > > make sure that ftrace_module_enable() is still called beforehand, after
> > > > setting MODULE_STATE_COMING state, due to the race described in 5156dca.
> > > > 
> > > > Something like:
> > > > 
> > > > [note: klp_module_notify_coming() is replaced with klp_module_enable()]
> > > > 
> > > > diff --git a/kernel/module.c b/kernel/module.c
> > > > index 8358f46..aeabd81 100644
> > > > --- a/kernel/module.c
> > > > +++ b/kernel/module.c
> > > > @@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
> > > >  	mod->state = MODULE_STATE_COMING;
> > > >  	mutex_unlock(&module_mutex);
> > > >  
> > > > +	ftrace_module_enable(mod);
> > > > +	err = klp_module_enable(mod);
> > > > +	if (err) {
> > > > +		ftrace_release_mod(mod);
> > > > +		return err;
> > > > +	}
> > > > +
> > > >  	blocking_notifier_call_chain(&module_notify_list,
> > > >  				     MODULE_STATE_COMING, mod);
> > > >  	return 0;
> > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > > index eca592f..c42cf37 100644
> > > > --- a/kernel/trace/ftrace.c
> > > > +++ b/kernel/trace/ftrace.c
> > > > @@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self,
> > > >  	struct module *mod = data;
> > > >  
> > > >  	switch (val) {
> > > > -	case MODULE_STATE_COMING:
> > > > -		ftrace_module_enable(mod);
> > > > -		break;
> > > >  	case MODULE_STATE_GOING:
> > > >  		ftrace_release_mod(mod);
> > > >  		break;
> > > 
> > > If we end up doing something like this, I would just say punt and have
> > > the ftrace code be hardcoded into the module code and remove the
> > > notifiers completely. ftrace (and live kernel patching for that matter)
> > > are rather special. They are not a filesystem or driver. They are core
> > > utilities and having them called directly from the module code may be
> > > prudent and better to understand and control.
> > 
> > Agreed, and we might as well make this change now to avoid more churn
> > later.
> 
> It is possible to achieve the same goal even with the notifiers. They are 
> processed synchronously in complete_formation(). So we can put our klp 
> hook after that, right? Or better, put it to load_module() after 
> complete_formation() call. There is an error handling code even today 
> (that is, parse_args() or mod_sysfs_setup() can fail). Moreover, we'll 
> have a hook there with Jessica's relocation rework patch set.

Well, my feeling is that we should really apply livepatch relocations
before allowing any other notifiers to run, in case the relocations
affect them.  But it's just a feeling; I don't have any specific
examples to justify it (yet).

> But Steven's reasoning is convincing, so I'm all up for it.
> 
> Regards,
> Miroslav

-- 
Josh

Re: livepatch: Implement separate coming and going module notifiers

[Jessica Yu]

+++ Josh Poimboeuf [29/01/16 13:42 -0600]:
>On Fri, Jan 29, 2016 at 08:25:15PM +0100, Miroslav Benes wrote:
>> On Fri, 29 Jan 2016, Josh Poimboeuf wrote:
>>
>> > On Fri, Jan 29, 2016 at 12:40:14PM -0500, Steven Rostedt wrote:
>> > > [ Added Rusty, as he's still maintainer of the module code ]
>> > >
>> > > On Fri, 29 Jan 2016 11:30:10 -0600
>> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > >
>> > > > On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
>> > > > > 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.
>> > > >
>> > > > Yeah, we'll need something like that eventually.  Though we'll need to
>> > > > make sure that ftrace_module_enable() is still called beforehand, after
>> > > > setting MODULE_STATE_COMING state, due to the race described in 5156dca.
>> > > >
>> > > > Something like:
>> > > >
>> > > > [note: klp_module_notify_coming() is replaced with klp_module_enable()]
>> > > >
>> > > > diff --git a/kernel/module.c b/kernel/module.c
>> > > > index 8358f46..aeabd81 100644
>> > > > --- a/kernel/module.c
>> > > > +++ b/kernel/module.c
>> > > > @@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
>> > > >  	mod->state = MODULE_STATE_COMING;
>> > > >  	mutex_unlock(&module_mutex);
>> > > >
>> > > > +	ftrace_module_enable(mod);
>> > > > +	err = klp_module_enable(mod);
>> > > > +	if (err) {
>> > > > +		ftrace_release_mod(mod);
>> > > > +		return err;
>> > > > +	}
>> > > > +
>> > > >  	blocking_notifier_call_chain(&module_notify_list,
>> > > >  				     MODULE_STATE_COMING, mod);
>> > > >  	return 0;
>> > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> > > > index eca592f..c42cf37 100644
>> > > > --- a/kernel/trace/ftrace.c
>> > > > +++ b/kernel/trace/ftrace.c
>> > > > @@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self,
>> > > >  	struct module *mod = data;
>> > > >
>> > > >  	switch (val) {
>> > > > -	case MODULE_STATE_COMING:
>> > > > -		ftrace_module_enable(mod);
>> > > > -		break;
>> > > >  	case MODULE_STATE_GOING:
>> > > >  		ftrace_release_mod(mod);
>> > > >  		break;
>> > >
>> > > If we end up doing something like this, I would just say punt and have
>> > > the ftrace code be hardcoded into the module code and remove the
>> > > notifiers completely. ftrace (and live kernel patching for that matter)
>> > > are rather special. They are not a filesystem or driver. They are core
>> > > utilities and having them called directly from the module code may be
>> > > prudent and better to understand and control.
>> >
>> > Agreed, and we might as well make this change now to avoid more churn
>> > later.
>>
>> It is possible to achieve the same goal even with the notifiers. They are
>> processed synchronously in complete_formation(). So we can put our klp
>> hook after that, right? Or better, put it to load_module() after
>> complete_formation() call. There is an error handling code even today
>> (that is, parse_args() or mod_sysfs_setup() can fail). Moreover, we'll
>> have a hook there with Jessica's relocation rework patch set.
>
>Well, my feeling is that we should really apply livepatch relocations
>before allowing any other notifiers to run, in case the relocations
>affect them.  But it's just a feeling; I don't have any specific
>examples to justify it (yet).

So what I'm gathering from this discussion is that we are leaning
towards completely removing the ftrace notifier in favor of inserting direct
calls to ftrace_module_enable() and ftrace_release_mod() in the
module loader, as well as removing the livepatch coming module notifier
and hard-coding klp_module_enable() (formerly klp_module_notify_coming).

Since we're already doing all that, might we be able to just completely remove
the klp notifiers altogether as well, and replace klp_module_notify_going()
with something like klp_module_disable()? This way everything is symmetrical.

Then the whole thing might look something like this -

diff --git a/kernel/module.c b/kernel/module.c
index 8358f46..eccd289 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -979,8 +979,12 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 	/* Final destruction now no one is using it. */
 	if (mod->exit != NULL)
 		mod->exit();
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	klp_module_disable(mod);
+	ftrace_release_mod(mod);
+
 	async_synchronize_full();
 
 	/* Store the name of the last unloaded module for diagnostic purposes */
@@ -3371,6 +3375,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	mod->state = MODULE_STATE_COMING;
 	mutex_unlock(&module_mutex);
 
+	ftrace_module_enable(mod);
+	err = klp_module_enable(mod); // write all relocations before calling coming notifiers
+	if (err) {
+		ftrace_release_mod(mod);
+		goto out;
+	}
+
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_COMING, mod);
 	return 0;

The function call ordering here should emulate the same ordering were they
notifiers instead, with the priorities Josh suggested in the other mail.

On module load, ftrace_module_enable() is called first (as if it had priority
INT_MAX), then klp_module_enable() (INT_MAX-1), then the rest of the coming
notifier call chain. For the GOING part, the going call chain is called first,
then klp_module_disable() (INT_MIN+1), then ftrace_release_mod() last (INT_MIN).

Note: There are multiple places where the GOING notifiers are called (i.e. in
delete_module(), and in the error paths for do_init_module() and
load_module()), but the calls would look the same there as well.

Does this all sound OK?

Thanks,
Jessica

Re: livepatch: Implement separate coming and going module notifiers

[Steven Rostedt]

On Fri, 29 Jan 2016 17:58:29 -0500
Jessica Yu <jeyu@redhat.com> wrote:

> diff --git a/kernel/module.c b/kernel/module.c
> index 8358f46..eccd289 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -979,8 +979,12 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  	/* Final destruction now no one is using it. */
>  	if (mod->exit != NULL)
>  		mod->exit();
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
> +	klp_module_disable(mod);
> +	ftrace_release_mod(mod);
> +
>  	async_synchronize_full();
>  
>  	/* Store the name of the last unloaded module for diagnostic purposes */
> @@ -3371,6 +3375,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	mod->state = MODULE_STATE_COMING;
>  	mutex_unlock(&module_mutex);
>  
> +	ftrace_module_enable(mod);
> +	err = klp_module_enable(mod); // write all relocations before calling coming notifiers
> +	if (err) {
> +		ftrace_release_mod(mod);

This isn't needed. If complete_formation() fails with an error, then
its caller (load_module) will do the clean up and call
ftrace_release_mod().

-- Steve

> +		goto out;
> +	}
> +
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_COMING, mod);
>  	return 0;
> 

Re: livepatch: Implement separate coming and going module notifiers

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 05:58:29PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [29/01/16 13:42 -0600]:
> >On Fri, Jan 29, 2016 at 08:25:15PM +0100, Miroslav Benes wrote:
> >>On Fri, 29 Jan 2016, Josh Poimboeuf wrote:
> >>
> >>> On Fri, Jan 29, 2016 at 12:40:14PM -0500, Steven Rostedt wrote:
> >>> > [ Added Rusty, as he's still maintainer of the module code ]
> >>> >
> >>> > On Fri, 29 Jan 2016 11:30:10 -0600
> >>> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >>> >
> >>> > > On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
> >>> > > > 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.
> >>> > >
> >>> > > Yeah, we'll need something like that eventually.  Though we'll need to
> >>> > > make sure that ftrace_module_enable() is still called beforehand, after
> >>> > > setting MODULE_STATE_COMING state, due to the race described in 5156dca.
> >>> > >
> >>> > > Something like:
> >>> > >
> >>> > > [note: klp_module_notify_coming() is replaced with klp_module_enable()]
> >>> > >
> >>> > > diff --git a/kernel/module.c b/kernel/module.c
> >>> > > index 8358f46..aeabd81 100644
> >>> > > --- a/kernel/module.c
> >>> > > +++ b/kernel/module.c
> >>> > > @@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
> >>> > >  	mod->state = MODULE_STATE_COMING;
> >>> > >  	mutex_unlock(&module_mutex);
> >>> > >
> >>> > > +	ftrace_module_enable(mod);
> >>> > > +	err = klp_module_enable(mod);
> >>> > > +	if (err) {
> >>> > > +		ftrace_release_mod(mod);
> >>> > > +		return err;
> >>> > > +	}
> >>> > > +
> >>> > >  	blocking_notifier_call_chain(&module_notify_list,
> >>> > >  				     MODULE_STATE_COMING, mod);
> >>> > >  	return 0;
> >>> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> >>> > > index eca592f..c42cf37 100644
> >>> > > --- a/kernel/trace/ftrace.c
> >>> > > +++ b/kernel/trace/ftrace.c
> >>> > > @@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self,
> >>> > >  	struct module *mod = data;
> >>> > >
> >>> > >  	switch (val) {
> >>> > > -	case MODULE_STATE_COMING:
> >>> > > -		ftrace_module_enable(mod);
> >>> > > -		break;
> >>> > >  	case MODULE_STATE_GOING:
> >>> > >  		ftrace_release_mod(mod);
> >>> > >  		break;
> >>> >
> >>> > If we end up doing something like this, I would just say punt and have
> >>> > the ftrace code be hardcoded into the module code and remove the
> >>> > notifiers completely. ftrace (and live kernel patching for that matter)
> >>> > are rather special. They are not a filesystem or driver. They are core
> >>> > utilities and having them called directly from the module code may be
> >>> > prudent and better to understand and control.
> >>>
> >>> Agreed, and we might as well make this change now to avoid more churn
> >>> later.
> >>
> >>It is possible to achieve the same goal even with the notifiers. They are
> >>processed synchronously in complete_formation(). So we can put our klp
> >>hook after that, right? Or better, put it to load_module() after
> >>complete_formation() call. There is an error handling code even today
> >>(that is, parse_args() or mod_sysfs_setup() can fail). Moreover, we'll
> >>have a hook there with Jessica's relocation rework patch set.
> >
> >Well, my feeling is that we should really apply livepatch relocations
> >before allowing any other notifiers to run, in case the relocations
> >affect them.  But it's just a feeling; I don't have any specific
> >examples to justify it (yet).
> 
> So what I'm gathering from this discussion is that we are leaning
> towards completely removing the ftrace notifier in favor of inserting direct
> calls to ftrace_module_enable() and ftrace_release_mod() in the
> module loader, as well as removing the livepatch coming module notifier
> and hard-coding klp_module_enable() (formerly klp_module_notify_coming).
> 
> Since we're already doing all that, might we be able to just completely remove
> the klp notifiers altogether as well, and replace klp_module_notify_going()
> with something like klp_module_disable()? This way everything is symmetrical.
> 
> Then the whole thing might look something like this -
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 8358f46..eccd289 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -979,8 +979,12 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> 	/* Final destruction now no one is using it. */
> 	if (mod->exit != NULL)
> 		mod->exit();
> 	blocking_notifier_call_chain(&module_notify_list,
> 				     MODULE_STATE_GOING, mod);
> +	klp_module_disable(mod);
> +	ftrace_release_mod(mod);
> +
> 	async_synchronize_full();
> 
> 	/* Store the name of the last unloaded module for diagnostic purposes */
> @@ -3371,6 +3375,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
> 	mod->state = MODULE_STATE_COMING;
> 	mutex_unlock(&module_mutex);
> 
> +	ftrace_module_enable(mod);
> +	err = klp_module_enable(mod); // write all relocations before calling coming notifiers
> +	if (err) {
> +		ftrace_release_mod(mod);
> +		goto out;
> +	}
> +
> 	blocking_notifier_call_chain(&module_notify_list,
> 				     MODULE_STATE_COMING, mod);
> 	return 0;
> 
> The function call ordering here should emulate the same ordering were they
> notifiers instead, with the priorities Josh suggested in the other mail.
> 
> On module load, ftrace_module_enable() is called first (as if it had priority
> INT_MAX), then klp_module_enable() (INT_MAX-1), then the rest of the coming
> notifier call chain. For the GOING part, the going call chain is called first,
> then klp_module_disable() (INT_MIN+1), then ftrace_release_mod() last (INT_MIN).
> 
> Note: There are multiple places where the GOING notifiers are called (i.e. in
> delete_module(), and in the error paths for do_init_module() and
> load_module()), but the calls would look the same there as well.
> 
> Does this all sound OK?

Sounds good to me :-)

-- 
Josh

Re: livepatch: Implement separate coming and going module notifiers

[Jessica Yu]

+++ Josh Poimboeuf [29/01/16 11:30 -0600]:
>On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
>> 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.
>
>Yeah, we'll need something like that eventually.  Though we'll need to
>make sure that ftrace_module_enable() is still called beforehand, after
>setting MODULE_STATE_COMING state, due to the race described in 5156dca.
>
>Something like:
>
>[note: klp_module_notify_coming() is replaced with klp_module_enable()]
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 8358f46..aeabd81 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
> 	mod->state = MODULE_STATE_COMING;
> 	mutex_unlock(&module_mutex);
>
>+	ftrace_module_enable(mod);
>+	err = klp_module_enable(mod);
>+	if (err) {
>+		ftrace_release_mod(mod);
>+		return err;
>+	}

If we go this route, should we should print a big warning ("Livepatch
couldn't patch loading module X") instead of aborting the module load
completely? 

>+
> 	blocking_notifier_call_chain(&module_notify_list,
> 				     MODULE_STATE_COMING, mod);
> 	return 0;
>diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>index eca592f..c42cf37 100644
>--- a/kernel/trace/ftrace.c
>+++ b/kernel/trace/ftrace.c
>@@ -5045,9 +5045,6 @@ static int ftrace_module_notify(struct notifier_block *self,
> 	struct module *mod = data;
>
> 	switch (val) {
>-	case MODULE_STATE_COMING:
>-		ftrace_module_enable(mod);
>-		break;
> 	case MODULE_STATE_GOING:
> 		ftrace_release_mod(mod);
> 		break;
>
>-- 
>Josh

Re: livepatch: Implement separate coming and going module notifiers

[Steven Rostedt]

On Fri, 29 Jan 2016 15:04:51 -0500
Jessica Yu <jeyu@redhat.com> wrote:

> >diff --git a/kernel/module.c b/kernel/module.c
> >index 8358f46..aeabd81 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
> > 	mod->state = MODULE_STATE_COMING;
> > 	mutex_unlock(&module_mutex);
> >
> >+	ftrace_module_enable(mod);
> >+	err = klp_module_enable(mod);
> >+	if (err) {
> >+		ftrace_release_mod(mod);
> >+		return err;
> >+	}  
> 
> If we go this route, should we should print a big warning ("Livepatch
> couldn't patch loading module X") instead of aborting the module load
> completely? 

If anything, that should be done in klp_module_enable() not in the
module code.

-- Steve
 

Re: livepatch: Implement separate coming and going module notifiers

[Steven Rostedt]

On Fri, 29 Jan 2016 15:09:31 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 29 Jan 2016 15:04:51 -0500
> Jessica Yu <jeyu@redhat.com> wrote:
> 
> > >diff --git a/kernel/module.c b/kernel/module.c
> > >index 8358f46..aeabd81 100644
> > >--- a/kernel/module.c
> > >+++ b/kernel/module.c
> > >@@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
> > > 	mod->state = MODULE_STATE_COMING;
> > > 	mutex_unlock(&module_mutex);
> > >
> > >+	ftrace_module_enable(mod);
> > >+	err = klp_module_enable(mod);
> > >+	if (err) {
> > >+		ftrace_release_mod(mod);
> > >+		return err;
> > >+	}    
> > 
> > If we go this route, should we should print a big warning ("Livepatch
> > couldn't patch loading module X") instead of aborting the module load
> > completely?   
> 
> If anything, that should be done in klp_module_enable() not in the
> module code.

And I take that back. The module wont load. The user will notice that
before any console warning. Thus no warning needed.

-- Steve

Re: livepatch: Implement separate coming and going module notifiers

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 03:04:51PM -0500, Jessica Yu wrote:
> +++ Josh Poimboeuf [29/01/16 11:30 -0600]:
> >On Fri, Jan 29, 2016 at 05:30:46PM +0100, Miroslav Benes wrote:
> >>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.
> >
> >Yeah, we'll need something like that eventually.  Though we'll need to
> >make sure that ftrace_module_enable() is still called beforehand, after
> >setting MODULE_STATE_COMING state, due to the race described in 5156dca.
> >
> >Something like:
> >
> >[note: klp_module_notify_coming() is replaced with klp_module_enable()]
> >
> >diff --git a/kernel/module.c b/kernel/module.c
> >index 8358f46..aeabd81 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -3371,6 +3371,13 @@ static int complete_formation(struct module *mod, struct load_info *info)
> >	mod->state = MODULE_STATE_COMING;
> >	mutex_unlock(&module_mutex);
> >
> >+	ftrace_module_enable(mod);
> >+	err = klp_module_enable(mod);
> >+	if (err) {
> >+		ftrace_release_mod(mod);
> >+		return err;
> >+	}
> 
> If we go this route, should we should print a big warning ("Livepatch
> couldn't patch loading module X") instead of aborting the module load
> completely?

I think aborting the module load is better.  Otherwise the patch would
be applied in an inconsistent state.  Which might not be all that bad
now, since we don't have a consistency model anyway; but it could be
disastrous once we get one, if somebody is relying on that consistency.

-- 
Josh

[PATCH 2/2] ftrace: Adjust priority of ftrace module notifier

[Jessica Yu]

Adjust the priority of the ftrace module notifier such that it 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@redhat.com>
---
 kernel/trace/ftrace.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index eca592f..bdd7bfc 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5067,7 +5067,12 @@ static int ftrace_module_notify(struct notifier_block *self,
 
 struct notifier_block ftrace_module_nb = {
 	.notifier_call = ftrace_module_notify,
-	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
+	/*
+	 * Run after anything that can remove kprobes and
+	 * after livepatch's going notifier, but run before
+	 * livepatch's coming notifier.
+	 */
+	.priority = INT_MIN+1,
 };
 
 void __init ftrace_init(void)
-- 
2.4.3

Re: [PATCH 2/2] ftrace: Adjust priority of ftrace module notifier

[Steven Rostedt]

On Fri, 29 Jan 2016 01:43:47 -0500
Jessica Yu <jeyu@redhat.com> wrote:


> ---
>  kernel/trace/ftrace.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index eca592f..bdd7bfc 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5067,7 +5067,12 @@ static int ftrace_module_notify(struct notifier_block *self,
>  
>  struct notifier_block ftrace_module_nb = {
>  	.notifier_call = ftrace_module_notify,
> -	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
> +	/*
> +	 * Run after anything that can remove kprobes and
> +	 * after livepatch's going notifier, but run before
> +	 * livepatch's coming notifier.
> +	 */
> +	.priority = INT_MIN+1,
>  };
>  
>  void __init ftrace_init(void)

Actually, I rather break up the ftrace notifiers into two different
notifiers. One for coming and one for going (I use to have that before
hard coding the module updates in the module code).

Have the coming notifier be INT_MAX, where it runs before everything
else (which it should, as tracing should be enabled then). And have the
module going to stay INT_MIN to run after everything.

-- Steve

Re: [PATCH 2/2] ftrace: Adjust priority of ftrace module notifier

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 09:38:00AM -0500, Steven Rostedt wrote:
> On Fri, 29 Jan 2016 01:43:47 -0500
> Jessica Yu <jeyu@redhat.com> wrote:
> 
> 
> > ---
> >  kernel/trace/ftrace.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index eca592f..bdd7bfc 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -5067,7 +5067,12 @@ static int ftrace_module_notify(struct notifier_block *self,
> >  
> >  struct notifier_block ftrace_module_nb = {
> >  	.notifier_call = ftrace_module_notify,
> > -	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
> > +	/*
> > +	 * Run after anything that can remove kprobes and
> > +	 * after livepatch's going notifier, but run before
> > +	 * livepatch's coming notifier.
> > +	 */
> > +	.priority = INT_MIN+1,
> >  };
> >  
> >  void __init ftrace_init(void)
> 
> Actually, I rather break up the ftrace notifiers into two different
> notifiers. One for coming and one for going (I use to have that before
> hard coding the module updates in the module code).
> 
> Have the coming notifier be INT_MAX, where it runs before everything
> else (which it should, as tracing should be enabled then). And have the
> module going to stay INT_MIN to run after everything.

That sounds good to me.

If we do that then I still think it would be a good idea to split up the
livepatch notifiers, with:

- INT_MAX-1 for coming so that relocations are all written before any
  other notifiers (besides ftrace) get a chance to run.

- INT_MIN-1 for going.  I don't have a good specific reason, but I think
  the symmetry will create less surprises and possibly fewer bugs if the
  module's patched state as seen by the other notifiers is the same for
  coming and going.

-- 
Josh

Re: [PATCH 2/2] ftrace: Adjust priority of ftrace module notifier

[Steven Rostedt]

On Fri, 29 Jan 2016 09:45:05 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> If we do that then I still think it would be a good idea to split up the
> livepatch notifiers, with:
> 
> - INT_MAX-1 for coming so that relocations are all written before any
>   other notifiers (besides ftrace) get a chance to run.
> 
> - INT_MIN-1 for going.  I don't have a good specific reason, but I think

Do you mean INT_MIN+1 ?

-- Steve

>   the symmetry will create less surprises and possibly fewer bugs if the
>   module's patched state as seen by the other notifiers is the same for
>   coming and going.
> 

Re: [PATCH 2/2] ftrace: Adjust priority of ftrace module notifier

[Josh Poimboeuf]

On Fri, Jan 29, 2016 at 10:49:50AM -0500, Steven Rostedt wrote:
> On Fri, 29 Jan 2016 09:45:05 -0600
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> 
> > If we do that then I still think it would be a good idea to split up the
> > livepatch notifiers, with:
> > 
> > - INT_MAX-1 for coming so that relocations are all written before any
> >   other notifiers (besides ftrace) get a chance to run.
> > 
> > - INT_MIN-1 for going.  I don't have a good specific reason, but I think
> 
> Do you mean INT_MIN+1 ?

Er, yeah.

> 
> -- Steve
> 
> >   the symmetry will create less surprises and possibly fewer bugs if the
> >   module's patched state as seen by the other notifiers is the same for
> >   coming and going.
> > 
> 

-- 
Josh