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

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

[Steven Rostedt]

On Fri, 29 Jan 2016 09:45:05 -0600
Josh Poimboeuf  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 09:38:00AM -0500, Steven Rostedt wrote:
> On Fri, 29 Jan 2016 01:43:47 -0500
> Jessica Yu  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 01:43:47 -0500
Jessica Yu  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

[Steven Rostedt]

On Fri, 29 Jan 2016 01:43:47 -0500
Jessica Yu  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  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

[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  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

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

[Steven Rostedt]

On Fri, 29 Jan 2016 09:45:05 -0600
Josh Poimboeuf  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.
> 

[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 
---
 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

[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 
---
 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