Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1635637 > unrolled thread

[PATCH 0/3] livepatch/rcu: Handle some subtle issues between livepatching and RCU

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] livepatch/rcu: Handle some subtle issues between livepatching and RCU Petr Mladek <pmladek@suse.com> - 2017-05-04 13:00 +0200
    [PATCH 1/3] livepatch/rcu: Guarantee consistency when patching idle kthreads Petr Mladek <pmladek@suse.com> - 2017-05-04 13:00 +0200
    [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code Petr Mladek <pmladek@suse.com> - 2017-05-04 13:00 +0200
      Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-08 19:00 +0200
        Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Steven Rostedt <rostedt@goodmis.org> - 2017-05-08 21:20 +0200
          Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-08 21:50 +0200
            Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-08 22:20 +0200
              Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-08 22:50 +0200
                Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-08 23:00 +0200
                  Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-08 23:10 +0200
                Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-08 23:10 +0200
                  Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Steven Rostedt <rostedt@goodmis.org> - 2017-05-08 23:20 +0200
                    Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-08 23:40 +0200
                  Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 00:20 +0200
                    Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-09 00:40 +0200
                      Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-09 18:20 +0200
                        Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-09 18:40 +0200
                        Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Petr Mladek <pmladek@suse.com> - 2017-05-10 18:10 +0200
                          Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-10 18:50 +0200
                          Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-10 20:00 +0200
                            Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken  in RCU code Miroslav Benes <mbenes@suse.cz> - 2017-05-11 14:50 +0200
                              Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-11 17:10 +0200
                Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Steven Rostedt <rostedt@goodmis.org> - 2017-05-08 23:20 +0200
            Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Steven Rostedt <rostedt@goodmis.org> - 2017-05-08 22:20 +0200
              Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken  in RCU code Miroslav Benes <mbenes@suse.cz> - 2017-05-11 15:00 +0200
          Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Petr Mladek <pmladek@suse.com> - 2017-05-11 16:00 +0200
            Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-11 17:00 +0200
            Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-11 17:30 +0200
        Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is  broken in RCU code Petr Mladek <pmladek@suse.com> - 2017-05-11 14:50 +0200
    Re: [PATCH 0/3] livepatch/rcu: Handle some subtle issues between  livepatching and RCU "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-04 19:00 +0200

Page 1 of 2  [1] 2  Next page →

#1635637 — [PATCH 0/3] livepatch/rcu: Handle some subtle issues between livepatching and RCU

Steven and Paul recently discussed some issues when using RCU
functionality in ftrace handlers. A good summary can be found at
https://lkml.kernel.org/r/20170412115304.3077dbc8@gandalf.local.home

This discussion made us to revisit the ftrace handler used by
the livepatches. Some changes seem to be needed. A perfect solution
looks rather complicated. I have implemented a sub-optimal
one and split it into three patches for easier review.

Please, note that we were on the safe side before introducing
the hybrid consistency model. The ftrace handler worked correctly
with empty function stack. Also the patch removal was not possible.
But we need to be more careful now.


Petr Mladek (3):
  livepatch/rcu: Guarantee consistency when patching idle kthreads
  livepatch/rcu: Warn when system consistency is broken in RCU code
  livepatch/rcu: Disable livepatch removal when safety is not guaranteed

 Documentation/livepatch/livepatch.txt | 19 +++++++++++++++++++
 kernel/livepatch/patch.c              | 14 ++++++++++++++
 kernel/livepatch/transition.c         |  7 ++++++-
 kernel/livepatch/transition.h         |  2 ++
 4 files changed, 41 insertions(+), 1 deletion(-)

-- 
1.8.5.6

[toc] | [next] | [standalone]

#1635638 — [PATCH 1/3] livepatch/rcu: Guarantee consistency when patching idle kthreads

RCU is not watching idle threads because they are not scheduled
on busy CPUs and might block finishing grace periods. As a result,
the livepatch ftrace handler might see ops->func_stack and other
flags in a wrong state. Then a livepatch might make the system
unstable.

Note that there might be serious consequences only when the livepatch
modifies semantic of functions used by idle kthreads. We are safe when
none of the patched functions is used by the idle kthreads. Also
everything is good when the functions might be changed one by one
(using the immediate flag). See Documentation/livepatch/livepatch.txt
for more details about the consistency model.

This patch makes sure that even the idle threads see the critical
section by calling rcu_irq_enter_irqson(). The same fix was used
also for the stack tracer, see the commit a2d7629048322ae62b
("tracing: Have stack tracer force RCU to be watching").

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/patch.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index f8269036bf0b..4c4fbe409008 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -59,6 +59,9 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 
 	ops = container_of(fops, struct klp_ops, fops);
 
+	/* RCU may not be watching, make it see us. */
+	rcu_irq_enter_irqson();
+
 	rcu_read_lock();
 
 	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
@@ -116,6 +119,7 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	rcu_read_unlock();
+	rcu_irq_exit_irqson();
 }
 
 /*
-- 
1.8.5.6

[toc] | [prev] | [next] | [standalone]

#1635639 — [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

RCU is not watching inside some RCU code. As a result, the livepatch
ftrace handler might see ops->func_stack and some other flags in
a wrong state. Then a livepatch might make the system unstable.

Note that there might be serious consequences only when the livepatch
modifies semantic of functions used by some parts of RCU infrastructure.
We are safe when none of the patched functions is used by this RCU code.
Also everything is good when the functions might be changed one by one
(using the immediate flag). See Documentation/livepatch/livepatch.txt
for more details about the consistency model.

Fortunately, the sensitive parts of the RCU code are annotated
and could be detected. This patch adds a warning when an insecure
usage is detected.

Unfortunately, the ftrace handler might be called when the problematic
patch has already been removed from ops->func stack. In this case,
it is not able to read the immediate flag. It makes the check
unreliable. We rather avoid it and report the problem even when
the system stability is not affected.

It would be possible to add some more complex logic to avoid
warnings when RCU infrastructure is modified using immediate
patches. But let's keep it simple until real life experience
forces us to do the opposite.

This patch is inspired by similar problems solved in stack
tracer, see
https://lkml.kernel.org/r/20170412115304.3077dbc8@gandalf.local.home

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/livepatch/livepatch.txt | 15 +++++++++++++++
 kernel/livepatch/patch.c              |  8 ++++++++
 2 files changed, 23 insertions(+)

diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index ecdb18104ab0..92619f7d86fd 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -476,6 +476,21 @@ The current Livepatch implementation has several limitations:
     by "notrace".
 
 
+  + Limited patching of RCU infrastructure
+
+    The livepatch ftrace handler uses RCU for handling the stack of patches.
+    Also the ftrace framework uses RCU to detect when the handlers are not
+    longer in use.
+
+    The directly used RCU functions could not be patched. Otherwise,
+    there would be an infinite recursion.
+
+    Some other RCU functions can not be patched safely because the RCU
+    framework might be in an inconsistent state. This context is annotated
+    and warning is printed. In theory, it might be safe to modify such
+    functions using immediate patches. But this is hard to detect properly
+    in the ftrace handler, so the warning is always printed.
+
 
   + Livepatch works reliably only when the dynamic ftrace is located at
     the very beginning of the function.
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 4c4fbe409008..ffdf5fa8005b 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -62,6 +62,14 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	/* RCU may not be watching, make it see us. */
 	rcu_irq_enter_irqson();
 
