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

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

[Petr Mladek]

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

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

[Petr Mladek]

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

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

[Petr Mladek]

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

[PATCH 3/3] livepatch/rcu: Disable livepatch removal when safety is not guaranteed

[Petr Mladek]

We already warn when RCU is not watching and the system consistency
is not guaranteed.

One problem is that tasks might use incompatible implementations
of the modified functions. We print the warning and could not
do much more about it.

Second problem is that we could not reliably detect unused code
after the patch was disabled. Here we could disable removing
patches to avoid possible damage.

The solution is sub-optimal. First, it might be too late to disable
the patch removal. Second, it affects all patches even though some
of them might be safe. But the question is if a better solution is
worth the effort. Most of the affected functions could not be patched
anyway. They are used by the ftrace handler and patching would
cause an infinite recursion.

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

diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 92619f7d86fd..87069ab46121 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -491,6 +491,10 @@ The current Livepatch implementation has several limitations:
     functions using immediate patches. But this is hard to detect properly
     in the ftrace handler, so the warning is always printed.
 
+    Also we are not able to detect when the code is not longer used in
+    the problematic context. We rather disable removing of livepatches
+    to avoid more possible damage.
+
 
   + 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 ffdf5fa8005b..c670a45d32f4 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -67,8 +67,10 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	 * the RCU infrastructure. Then we might see wrong state of
 	 * func->stack and other flags.
 	 */
-	if (unlikely(!rcu_is_watching()))
+	if (unlikely(!rcu_is_watching())) {
+		klp_block_patch_removal = true;
 		WARN_ONCE(1, "Livepatch modified a function that can not be handled a safe way.!");
+	}
 
 	rcu_read_lock();
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index adc0cc64aa4b..0669517ea6a8 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -31,6 +31,8 @@
 
 struct klp_patch *klp_transition_patch;
 
