tracing vs rcu vs nmi

The functions do in fact use local_irq_{save,restore}() and can therefore be used when IRQs are in fact disabled. Worse, they are already used in places where IRQs are disabled, leading to great confusion when reading the code.

Rename them to fix this confusion.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/rcupdate.h   |    4 ++--
 include/linux/rcutiny.h    |    4 ++--
 include/linux/rcutree.h    |    4 ++--
 include/linux/tracepoint.h |    4 ++--
 kernel/cpu_pm.c            |    4 ++--
 kernel/rcu/tree.c          |    8 ++++----
 kernel/trace/trace.c       |    4 ++--
 7 files changed, 16 insertions(+), 16 deletions(-)

include/linux/rcupdate.h +2 -2

@@ -120,9 +120,9 @@ static inline void rcu_init_nohz(void) {
  */
 #define RCU_NONIDLE(a) \
 	do { \
-		rcu_irq_enter_irqson(); \
+		rcu_irq_enter_irqsave(); \
 		do { a; } while (0); \
-		rcu_irq_exit_irqson(); \
+		rcu_irq_exit_irqsave(); \
 	} while (0)
 
 /*

include/linux/rcutiny.h +2 -2

@@ -68,8 +68,8 @@ static inline int rcu_jiffies_till_stall
 static inline void rcu_idle_enter(void) { }
 static inline void rcu_idle_exit(void) { }
 static inline void rcu_irq_enter(void) { }
-static inline void rcu_irq_exit_irqson(void) { }
-static inline void rcu_irq_enter_irqson(void) { }
+static inline void rcu_irq_exit_irqsave(void) { }
+static inline void rcu_irq_enter_irqsave(void) { }
 static inline void rcu_irq_exit(void) { }
 static inline void exit_rcu(void) { }
 static inline bool rcu_preempt_need_deferred_qs(struct task_struct *t)

include/linux/rcutree.h +2 -2

@@ -46,8 +46,8 @@ void rcu_idle_enter(void);
 void rcu_idle_exit(void);
 void rcu_irq_enter(void);
 void rcu_irq_exit(void);
-void rcu_irq_enter_irqson(void);
-void rcu_irq_exit_irqson(void);
+void rcu_irq_enter_irqsave(void);
+void rcu_irq_exit_irqsave(void);
 
 void exit_rcu(void);
 

include/linux/tracepoint.h +2 -2

@@ -181,7 +181,7 @@ static inline struct tracepoint *tracepo
 		 */							\
 		if (rcuidle) {						\
 			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-			rcu_irq_enter_irqson();				\
+			rcu_irq_enter_irqsave();			\
 		}							\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\

@@ -195,7 +195,7 @@ static inline struct tracepoint *tracepo
 		}							\
 									\
 		if (rcuidle) {						\
-			rcu_irq_exit_irqson();				\
+			rcu_irq_exit_irqsave();				\
 			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
 		}							\
 									\

kernel/cpu_pm.c +2 -2

@@ -24,10 +24,10 @@ static int cpu_pm_notify(enum cpu_pm_eve
 	 * could be disfunctional in cpu idle. Copy RCU_NONIDLE code to let
 	 * RCU know this.
 	 */
-	rcu_irq_enter_irqson();
+	rcu_irq_enter_irqsave();
 	ret = __atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL,
 		nr_to_call, nr_calls);
-	rcu_irq_exit_irqson();
+	rcu_irq_exit_irqsave();
 
 	return notifier_to_errno(ret);
 }

kernel/rcu/tree.c +4 -4

@@ -699,10 +699,10 @@ void rcu_irq_exit(void)
 /*
  * Wrapper for rcu_irq_exit() where interrupts are enabled.
  *
- * If you add or remove a call to rcu_irq_exit_irqson(), be sure to test
+ * If you add or remove a call to rcu_irq_exit_irqsave(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-void rcu_irq_exit_irqson(void)
+void rcu_irq_exit_irqsave(void)
 {
 	unsigned long flags;
 

@@ -875,10 +875,10 @@ void rcu_irq_enter(void)
 /*
  * Wrapper for rcu_irq_enter() where interrupts are enabled.
  *
- * If you add or remove a call to rcu_irq_enter_irqson(), be sure to test
+ * If you add or remove a call to rcu_irq_enter_irqsave(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-void rcu_irq_enter_irqson(void)
+void rcu_irq_enter_irqsave(void)
 {
 	unsigned long flags;
 

kernel/trace/trace.c +2 -2

@@ -3004,9 +3004,9 @@ void __trace_stack(struct trace_array *t
 	if (unlikely(in_nmi()))
 		return;
 
-	rcu_irq_enter_irqson();
+	rcu_irq_enter_irqsave();
 	__ftrace_trace_stack(buffer, flags, skip, pc, NULL);
-	rcu_irq_exit_irqson();
+	rcu_irq_exit_irqsave();
 }
 
 /**

Since rcu_is_watching() is notrace (and needs to be, as it can be called from the tracers), make sure everything it in turn calls is notrace too.

To that effect, mark rcu_dynticks_curr_cpu_in_eqs() inline, which implies notrace, as the function is tiny.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/rcu/tree.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I think there are ways to turn off function inlining, such as gcc's:
-fkeep-inline-functions

And just to be sure weird compilers (clang *cough*) don't screw this up, could we make it static inline notrace?

Quoted 5 lines · On Wed, 12 Feb 2020 17:38:18 -0500 Joel Fernandes <[email protected]>…

I think there are ways to turn off function inlining, such as gcc's:
-fkeep-inline-functions

And just to be sure weird compilers (clang *cough*) don't screw this up, could we make it static inline notrace?

inline is defined as notrace, so not needed.

I did that because of surprises when functions marked as inline suddenly became non inlined and traced, which caused issues with function tracing (before I finally got recursion protection working). But even then, I figured, if something is inlined and gcc actually inlines it, it wont be traced. For consistency, if something is marked inline, it should not be traced.

To facilitate tracers that need RCU, add some helpers to wrap the magic required.

The problem is that we can call into tracers (trace events and function tracing) while RCU isn't watching and this can happen from any context, including NMI.

It is this latter that is causing most of the trouble; we must make sure in_nmi() returns true before we land in anything tracing, otherwise we cannot recover.

These helpers are macros because of header-hell; they're placed here because of the proximity to nmi_{enter,exit{().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/hardirq.h |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

include/linux/hardirq.h +19 -0

@@ -89,4 +89,52 @@ extern void irq_exit(void);
 		arch_nmi_exit();				\
 	} while (0)
 
+/*
+ * Tracing vs RCU
+ * --------------
+ *
+ * tracepoints and function-tracing can happen when RCU isn't watching (idle,
+ * or early IRQ/NMI entry).
+ *
+ * When it happens during idle or early during IRQ entry, tracing will have
+ * to inform RCU that it ought to pay attention, this is done by calling
+ * rcu_irq_enter_irqsave().
+ *
+ * On NMI entry, we must be very careful that tracing only happens after we've
+ * incremented preempt_count(), otherwise we cannot tell we're in NMI and take
+ * the special path.
+ */
+
+#define __TR_IRQ	1
+#define __TR_NMI	2
+

include/linux/hardirq.h +19 -0

+		}						\
+	}							\
+	state;							\
+})
+
+#define trace_rcu_exit(state)					\
+do {								\
+	switch (state) {					\
+	case __TR_IRQ:						\
+		rcu_irq_exit_irqsave();				\
+		break;						\
+	case __TR_NMI:						\
+		rcu_nmi_exit();					\
+		break;						\
+	default:						\
+		break;						\
+	}							\
+} while (0)
+
 #endif /* LINUX_HARDIRQ_H */

include/linux/hardirq.h extern void irq_exit(void); · line 120

Quoted 10 lines · Peter Zijlstra

include/linux/hardirq.h +10 -0