+	/*
+	 * RCU still might not see us if we patch a function inside
+	 * the RCU infrastructure. Then we might see wrong state of
+	 * func->stack and other flags.
+	 */
+	if (unlikely(!rcu_is_watching()))
+		WARN_ONCE(1, "Livepatch modified a function that can not be handled a safe way.!");
+
 	rcu_read_lock();
 
 	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
-- 
1.8.5.6

[toc] | [prev] | [next] | [standalone]

#1637588 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Thu, May 04, 2017 at 12:55:15PM +0200, Petr Mladek wrote:
> RCU is not watching inside some RCU code. As a result, the livepatch
> ftrace handler might see ops->func_stack and some other flags in
> a wrong state. Then a livepatch might make the system unstable.
> 
> Note that there might be serious consequences only when the livepatch
> modifies semantic of functions used by some parts of RCU infrastructure.
> We are safe when none of the patched functions is used by this RCU code.
> Also everything is good when the functions might be changed one by one
> (using the immediate flag). See Documentation/livepatch/livepatch.txt
> for more details about the consistency model.
> 
> Fortunately, the sensitive parts of the RCU code are annotated
> and could be detected. This patch adds a warning when an insecure
> usage is detected.
> 
> Unfortunately, the ftrace handler might be called when the problematic
> patch has already been removed from ops->func stack. In this case,
> it is not able to read the immediate flag. It makes the check
> unreliable. We rather avoid it and report the problem even when
> the system stability is not affected.
> 
> It would be possible to add some more complex logic to avoid
> warnings when RCU infrastructure is modified using immediate
> patches. But let's keep it simple until real life experience
> forces us to do the opposite.
> 
> This patch is inspired by similar problems solved in stack
> tracer, see
> https://lkml.kernel.org/r/20170412115304.3077dbc8@gandalf.local.home
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  Documentation/livepatch/livepatch.txt | 15 +++++++++++++++
>  kernel/livepatch/patch.c              |  8 ++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
> index ecdb18104ab0..92619f7d86fd 100644
> --- a/Documentation/livepatch/livepatch.txt
> +++ b/Documentation/livepatch/livepatch.txt
> @@ -476,6 +476,21 @@ The current Livepatch implementation has several limitations:
>      by "notrace".
>  
>  
> +  + Limited patching of RCU infrastructure
> +
> +    The livepatch ftrace handler uses RCU for handling the stack of patches.
> +    Also the ftrace framework uses RCU to detect when the handlers are not
> +    longer in use.
> +
> +    The directly used RCU functions could not be patched. Otherwise,
> +    there would be an infinite recursion.
> +
> +    Some other RCU functions can not be patched safely because the RCU
> +    framework might be in an inconsistent state. This context is annotated
> +    and warning is printed. In theory, it might be safe to modify such
> +    functions using immediate patches. But this is hard to detect properly
> +    in the ftrace handler, so the warning is always printed.
> +
>    + Livepatch works reliably only when the dynamic ftrace is located at
>      the very beginning of the function.
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 4c4fbe409008..ffdf5fa8005b 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -62,6 +62,14 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>  	/* RCU may not be watching, make it see us. */
>  	rcu_irq_enter_irqson();
>  
> +	/*
> +	 * RCU still might not see us if we patch a function inside
> +	 * the RCU infrastructure. Then we might see wrong state of
> +	 * func->stack and other flags.
> +	 */
> +	if (unlikely(!rcu_is_watching()))
> +		WARN_ONCE(1, "Livepatch modified a function that can not be handled a safe way.!");
> +
>  	rcu_read_lock();
>  
>  	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,

So the ftrace stack tracer seems to do this check a little differently.
It calls rcu_irq_enter_disabled() first, and then calls rcu_irq_enter().
Any reason we're not doing it that way?

The warning would be more helpful if it printed a little more
information, like the ip and the parent_ip.  And it should probably
mention that RCU is broken.

Also I wonder if we can constrain the warning somehow.  I think the
warning only applies if a patch is in progress, right?  In that case, if
RCU is broken, would it be feasible to mutex_trylock() the klp mutex to
try to ensure that no patches are being applied while the ftrace handler
is running?  Then it wouldn't matter if RCU were broken because the func
stack wouldn't be changing anyway.  Then it could only warn if it failed
to get the mutex.

Stepping back a bit, the documentation and comments describe patches to
functions inside the RCU infrastructure.  As far as I can tell, only a
single function would be affected: rcu_dynticks_eqs_enter().  Because
it's the only function called after the "Breaks tracing momentarily"
comment in rcu_eqs_enter_common().  Any reason why we couldn't just
annotate rcu_dynticks_eqs_enter() with notrace?

Stepping back even further, if I'm understanding this issue correctly,
this warning can also affect patches to functions which are called from
NMI context.  If the NMI occurs in the part of rcu_eqs_enter_common()
where rcu_irq_enter() doesn't work, then RCU won't work here, right?  If
so, that worries me because there are a lot of functions which can be
called (and patched) from NMI context.

I wonder if there's some way to solve this by changing RCU code, but I'm
not familiar enough with RCU to have any ideas there.

Another idea would be to figure out a way to stop using RCU in
klp_ftrace_handler() altogether.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1637661 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, 8 May 2017 11:51:08 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> > --- a/kernel/livepatch/patch.c
> > +++ b/kernel/livepatch/patch.c
> > @@ -62,6 +62,14 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> >  	/* RCU may not be watching, make it see us. */
> >  	rcu_irq_enter_irqson();
> >  
> > +	/*
> > +	 * RCU still might not see us if we patch a function inside
> > +	 * the RCU infrastructure. Then we might see wrong state of
> > +	 * func->stack and other flags.
> > +	 */
> > +	if (unlikely(!rcu_is_watching()))
> > +		WARN_ONCE(1, "Livepatch modified a function that can not be handled a safe way.!");
> > +
> >  	rcu_read_lock();
> >  
> >  	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,  
> 
> So the ftrace stack tracer seems to do this check a little differently.
> It calls rcu_irq_enter_disabled() first, and then calls rcu_irq_enter().
> Any reason we're not doing it that way?

I thought the same. Also can it not return, and not do anything.
Because continuing after the warning, is dangerous.

Not to mention, why the if statement in the first place? And then pass
a 1. Makes no sense.

