tracing/rcu: Fix save_stack_trace() called when RCU is not watching
Subsystems (3)
Steven Rostedt
maintainerLinus,
Stack tracing and RCU has been having issues with each other and lockdep has been pointing out constant problems. The changes have been going into the stack tracer, but it has been discovered that the problem isn't with the stack tracer itself, but it is with calling save_stack_trace() from within the internals of RCU. The stack tracer is the one that can trigger the issue the easiest, but examining the problem further, it could also happen from a WARN() in the wrong place, or even if an NMI happened in this area and it did an rcu_read_lock().
The critical area is where RCU is not watching. Which can happen while going to and from idle, or bringing up or taking down a CPU.
The final fix was to put the protection in kernel_text_address() as it is the one that requires RCU to be watching while doing the stack trace.
To make this work properly, Paul had to allow rcu_irq_enter() happen after rcu_nmi_enter(). This should have been done anyway, since an NMI can page fault (reading vmalloc area), and a page fault triggers rcu_irq_enter().
One patch is just a consolidation of code so that the fix only needed to be done in one location.
Please pull the latest trace-v4.14-rc1-2 tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v4.14-rc1-2
Tag SHA1: cdd226e99fbb8f22e5317e7968b37387108ce568 Head SHA1: 15516c89acce948debc4c598e03c3fee53045797
Paul E. McKenney (1):
rcu: Allow for page faults in NMI handlers
Steven Rostedt (VMware) (3):
extable: Consolidate *kernel_text_address() functions
extable: Enable RCU if it is not watching in kernel_text_address()
tracing: Remove RCU work arounds from stack tracer
----
kernel/extable.c | 45 +++++++++++++++++++++++++++++++-------------- kernel/rcu/tree.c | 10 ++++++++++ kernel/trace/trace_stack.c | 15 --------------- 3 files changed, 41 insertions(+), 29 deletions(-)
Steven Rostedt
maintainerPATCH 1/4 rcu: Allow for page faults in NMI handlers
From: "Paul E. McKenney" <[email protected]>
A number of architecture invoke rcu_irq_enter() on exception entry in order to allow RCU read-side critical sections in the exception handler when the exception is from an idle or nohz_full CPU. This works, at least unless the exception happens in an NMI handler. In that case, rcu_nmi_enter() would already have exited the extended quiescent state, which would mean that rcu_irq_enter() would (incorrectly) cause RCU to think that it is again in an extended quiescent state. This will in turn result in lockdep splats in response to later RCU read-side critical sections.
This commit therefore causes rcu_irq_enter() and rcu_irq_exit() to take no action if there is an rcu_nmi_enter() in effect, thus avoiding the unscheduled return to RCU quiescent state. This in turn should make the kernel safe for on-demand RCU voyeurism.
--- kernel/rcu/tree.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
kernel/rcu/tree.c +10 -0
@@ -888,6 +888,11 @@ void rcu_irq_exit(void)RCU_LOCKDEP_WARN (!irqs_disabled (),"rcu_irq_exit() invoked with irqs enabled!!!" ); rdtp =this_cpu_ptr (&rcu_dynticks); + +/* Page faults can happen in NMI handlers, so check... */ +if (READ_ONCE (rdtp->dynticks_nmi_nesting)) +return ; +WARN_ON_ONCE (IS_ENABLED (CONFIG_RCU_EQS_DEBUG ) && rdtp->dynticks_nesting <1 );if (rdtp->dynticks_nesting <=1 ) {
@@ -1020,6 +1025,11 @@ void rcu_irq_enter(void)RCU_LOCKDEP_WARN (!irqs_disabled (),"rcu_irq_enter() invoked with irqs enabled!!!" ); rdtp =this_cpu_ptr (&rcu_dynticks); + +/* Page faults can happen in NMI handlers, so check... */ +if (READ_ONCE (rdtp->dynticks_nmi_nesting)) +return ; + oldval = rdtp->dynticks_nesting; rdtp->dynticks_nesting++; WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
Signature · 1 line
2.13.2
Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers
kernel/rcu/tree.c void rcu_irq_exit(void) · line 895
Quoted 5 lines · Steven Rostedt
kernel/rcu/tree.c +5
-0+ +/* Page faults can happen in NMI handlers, so check... */ +if (READ_ONCE (rdtp->dynticks_nmi_nesting)) +return ; +
What is the reason for the READ_ONCE() here (and in the other case)?
It doesn't seem to have any actual reason. It's a "stable" per-cpu value in that even if an NMI were to happen, it gets incremented and then decremented, so there is nothing really volatile about it anywhere that I can see.
So the READ_ONCE() seems to be just pure confusion.
What am I missing?
Linus
Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers
Quoted 9 lines · Linus Torvalds
kernel/rcu/tree.c void rcu_irq_exit(void) · line 895
Quoted 5 lines · Steven Rostedt
kernel/rcu/tree.c +5
-0+ +/* Page faults can happen in NMI handlers, so check... */ +if (READ_ONCE (rdtp->dynticks_nmi_nesting)) +return ; +What is the reason for the
READ_ONCE()here (and in the other case)?
Quoted 8 lines
It doesn't seem to have any actual reason. It's a "stable" per-cpu value in that even if an NMI were to happen, it gets incremented and then decremented, so there is nothing really volatile about it anywhere that I can see.
So the
READ_ONCE()seems to be just pure confusion.What am I missing?
Mostly just paranoia on my part. I would be happy to remove it if you prefer. Or you or Steve can do so if that is more convenient.
And yes, consistency would dictate that the uses in rcu_nmi_enter() and rcu_nmi_exit() should be _ONCE(), particularly the stores to ->dynticks_nmi_nesting. But I am not too worried about that right now because I suspect that I should be able to combine rcu_irq_{enter,exit}() and rcu_nmi_{enter,exit}(), which would be a good simplification.
Thanx, aul
Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers
Quoted 3 lines · Paul E. McKenney
Mostly just paranoia on my part. I would be happy to remove it if you prefer. Or you or Steve can do so if that is more convenient.
I really don't think it's warranted. The values are *stable*. There's no subtle lack of locking, or some optimistic access to a value that can change.
The compiler can generate code to read the value fifteen billion times, and it will always get the same value.
Yes, maybe in between the different accesses, an NMI will happen, and the value will be incremented, but then as the NMI exits, it will decrement again, so the code that got interrupted will not actually see the change.
So the READ_ONCE() isn't "paranoia". It's just confusing.
Quoted 3 lines
And yes, consistency would dictate that the uses in
rcu_nmi_enter()andrcu_nmi_exit()should be_ONCE(), particularly the stores to ->dynticks_nmi_nesting.
NO.
That would be just more of that confusion.
That value is STABLE. It's stable even within an NMI handler. The NMI code can read it, modify it, write it back, do a little dance, all without having to care. There's no "_ONCE()" about it - not for the readers, not for the writers, not for _anybody_.
So adding even more READ/WRITE_ONCE() accesses wouldn't be "consistent", it would just be insanity.
Now, if an NMI happens and the value would be different on entry than it is on exit, that would be something else. Then it really wouldn't be stable wrt random users. But that would also be a major bug in the NMI handler, as far as I can tell.
So the reason I'm objecting to that READ_ONCE() is that it isn't "paranoia", it's "voodoo programming". And we don't do voodoo programming.
Linus
Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers
Quoted 36 lines · Linus Torvalds
Quoted 3 lines · Paul E. McKenney
Mostly just paranoia on my part. I would be happy to remove it if you prefer. Or you or Steve can do so if that is more convenient.
I really don't think it's warranted. The values are *stable*. There's no subtle lack of locking, or some optimistic access to a value that can change.
The compiler can generate code to read the value fifteen billion times, and it will always get the same value.
Yes, maybe in between the different accesses, an NMI will happen, and the value will be incremented, but then as the NMI exits, it will decrement again, so the code that got interrupted will not actually see the change.
So the
READ_ONCE()isn't "paranoia". It's just confusing.Quoted 3 lines
And yes, consistency would dictate that the uses in
rcu_nmi_enter()andrcu_nmi_exit()should be_ONCE(), particularly the stores to ->dynticks_nmi_nesting.NO.
That would be just more of that confusion.
That value is STABLE. It's stable even within an NMI handler. The NMI code can read it, modify it, write it back, do a little dance, all without having to care. There's no "
_ONCE()" about it - not for the readers, not for the writers, not for _anybody_.So adding even more READ/WRITE_ONCE() accesses wouldn't be "consistent", it would just be insanity.
Quoted 8 lines
Now, if an NMI happens and the value would be different on entry than it is on exit, that would be something else. Then it really wouldn't be stable wrt random users. But that would also be a major bug in the NMI handler, as far as I can tell.
So the reason I'm objecting to that
READ_ONCE()is that it isn't "paranoia", it's "voodoo programming". And we don't do voodoo programming.
I already agreed that the READ_ONCE() can be removed.
But without the WRITE_ONCE(), the compiler could theoretically tear the store. Now we might be asserting that our compilers don't do that, and that if they ever do, we will file a bug or whatever.
So are we asserting that our compilers won't ever do store tearing?
Thanx, Paul
Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers
Quoted 42 lines · Paul E. McKenney
Quoted 41 lines · Linus Torvalds
Quoted 3 lines · Paul E. McKenney
Mostly just paranoia on my part. I would be happy to remove it if you prefer. Or you or Steve can do so if that is more convenient.
I really don't think it's warranted. The values are *stable*. There's no subtle lack of locking, or some optimistic access to a value that can change.
The compiler can generate code to read the value fifteen billion times, and it will always get the same value.
Yes, maybe in between the different accesses, an NMI will happen, and the value will be incremented, but then as the NMI exits, it will decrement again, so the code that got interrupted will not actually see the change.
So the
READ_ONCE()isn't "paranoia". It's just confusing.Quoted 3 lines
And yes, consistency would dictate that the uses in
rcu_nmi_enter()andrcu_nmi_exit()should be_ONCE(), particularly the stores to ->dynticks_nmi_nesting.NO.
That would be just more of that confusion.
That value is STABLE. It's stable even within an NMI handler. The NMI code can read it, modify it, write it back, do a little dance, all without having to care. There's no "
_ONCE()" about it - not for the readers, not for the writers, not for _anybody_.So adding even more READ/WRITE_ONCE() accesses wouldn't be "consistent", it would just be insanity.
Now, if an NMI happens and the value would be different on entry than it is on exit, that would be something else. Then it really wouldn't be stable wrt random users. But that would also be a major bug in the NMI handler, as far as I can tell.
Quoted 5 lines
Quoted 3 lines
So the reason I'm objecting to that
READ_ONCE()is that it isn't "paranoia", it's "voodoo programming". And we don't do voodoo programming.I already agreed that the
READ_ONCE()can be removed.
And for whatever it is worth, here is the updated patch.
Thanx, Paul
------------------------------------------------------------------------
commit 3e2baa988b9c13095995c46c51e0e32c0b6a7d43
Author: Paul E. McKenney <[email protected]> Date: Fri Sep 22 13:14:42 2017 -0700
rcu: Allow for page faults in NMI handlers
A number of architecture invoke rcu_irq_enter() on exception entry in order to allow RCU read-side critical sections in the exception handler when the exception is from an idle or nohz_full CPU. This works, at least unless the exception happens in an NMI handler. In that case, rcu_nmi_enter() would already have exited the extended quiescent state, which would mean that rcu_irq_enter() would (incorrectly) cause RCU to think that it is again in an extended quiescent state. This will in turn result in lockdep splats in response to later RCU read-side critical sections.
This commit therefore causes rcu_irq_enter() and rcu_irq_exit() to take no action if there is an rcu_nmi_enter() in effect, thus avoiding the unscheduled return to RCU quiescent state. This in turn should make the kernel safe for on-demand RCU voyeurism.
kernel/rcu/tree.c +10 -0
@@ -891,6 +891,11 @@ void rcu_irq_exit(void)RCU_LOCKDEP_WARN (!irqs_disabled (),"rcu_irq_exit() invoked with irqs enabled!!!" ); rdtp =this_cpu_ptr (&rcu_dynticks); + +/* Page faults can happen in NMI handlers, so check... */ +if (rdtp->dynticks_nmi_nesting) +return ; +WARN_ON_ONCE (IS_ENABLED (CONFIG_RCU_EQS_DEBUG ) && rdtp->dynticks_nesting <1 );if (rdtp->dynticks_nesting <=1 ) {
@@ -1036,6 +1041,11 @@ void rcu_irq_enter(void)RCU_LOCKDEP_WARN (!irqs_disabled (),"rcu_irq_enter() invoked with irqs enabled!!!" ); rdtp =this_cpu_ptr (&rcu_dynticks); + +/* Page faults can happen in NMI handlers, so check... */ +if (rdtp->dynticks_nmi_nesting) +return ; + oldval = rdtp->dynticks_nesting; rdtp->dynticks_nesting++; WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
Steven Rostedt
maintainerRe: [PATCH 1/4] rcu: Allow for page faults in NMI handlers
Sorry for the top post, currently on a train to Paris.
This series already went through all my testing, and I would hate to rebase it for this reason. Can you just add a patch to remove the READ_ONCE()s?
Thanks,
-- Steve
Quoted 120 lines · Paul E. McKenney
Quoted 5 lines · Paul E. McKenney
Quoted 4 lines · Linus Torvalds
Quoted 2 lines · Paul E. McKenney
Mostly just paranoia on my part. I would be happy to remove it
if
Quoted 1 line
Quoted 1 line
Quoted 1 line
you prefer. Or you or Steve can do so if that is more
convenient.
Quoted 2 lines
Quoted 2 lines
I really don't think it's warranted. The values are *stable*.
There's
Quoted 1 line
Quoted 1 line
no subtle lack of locking, or some optimistic access to a value
that
Quoted 6 lines
Quoted 6 lines
can change.
The compiler can generate code to read the value fifteen billion times, and it will always get the same value.
Yes, maybe in between the different accesses, an NMI will happen,
and
Quoted 7 lines
Quoted 7 lines
the value will be incremented, but then as the NMI exits, it will decrement again, so the code that got interrupted will not actually see the change.
So the
READ_ONCE()isn't "paranoia". It's just confusing.Quoted 1 line
And yes, consistency would dictate that the uses in
rcu_nmi_enter()Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
and
rcu_nmi_exit()should be_ONCE(), particularly the stores to ->dynticks_nmi_nesting.NO.
That would be just more of that confusion.
That value is STABLE. It's stable even within an NMI handler. The
NMI
Quoted 8 lines
Quoted 8 lines
code can read it, modify it, write it back, do a little dance, all without having to care. There's no "
_ONCE()" about it - not for the readers, not for the writers, not for _anybody_.So adding even more READ/WRITE_ONCE() accesses wouldn't be "consistent", it would just be insanity.
Now, if an NMI happens and the value would be different on entry
than
Quoted 1 line
Quoted 1 line
it is on exit, that would be something else. Then it really
wouldn't
Quoted 1 line
Quoted 1 line
be stable wrt random users. But that would also be a major bug in
the
Quoted 7 lines
Quoted 5 lines
NMI handler, as far as I can tell.
So the reason I'm objecting to that
READ_ONCE()is that it isn't "paranoia", it's "voodoo programming". And we don't do voodoo programming.I already agreed that the
READ_ONCE()can be removed.And for whatever it is worth, here is the updated patch.
Thanx, Paul
------------------------------------------------------------------------
commit
3e2baa988b9c13095995c46c51e0e32c0b6a7d43Author: Paul E. McKenney <[email protected]> Date: Fri Sep 22 13:14:42 2017 -0700rcu: Allow for page faults in NMI handlers
A number of architecture invoke
rcu_irq_enter()on exception entry in
order to allow RCU read-side critical sections in the exception handler
when the exception is from an idle ornohz_fullCPU. This works, at least unless the exception happens in an NMI handler. In that case,rcu_nmi_enter()would already have exited the extended quiescent state,
which would mean thatrcu_irq_enter()would (incorrectly) cause RCU
to think that it is again in an extended quiescent state. This will
in turn result in lockdep splats in response to later RCU read-side critical sections.This commit therefore causes
rcu_irq_enter()andrcu_irq_exit()to
take no action if there is anrcu_nmi_enter()in effect, thus avoiding
the unscheduled return to RCU quiescent state. This in turn should make the kernel safe for on-demand RCU voyeurism.Reported-by Steven Rostedt <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>[ paulmck: Remove READ_ONCE() per Linux Torvalds feedback. ]kernel/rcu/tree.c +5
-0@@ -891,6 +891,11 @@ void rcu_irq_exit(void) RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs enabled!!!"); rdtp = this_cpu_ptr(&rcu_dynticks); + +/* Page faults can happen in NMI handlers, so check... */ +if (rdtp->dynticks_nmi_nesting) +return ; +WARN_ON_ONCE (IS_ENABLED (CONFIG_RCU_EQS_DEBUG ) && rdtp->dynticks_nesting <1 );if (rdtp->dynticks_nesting <= 1) {kernel/rcu/tree.c +5
-0@@ -1036,6 +1041,11 @@ void rcu_irq_enter(void) RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_enter() invoked with irqs enabled!!!"); rdtp = this_cpu_ptr(&rcu_dynticks); + +/* Page faults can happen in NMI handlers, so check... */ +if (rdtp->dynticks_nmi_nesting) +return ; + oldval = rdtp->dynticks_nesting; rdtp->dynticks_nesting++;WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers
Quoted 3 lines · Steven Rostedt
Sorry for the top post, currently on a train to Paris.
This series already went through all my testing, and I would hate to rebase it for this reason. Can you just add a patch to remove the READ_ONCE()s?
If Linus accepts the original series, easy enough.
Thanx, Paul
Quoted 130 lines
Thanks,
-- Steve
Quoted 120 lines · Paul E. McKenney
Quoted 5 lines · Paul E. McKenney
Quoted 4 lines · Linus Torvalds
Quoted 2 lines · Paul E. McKenney
Mostly just paranoia on my part. I would be happy to remove it
if
Quoted 1 line
Quoted 1 line
Quoted 1 line
you prefer. Or you or Steve can do so if that is more
convenient.
Quoted 2 lines
Quoted 2 lines
I really don't think it's warranted. The values are *stable*.
There's
Quoted 1 line
Quoted 1 line
no subtle lack of locking, or some optimistic access to a value
that
Quoted 6 lines
Quoted 6 lines
can change.
The compiler can generate code to read the value fifteen billion times, and it will always get the same value.
Yes, maybe in between the different accesses, an NMI will happen,
and
Quoted 7 lines
Quoted 7 lines
the value will be incremented, but then as the NMI exits, it will decrement again, so the code that got interrupted will not actually see the change.
So the
READ_ONCE()isn't "paranoia". It's just confusing.Quoted 1 line
And yes, consistency would dictate that the uses in
rcu_nmi_enter()Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
and
rcu_nmi_exit()should be_ONCE(), particularly the stores to ->dynticks_nmi_nesting.NO.
That would be just more of that confusion.
That value is STABLE. It's stable even within an NMI handler. The
NMI
Quoted 8 lines
Quoted 8 lines
code can read it, modify it, write it back, do a little dance, all without having to care. There's no "
_ONCE()" about it - not for the readers, not for the writers, not for _anybody_.So adding even more READ/WRITE_ONCE() accesses wouldn't be "consistent", it would just be insanity.
Now, if an NMI happens and the value would be different on entry
than
Quoted 1 line
Quoted 1 line
it is on exit, that would be something else. Then it really
wouldn't
Quoted 1 line
Quoted 1 line
be stable wrt random users. But that would also be a major bug in
the
Quoted 7 lines
Quoted 5 lines
NMI handler, as far as I can tell.
So the reason I'm objecting to that
READ_ONCE()is that it isn't "paranoia", it's "voodoo programming". And we don't do voodoo programming.I already agreed that the
READ_ONCE()can be removed.And for whatever it is worth, here is the updated patch.
Thanx, Paul
------------------------------------------------------------------------
commit
3e2baa988b9c13095995c46c51e0e32c0b6a7d43Author: Paul E. McKenney <[email protected]> Date: Fri Sep 22 13:14:42 2017 -0700rcu: Allow for page faults in NMI handlers
A number of architecture invoke
rcu_irq_enter()on exception entry in
order to allow RCU read-side critical sections in the exception handler
when the exception is from an idle ornohz_fullCPU. This works, at least unless the exception happens in an NMI handler. In that case,rcu_nmi_enter()would already have exited the extended quiescent state,
which would mean thatrcu_irq_enter()would (incorrectly) cause RCU
to think that it is again in an extended quiescent state. This will
in turn result in lockdep splats in response to later RCU read-side critical sections.This commit therefore causes
rcu_irq_enter()andrcu_irq_exit()to
take no action if there is anrcu_nmi_enter()in effect, thus avoiding
the unscheduled return to RCU quiescent state. This in turn should make the kernel safe for on-demand RCU voyeurism.Reported-by Steven Rostedt <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>[ paulmck: Remove READ_ONCE() per Linux Torvalds feedback. ]kernel/rcu/tree.c +5
-0@@ -891,6 +891,11 @@ void rcu_irq_exit(void) RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs enabled!!!"); rdtp = this_cpu_ptr(&rcu_dynticks); + +/* Page faults can happen in NMI handlers, so check... */ +if (rdtp->dynticks_nmi_nesting) +return ; +WARN_ON_ONCE (IS_ENABLED (CONFIG_RCU_EQS_DEBUG ) && rdtp->dynticks_nesting <1 );if (rdtp->dynticks_nesting <= 1) {kernel/rcu/tree.c +5
-0@@ -1036,6 +1041,11 @@ void rcu_irq_enter(void) RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_enter() invoked with irqs enabled!!!"); rdtp = this_cpu_ptr(&rcu_dynticks); + +/* Page faults can happen in NMI handlers, so check... */ +if (rdtp->dynticks_nmi_nesting) +return ; + oldval = rdtp->dynticks_nesting; rdtp->dynticks_nesting++;WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers
Quoted 6 lines · Paul E. McKenney
Quoted 3 lines · Steven Rostedt
Sorry for the top post, currently on a train to Paris.
This series already went through all my testing, and I would hate to rebase it for this reason. Can you just add a patch to remove the READ_ONCE()s?
If Linus accepts the original series, easy enough.
And he did, so here is the READ_ONCE()-removal commit that I have queued for the next merge window. If anyone feels it is needed sooner, please let me know. (Can't see why it is urgent myself, but who knows...)
Thanx, Paul
------------------------------------------------------------------------
commit 79e6337d3f3a629be48cd45d5075d058788ce90f
Author: Paul E. McKenney <[email protected]> Date: Mon Sep 25 20:07:49 2017 -0700
rcu: Remove extraneous READ_ONCE()s from rcu_irq_{enter,exit}()
The read of ->dynticks_nmi_nesting in rcu_irq_enter() and rcu_irq_exit() is currently protected with READ_ONCE(). However, this protection is unnecessary because (1) ->dynticks_nmi_nesting is updated only by the current CPU, (2) Although NMI handlers can update this field, they reset it back to its old value before return, and (3) Interrupts are disabled, so nothing else can modify it. The value of ->dynticks_nmi_nesting is thus effectively constant, and so no protection is required.
This commit therefore removes the READ_ONCE() protection from these two accesses.
kernel/rcu/tree.c +2 -2
@@ -893,7 +893,7 @@ void rcu_irq_exit(void) rdtp =this_cpu_ptr (&rcu_dynticks);/* Page faults can happen in NMI handlers, so check... */ -if (READ_ONCE (rdtp->dynticks_nmi_nesting)) +if (rdtp->dynticks_nmi_nesting)return ; WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
@@ -1043,7 +1043,7 @@ void rcu_irq_enter(void) rdtp =this_cpu_ptr (&rcu_dynticks);/* Page faults can happen in NMI handlers, so check... */ -if (READ_ONCE (rdtp->dynticks_nmi_nesting)) +if (rdtp->dynticks_nmi_nesting)return ; oldval = rdtp->dynticks_nesting;
Steven Rostedt
maintainerPATCH 2/4 extable: Consolidate *kernel_text_address() functions
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The functionality between kernel_text_address() and _kernel_text_address() is the same except that _kernel_text_address() does a little more (that function needs a rename, but that can be done another time). Instead of having duplicate code in both, simply have _kernel_text_address() calls kernel_text_address() instead.
This is marked for stable because there's an RCU bug that can happen if one of these functions gets called while RCU is not watching. That fix depends on this fix to keep from having to write the fix twice.
--- kernel/extable.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
kernel/extable.c +1 -9
@@ -102,15 +102,7 @@ int core_kernel_data(unsigned long addr)int __kernel_text_address (unsigned long addr) { -if (core_kernel_text (addr)) -return 1 ; -if (is_module_text_address (addr)) -return 1 ; -if (is_ftrace_trampoline (addr)) -return 1 ; -if (is_kprobe_optinsn_slot (addr) ||is_kprobe_insn_slot (addr)) -return 1 ; -if (is_bpf_text_address (addr)) +if (kernel_text_address (addr))return 1 ;/* * There might be init symbols in saved stacktraces.
Signature · 1 line
2.13.2
Steven Rostedt
maintainerPATCH 3/4 extable: Enable RCU if it is not watching in kernel_text_address()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
If kernel_text_address() is called when RCU is not watching, it can cause an RCU bug because is_module_text_address(), the is_kprobe_*insn_slot() and is_bpf_text_address() functions require the use of RCU.
Only enable RCU if it is not currently watching before it calls is_module_text_address(). The use of rcu_nmi_enter() is used to enable RCU because kernel_text_address() can happen pretty much anywhere (like an NMI), and even from within an NMI. It is called via save_stack_trace() that can be called by any WARN() or tracing function, which can happen while RCU is not watching (for example, going to or coming from idle, or during CPU take down or bring up).
--- kernel/extable.c | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-)
kernel/extable.c +30 -5
@@ -119,17 +119,42 @@ int __kernel_text_address(unsigned long addr)int kernel_text_address (unsigned long addr) { +bool no_rcu; +int ret =1 ; +if (core_kernel_text (addr))return 1 ; + +/* +* If a stack dump happens while RCU is not watching, then +* RCU needs to be notified that it requires to start +* watching again. This can happen either by tracing that +* triggers a stack trace, or a WARN() that happens during +* coming back from idle, or cpu on or offlining. +* +* is_module_text_address() as well as the kprobe slots +* and is_bpf_text_address() require RCU to be watching. +*/ + no_rcu = !rcu_is_watching (); + +/* Treat this like an NMI as it can happen anywhere */ +if (no_rcu) +rcu_nmi_enter (); +if (is_module_text_address (addr)) -return 1 ; + goto out;if (is_ftrace_trampoline (addr)) -return 1 ; + goto out;if (is_kprobe_optinsn_slot (addr) ||is_kprobe_insn_slot (addr)) -return 1 ; + goto out;if (is_bpf_text_address (addr)) -return 1 ; -return 0 ; + goto out; + ret =0 ; +out: +if (no_rcu) +rcu_nmi_exit (); + +return ret; }/*
Signature · 1 line
2.13.2
Steven Rostedt
maintainerPATCH 4/4 tracing: Remove RCU work arounds from stack tracer
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
Currently the stack tracer calls rcu_irq_enter() to make sure RCU is watching when it records a stack trace. But if the stack tracer is triggered while tracing inside of a rcu_irq_enter(), calling rcu_irq_enter() unconditionally can be problematic.
The reason for having rcu_irq_enter() in the first place has been fixed from within the saving of the stack trace code, and there's no reason for doing it in the stack tracer itself. Just remove it.
--- kernel/trace/trace_stack.c | 15 --------------- 1 file changed, 15 deletions(-)
kernel/trace/trace_stack.c +0 -15
@@ -96,23 +96,9 @@ check_stack(unsigned long ip, unsigned long *stack)if (in_nmi ())return ; -/* -* There's a slight chance that we are tracing inside the -* RCU infrastructure, and rcu_irq_enter() will not work -* as expected. -*/ -if (unlikely (rcu_irq_enter_disabled ())) -return ; -local_irq_save (flags);arch_spin_lock (&stack_trace_max_lock); -/* -* RCU may not be watching, make it see us. -* The stack trace code uses rcu_sched. -*/ -rcu_irq_enter (); -/* In case another CPU set the tracer_frame on us */ if (unlikely (!frame_size)) this_size -= tracer_frame;
@@ -205,7 +191,6 @@ check_stack(unsigned long ip, unsigned long *stack) } out: -rcu_irq_exit ();arch_spin_unlock (&stack_trace_max_lock);local_irq_restore (flags); }
Signature · 1 line
2.13.2