+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		if (in_nmi()) {					\
+			state = __TR_NMI;			\
+			rcu_nmi_enter();			\
+		} else {					\
+			state = __TR_IRQ;			\
+			rcu_irq_enter_irqsave();		\

I think this can be simplified. You don't need to rely on in_nmi() here. I believe for NMI's, you can just call rcu_irq_enter_irqsave() and that should be sufficient to get RCU watching. Paul can correct me if I'm wrong, but I am pretty sure that would work.

In fact, I think a better naming for rcu_irq_enter_irqsave() pair could be (in the first patch):

rcu_ensure_watching_begin();
rcu_ensure_watching_end();

Quoted 18 lines · Joel Fernandes

include/linux/hardirq.h extern void irq_exit(void); · line 120

Quoted 10 lines · Peter Zijlstra

include/linux/hardirq.h +10 -0

+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		if (in_nmi()) {					\
+			state = __TR_NMI;			\
+			rcu_nmi_enter();			\
+		} else {					\
+			state = __TR_IRQ;			\
+			rcu_irq_enter_irqsave();		\

I think this can be simplified. You don't need to rely on in_nmi() here. I believe for NMI's, you can just call rcu_irq_enter_irqsave() and that should be sufficient to get RCU watching. Paul can correct me if I'm wrong, but I am pretty sure that would work.

Quoted 5 lines

In fact, I think a better naming for rcu_irq_enter_irqsave() pair could be (in the first patch):

rcu_ensure_watching_begin();
rcu_ensure_watching_end();

So I hadn't looked deeply into rcu_irq_enter(), it seems to call rcu_nmi_enter_common(), but with @irq=true.

What exactly is the purpose of that @irq argument, and how much will it hurt to lie there? Will it come apart if we have @irq != !in_nmi() for example?

There is a comment in there that says ->dynticks_nmi_nesting ought to be odd only if we're in NMI. The only place that seems to care is rcu_nmi_exit_common(), and that does indeed do something different for IRQs vs NMIs.

Quoted 18 lines · Joel Fernandes

include/linux/hardirq.h extern void irq_exit(void); · line 120

Quoted 10 lines · Peter Zijlstra

include/linux/hardirq.h +10 -0

+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		if (in_nmi()) {					\
+			state = __TR_NMI;			\
+			rcu_nmi_enter();			\
+		} else {					\
+			state = __TR_IRQ;			\
+			rcu_irq_enter_irqsave();		\

I think this can be simplified. You don't need to rely on in_nmi() here. I believe for NMI's, you can just call rcu_irq_enter_irqsave() and that should be sufficient to get RCU watching. Paul can correct me if I'm wrong, but I am pretty sure that would work.

Quoted 5 lines

In fact, I think a better naming for rcu_irq_enter_irqsave() pair could be (in the first patch):

rcu_ensure_watching_begin();
rcu_ensure_watching_end();

So I hadn't looked deeply into rcu_irq_enter(), it seems to call rcu_nmi_enter_common(), but with @irq=true.

What exactly is the purpose of that @irq argument, and how much will it hurt to lie there? Will it come apart if we have @irq != !in_nmi() for example?

There is a comment in there that says ->dynticks_nmi_nesting ought to be odd only if we're in NMI. The only place that seems to care is rcu_nmi_exit_common(), and that does indeed do something different for IRQs vs NMIs.

So I don't think we can blindly unify this. But perhaps Paul sees a way?

The reason for the irq argument is to avoid invoking rcu_prepare_for_idle() and rcu_dynticks_task_enter() from NMI context from rcu_nmi_exit_common(). Similarly, we need to avoid invoking rcu_dynticks_task_exit() and rcu_cleanup_after_idle() from NMI context from rcu_nmi_enter_common().

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 5 lines · Paul E. McKenney

The reason for the irq argument is to avoid invoking rcu_prepare_for_idle() and rcu_dynticks_task_enter() from NMI context from rcu_nmi_exit_common(). Similarly, we need to avoid invoking rcu_dynticks_task_exit() and rcu_cleanup_after_idle() from NMI context from rcu_nmi_enter_common().

Aaah, I see. I didn't grep hard enough earlier today (I only found stubs). Yes, those take locks, we mustn't call them from NMI context.

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

---8<-----------------------

kernel/rcu/tree.c +15 -31

index d91c9156fab2e..bbcc7767f18ee 100644
@@ -614,16 +614,18 @@ void rcu_user_enter(void)
 }
 #endif /* CONFIG_NO_HZ_FULL */
 
-/*
+/**
+ * rcu_nmi_exit - inform RCU of exit from NMI context
+ *
  * If we are returning from the outermost NMI handler that interrupted an
  * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
  * to let the RCU grace-period handling know that the CPU is back to
  * being RCU-idle.
  *
- * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_exit(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_exit_common(bool irq)
+__always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

@@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 
-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }
-
-/**
- * rcu_nmi_exit - inform RCU of exit from NMI context
- *
- * If you add or remove a call to rcu_nmi_exit(), be sure to test
- * with CONFIG_RCU_EQS_DEBUG=y.
- */
-void rcu_nmi_exit(void)
-{
-	rcu_nmi_exit_common(false);
-}
+NOKPROBE_SYMBOL(rcu_nmi_exit);
 
 /**
  * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle

@@ -693,7 +685,7 @@ void rcu_nmi_exit(void)
 void rcu_irq_exit(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_exit_common(true);
+	rcu_nmi_exit();
 }
 
 /*

@@ -777,7 +769,7 @@ void rcu_user_exit(void)
 #endif /* CONFIG_NO_HZ_FULL */
 
 /**
- * rcu_nmi_enter_common - inform RCU of entry to NMI context
+ * rcu_nmi_enter - inform RCU of entry to NMI context
  * @irq: Is this call from rcu_irq_enter?
  *
  * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and

@@ -786,10 +778,10 @@ void rcu_user_exit(void)
  * long as the nesting level does not overflow an int.  (You will probably
  * run out of stack space first.)
  *
- * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_enter(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_enter_common(bool irq)
+__always_inline void rcu_nmi_enter(void)
 {
 	long incby = 2;
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

@@ -807,12 +799,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 	 */
 	if (rcu_dynticks_curr_cpu_in_eqs()) {
 
-		if (irq)
+		if (!in_nmi())
 			rcu_dynticks_task_exit();
 
 		rcu_dynticks_eqs_exit();
 
-		if (irq)
+		if (!in_nmi())
 			rcu_cleanup_after_idle();
 
 		incby = 1;

@@ -834,14 +826,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
-
-/**
- * rcu_nmi_enter - inform RCU of entry to NMI context
- */
-void rcu_nmi_enter(void)
-{
-	rcu_nmi_enter_common(false);
-}
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

@@ -869,7 +853,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
 void rcu_irq_enter(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_enter_common(true);
+	rcu_nmi_enter();
 }
 
 /*

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

Thanx, Paul

Quoted 127 lines

---8<-----------------------

kernel/rcu/tree.c +15 -31

index d91c9156fab2e..bbcc7767f18ee 100644
@@ -614,16 +614,18 @@ void rcu_user_enter(void)
 }
 #endif /* CONFIG_NO_HZ_FULL */
 
-/*
+/**
+ * rcu_nmi_exit - inform RCU of exit from NMI context
+ *
  * If we are returning from the outermost NMI handler that interrupted an
  * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
  * to let the RCU grace-period handling know that the CPU is back to
  * being RCU-idle.
  *
- * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_exit(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_exit_common(bool irq)
+__always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

@@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 
-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }
-
-/**
- * rcu_nmi_exit - inform RCU of exit from NMI context
- *
- * If you add or remove a call to rcu_nmi_exit(), be sure to test
- * with CONFIG_RCU_EQS_DEBUG=y.
- */
-void rcu_nmi_exit(void)
-{
-	rcu_nmi_exit_common(false);
-}
+NOKPROBE_SYMBOL(rcu_nmi_exit);
 
 /**
  * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle

@@ -693,7 +685,7 @@ void rcu_nmi_exit(void)
 void rcu_irq_exit(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_exit_common(true);
+	rcu_nmi_exit();
 }
 
 /*

@@ -777,7 +769,7 @@ void rcu_user_exit(void)
 #endif /* CONFIG_NO_HZ_FULL */
 
 /**
- * rcu_nmi_enter_common - inform RCU of entry to NMI context
+ * rcu_nmi_enter - inform RCU of entry to NMI context
  * @irq: Is this call from rcu_irq_enter?
  *
  * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and

@@ -786,10 +778,10 @@ void rcu_user_exit(void)
  * long as the nesting level does not overflow an int.  (You will probably
  * run out of stack space first.)
  *
- * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_enter(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_enter_common(bool irq)
+__always_inline void rcu_nmi_enter(void)
 {
 	long incby = 2;
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

@@ -807,12 +799,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 	 */
 	if (rcu_dynticks_curr_cpu_in_eqs()) {
 
-		if (irq)
+		if (!in_nmi())
 			rcu_dynticks_task_exit();
 
 		rcu_dynticks_eqs_exit();
 
-		if (irq)
+		if (!in_nmi())
 			rcu_cleanup_after_idle();
 
 		incby = 1;

@@ -834,14 +826,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
-
-/**
- * rcu_nmi_enter - inform RCU of entry to NMI context
- */
-void rcu_nmi_enter(void)
-{
-	rcu_nmi_enter_common(false);
-}
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

@@ -869,7 +853,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
 void rcu_irq_enter(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_enter_common(true);
+	rcu_nmi_enter();
 }
 
 /*

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

[ Added Masami ]

Quoted 28 lines · On Thu, 13 Feb 2020 16:19:30 -0500 Joel Fernandes <[email protected]>…

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

Quoted 6 lines

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe code before kprobe_int3_handler() is called. Because ist_enter() (in do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had nothing to do with it being RCU nor NMI, but because it was simply called in do_int3().

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont to why rcu_nmi_enter() would probably be useful, like below:

-- Steve

kernel/rcu/tree.c +6 -0

index 1694a6b57ad8..e2c9e3e2f480 100644
@@ -846,6 +846,12 @@ void rcu_nmi_enter(void)
 {
 	rcu_nmi_enter_common(false);
 }
+/*
+ * On x86, All functions in do_int3() must be marked NOKPROBE before
+ * kprobe_int3_handler() is called. ist_enter() which is called in do_int3()
+ * before kprobe_int3_handle() happens to call rcu_nmi_enter() in which case
+ * rcu_nmi_enter() must be marked NOKRPOBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

Quoted 50 lines · Steven Rostedt

[ Added Masami ]

Quoted 28 lines · On Thu, 13 Feb 2020 16:19:30 -0500 Joel Fernandes <[email protected]>…

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

Quoted 6 lines

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe code before kprobe_int3_handler() is called. Because ist_enter() (in do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had nothing to do with it being RCU nor NMI, but because it was simply called in do_int3().

Quoted 2 lines

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont to why rcu_nmi_enter() would probably be useful, like below:

Thank you, Steve! Could I please have your Signed-off-by for this?

Quoted 51 lines · On Thu, 13 Feb 2020 13:50:04 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 50 lines · Steven Rostedt

[ Added Masami ]

Quoted 28 lines · On Thu, 13 Feb 2020 16:19:30 -0500 Joel Fernandes <[email protected]>…

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

Quoted 6 lines

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe code before kprobe_int3_handler() is called. Because ist_enter() (in do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had nothing to do with it being RCU nor NMI, but because it was simply called in do_int3().

Quoted 4 lines

Quoted 2 lines

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont to why rcu_nmi_enter() would probably be useful, like below:

Thank you, Steve! Could I please have your Signed-off-by for this?

Sure, but it was untested ;-)

Signed-off-by Steven Rostedt (VMware) <[email protected]>

I'd like a Reviewed-by from Masami though.

Quoted 54 lines · Steven Rostedt

Quoted 51 lines · On Thu, 13 Feb 2020 13:50:04 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 50 lines · Steven Rostedt

[ Added Masami ]

Quoted 28 lines · On Thu, 13 Feb 2020 16:19:30 -0500 Joel Fernandes <[email protected]>…

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

Quoted 6 lines

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe code before kprobe_int3_handler() is called. Because ist_enter() (in do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had nothing to do with it being RCU nor NMI, but because it was simply called in do_int3().

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont to why rcu_nmi_enter() would probably be useful, like below:

Thank you, Steve! Could I please have your Signed-off-by for this?

Sure, but it was untested ;-)

No problem! I will fire up rcutorture on it. ;-)

But experience indicates that you cannot even make a joke around here. There is probably already someone out there somewhere building a comment-checker based on deep semantic analysis and machine learning. :-/

Quoted 3 lines

Signed-off-by Steven Rostedt (VMware) <[email protected]>

I'd like a Reviewed-by from Masami though.

Sounds good! Masami, would you be willing to review?

Quoted 70 lines · On Thu, 13 Feb 2020 14:39:18 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 54 lines · Steven Rostedt

Quoted 51 lines · On Thu, 13 Feb 2020 13:50:04 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 50 lines · Steven Rostedt

[ Added Masami ]

Quoted 28 lines · On Thu, 13 Feb 2020 16:19:30 -0500 Joel Fernandes <[email protected]>…

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

Quoted 6 lines

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe code before kprobe_int3_handler() is called. Because ist_enter() (in do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had nothing to do with it being RCU nor NMI, but because it was simply called in do_int3().

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont to why rcu_nmi_enter() would probably be useful, like below:

Thank you, Steve! Could I please have your Signed-off-by for this?

Sure, but it was untested ;-)

No problem! I will fire up rcutorture on it. ;-)

But experience indicates that you cannot even make a joke around here. There is probably already someone out there somewhere building a comment-checker based on deep semantic analysis and machine learning. :-/

Quoted 2 lines

Signed-off-by Steven Rostedt (VMware) <[email protected]>

Quoted 3 lines

Quoted 1 line

I'd like a Reviewed-by from Masami though.

Sounds good! Masami, would you be willing to review?

Yes, the functions before calling kprobe_int3_handler() must not be kprobed. It can cause an infinite recursive int3 trapping.

Reviewed-by Masami Hiramatsu <[email protected]>

Quoted 80 lines · Masami Hiramatsu

Quoted 68 lines · On Thu, 13 Feb 2020 14:39:18 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 54 lines · Steven Rostedt

Quoted 51 lines · On Thu, 13 Feb 2020 13:50:04 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 50 lines · Steven Rostedt

[ Added Masami ]

Quoted 28 lines · On Thu, 13 Feb 2020 16:19:30 -0500 Joel Fernandes <[email protected]>…

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

Quoted 6 lines

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe code before kprobe_int3_handler() is called. Because ist_enter() (in do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had nothing to do with it being RCU nor NMI, but because it was simply called in do_int3().

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont to why rcu_nmi_enter() would probably be useful, like below:

Thank you, Steve! Could I please have your Signed-off-by for this?

Sure, but it was untested ;-)

No problem! I will fire up rcutorture on it. ;-)

But experience indicates that you cannot even make a joke around here. There is probably already someone out there somewhere building a comment-checker based on deep semantic analysis and machine learning. :-/

Quoted 5 lines

Quoted 3 lines

Signed-off-by Steven Rostedt (VMware) <[email protected]>

I'd like a Reviewed-by from Masami though.

Sounds good! Masami, would you be willing to review?

Yes, the functions before calling kprobe_int3_handler() must not be kprobed. It can cause an infinite recursive int3 trapping.

Quoted 1 line

Reviewed-by Masami Hiramatsu <[email protected]>

Thank you both!

Like this?

Thanx, Paul

------------------------------------------------------------------------

commit 1817fdc8f4e4bd18c76305c9b937fb0dccbb1583

Author: Steven Rostedt <[email protected]>
Date:   Sat Feb 15 06:54:50 2020 -0800

rcu: Provide comment for NOKPROBE() on rcu_nmi_enter()

The rcu_nmi_enter() function was marked NOKPROBE() by commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()") because the do_int3() call kprobe code must not be invoked before kprobe_int3_handler() is called. It turns out that ist_enter() (in do_int3()) calls rcu_nmi_enter(), hence the marking NOKPROBE() being added to rcu_nmi_enter().

This commit therefore adds a comment documenting this line of reasoning.

Signed-off-by Steven Rostedt <[email protected]>

Reviewed-by Masami Hiramatsu <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

kernel/rcu/tree.c +6 -0

index 132b53e..4a885af 100644
@@ -835,6 +835,12 @@ void rcu_nmi_enter(void)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
+/*
+ * On x86, All functions in do_int3() must be marked NOKPROBE before
+ * kprobe_int3_handler() is called. ist_enter() which is called in do_int3()
+ * before kprobe_int3_handle() happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

Quoted 83 lines · On Sat, 15 Feb 2020 06:59:34 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 81 lines · Masami Hiramatsu

Quoted 68 lines · On Thu, 13 Feb 2020 14:39:18 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 54 lines · Steven Rostedt

Quoted 51 lines · On Thu, 13 Feb 2020 13:50:04 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 50 lines · Steven Rostedt

[ Added Masami ]

Quoted 28 lines · On Thu, 13 Feb 2020 16:19:30 -0500 Joel Fernandes <[email protected]>…

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but
rcu_prepare_for_idle() in particular would be a bit ugly at best.
So, before looking into that, I have a question. Given these proposed
changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

Quoted 6 lines

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe code before kprobe_int3_handler() is called. Because ist_enter() (in do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had nothing to do with it being RCU nor NMI, but because it was simply called in do_int3().

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont to why rcu_nmi_enter() would probably be useful, like below:

Thank you, Steve! Could I please have your Signed-off-by for this?

Sure, but it was untested ;-)

No problem! I will fire up rcutorture on it. ;-)

But experience indicates that you cannot even make a joke around here. There is probably already someone out there somewhere building a comment-checker based on deep semantic analysis and machine learning. :-/

Quoted 5 lines

Quoted 3 lines

Signed-off-by Steven Rostedt (VMware) <[email protected]>

I'd like a Reviewed-by from Masami though.

Sounds good! Masami, would you be willing to review?

Yes, the functions before calling kprobe_int3_handler() must not be kprobed. It can cause an infinite recursive int3 trapping.

Reviewed-by Masami Hiramatsu <[email protected]>

Quoted 6 lines

Thank you both!

Like this?

Thanx, Paul

This is good to me.

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Thank you,

Quoted 38 lines

------------------------------------------------------------------------

commit 1817fdc8f4e4bd18c76305c9b937fb0dccbb1583

Author: Steven Rostedt <[email protected]>
Date:   Sat Feb 15 06:54:50 2020 -0800

rcu: Provide comment for NOKPROBE() on rcu_nmi_enter()

The rcu_nmi_enter() function was marked NOKPROBE() by commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()") because the do_int3() call kprobe code must not be invoked before kprobe_int3_handler() is called. It turns out that ist_enter() (in do_int3()) calls rcu_nmi_enter(), hence the marking NOKPROBE() being added to rcu_nmi_enter().

This commit therefore adds a comment documenting this line of reasoning.

Signed-off-by Steven Rostedt <[email protected]>

Reviewed-by Masami Hiramatsu <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

kernel/rcu/tree.c +6 -0

index 132b53e..4a885af 100644
@@ -835,6 +835,12 @@ void rcu_nmi_enter(void)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
+/*
+ * On x86, All functions in do_int3() must be marked NOKPROBE before
+ * kprobe_int3_handler() is called. ist_enter() which is called in do_int3()
+ * before kprobe_int3_handle() happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

Signature · 1 line

Masami Hiramatsu <[email protected]>

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Quoted 7 lines · On Mon, 17 Feb 2020 08:31:12 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for arm64 too. What about following?

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Quoted 15 lines · Masami Hiramatsu

Quoted 7 lines · On Mon, 17 Feb 2020 08:31:12 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for arm64 too. What about following?

Quoted 7 lines

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Would it work to describe the general problem, then give x86 details as a specific example, as follows?

/*
* On some architectures, certain exceptions prohibit use of kprobes until
* the exception code path reaches a certain point. For example, on x86 all
* functions called by do_int3() must be marked NOKPROBE. However, once
* kprobe_int3_handler() is called, kprobing is permitted. Specifically,
* ist_enter() is called in do_int3() before kprobe_int3_handle().
* Furthermore, ist_enter() calls rcu_nmi_enter(), which means that
* rcu_nmi_enter() must be marked NOKRPOBE.
*/

That way, I don't feel like I need to update the commment each time a new architecture adds this capability. ;-)

Would it work to describe the general problem, then give x86 details as a specific example, as follows?

/*
* On some architectures, certain exceptions prohibit use of kprobes until
* the exception code path reaches a certain point. For example, on x86 all
* functions called by do_int3() must be marked NOKPROBE. However, once
* kprobe_int3_handler() is called, kprobing is permitted. Specifically,
* ist_enter() is called in do_int3() before kprobe_int3_handle().
* Furthermore, ist_enter() calls rcu_nmi_enter(), which means that
* rcu_nmi_enter() must be marked NOKRPOBE.
*/

That way, I don't feel like I need to update the commment each time a new architecture adds this capability. ;-)

Quoted 7 lines · On Mon, 17 Feb 2020 08:31:12 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for arm64 too. What about following?

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Quoted 15 lines · On Tue, 18 Feb 2020 13:33:35 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 7 lines · On Mon, 17 Feb 2020 08:31:12 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for arm64 too. What about following?

Quoted 8 lines

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Ah, why don't we just say...

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

And that way we don't make this an arch specific comment.

Quoted 38 lines · Steven Rostedt

Quoted 15 lines · On Tue, 18 Feb 2020 13:33:35 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 7 lines · On Mon, 17 Feb 2020 08:31:12 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for arm64 too. What about following?

Quoted 8 lines

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Ah, why don't we just say...

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

Quoted 1 line

And that way we don't make this an arch specific comment.

That looks good to me. Masami, does this work for you?

Quoted 39 lines · On Tue, 18 Feb 2020 12:18:06 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 38 lines · Steven Rostedt

Quoted 15 lines · On Tue, 18 Feb 2020 13:33:35 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 7 lines · On Mon, 17 Feb 2020 08:31:12 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for arm64 too. What about following?

Quoted 8 lines

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Ah, why don't we just say...

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

Quoted 3 lines

Quoted 1 line

And that way we don't make this an arch specific comment.

That looks good to me. Masami, does this work for you?

Yes, that looks good to me too :)

Hi,

Quoted 47 lines · On Wed, 19 Feb 2020 11:45:10 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 39 lines · On Tue, 18 Feb 2020 12:18:06 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 38 lines · Steven Rostedt

Quoted 15 lines · On Tue, 18 Feb 2020 13:33:35 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 7 lines · On Mon, 17 Feb 2020 08:31:12 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for arm64 too. What about following?

Quoted 8 lines

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Ah, why don't we just say...

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

Quoted 3 lines

Quoted 1 line

And that way we don't make this an arch specific comment.

That looks good to me. Masami, does this work for you?

Yes, that looks good to me too :)

Oops, I'm guilty!
Sorry *rcu_nmi_exit()* also must be NOKPROBE, since even if we could catch a recursive kprobe call, we can only skip the kprobe handler, but we must exit from do_int3() and hit rcu_nmi_exit() again!

[45235.497591] Unrecoverable kprobe detected.
[45235.501400] Dumping kprobe:
[45235.502433] Name: (null)
[45235.502433] Offset: 0
[45235.502433] Address: rcu_nmi_exit+0x0/0x290
[45235.504044] ------------[ cut here ]------------
[45235.504855] kernel BUG at arch/x86/kernel/kprobes/core.c:646!
[45235.505816] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[45235.506615] CPU: 7 PID: 143 Comm: sh Not tainted 5.6.0-rc3+ #143
[45235.507662] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
[45235.509764] RIP: 0010:reenter_kprobe.cold+0x14/0x16
[45235.510630] Code: 48 8b 75 10 48 c7 c7 f0 70 0e 82 48 8b 56 28 e8 22 91 08 00 0f 0b 48 c7 c7 20 71 0e 82 e8 14 91 08 00 48 89 ef e8 23 ee 0f 00 <0f> 0b 48 89 ee 48 c7 c7 48 71 0e 82 e8 fb 90 08 00 e9 c3 fc ff ff
[45235.513948] RSP: 0018:ffffc90000347bf8 EFLAGS: 00010046
[45235.514906] RAX: 0000000000000036 RBX: 0000000000017f20 RCX: 0000000000000000
[45235.516109] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000001
[45235.517278] RBP: ffff88807c9820c0 R08: 0000000000000000 R09: 0000000000000001
[45235.518415] R10: 0000000000000000 R11: ffff88807c9d1f18 R12: ffff88807d9d7f20
[45235.519609] R13: ffffc90000347c68 R14: ffffffff810e8a60 R15: ffffffff810e8a61
[45235.520787] FS:  0000000001d9a8c0(0000) GS:ffff88807d9c0000(0000) knlGS:0000000000000000
[45235.522198] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[45235.523172] CR2: 0000000001da9000 CR3: 000000007a880000 CR4: 00000000000006a0
[45235.524288] Call Trace:
[45235.524825]  kprobe_int3_handler+0x74/0x150
[45235.525627]  do_int3+0x36/0xf0
[45235.526244]  int3+0x42/0x50
[45235.526767] RIP: 0010:rcu_nmi_exit+0x1/0x290
[45235.527551] Code: a2 0d 82 be c2 01 00 00 48 c7 c7 d5 44 0f 82 c6 05 e7 ac 24 01 01 e8 1f ba fd ff eb b8 66 66 2e 0f 1f 84 00 00 00 00 00 90 cc <57> 41 56 41 55 41 54 55 48 c7 c5 40 c2 02 00 53 48 89 eb e8 77 75
[45235.530898] RSP: 0018:ffffc90000347d40 EFLAGS: 00000046
[45235.531816] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[45235.533001] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
[45235.534252] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[45235.535516] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[45235.536759] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[45235.537945]  ? ist_exit+0xe/0x20
[45235.538593]  ? ist_exit+0xe/0x20
[45235.539239]  ? rcu_nmi_exit+0x1/0x290
[45235.541182]  int3+0x42/0x50
[45235.541687] RIP: 0010:0xffffffffa000005a
[45235.542363] Code: 2e 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 1f 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 20 16 13 e1 00 00 00 00 00 00 00 00 <41> 57 e9 01 8a 0e e1 00 00 00 00 00 00 00 00 41 57 e9 f2 22 26 e1
[45235.545628] RSP: 0018:ffffc90000347e20 EFLAGS: 00000146
[45235.546596] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[45235.547989] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
[45235.550183] RBP: 0000000000000000 R08: 0000000000000001 R09: ffff88807d2aa000
[45235.551591] R10: 0000000000000a4c R11: ffff88807bfec600 R12: 0000000000000000
[45235.552893] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[45235.554633]  ? ist_exit+0xe/0x20
[45235.555537]  ? ist_exit+0xe/0x20
[45235.556565]  ? rcu_nmi_exit+0x1/0x290
[45235.557909]  ? int3+0x42/0x50
[45235.559156]  ? 0xffffffffa0000069
[45235.560547]  ? vfs_read+0x1/0x150
[45235.561522]  ? ksys_read+0x60/0xe0
[45235.562458]  ? do_syscall_64+0x4b/0x1e0
[45235.563404]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[45235.564705] Modules linked in:
[45235.565556] ---[ end trace 870af8724dba9ac8 ]---

So all functions called from do_int3() must be NOKPROBE.

Thank you,

Signature · 1 line

Masami Hiramatsu <[email protected]>

Hi,

Quoted 42 lines · On Wed, 19 Feb 2020 11:45:10 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 39 lines · On Tue, 18 Feb 2020 12:18:06 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 38 lines · Steven Rostedt

Quoted 15 lines · On Tue, 18 Feb 2020 13:33:35 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 7 lines · On Mon, 17 Feb 2020 08:31:12 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for arm64 too. What about following?

Quoted 8 lines

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Ah, why don't we just say...

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

And that way we don't make this an arch specific comment.

That looks good to me. Masami, does this work for you?

Yes, that looks good to me too :)

Oops, I'm guilty!
Sorry *rcu_nmi_exit()* also must be NOKPROBE, since even if we could catch a recursive kprobe call, we can only skip the kprobe handler, but we must exit from do_int3() and hit rcu_nmi_exit() again!

[45235.497591] Unrecoverable kprobe detected.
[45235.501400] Dumping kprobe:
[45235.502433] Name: (null)
[45235.502433] Offset: 0
[45235.502433] Address: rcu_nmi_exit+0x0/0x290
[45235.504044] ------------[ cut here ]------------
[45235.504855] kernel BUG at arch/x86/kernel/kprobes/core.c:646!
[45235.505816] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[45235.506615] CPU: 7 PID: 143 Comm: sh Not tainted 5.6.0-rc3+ #143
[45235.507662] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
[45235.509764] RIP: 0010:reenter_kprobe.cold+0x14/0x16
[45235.510630] Code: 48 8b 75 10 48 c7 c7 f0 70 0e 82 48 8b 56 28 e8 22 91 08 00 0f 0b 48 c7 c7 20 71 0e 82 e8 14 91 08 00 48 89 ef e8 23 ee 0f 00 <0f> 0b 48 89 ee 48 c7 c7 48 71 0e 82 e8 fb 90 08 00 e9 c3 fc ff ff
[45235.513948] RSP: 0018:ffffc90000347bf8 EFLAGS: 00010046
[45235.514906] RAX: 0000000000000036 RBX: 0000000000017f20 RCX: 0000000000000000
[45235.516109] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000001
[45235.517278] RBP: ffff88807c9820c0 R08: 0000000000000000 R09: 0000000000000001
[45235.518415] R10: 0000000000000000 R11: ffff88807c9d1f18 R12: ffff88807d9d7f20
[45235.519609] R13: ffffc90000347c68 R14: ffffffff810e8a60 R15: ffffffff810e8a61
[45235.520787] FS:  0000000001d9a8c0(0000) GS:ffff88807d9c0000(0000) knlGS:0000000000000000
[45235.522198] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[45235.523172] CR2: 0000000001da9000 CR3: 000000007a880000 CR4: 00000000000006a0
[45235.524288] Call Trace:
[45235.524825]  kprobe_int3_handler+0x74/0x150
[45235.525627]  do_int3+0x36/0xf0
[45235.526244]  int3+0x42/0x50
[45235.526767] RIP: 0010:rcu_nmi_exit+0x1/0x290
[45235.527551] Code: a2 0d 82 be c2 01 00 00 48 c7 c7 d5 44 0f 82 c6 05 e7 ac 24 01 01 e8 1f ba fd ff eb b8 66 66 2e 0f 1f 84 00 00 00 00 00 90 cc <57> 41 56 41 55 41 54 55 48 c7 c5 40 c2 02 00 53 48 89 eb e8 77 75
[45235.530898] RSP: 0018:ffffc90000347d40 EFLAGS: 00000046
[45235.531816] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[45235.533001] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
[45235.534252] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[45235.535516] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[45235.536759] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[45235.537945]  ? ist_exit+0xe/0x20
[45235.538593]  ? ist_exit+0xe/0x20
[45235.539239]  ? rcu_nmi_exit+0x1/0x290
[45235.541182]  int3+0x42/0x50
[45235.541687] RIP: 0010:0xffffffffa000005a
[45235.542363] Code: 2e 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 1f 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 20 16 13 e1 00 00 00 00 00 00 00 00 <41> 57 e9 01 8a 0e e1 00 00 00 00 00 00 00 00 41 57 e9 f2 22 26 e1
[45235.545628] RSP: 0018:ffffc90000347e20 EFLAGS: 00000146
[45235.546596] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[45235.547989] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
[45235.550183] RBP: 0000000000000000 R08: 0000000000000001 R09: ffff88807d2aa000
[45235.551591] R10: 0000000000000a4c R11: ffff88807bfec600 R12: 0000000000000000
[45235.552893] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[45235.554633]  ? ist_exit+0xe/0x20
[45235.555537]  ? ist_exit+0xe/0x20
[45235.556565]  ? rcu_nmi_exit+0x1/0x290
[45235.557909]  ? int3+0x42/0x50
[45235.559156]  ? 0xffffffffa0000069
[45235.560547]  ? vfs_read+0x1/0x150
[45235.561522]  ? ksys_read+0x60/0xe0
[45235.562458]  ? do_syscall_64+0x4b/0x1e0
[45235.563404]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[45235.564705] Modules linked in:
[45235.565556] ---[ end trace 870af8724dba9ac8 ]---

So all functions called from do_int3() must be NOKPROBE.

Thank you,

Signature · 1 line

Masami Hiramatsu <[email protected]>

Quoted 115 lines · Masami Hiramatsu

Hi,

Quoted 42 lines · On Wed, 19 Feb 2020 11:45:10 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 39 lines · On Tue, 18 Feb 2020 12:18:06 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 38 lines · Steven Rostedt

Quoted 15 lines · On Tue, 18 Feb 2020 13:33:35 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 7 lines · On Mon, 17 Feb 2020 08:31:12 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 4 lines

BTW, if you consider the x86 specific code is in the generic file, we can move NOKPROBE_SYMBOL() in arch/x86/kernel/traps.c. (Sorry, I've hit this idea right now)

Might this affect other architectures with NMIs and probe-like things? If so, it might make sense to leave it where it is.

Yes, git grep shows that arm64 is using rcu_nmi_enter() in debug_exception_enter().
OK, let's keep it, but maybe it is good to update the comment for arm64 too. What about following?

Quoted 8 lines

+/*
+ * All functions in do_int3() on x86, do_debug_exception() on arm64 must be
+ * marked NOKPROBE before kprobes handler is called.
+ * ist_enter() on x86 and debug_exception_enter() on arm64 which is called
+ * before kprobes handle happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */

Ah, why don't we just say...

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

And that way we don't make this an arch specific comment.

That looks good to me. Masami, does this work for you?

Yes, that looks good to me too :)

Oops, I'm guilty!
Sorry *rcu_nmi_exit()* also must be NOKPROBE, since even if we could catch a recursive kprobe call, we can only skip the kprobe handler, but we must exit from do_int3() and hit rcu_nmi_exit() again!

[45235.497591] Unrecoverable kprobe detected.
[45235.501400] Dumping kprobe:
[45235.502433] Name: (null)
[45235.502433] Offset: 0
[45235.502433] Address: rcu_nmi_exit+0x0/0x290
[45235.504044] ------------[ cut here ]------------
[45235.504855] kernel BUG at arch/x86/kernel/kprobes/core.c:646!
[45235.505816] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[45235.506615] CPU: 7 PID: 143 Comm: sh Not tainted 5.6.0-rc3+ #143
[45235.507662] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
[45235.509764] RIP: 0010:reenter_kprobe.cold+0x14/0x16
[45235.510630] Code: 48 8b 75 10 48 c7 c7 f0 70 0e 82 48 8b 56 28 e8 22 91 08 00 0f 0b 48 c7 c7 20 71 0e 82 e8 14 91 08 00 48 89 ef e8 23 ee 0f 00 <0f> 0b 48 89 ee 48 c7 c7 48 71 0e 82 e8 fb 90 08 00 e9 c3 fc ff ff
[45235.513948] RSP: 0018:ffffc90000347bf8 EFLAGS: 00010046
[45235.514906] RAX: 0000000000000036 RBX: 0000000000017f20 RCX: 0000000000000000
[45235.516109] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000001
[45235.517278] RBP: ffff88807c9820c0 R08: 0000000000000000 R09: 0000000000000001
[45235.518415] R10: 0000000000000000 R11: ffff88807c9d1f18 R12: ffff88807d9d7f20
[45235.519609] R13: ffffc90000347c68 R14: ffffffff810e8a60 R15: ffffffff810e8a61
[45235.520787] FS:  0000000001d9a8c0(0000) GS:ffff88807d9c0000(0000) knlGS:0000000000000000
[45235.522198] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[45235.523172] CR2: 0000000001da9000 CR3: 000000007a880000 CR4: 00000000000006a0
[45235.524288] Call Trace:
[45235.524825]  kprobe_int3_handler+0x74/0x150
[45235.525627]  do_int3+0x36/0xf0
[45235.526244]  int3+0x42/0x50
[45235.526767] RIP: 0010:rcu_nmi_exit+0x1/0x290
[45235.527551] Code: a2 0d 82 be c2 01 00 00 48 c7 c7 d5 44 0f 82 c6 05 e7 ac 24 01 01 e8 1f ba fd ff eb b8 66 66 2e 0f 1f 84 00 00 00 00 00 90 cc <57> 41 56 41 55 41 54 55 48 c7 c5 40 c2 02 00 53 48 89 eb e8 77 75
[45235.530898] RSP: 0018:ffffc90000347d40 EFLAGS: 00000046
[45235.531816] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[45235.533001] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
[45235.534252] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[45235.535516] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[45235.536759] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[45235.537945]  ? ist_exit+0xe/0x20
[45235.538593]  ? ist_exit+0xe/0x20
[45235.539239]  ? rcu_nmi_exit+0x1/0x290
[45235.541182]  int3+0x42/0x50
[45235.541687] RIP: 0010:0xffffffffa000005a
[45235.542363] Code: 2e 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 1f 16 13 e1 00 00 00 00 00 00 00 00 89 f8 e9 20 16 13 e1 00 00 00 00 00 00 00 00 <41> 57 e9 01 8a 0e e1 00 00 00 00 00 00 00 00 41 57 e9 f2 22 26 e1
[45235.545628] RSP: 0018:ffffc90000347e20 EFLAGS: 00000146
[45235.546596] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[45235.547989] RDX: 0000000000000001 RSI: ffffffff8101e1fe RDI: ffffffff8101e1fe
[45235.550183] RBP: 0000000000000000 R08: 0000000000000001 R09: ffff88807d2aa000
[45235.551591] R10: 0000000000000a4c R11: ffff88807bfec600 R12: 0000000000000000
[45235.552893] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[45235.554633]  ? ist_exit+0xe/0x20
[45235.555537]  ? ist_exit+0xe/0x20
[45235.556565]  ? rcu_nmi_exit+0x1/0x290
[45235.557909]  ? int3+0x42/0x50
[45235.559156]  ? 0xffffffffa0000069
[45235.560547]  ? vfs_read+0x1/0x150
[45235.561522]  ? ksys_read+0x60/0xe0
[45235.562458]  ? do_syscall_64+0x4b/0x1e0
[45235.563404]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[45235.564705] Modules linked in:
[45235.565556] ---[ end trace 870af8724dba9ac8 ]---

Quoted 1 line

So all functions called from do_int3() must be NOKPROBE.

Makes sense, I am curious though why you thought before that the breakpoint recursion was not possible *after* kprobe_int3_handler(). In the below thread you said it is to be marked only for functions *before* kprobe_int3_handler(). Was there a reason?
https://lore.kernel.org/lkml/154998793571.31052.11301258949601150994.stgit@devbox/

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit

c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe code before kprobe_int3_handler() is called. Because ist_enter() (in do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had nothing to do with it being RCU nor NMI, but because it was simply called in do_int3().

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont to why rcu_nmi_enter() would probably be useful, like below:

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

thanks,

- Joel

Quoted 129 lines

Thanx, Paul

Quoted 127 lines

---8<-----------------------

kernel/rcu/tree.c +15 -31

index d91c9156fab2e..bbcc7767f18ee 100644
@@ -614,16 +614,18 @@ void rcu_user_enter(void)
 }
 #endif /* CONFIG_NO_HZ_FULL */
 
-/*
+/**
+ * rcu_nmi_exit - inform RCU of exit from NMI context
+ *
  * If we are returning from the outermost NMI handler that interrupted an
  * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
  * to let the RCU grace-period handling know that the CPU is back to
  * being RCU-idle.
  *
- * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_exit(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_exit_common(bool irq)
+__always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

@@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 
-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }
-
-/**
- * rcu_nmi_exit - inform RCU of exit from NMI context
- *
- * If you add or remove a call to rcu_nmi_exit(), be sure to test
- * with CONFIG_RCU_EQS_DEBUG=y.
- */
-void rcu_nmi_exit(void)
-{
-	rcu_nmi_exit_common(false);
-}
+NOKPROBE_SYMBOL(rcu_nmi_exit);
 
 /**
  * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle

@@ -693,7 +685,7 @@ void rcu_nmi_exit(void)
 void rcu_irq_exit(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_exit_common(true);
+	rcu_nmi_exit();
 }
 
 /*

@@ -777,7 +769,7 @@ void rcu_user_exit(void)
 #endif /* CONFIG_NO_HZ_FULL */
 
 /**
- * rcu_nmi_enter_common - inform RCU of entry to NMI context
+ * rcu_nmi_enter - inform RCU of entry to NMI context
  * @irq: Is this call from rcu_irq_enter?
  *
  * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and

@@ -786,10 +778,10 @@ void rcu_user_exit(void)
  * long as the nesting level does not overflow an int.  (You will probably
  * run out of stack space first.)
  *
- * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_enter(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_enter_common(bool irq)
+__always_inline void rcu_nmi_enter(void)
 {
 	long incby = 2;
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

@@ -807,12 +799,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 	 */
 	if (rcu_dynticks_curr_cpu_in_eqs()) {
 
-		if (irq)
+		if (!in_nmi())
 			rcu_dynticks_task_exit();
 
 		rcu_dynticks_eqs_exit();
 
-		if (irq)
+		if (!in_nmi())
 			rcu_cleanup_after_idle();
 
 		incby = 1;

@@ -834,14 +826,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
-
-/**
- * rcu_nmi_enter - inform RCU of entry to NMI context
- */
-void rcu_nmi_enter(void)
-{
-	rcu_nmi_enter_common(false);
-}
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

@@ -869,7 +853,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
 void rcu_irq_enter(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_enter_common(true);
+	rcu_nmi_enter();
 }
 
 /*

Quoted 28 lines · Joel Fernandes

Quoted 22 lines · Paul E. McKenney

Quoted 16 lines · Joel Fernandes

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

It might well be that I could make these functions be NMI-safe, but rcu_prepare_for_idle() in particular would be a bit ugly at best. So, before looking into that, I have a question. Given these proposed changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able to just use in_nmi()?

Quoted 7 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

So something like this, then? This is untested, probably doesn't even build, and could use some careful review from both Peter and Steve, at least. As in the below is the second version of the patch, the first having been missing a couple of important "!" characters.

Quoted 4 lines

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

Quoted 4 lines

My compiler complained about the static and the __always_inline, so I fixed those. But please help me out on adding the NOKPROBE_SYMBOL() to rcu_nmi_exit(). What bad thing happens if we leave this on only rcu_nmi_enter()?

Quoted 5 lines

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but allowing it on exit (from a code reading standpoint) so my reaction was to add it to both, but we could probably keep that as a separate patch/discussion since it is slightly unrelated to the patch.. Sorry to confuse the topic.

Actually and perhaps unusually, I was not being sarcastic, but was instead asking a serious question. Is the current code correct? Should the current NOKPROBE_SYMBOL() be removed? Should the other NOKPROBE_SYMBOL() be added? Something else? And either way, why?

Thanx, Paul

Quoted 134 lines

thanks,

- Joel

Quoted 129 lines

Thanx, Paul

Quoted 127 lines

---8<-----------------------

kernel/rcu/tree.c +15 -31

index d91c9156fab2e..bbcc7767f18ee 100644
@@ -614,16 +614,18 @@ void rcu_user_enter(void)
 }
 #endif /* CONFIG_NO_HZ_FULL */
 
-/*
+/**
+ * rcu_nmi_exit - inform RCU of exit from NMI context
+ *
  * If we are returning from the outermost NMI handler that interrupted an
  * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
  * to let the RCU grace-period handling know that the CPU is back to
  * being RCU-idle.
  *
- * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_exit(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_exit_common(bool irq)
+__always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

@@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 
-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }
-
-/**
- * rcu_nmi_exit - inform RCU of exit from NMI context
- *
- * If you add or remove a call to rcu_nmi_exit(), be sure to test
- * with CONFIG_RCU_EQS_DEBUG=y.
- */
-void rcu_nmi_exit(void)
-{
-	rcu_nmi_exit_common(false);
-}
+NOKPROBE_SYMBOL(rcu_nmi_exit);
 
 /**
  * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle

@@ -693,7 +685,7 @@ void rcu_nmi_exit(void)
 void rcu_irq_exit(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_exit_common(true);
+	rcu_nmi_exit();
 }
 
 /*

@@ -777,7 +769,7 @@ void rcu_user_exit(void)
 #endif /* CONFIG_NO_HZ_FULL */
 
 /**
- * rcu_nmi_enter_common - inform RCU of entry to NMI context
+ * rcu_nmi_enter - inform RCU of entry to NMI context
  * @irq: Is this call from rcu_irq_enter?
  *
  * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and

@@ -786,10 +778,10 @@ void rcu_user_exit(void)
  * long as the nesting level does not overflow an int.  (You will probably
  * run out of stack space first.)
  *
- * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_enter(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_enter_common(bool irq)
+__always_inline void rcu_nmi_enter(void)
 {
 	long incby = 2;
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

@@ -807,12 +799,12 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 	 */
 	if (rcu_dynticks_curr_cpu_in_eqs()) {
 
-		if (irq)
+		if (!in_nmi())
 			rcu_dynticks_task_exit();
 
 		rcu_dynticks_eqs_exit();
 
-		if (irq)
+		if (!in_nmi())
 			rcu_cleanup_after_idle();
 
 		incby = 1;

@@ -834,14 +826,6 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
-
-/**
- * rcu_nmi_enter - inform RCU of entry to NMI context
- */
-void rcu_nmi_enter(void)
-{
-	rcu_nmi_enter_common(false);
-}
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

@@ -869,7 +853,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
 void rcu_irq_enter(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_enter_common(true);
+	rcu_nmi_enter();
 }
 
 /*

Oh ok, it was a fair question. Seems Steve nailed it, only the rcu_nmi_enter() needs NOKPROBE, although as you mentioned in the other thread, it would be good to get Masami's eyes on it since he introduced the NOKPROBE.

Quoted 2 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

kernel/rcu/tree.c +1 -0

+__always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

@@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 

Quoted 16 lines · Joel Fernandes

Quoted 2 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

kernel/rcu/tree.c +1 -0

+__always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

@@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 

kernel/rcu/tree.c static __always_inline void rcu_nmi_exit_common(bool irq) · line 663

Quoted 10 lines

kernel/rcu/tree.c +2 -2

-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }

Boris and me have been going over the #MC code (and finding loads of 'interesting' code) and ran into ist_enter(), whish has the following code:

/*
* We might have interrupted pretty much anything. In
* fact, if we're a machine check, we can even interrupt
* NMI processing. We don't want in_nmi() to return true,
* but we need to notify RCU.
*/
rcu_nmi_enter();

Which, to me, sounds all sorts of broken. The IST (be it #DB or #MC) can happen while we're holding all sorts of locks. This must be an NMI-like context.

Quoted 42 lines · Peter Zijlstra

Quoted 16 lines · Joel Fernandes

Quoted 2 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

kernel/rcu/tree.c +1 -0

+__always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

@@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 

kernel/rcu/tree.c static __always_inline void rcu_nmi_exit_common(bool irq) · line 663

Quoted 10 lines

kernel/rcu/tree.c +2 -2

-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }

Boris and me have been going over the #MC code (and finding loads of 'interesting' code) and ran into ist_enter(), whish has the following code:

/*
* We might have interrupted pretty much anything. In
* fact, if we're a machine check, we can even interrupt
* NMI processing. We don't want in_nmi() to return true,
* but we need to notify RCU.
*/
rcu_nmi_enter();

Quoted 3 lines

Which, to me, sounds all sorts of broken. The IST (be it #DB or #MC) can happen while we're holding all sorts of locks. This must be an NMI-like context.

Ouch! Looks like I need to hold off on getting rid of the "irq" parameters if in_nmi() isn't going to be accurate.

Quoted 47 lines · Paul E. McKenney

Quoted 45 lines · Peter Zijlstra

Quoted 16 lines · Joel Fernandes

Quoted 2 lines

Quoted 2 lines

That _should_ already be the case today. That is, if we end up in a tracer and in_nmi() is unreliable we're already screwed anyway.

I removed the static from rcu_nmi_enter()/exit() as it is called from outside, that makes it build now. Updated below is Paul's diff. I also added NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed asymmetric.

kernel/rcu/tree.c +1 -0

+__always_inline void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

@@ -651,25 +653,15 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 

kernel/rcu/tree.c static __always_inline void rcu_nmi_exit_common(bool irq) · line 663

Quoted 10 lines

kernel/rcu/tree.c +2 -2

-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }

Boris and me have been going over the #MC code (and finding loads of 'interesting' code) and ran into ist_enter(), whish has the following code:

/*
* We might have interrupted pretty much anything. In
* fact, if we're a machine check, we can even interrupt
* NMI processing. We don't want in_nmi() to return true,
* but we need to notify RCU.
*/
rcu_nmi_enter();

Which, to me, sounds all sorts of broken. The IST (be it #DB or #MC) can happen while we're holding all sorts of locks. This must be an NMI-like context.

Quoted 2 lines

Ouch! Looks like I need to hold off on getting rid of the "irq" parameters if in_nmi() isn't going to be accurate.

I'm currently trying to twist my brain around all this, because I suspect it's all completely broken one way or another.

But yes, we definitely need to fix this before your patch goes in.

OK. I will drop it later today, but leave tag in_nmi.2020.02.18a pointing at it for future reference.

Quoted 2 lines · Paul E. McKenney

OK. I will drop it later today, but leave tag in_nmi.2020.02.18a pointing at it for future reference.

Thanks, I've picked it up in my series.

To facilitate tracers that need RCU, add some helpers to wrap the magic required.

The problem is that we can call into tracers (trace events and function tracing) while RCU isn't watching and this can happen from any context, including NMI.

It is this latter that is causing most of the trouble; we must make sure in_nmi() returns true before we land in anything tracing, otherwise we cannot recover.

These helpers are macros because of header-hell; they're placed here because of the proximity to nmi_{enter,exit{().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/hardirq.h |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

include/linux/hardirq.h +19 -0

@@ -89,4 +89,52 @@ extern void irq_exit(void);
 		arch_nmi_exit();				\
 	} while (0)
 
+/*
+ * Tracing vs RCU
+ * --------------
+ *
+ * tracepoints and function-tracing can happen when RCU isn't watching (idle,
+ * or early IRQ/NMI entry).
+ *
+ * When it happens during idle or early during IRQ entry, tracing will have
+ * to inform RCU that it ought to pay attention, this is done by calling
+ * rcu_irq_enter_irqsave().
+ *
+ * On NMI entry, we must be very careful that tracing only happens after we've
+ * incremented preempt_count(), otherwise we cannot tell we're in NMI and take
+ * the special path.
+ */
+
+#define __TR_IRQ	1
+#define __TR_NMI	2
+

include/linux/hardirq.h +19 -0

+		}						\
+	}							\
+	state;							\
+})
+
+#define trace_rcu_exit(state)					\
+do {								\
+	switch (state) {					\
+	case __TR_IRQ:						\
+		rcu_irq_exit_irqsave();				\
+		break;						\
+	case __TR_NMI:						\
+		rcu_nmi_exit();					\
+		break;						\
+	default:						\
+		break;						\
+	}							\
+} while (0)
+
 #endif /* LINUX_HARDIRQ_H */