Although you should have:

	if (WARN_ONCE(!rcu_is_watching,
			"Livepatch ..."))
		return;

or something to not cause any damage.

> 
> The warning would be more helpful if it printed a little more
> information, like the ip and the parent_ip.  And it should probably
> mention that RCU is broken.

Well, the warning would also print a stack trace. How is RCU broken? It
could simply be that you are patching a function that is in a place
that RCU doesn't "watch". Like going to idle or userspace. Or even in
RCU itself.

> 
> Also I wonder if we can constrain the warning somehow.  I think the
> warning only applies if a patch is in progress, right?  In that case, if
> RCU is broken, would it be feasible to mutex_trylock() the klp mutex to
> try to ensure that no patches are being applied while the ftrace handler
> is running?  Then it wouldn't matter if RCU were broken because the func
> stack wouldn't be changing anyway.  Then it could only warn if it failed
> to get the mutex.

How would RCU be broken?

> 
> Stepping back a bit, the documentation and comments describe patches to
> functions inside the RCU infrastructure.  As far as I can tell, only a
> single function would be affected: rcu_dynticks_eqs_enter().  Because
> it's the only function called after the "Breaks tracing momentarily"
> comment in rcu_eqs_enter_common().  Any reason why we couldn't just
> annotate rcu_dynticks_eqs_enter() with notrace?

Note, there's places in the kernel (on the way to idle and userspace)
that rcu is not watching. Ftrace handles this differently than most
places. But anything that requires calling rcu_read_lock(), well, you
need to beware.

> 
> Stepping back even further, if I'm understanding this issue correctly,
> this warning can also affect patches to functions which are called from
> NMI context.  If the NMI occurs in the part of rcu_eqs_enter_common()
> where rcu_irq_enter() doesn't work, then RCU won't work here, right?  If
> so, that worries me because there are a lot of functions which can be
> called (and patched) from NMI context.

Note, the "rcu_dynticks_eqs_enter()" is the only place that can't make
rcu "watch" again.

If rcu is not watching, calling rcu_enter_irq() will have it watch
again. Even in NMI context I believe.

> 
> I wonder if there's some way to solve this by changing RCU code, but I'm
> not familiar enough with RCU to have any ideas there.

You don't want to go there.

> 
> Another idea would be to figure out a way to stop using RCU in
> klp_ftrace_handler() altogether.
> 

That may work if rcu_enter_irq() doesn't. But that's how NMIs use rcu.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1637678 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 03:13:22PM -0400, Steven Rostedt wrote:
> On Mon, 8 May 2017 11:51:08 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > --- a/kernel/livepatch/patch.c
> > > +++ b/kernel/livepatch/patch.c
> > > @@ -62,6 +62,14 @@ static void notrace klp_ftrace_handler(unsigned long ip,
> > >  	/* RCU may not be watching, make it see us. */
> > >  	rcu_irq_enter_irqson();
> > >  
> > > +	/*
> > > +	 * RCU still might not see us if we patch a function inside
> > > +	 * the RCU infrastructure. Then we might see wrong state of
> > > +	 * func->stack and other flags.
> > > +	 */
> > > +	if (unlikely(!rcu_is_watching()))
> > > +		WARN_ONCE(1, "Livepatch modified a function that can not be handled a safe way.!");
> > > +
> > >  	rcu_read_lock();
> > >  
> > >  	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,  
> > 
> > So the ftrace stack tracer seems to do this check a little differently.
> > It calls rcu_irq_enter_disabled() first, and then calls rcu_irq_enter().
> > Any reason we're not doing it that way?
> 
> I thought the same. Also can it not return, and not do anything.
> Because continuing after the warning, is dangerous.
> 
> Not to mention, why the if statement in the first place? And then pass
> a 1. Makes no sense.
> 
> Although you should have:
> 
> 	if (WARN_ONCE(!rcu_is_watching,
> 			"Livepatch ..."))
> 		return;
> 
> or something to not cause any damage.

My understanding is that returning would be more dangerous than
continuing here.

By continuing to run, there's only a small chance that it will get stale
data, which would break the consistency model by executing an old
version of the function and possibly crashing the system.

On the other hand, returning would unconditionally break the consistency
model by *always* executing an old version of the function.  So that
greatly increases the risk of a crash.

> > The warning would be more helpful if it printed a little more
> > information, like the ip and the parent_ip.  And it should probably
> > mention that RCU is broken.
> 
> Well, the warning would also print a stack trace. How is RCU broken? It
> could simply be that you are patching a function that is in a place
> that RCU doesn't "watch". Like going to idle or userspace. Or even in
> RCU itself.

As I understand it, RCU would be "broken" because this ftrace handler
has an RCU read critical section.  And in the case where RCU isn't
watching, rcu_read_lock() will not function as advertised, right?

> > Also I wonder if we can constrain the warning somehow.  I think the
> > warning only applies if a patch is in progress, right?  In that case, if
> > RCU is broken, would it be feasible to mutex_trylock() the klp mutex to
> > try to ensure that no patches are being applied while the ftrace handler
> > is running?  Then it wouldn't matter if RCU were broken because the func
> > stack wouldn't be changing anyway.  Then it could only warn if it failed
> > to get the mutex.
> 
> How would RCU be broken?
> 
> > 
> > Stepping back a bit, the documentation and comments describe patches to
> > functions inside the RCU infrastructure.  As far as I can tell, only a
> > single function would be affected: rcu_dynticks_eqs_enter().  Because
> > it's the only function called after the "Breaks tracing momentarily"
> > comment in rcu_eqs_enter_common().  Any reason why we couldn't just
> > annotate rcu_dynticks_eqs_enter() with notrace?
> 
> Note, there's places in the kernel (on the way to idle and userspace)
> that rcu is not watching. Ftrace handles this differently than most
> places. But anything that requires calling rcu_read_lock(), well, you
> need to beware.
> 
> > 
> > Stepping back even further, if I'm understanding this issue correctly,
> > this warning can also affect patches to functions which are called from
> > NMI context.  If the NMI occurs in the part of rcu_eqs_enter_common()
> > where rcu_irq_enter() doesn't work, then RCU won't work here, right?  If
> > so, that worries me because there are a lot of functions which can be
> > called (and patched) from NMI context.
> 
> Note, the "rcu_dynticks_eqs_enter()" is the only place that can't make
> rcu "watch" again.

So would it make sense to annotate it with 'notrace'?

> If rcu is not watching, calling rcu_enter_irq() will have it watch
> again. Even in NMI context I believe.

What if you get an NMI while running in rcu_dynticks_eqs_enter() before
it increments rdtp->dynticks?  Will rcu_enter_irq() still work from the
NMI?

I'm just trying to understand what are the cases where rcu_enter_irq()
*doesn't* work from an ftrace handler.

> > I wonder if there's some way to solve this by changing RCU code, but I'm
> > not familiar enough with RCU to have any ideas there.
> 
> You don't want to go there.

I believe you :-)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1637694 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 02:47:29PM -0500, Josh Poimboeuf wrote:
> On Mon, May 08, 2017 at 03:13:22PM -0400, Steven Rostedt wrote:

[ . . . ]

> > If rcu is not watching, calling rcu_enter_irq() will have it watch
> > again. Even in NMI context I believe.
> 
> What if you get an NMI while running in rcu_dynticks_eqs_enter() before
> it increments rdtp->dynticks?  Will rcu_enter_irq() still work from the
                                      rcu_irq_enter()
> NMI?

The rcu_nmi_enter() function willl notice that RCU is not watching, and
will therefore atomically increment RCU's dynticks-idle counter, which
will be atomically incremented again upon return.  Since the bottom bit
of this counter controls whether or not RCU is watching, RCU will be
watching during the NMI, will stop watching upon return from the NMI,
which restores state so as to allow rcu_irq_enter() to cause RCU to once
again watch.  (NMI algorithm due to Andy Lutomirski.)

> I'm just trying to understand what are the cases where rcu_enter_irq()
> *doesn't* work from an ftrace handler.

It doesn't work from an NMI handler.  Aside from possible architecture
specific special cases, it should work everywhere else.

> > > I wonder if there's some way to solve this by changing RCU code, but I'm
> > > not familiar enough with RCU to have any ideas there.
> > 
> > You don't want to go there.
> 
> I believe you :-)

Adding a notrace seems simple enough, but some care is indeed required
for more pervasive changes to RCU.  ;-)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1637711 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 01:15:58PM -0700, Paul E. McKenney wrote:
> On Mon, May 08, 2017 at 02:47:29PM -0500, Josh Poimboeuf wrote:
> > On Mon, May 08, 2017 at 03:13:22PM -0400, Steven Rostedt wrote:
> 
> [ . . . ]
> 
> > > If rcu is not watching, calling rcu_enter_irq() will have it watch
> > > again. Even in NMI context I believe.
> > 
> > What if you get an NMI while running in rcu_dynticks_eqs_enter() before
> > it increments rdtp->dynticks?  Will rcu_enter_irq() still work from the
>                                       rcu_irq_enter()
> > NMI?
> 
> The rcu_nmi_enter() function willl notice that RCU is not watching, and
> will therefore atomically increment RCU's dynticks-idle counter, which
> will be atomically incremented again upon return.  Since the bottom bit
> of this counter controls whether or not RCU is watching, RCU will be
> watching during the NMI, will stop watching upon return from the NMI,
> which restores state so as to allow rcu_irq_enter() to cause RCU to once
> again watch.  (NMI algorithm due to Andy Lutomirski.)
> 
> > I'm just trying to understand what are the cases where rcu_enter_irq()
> > *doesn't* work from an ftrace handler.
> 
> It doesn't work from an NMI handler.  Aside from possible architecture
> specific special cases, it should work everywhere else.

Ok, so just to clarify.  Is there a bug in the ftrace stack tracer in
the following situation?

1. RCU isn't watching
2. An NMI hits
3. ist_enter() calls into the ftrace stack tracer, before
   rcu_nmi_enter() is called, so RCU isn't watching yet
4. The ftrace stack tracer calls rcu_irq_enter(), which has no effect,
   so RCU still isn't watching
5. Hilarity ensues in the ftrace stack tracer

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1637712 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 03:43:33PM -0500, Josh Poimboeuf wrote:
> On Mon, May 08, 2017 at 01:15:58PM -0700, Paul E. McKenney wrote:
> > On Mon, May 08, 2017 at 02:47:29PM -0500, Josh Poimboeuf wrote:
> > > On Mon, May 08, 2017 at 03:13:22PM -0400, Steven Rostedt wrote:
> > 
> > [ . . . ]
> > 
> > > > If rcu is not watching, calling rcu_enter_irq() will have it watch
> > > > again. Even in NMI context I believe.
> > > 
> > > What if you get an NMI while running in rcu_dynticks_eqs_enter() before
> > > it increments rdtp->dynticks?  Will rcu_enter_irq() still work from the
> >                                       rcu_irq_enter()
> > > NMI?
> > 
> > The rcu_nmi_enter() function willl notice that RCU is not watching, and
> > will therefore atomically increment RCU's dynticks-idle counter, which
> > will be atomically incremented again upon return.  Since the bottom bit
> > of this counter controls whether or not RCU is watching, RCU will be
> > watching during the NMI, will stop watching upon return from the NMI,
> > which restores state so as to allow rcu_irq_enter() to cause RCU to once
> > again watch.  (NMI algorithm due to Andy Lutomirski.)
> > 
> > > I'm just trying to understand what are the cases where rcu_enter_irq()
> > > *doesn't* work from an ftrace handler.
> > 
> > It doesn't work from an NMI handler.  Aside from possible architecture
> > specific special cases, it should work everywhere else.
> 
> Ok, so just to clarify.  Is there a bug in the ftrace stack tracer in
> the following situation?
> 
> 1. RCU isn't watching
> 2. An NMI hits
> 3. ist_enter() calls into the ftrace stack tracer, before
>    rcu_nmi_enter() is called, so RCU isn't watching yet
> 4. The ftrace stack tracer calls rcu_irq_enter(), which has no effect,
>    so RCU still isn't watching
> 5. Hilarity ensues in the ftrace stack tracer

Hm, technically, ist_enter() is for exceptions other than NMI, so the
question itself is buggy.  I suppose the scenario is still possible if
you replace NMI with a debug exception or a double fault.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1637721 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 03:51:43PM -0500, Josh Poimboeuf wrote:
> On Mon, May 08, 2017 at 03:43:33PM -0500, Josh Poimboeuf wrote:
> > On Mon, May 08, 2017 at 01:15:58PM -0700, Paul E. McKenney wrote:
> > > On Mon, May 08, 2017 at 02:47:29PM -0500, Josh Poimboeuf wrote:
> > > > On Mon, May 08, 2017 at 03:13:22PM -0400, Steven Rostedt wrote:
> > > 
> > > [ . . . ]
> > > 
> > > > > If rcu is not watching, calling rcu_enter_irq() will have it watch
> > > > > again. Even in NMI context I believe.
> > > > 
> > > > What if you get an NMI while running in rcu_dynticks_eqs_enter() before
> > > > it increments rdtp->dynticks?  Will rcu_enter_irq() still work from the
> > >                                       rcu_irq_enter()
> > > > NMI?
> > > 
> > > The rcu_nmi_enter() function willl notice that RCU is not watching, and
> > > will therefore atomically increment RCU's dynticks-idle counter, which
> > > will be atomically incremented again upon return.  Since the bottom bit
> > > of this counter controls whether or not RCU is watching, RCU will be
> > > watching during the NMI, will stop watching upon return from the NMI,
> > > which restores state so as to allow rcu_irq_enter() to cause RCU to once
> > > again watch.  (NMI algorithm due to Andy Lutomirski.)
> > > 
> > > > I'm just trying to understand what are the cases where rcu_enter_irq()
> > > > *doesn't* work from an ftrace handler.
> > > 
> > > It doesn't work from an NMI handler.  Aside from possible architecture
> > > specific special cases, it should work everywhere else.
> > 
> > Ok, so just to clarify.  Is there a bug in the ftrace stack tracer in
> > the following situation?
> > 
> > 1. RCU isn't watching
> > 2. An NMI hits
> > 3. ist_enter() calls into the ftrace stack tracer, before
> >    rcu_nmi_enter() is called, so RCU isn't watching yet
> > 4. The ftrace stack tracer calls rcu_irq_enter(), which has no effect,
> >    so RCU still isn't watching
> > 5. Hilarity ensues in the ftrace stack tracer
> 
> Hm, technically, ist_enter() is for exceptions other than NMI, so the
> question itself is buggy.  I suppose the scenario is still possible if
> you replace NMI with a debug exception or a double fault.