+bool klp_block_patch_removal;
+
 static int klp_target_state = KLP_UNDEFINED;
 
 /*
@@ -87,8 +89,11 @@ static void klp_complete_transition(void)
 		}
 	}
 
-	if (klp_target_state == KLP_UNPATCHED && !immediate_func)
+	if (klp_target_state == KLP_UNPATCHED &&
+	    !immediate_func &&
+	    !klp_block_patch_removal) {
 		module_put(klp_transition_patch->mod);
+	}
 
 	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
 	if (klp_target_state == KLP_PATCHED)
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index ce09b326546c..cc20b19d8707 100644
--- a/kernel/livepatch/transition.h
+++ b/kernel/livepatch/transition.h
@@ -5,6 +5,8 @@
 
 extern struct klp_patch *klp_transition_patch;
 
+extern bool klp_block_patch_removal;
+
 void klp_init_transition(struct klp_patch *patch, int state);
 void klp_cancel_transition(void);
 void klp_start_transition(void);
-- 
1.8.5.6

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

[Paul E. McKenney]

On Thu, May 04, 2017 at 12:55:13PM +0200, Petr Mladek wrote:
> 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.

I don't know enough to say much about the live patching, but the
characterization of how RCU works is correct.

						Thanx, Paul

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

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

[Josh Poimboeuf]

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

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

[Steven Rostedt]

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

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

[Josh Poimboeuf]

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

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

[Paul E. McKenney]

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

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

[Steven Rostedt]

On Mon, 8 May 2017 14:47:29 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

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

I was being oversimplified by saying 'return', perhaps go into a
critical mode that can try again, or perhaps even back out the patch.
As in a transaction style. Yes, this will need to be thought through to
know how to get out. My comment wasn't meant to be simple.

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

Well, it's not RCU that's broken. It's the users ;-)

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

No. Because ftrace doesn't require that and works fine. The strack
tracer is the only thing that has issues in the ftrace code.

If we were to start slapping in notrace for each user of ftrace that
has issues, then we would have nothing left to trace ;-)


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

Good question. Paul?

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

I think the only place is that one function. But you are right. What
happens if an NMI comes in?

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

-- Steve

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

[Josh Poimboeuf]

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

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

[Josh Poimboeuf]

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

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

[Paul E. McKenney]

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

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

[Paul E. McKenney]

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

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

[Steven Rostedt]

On Mon, 8 May 2017 15:43:33 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> 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

No, because in the ftrace stack tracer there's:

	if (in_nmi())
		return;

At the very beginning.

-- Steve

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

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

[Steven Rostedt]

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

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

[Paul E. McKenney]

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

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

[Josh Poimboeuf]

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

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

[Paul E. McKenney]

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

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

[Josh Poimboeuf]

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

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

[Paul E. McKenney]

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

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

[Petr Mladek]

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

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

[Paul E. McKenney]

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
> 

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

[Josh Poimboeuf]

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

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

[Miroslav Benes]

Being somewhat late to the party I missed all the fun...

On Wed, 10 May 2017, Josh Poimboeuf wrote:

> On Wed, May 10, 2017 at 06:04:23PM +0200, Petr Mladek wrote:
> 
> > 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 ;-)

Agreed.

Petr, could you also improve the changelog a bit? Certain parts confuse 
me and I think that your changes to the documentation describe it better.

I mean these two paragraphs:

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

I'm still not sure if we know for 100 percent what we're doing :)

Miroslav

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

[Petr Mladek]

On Mon 2017-05-08 11:51:08, Josh Poimboeuf wrote:
> 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.
> > 
> > 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,
> 
> Also I wonder if we can constrain the warning somehow.  I think the
> warning only applies if a patch is in progress, right?

I think that this was not addressed in the other mails.

I wanted to add some constrains but all my attempts have failed
so far. For example, I wanted to avoid the warning when the function
was patched with the immediate flag set. We would be on the safe side.
Such a patch could not be removed. And the consistency model is week
enough.

The problem is that the transaction might finish too early when a
patched function cannot be watched by RCU. It means that the finished
transaction does not mean that we are safe. Also a finished transaction
allows to start a new one. Then the ftrace handler might see an
outdated function stack. Therefore it is not sure if the previously
used patch was immediate ("safely" handled) ...

Fortunately, this situation is a real corner case. It might actually be
good that the problem is always reported. It helps to detect it during
testing and avoid sending such a patch to users.


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

If we could not guarantee that a function might be patched a safe way,
we should not allow the patching in the first place.

The problem here is that this situation is detected at runtime.
We should do our best to avoid a damage if this happens. The 3rd patch
is from this category. But we should make it as easy as possible
to catch potential problems during patch creation or testing.

Best Regards,
Petr

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

[Miroslav Benes]

On Mon, 8 May 2017, Steven Rostedt wrote:

> On Mon, 8 May 2017 14:47:29 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > > 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.
> 
> I was being oversimplified by saying 'return', perhaps go into a
> critical mode that can try again, or perhaps even back out the patch.
> As in a transaction style. Yes, this will need to be thought through to
> know how to get out. My comment wasn't meant to be simple.

Well, live patching is in fact transactional. To some extent. If we fail 
during ftrace registration we abort the action gracefully. After the 
registration it gets more interesting because the whole system is 
"asynchronously" migrated to a final patched state. Even during this stage 
we can reverse the process (klp_reverse_transition()), but it was not easy 
to get it right...

... and to implement or start this logic from the handler sends shivers 
down my spine.

We still can try.

Thanks,
Miroslav

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

[Petr Mladek]

On Mon 2017-05-08 15:13:22, Steven Rostedt wrote:
> On Mon, 8 May 2017 11:51:08 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > 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.

I am a bit confused by the above. Does it mean that RCU could not be
used in NMI handlers?


Anyway, a crazy idea is to use the livepatch consistency model instead
of RCU to protect the function stack. The model makes sure that all
tasks, including the idle ones, were not running any patched function
(and their ftrace handlers) at some point. It should be safe
but I am not sure if it is worth it.

Alternatively, it might be enough to use the probably more lightwight
solution that is used when ftrace handlers are deregistered, I mean:

	/*
	 * We need to do a hard force of sched synchronization.
	 * This is because we use preempt_disable() to do RCU, but
	 * the function tracers can be called where RCU is not watching
	 * (like before user_exit()). We can not rely on the RCU
	 * infrastructure to do the synchronization, thus we must do it
	 * ourselves.
	 */
	schedule_on_each_cpu(ftrace_sync);

	/*
	 * When the kernel is preeptive, tasks can be preempted
	 * while on a ftrace trampoline. Just scheduling a task on
	 * a CPU is not good enough to flush them. Calling
	 * synchornize_rcu_tasks() will wait for those tasks to
	 * execute and either schedule voluntarily or enter user space.
	 */
	if (IS_ENABLED(CONFIG_PREEMPT))
		synchronize_rcu_tasks();