include/linux/hardirq.h extern void irq_exit(void); · line 120

Quoted 10 lines · Peter Zijlstra

include/linux/hardirq.h +10 -0

+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		if (in_nmi()) {					\
+			state = __TR_NMI;			\
+			rcu_nmi_enter();			\
+		} else {					\
+			state = __TR_IRQ;			\
+			rcu_irq_enter_irqsave();		\

Since rcu_irq_enter_irqsave can be called from a tracer context, should those be marked with notrace as well? AFAICS, there's no notrace marking on them.

Quoted 15 lines · Joel Fernandes

include/linux/hardirq.h extern void irq_exit(void); · line 120

Quoted 10 lines · Peter Zijlstra

include/linux/hardirq.h +10 -0

+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		if (in_nmi()) {					\
+			state = __TR_NMI;			\
+			rcu_nmi_enter();			\
+		} else {					\
+			state = __TR_IRQ;			\
+			rcu_irq_enter_irqsave();		\

Since rcu_irq_enter_irqsave can be called from a tracer context, should those be marked with notrace as well? AFAICS, there's no notrace marking on them.

It should work, these functions are re-entrant (as are IRQs / NMIs) and Steve wants to be able to trace RCU itself.

The tracepoint interface will stop providing regular RCU context; make sure we do it ourselves, since perf makes use of regular RCU protected data.

Suggested-by Steven Rostedt (VMware) <[email protected]>

Suggested-by Mathieu Desnoyers <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/events/core.c |    5 +++++
 1 file changed, 5 insertions(+)

kernel/events/core.c +4 -0

 	struct perf_raw_record raw = {
 		.frag = {

@@ -8961,6 +8962,8 @@ void perf_tp_event(u16 event_type, u64 c
 	perf_sample_data_init(&data, 0, 0);
 	data.raw = &raw;
 
+	rcu_flags = trace_rcu_enter();
+
 	perf_trace_buf_update(record, event_type);
 
 	hlist_for_each_entry_rcu(event, head, hlist_entry) {

@@ -8996,6 +8999,8 @@ void perf_tp_event(u16 event_type, u64 c
 	}
 
 	perf_swevent_put_recursion_context(rctx);
+
+	trace_rcu_exit(rcu_flags);
 }
 EXPORT_SYMBOL_GPL(perf_tp_event);
 

Quoted 11 lines · Peter Zijlstra

The tracepoint interface will stop providing regular RCU context; make sure we do it ourselves, since perf makes use of regular RCU protected data.

Suggested-by Steven Rostedt (VMware) <[email protected]>

Suggested-by Mathieu Desnoyers <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/events/core.c |    5 +++++
 1 file changed, 5 insertions(+)

kernel/events/core.c void perf_tp_event(u16 event_type, u64 c · line 8953

Quoted 7 lines

kernel/events/core.c +1 -0

@@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 c
 {
 	struct perf_sample_data data;
 	struct perf_event *event;
+	unsigned long rcu_flags;

The flags are not needed I guess, if you agree on not using in_nmi() in trace_rcu_enter().

Quoted 12 lines · Joel Fernandes

Quoted 11 lines · Peter Zijlstra

The tracepoint interface will stop providing regular RCU context; make sure we do it ourselves, since perf makes use of regular RCU protected data.

Suggested-by Steven Rostedt (VMware) <[email protected]>

Suggested-by Mathieu Desnoyers <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/events/core.c |    5 +++++
 1 file changed, 5 insertions(+)

Quoted 10 lines

kernel/events/core.c void perf_tp_event(u16 event_type, u64 c · line 8953

Quoted 7 lines

kernel/events/core.c +1 -0

@@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 c
 {
 	struct perf_sample_data data;
 	struct perf_event *event;
+	unsigned long rcu_flags;

The flags are not needed I guess, if you agree on not using in_nmi() in trace_rcu_enter().

Even then we need to store the state: 'didn't do nothing' vs 'did call rcu_needs_to_wake_up_and_pay_attention_noaw'. That is, we only need to do something (expensive!) when !rcu_is_watching().

Since perf is now able to deal with !rcu_is_watching() contexts, remove the restraint.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/trace/trace_event_perf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 14 lines · Peter Zijlstra

Since perf is now able to deal with !rcu_is_watching() contexts, remove the restraint.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/trace/trace_event_perf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/trace/trace_event_perf.c +0 -0

@@ -477,7 +477,7 @@ static int perf_ftrace_function_register
 {
 	struct ftrace_ops *ops = &event->ftrace_ops;
 

kernel/trace/trace_event_perf.c static int perf_ftrace_function_register · line 480

Quoted 2 lines

kernel/trace/trace_event_perf.c +1 -1

-	ops->flags   = FTRACE_OPS_FL_RCU;
+	ops->flags   = 0;

FTRACE_OPS_FL_ENABLED?

Quoted 9 lines

kernel/trace/trace_event_perf.c static int perf_ftrace_function_register · line 480

Quoted 7 lines

+++ b/kernel/trace/trace_event_perf.c

kernel/trace/trace_event_perf.c +1 -1

@@ -477,7 +477,7 @@ static int perf_ftrace_function_register
 {
 	struct ftrace_ops *ops = &event->ftrace_ops;
 
-	ops->flags   = FTRACE_OPS_FL_RCU;
+	ops->flags   = 0;  

FTRACE_OPS_FL_ENABLED?

No, never mind.

Since perf is now able to deal with !rcu_is_watching() contexts, remove the restraint.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/trace/trace_event_perf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 14 lines · Peter Zijlstra

Since perf is now able to deal with !rcu_is_watching() contexts, remove the restraint.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/trace/trace_event_perf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/trace/trace_event_perf.c +0 -0

@@ -477,7 +477,7 @@ static int perf_ftrace_function_register
 {
 	struct ftrace_ops *ops = &event->ftrace_ops;
 

kernel/trace/trace_event_perf.c static int perf_ftrace_function_register · line 482

Quoted 4 lines

kernel/trace/trace_event_perf.c +1 -1

-	ops->flags   = FTRACE_OPS_FL_RCU;
+	ops->flags   = 0;
 	ops->func    = perf_ftrace_function_call;
 	ops->private = (void *)(unsigned long)nr_cpu_ids;  

If this is the last user of the flag, should all remaining FTRACE_OPS_FL_RCU references be removed, too?