There are some exceptions on some architectures that look to RCU just
like NMIs, which is why RCU has to handle nested NMIs.  ;-)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1637720 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 03:43:33PM -0500, Josh Poimboeuf wrote:
> On Mon, May 08, 2017 at 01:15:58PM -0700, Paul E. McKenney wrote:
> > On Mon, May 08, 2017 at 02:47:29PM -0500, Josh Poimboeuf wrote:
> > > On Mon, May 08, 2017 at 03:13:22PM -0400, Steven Rostedt wrote:
> > 
> > [ . . . ]
> > 
> > > > If rcu is not watching, calling rcu_enter_irq() will have it watch
> > > > again. Even in NMI context I believe.
> > > 
> > > What if you get an NMI while running in rcu_dynticks_eqs_enter() before
> > > it increments rdtp->dynticks?  Will rcu_enter_irq() still work from the
> >                                       rcu_irq_enter()
> > > NMI?
> > 
> > The rcu_nmi_enter() function willl notice that RCU is not watching, and
> > will therefore atomically increment RCU's dynticks-idle counter, which
> > will be atomically incremented again upon return.  Since the bottom bit
> > of this counter controls whether or not RCU is watching, RCU will be
> > watching during the NMI, will stop watching upon return from the NMI,
> > which restores state so as to allow rcu_irq_enter() to cause RCU to once
> > again watch.  (NMI algorithm due to Andy Lutomirski.)
> > 
> > > I'm just trying to understand what are the cases where rcu_enter_irq()
> > > *doesn't* work from an ftrace handler.
> > 
> > It doesn't work from an NMI handler.  Aside from possible architecture
> > specific special cases, it should work everywhere else.
> 
> Ok, so just to clarify.  Is there a bug in the ftrace stack tracer in
> the following situation?
> 
> 1. RCU isn't watching
> 2. An NMI hits
> 3. ist_enter() calls into the ftrace stack tracer, before
>    rcu_nmi_enter() is called, so RCU isn't watching yet
> 4. The ftrace stack tracer calls rcu_irq_enter(), which has no effect,
>    so RCU still isn't watching
> 5. Hilarity ensues in the ftrace stack tracer

This would be a problem if step 2's NMI hit rcu_irq_enter(),
rcu_irq_exit(), and friends in just the wrong place.

I would suggest that ftrace() do something like this...

	if (in_nmi())
		rcu_nmi_enter();
	else
		rcu_irq_enter();

Except that, as Steven will quickly point out, this won't work at the
very edges of the NMI, when NMI_MASK won't be set in preempt_count().

Other thoughts?

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1637730 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, 8 May 2017 14:07:54 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
 
> Except that, as Steven will quickly point out, this won't work at the
> very edges of the NMI, when NMI_MASK won't be set in preempt_count().

I believe those parts of the NMI has "notrace" because it can break
other parts of ftrace too.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1637739 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 05:18:20PM -0400, Steven Rostedt wrote:
> On Mon, 8 May 2017 14:07:54 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > Except that, as Steven will quickly point out, this won't work at the
> > very edges of the NMI, when NMI_MASK won't be set in preempt_count().
> 
> I believe those parts of the NMI has "notrace" because it can break
> other parts of ftrace too.

Should be good clean fun to validate that belief.  ;-)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1637765 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 02:07:54PM -0700, Paul E. McKenney wrote:
> On Mon, May 08, 2017 at 03:43:33PM -0500, Josh Poimboeuf wrote:
> > On Mon, May 08, 2017 at 01:15:58PM -0700, Paul E. McKenney wrote:
> > > On Mon, May 08, 2017 at 02:47:29PM -0500, Josh Poimboeuf wrote:
> > > > On Mon, May 08, 2017 at 03:13:22PM -0400, Steven Rostedt wrote:
> > > 
> > > [ . . . ]
> > > 
> > > > > If rcu is not watching, calling rcu_enter_irq() will have it watch
> > > > > again. Even in NMI context I believe.
> > > > 
> > > > What if you get an NMI while running in rcu_dynticks_eqs_enter() before
> > > > it increments rdtp->dynticks?  Will rcu_enter_irq() still work from the
> > >                                       rcu_irq_enter()
> > > > NMI?
> > > 
> > > The rcu_nmi_enter() function willl notice that RCU is not watching, and
> > > will therefore atomically increment RCU's dynticks-idle counter, which
> > > will be atomically incremented again upon return.  Since the bottom bit
> > > of this counter controls whether or not RCU is watching, RCU will be
> > > watching during the NMI, will stop watching upon return from the NMI,
> > > which restores state so as to allow rcu_irq_enter() to cause RCU to once
> > > again watch.  (NMI algorithm due to Andy Lutomirski.)
> > > 
> > > > I'm just trying to understand what are the cases where rcu_enter_irq()
> > > > *doesn't* work from an ftrace handler.
> > > 
> > > It doesn't work from an NMI handler.  Aside from possible architecture
> > > specific special cases, it should work everywhere else.
> > 
> > Ok, so just to clarify.  Is there a bug in the ftrace stack tracer in
> > the following situation?
> > 
> > 1. RCU isn't watching
> > 2. An NMI hits
> > 3. ist_enter() calls into the ftrace stack tracer, before
> >    rcu_nmi_enter() is called, so RCU isn't watching yet
> > 4. The ftrace stack tracer calls rcu_irq_enter(), which has no effect,
> >    so RCU still isn't watching
> > 5. Hilarity ensues in the ftrace stack tracer
> 
> This would be a problem if step 2's NMI hit rcu_irq_enter(),
> rcu_irq_exit(), and friends in just the wrong place.
> 
> I would suggest that ftrace() do something like this...
> 
> 	if (in_nmi())
> 		rcu_nmi_enter();
> 	else
> 		rcu_irq_enter();
> 
> Except that, as Steven will quickly point out, this won't work at the
> very edges of the NMI, when NMI_MASK won't be set in preempt_count().
> 
> Other thoughts?