Best  Regards,
Petr

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

[Paul E. McKenney]

On Thu, May 11, 2017 at 03:52:46PM +0200, Petr Mladek wrote:
> On Mon 2017-05-08 15:13:22, Steven Rostedt wrote:
> > On Mon, 8 May 2017 11:51:08 -0500
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > > 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.
> 
> I am a bit confused by the above. Does it mean that RCU could not be
> used in NMI handlers?

Only RCU readers can be used in NMI handlers, that is, rcu_read_lock(),
rcu_read_unlock(), rcu_dereference(), and so on.

							Thanx, Paul

> Anyway, a crazy idea is to use the livepatch consistency model instead
> of RCU to protect the function stack. The model makes sure that all
> tasks, including the idle ones, were not running any patched function
> (and their ftrace handlers) at some point. It should be safe
> but I am not sure if it is worth it.
> 
> Alternatively, it might be enough to use the probably more lightwight
> solution that is used when ftrace handlers are deregistered, I mean:
> 
> 	/*
> 	 * We need to do a hard force of sched synchronization.
> 	 * This is because we use preempt_disable() to do RCU, but
> 	 * the function tracers can be called where RCU is not watching
> 	 * (like before user_exit()). We can not rely on the RCU
> 	 * infrastructure to do the synchronization, thus we must do it
> 	 * ourselves.
> 	 */
> 	schedule_on_each_cpu(ftrace_sync);
> 
> 	/*
> 	 * When the kernel is preeptive, tasks can be preempted
> 	 * while on a ftrace trampoline. Just scheduling a task on
> 	 * a CPU is not good enough to flush them. Calling
> 	 * synchornize_rcu_tasks() will wait for those tasks to
> 	 * execute and either schedule voluntarily or enter user space.
> 	 */
> 	if (IS_ENABLED(CONFIG_PREEMPT))
> 		synchronize_rcu_tasks();
> 
> 
> 
> Best  Regards,
> Petr
> 

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

[Josh Poimboeuf]

On Thu, May 11, 2017 at 02:40:42PM +0200, Miroslav Benes wrote:
> I'm still not sure if we know for 100 percent what we're doing :)

At least we know that we don't know!

-- 
Josh

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

[Josh Poimboeuf]

On Thu, May 11, 2017 at 03:52:46PM +0200, Petr Mladek wrote:
> Anyway, a crazy idea is to use the livepatch consistency model instead
> of RCU to protect the function stack. The model makes sure that all
> tasks, including the idle ones, were not running any patched function
> (and their ftrace handlers) at some point. It should be safe
> but I am not sure if it is worth it.

http://i3.kym-cdn.com/photos/images/original/000/173/580/Wat.jpg

> Alternatively, it might be enough to use the probably more lightwight
> solution that is used when ftrace handlers are deregistered, I mean:
> 
> 	/*
> 	 * We need to do a hard force of sched synchronization.
> 	 * This is because we use preempt_disable() to do RCU, but
> 	 * the function tracers can be called where RCU is not watching
> 	 * (like before user_exit()). We can not rely on the RCU
> 	 * infrastructure to do the synchronization, thus we must do it
> 	 * ourselves.
> 	 */
> 	schedule_on_each_cpu(ftrace_sync);
> 
> 	/*
> 	 * When the kernel is preeptive, tasks can be preempted
> 	 * while on a ftrace trampoline. Just scheduling a task on
> 	 * a CPU is not good enough to flush them. Calling
> 	 * synchornize_rcu_tasks() will wait for those tasks to
> 	 * execute and either schedule voluntarily or enter user space.
> 	 */
> 	if (IS_ENABLED(CONFIG_PREEMPT))
> 		synchronize_rcu_tasks();

I couldn't grok the first idea, but this one sounds promising...

-- 
Josh