Ok.  So I think the livepatch ftrace handler would need the in_nmi()
check, in case it's called early in the NMI.

But on x86, rcu_nmi_enter() is also called in some non-NMI exception
cases, from ist_enter().  So it appears that the in_nmi() check wouldn't
be sufficient.  We might instead need something like:

	if (in_nmi() || in_some_other_exception())
		rcu_nmi_enter();
	else
		rcu_irq_enter();

But unfortunately the in_some_other_exception() function doesn't
currently exist.

So, one more question.  Would it work if we just always called
rcu_nmi_enter()?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1637777 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 05:16:09PM -0500, Josh Poimboeuf wrote:
> On Mon, May 08, 2017 at 02:07:54PM -0700, Paul E. McKenney wrote:
> > On Mon, May 08, 2017 at 03:43:33PM -0500, Josh Poimboeuf wrote:
> > > On Mon, May 08, 2017 at 01:15:58PM -0700, Paul E. McKenney wrote:
> > > > On Mon, May 08, 2017 at 02:47:29PM -0500, Josh Poimboeuf wrote:
> > > > > On Mon, May 08, 2017 at 03:13:22PM -0400, Steven Rostedt wrote:
> > > > 
> > > > [ . . . ]
> > > > 
> > > > > > If rcu is not watching, calling rcu_enter_irq() will have it watch
> > > > > > again. Even in NMI context I believe.
> > > > > 
> > > > > What if you get an NMI while running in rcu_dynticks_eqs_enter() before
> > > > > it increments rdtp->dynticks?  Will rcu_enter_irq() still work from the
> > > >                                       rcu_irq_enter()
> > > > > NMI?
> > > > 
> > > > The rcu_nmi_enter() function willl notice that RCU is not watching, and
> > > > will therefore atomically increment RCU's dynticks-idle counter, which
> > > > will be atomically incremented again upon return.  Since the bottom bit
> > > > of this counter controls whether or not RCU is watching, RCU will be
> > > > watching during the NMI, will stop watching upon return from the NMI,
> > > > which restores state so as to allow rcu_irq_enter() to cause RCU to once
> > > > again watch.  (NMI algorithm due to Andy Lutomirski.)
> > > > 
> > > > > I'm just trying to understand what are the cases where rcu_enter_irq()
> > > > > *doesn't* work from an ftrace handler.
> > > > 
> > > > It doesn't work from an NMI handler.  Aside from possible architecture
> > > > specific special cases, it should work everywhere else.
> > > 
> > > Ok, so just to clarify.  Is there a bug in the ftrace stack tracer in
> > > the following situation?
> > > 
> > > 1. RCU isn't watching
> > > 2. An NMI hits
> > > 3. ist_enter() calls into the ftrace stack tracer, before
> > >    rcu_nmi_enter() is called, so RCU isn't watching yet
> > > 4. The ftrace stack tracer calls rcu_irq_enter(), which has no effect,
> > >    so RCU still isn't watching
> > > 5. Hilarity ensues in the ftrace stack tracer
> > 
> > This would be a problem if step 2's NMI hit rcu_irq_enter(),
> > rcu_irq_exit(), and friends in just the wrong place.
> > 
> > I would suggest that ftrace() do something like this...
> > 
> > 	if (in_nmi())
> > 		rcu_nmi_enter();
> > 	else
> > 		rcu_irq_enter();
> > 
> > Except that, as Steven will quickly point out, this won't work at the
> > very edges of the NMI, when NMI_MASK won't be set in preempt_count().
> > 
> > Other thoughts?
> 
> Ok.  So I think the livepatch ftrace handler would need the in_nmi()
> check, in case it's called early in the NMI.
> 
> But on x86, rcu_nmi_enter() is also called in some non-NMI exception
> cases, from ist_enter().  So it appears that the in_nmi() check wouldn't
> be sufficient.  We might instead need something like:
> 
> 	if (in_nmi() || in_some_other_exception())
> 		rcu_nmi_enter();
> 	else
> 		rcu_irq_enter();
> 
> But unfortunately the in_some_other_exception() function doesn't
> currently exist.
> 
> So, one more question.  Would it work if we just always called
> rcu_nmi_enter()?

I am a bit nervous about this.  It would -at- -least- be necessary to have
interrupts disabled throughout the entire time from the rcu_nmi_enter()
through the matching rcu_nmi_exit().  And there might be other failure
modes that I don't immediately see.

But do we really need this, given the in_nmi() check that Steven
pointed out?

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1638258 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Mon, May 08, 2017 at 03:36:00PM -0700, Paul E. McKenney wrote:
> On Mon, May 08, 2017 at 05:16:09PM -0500, Josh Poimboeuf wrote:
> > On Mon, May 08, 2017 at 02:07:54PM -0700, Paul E. McKenney wrote:
> > > This would be a problem if step 2's NMI hit rcu_irq_enter(),
> > > rcu_irq_exit(), and friends in just the wrong place.
> > > 
> > > I would suggest that ftrace() do something like this...
> > > 
> > > 	if (in_nmi())
> > > 		rcu_nmi_enter();
> > > 	else
> > > 		rcu_irq_enter();
> > > 
> > > Except that, as Steven will quickly point out, this won't work at the
> > > very edges of the NMI, when NMI_MASK won't be set in preempt_count().
> > > 
> > > Other thoughts?
> > 
> > Ok.  So I think the livepatch ftrace handler would need the in_nmi()
> > check, in case it's called early in the NMI.
> > 
> > But on x86, rcu_nmi_enter() is also called in some non-NMI exception
> > cases, from ist_enter().  So it appears that the in_nmi() check wouldn't
> > be sufficient.  We might instead need something like:
> > 
> > 	if (in_nmi() || in_some_other_exception())
> > 		rcu_nmi_enter();
> > 	else
> > 		rcu_irq_enter();
> > 
> > But unfortunately the in_some_other_exception() function doesn't
> > currently exist.
> > 
> > So, one more question.  Would it work if we just always called
> > rcu_nmi_enter()?
> 
> I am a bit nervous about this.  It would -at- -least- be necessary to have
> interrupts disabled throughout the entire time from the rcu_nmi_enter()
> through the matching rcu_nmi_exit().  And there might be other failure
> modes that I don't immediately see.

Ok, let's forget about that idea for now then :-)

> But do we really need this, given the in_nmi() check that Steven
> pointed out?

The in_nmi() check doesn't work for non-NMI exceptions.  An exception
can come from anywhere, which is presumably why ist_enter() calls
rcu_nmi_enter(), even though it might not have been in NMI context.  The
exception could, for example, happen while you're twiddling important
bits in rcu_irq_enter().  Or it could happen early in do_nmi(), before
it had a chance to set NMI_MASK or call rcu_nmi_enter().  In either
case, in_nmi() would be false, yet calling rcu_irq_enter() would be bad.

I think I have convinced myself that, as long as the user doesn't patch
ist_enter() or rcu_dynticks_eqs_enter(), it'll be fine.  So the
following should be sufficient:

	if (in_nmi())
		rcu_nmi_enter(); /* in case we're called before nmi_enter() */
	else
		rcu_irq_enter_irqson();

	if (unlikely(!rcu_is_watching())) {
		klp_block_patch_removal = true;
		WARN_ON_ONCE(1); /* this presumably means */
	}

I think the alternative, calling rcu_irq_enter_disabled() beforehand,
isn't sufficient, because it only checks the rcu_dynticks_eqs_enter()
case.  It doesn't check the IST exception ist_enter() case, before
rcu_nmi_enter() has been called.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1638275 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Tue, May 09, 2017 at 11:18:35AM -0500, Josh Poimboeuf wrote:
> On Mon, May 08, 2017 at 03:36:00PM -0700, Paul E. McKenney wrote:
> > On Mon, May 08, 2017 at 05:16:09PM -0500, Josh Poimboeuf wrote:
> > > On Mon, May 08, 2017 at 02:07:54PM -0700, Paul E. McKenney wrote:
> > > > This would be a problem if step 2's NMI hit rcu_irq_enter(),
> > > > rcu_irq_exit(), and friends in just the wrong place.
> > > > 
> > > > I would suggest that ftrace() do something like this...
> > > > 
> > > > 	if (in_nmi())
> > > > 		rcu_nmi_enter();
> > > > 	else
> > > > 		rcu_irq_enter();
> > > > 
> > > > Except that, as Steven will quickly point out, this won't work at the
> > > > very edges of the NMI, when NMI_MASK won't be set in preempt_count().
> > > > 
> > > > Other thoughts?
> > > 
> > > Ok.  So I think the livepatch ftrace handler would need the in_nmi()
> > > check, in case it's called early in the NMI.
> > > 
> > > But on x86, rcu_nmi_enter() is also called in some non-NMI exception
> > > cases, from ist_enter().  So it appears that the in_nmi() check wouldn't
> > > be sufficient.  We might instead need something like:
> > > 
> > > 	if (in_nmi() || in_some_other_exception())
> > > 		rcu_nmi_enter();
> > > 	else
> > > 		rcu_irq_enter();
> > > 
> > > But unfortunately the in_some_other_exception() function doesn't
> > > currently exist.
> > > 
> > > So, one more question.  Would it work if we just always called
> > > rcu_nmi_enter()?
> > 
> > I am a bit nervous about this.  It would -at- -least- be necessary to have
> > interrupts disabled throughout the entire time from the rcu_nmi_enter()
> > through the matching rcu_nmi_exit().  And there might be other failure
> > modes that I don't immediately see.
> 
> Ok, let's forget about that idea for now then :-)

Whew!!!  ;-)

> > But do we really need this, given the in_nmi() check that Steven
> > pointed out?
> 
> The in_nmi() check doesn't work for non-NMI exceptions.  An exception
> can come from anywhere, which is presumably why ist_enter() calls
> rcu_nmi_enter(), even though it might not have been in NMI context.  The
> exception could, for example, happen while you're twiddling important
> bits in rcu_irq_enter().  Or it could happen early in do_nmi(), before
> it had a chance to set NMI_MASK or call rcu_nmi_enter().  In either
> case, in_nmi() would be false, yet calling rcu_irq_enter() would be bad.
> 
> I think I have convinced myself that, as long as the user doesn't patch
> ist_enter() or rcu_dynticks_eqs_enter(), it'll be fine.  So the
> following should be sufficient:
> 
> 	if (in_nmi())
> 		rcu_nmi_enter(); /* in case we're called before nmi_enter() */
> 	else
> 		rcu_irq_enter_irqson();
> 
> 	if (unlikely(!rcu_is_watching())) {
> 		klp_block_patch_removal = true;
> 		WARN_ON_ONCE(1); /* this presumably means */
> 	}

As long as you have a similar setup on exit, so that each call to
rcu_nmi_enter() is balanced by a corresponding call to rcu_nmi_exit().
Ditto for rcu_irq_enter_irqson(), of course.

> I think the alternative, calling rcu_irq_enter_disabled() beforehand,
> isn't sufficient, because it only checks the rcu_dynticks_eqs_enter()
> case.  It doesn't check the IST exception ist_enter() case, before
> rcu_nmi_enter() has been called.

Yes, calling rcu_irq_enter_disabled() beforehand would be unfortunate
if this was an NMI that occurred in just the wrong place in (say)
rcu_irq_enter().  ;-)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1638935 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Tue 2017-05-09 11:18:35, Josh Poimboeuf wrote:
> On Mon, May 08, 2017 at 03:36:00PM -0700, Paul E. McKenney wrote:
> > On Mon, May 08, 2017 at 05:16:09PM -0500, Josh Poimboeuf wrote:
> > > On Mon, May 08, 2017 at 02:07:54PM -0700, Paul E. McKenney wrote:
> > But do we really need this, given the in_nmi() check that Steven
> > pointed out?
> 
> The in_nmi() check doesn't work for non-NMI exceptions.  An exception
> can come from anywhere, which is presumably why ist_enter() calls
> rcu_nmi_enter(), even though it might not have been in NMI context.  The
> exception could, for example, happen while you're twiddling important
> bits in rcu_irq_enter().  Or it could happen early in do_nmi(), before
> it had a chance to set NMI_MASK or call rcu_nmi_enter().  In either
> case, in_nmi() would be false, yet calling rcu_irq_enter() would be bad.
> 
> I think I have convinced myself that, as long as the user doesn't patch
> ist_enter() or rcu_dynticks_eqs_enter(), it'll be fine.  So the
> following should be sufficient:
> 
> 	if (in_nmi())
> 		rcu_nmi_enter(); /* in case we're called before nmi_enter() */

This does not work as expected. in_nmi() is implemented as

	(preempt_count() & NMI_MASK)

These bits are set in nmi_enter(), see

	preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET);

Note that nmi_enter() calls rcu_nmi_enter() right after
setting the preempt_count bit.

It means that if in_nmi() returns true, we should already
on the safe side regarding using rcu_read_lock()/unlock().


The patch was designed to use basically the same solution
as is used in the stack tracer. It is using
rcu_read_lock()/unlock() as we do.

The stack tracer is different in the following ways:

    + It takes a spin lock. This is why it has to give
      up in NMI completely.

    + It disables interrupts. I guess that it is because
      of the spin lock as well. Otherwise, it would not
      be safe in IRQ context.

    + It checks whether local_irq_save() has a chance to
      work and gives up if it does not.


On the other hand, the live patch handler:

    + does not need any lock => could be used in NMI

    + does not need to disable interrupts because
      it does not use any lock

    + checks if local_irq_save() actually succeeded.
      It seems more reliable to me.


I am not sure if we all understand the problem. IMHO, the point
is that RCU must be aware when we call rcu_read_lock()/unlock().

My understanding is that rcu_irq_enter() tries to make RCU watching
when it was not. Then rcu_is_watching() reports if we are on
the safe side.

But it is possible that I miss something. One question is if
rcu_irq_enter()/exit() calls can be nested.

I still need to think about it.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1638966 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Wed, May 10, 2017 at 06:04:23PM +0200, Petr Mladek wrote:
> On Tue 2017-05-09 11:18:35, Josh Poimboeuf wrote:
> > On Mon, May 08, 2017 at 03:36:00PM -0700, Paul E. McKenney wrote:
> > > On Mon, May 08, 2017 at 05:16:09PM -0500, Josh Poimboeuf wrote:
> > > > On Mon, May 08, 2017 at 02:07:54PM -0700, Paul E. McKenney wrote:
> > > But do we really need this, given the in_nmi() check that Steven
> > > pointed out?
> > 
> > The in_nmi() check doesn't work for non-NMI exceptions.  An exception
> > can come from anywhere, which is presumably why ist_enter() calls
> > rcu_nmi_enter(), even though it might not have been in NMI context.  The
> > exception could, for example, happen while you're twiddling important
> > bits in rcu_irq_enter().  Or it could happen early in do_nmi(), before
> > it had a chance to set NMI_MASK or call rcu_nmi_enter().  In either
> > case, in_nmi() would be false, yet calling rcu_irq_enter() would be bad.
> > 
> > I think I have convinced myself that, as long as the user doesn't patch
> > ist_enter() or rcu_dynticks_eqs_enter(), it'll be fine.  So the
> > following should be sufficient:
> > 
> > 	if (in_nmi())
> > 		rcu_nmi_enter(); /* in case we're called before nmi_enter() */
> 
> This does not work as expected. in_nmi() is implemented as
> 
> 	(preempt_count() & NMI_MASK)
> 
> These bits are set in nmi_enter(), see
> 
> 	preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET);
> 
> Note that nmi_enter() calls rcu_nmi_enter() right after
> setting the preempt_count bit.
> 
> It means that if in_nmi() returns true, we should already
> on the safe side regarding using rcu_read_lock()/unlock().
> 
> 
> The patch was designed to use basically the same solution
> as is used in the stack tracer. It is using
> rcu_read_lock()/unlock() as we do.
> 
> The stack tracer is different in the following ways:
> 
>     + It takes a spin lock. This is why it has to give
>       up in NMI completely.
> 
>     + It disables interrupts. I guess that it is because
>       of the spin lock as well. Otherwise, it would not
>       be safe in IRQ context.
> 
>     + It checks whether local_irq_save() has a chance to
>       work and gives up if it does not.
> 
> 
> On the other hand, the live patch handler:
> 
>     + does not need any lock => could be used in NMI
> 
>     + does not need to disable interrupts because
>       it does not use any lock
> 
>     + checks if local_irq_save() actually succeeded.
>       It seems more reliable to me.
> 
> 
> I am not sure if we all understand the problem. IMHO, the point
> is that RCU must be aware when we call rcu_read_lock()/unlock().

I for one am sure that I do -not- fully understand the problem.  ;-)
But yes, the key point is that RCU be able to see and respond to
the read-side critical sections.

> My understanding is that rcu_irq_enter() tries to make RCU watching
> when it was not. Then rcu_is_watching() reports if we are on
> the safe side.
> 
> But it is possible that I miss something. One question is if
> rcu_irq_enter()/exit() calls can be nested.

Yes, they can.  You get about 50 bits worth of nesting counter.

You can also nest rcu_nmi_enter()/exit() calls, but you "only"
get 31 bits of nesting counter.

							Thanx, Paul

> I still need to think about it.
> 
> Best Regards,
> Petr
> 

[toc] | [prev] | [next] | [standalone]

#1638985 — Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

On Wed, May 10, 2017 at 06:04:23PM +0200, Petr Mladek wrote:
> On Tue 2017-05-09 11:18:35, Josh Poimboeuf wrote:
> > 	if (in_nmi())
> > 		rcu_nmi_enter(); /* in case we're called before nmi_enter() */
> 
> This does not work as expected. in_nmi() is implemented as
> 
> 	(preempt_count() & NMI_MASK)
> 
> These bits are set in nmi_enter(), see
> 
> 	preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET);
> 
> Note that nmi_enter() calls rcu_nmi_enter() right after
> setting the preempt_count bit.
> 
> It means that if in_nmi() returns true, we should already
> on the safe side regarding using rcu_read_lock()/unlock().

Ah, you're right.  I was worried about the gap between the start of
do_nmi() and when it calls rcu_nmi_enter(), but it seems all the
functions it calls in that gap are 'notrace', so they couldn't be
patched anyway.  And as you pointed out, in_nmi() wouldn't work.

> The patch was designed to use basically the same solution
> as is used in the stack tracer. It is using
> rcu_read_lock()/unlock() as we do.
> 
> The stack tracer is different in the following ways:
> 
>     + It takes a spin lock. This is why it has to give
>       up in NMI completely.
> 
>     + It disables interrupts. I guess that it is because
>       of the spin lock as well. Otherwise, it would not
>       be safe in IRQ context.
> 
>     + It checks whether local_irq_save() has a chance to
>       work and gives up if it does not.
> 
> 
> On the other hand, the live patch handler:
> 
>     + does not need any lock => could be used in NMI
> 
>     + does not need to disable interrupts because
>       it does not use any lock
> 
>     + checks if local_irq_save() actually succeeded.
>       It seems more reliable to me.
> 
> 
> I am not sure if we all understand the problem.

No kidding :-)

> IMHO, the point is that RCU must be aware when we call
> rcu_read_lock()/unlock().
> 
> My understanding is that rcu_irq_enter() tries to make RCU watching
> when it was not. Then rcu_is_watching() reports if we are on
> the safe side.
> 
> But it is possible that I miss something. One question is if
> rcu_irq_enter()/exit() calls can be nested.
> 
> I still need to think about it.

The code looks ok to me now, except for a few minor issues:

- The warning message should be more specific.

- The documentation should probably mention the name of the specific RCU
  function which shouldn't be patched.

- The documentation might also mention that the warning could also be
  triggered in early NMI or exception code, e.g. if there are any calls
  to functions with fentry calls which have been patched.

- The code comment should probably refer to the documentation, otherwise
  nobody will ever read it ;-)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web