tracing vs world

kernel/locking/lockdep.c +1 -1

@@ -379,13 +379,13 @@ void lockdep_init_task(struct task_struc
 
 void lockdep_off(void)
 {
-	current->lockdep_recursion++;
+	current->lockdep_recursion += BIT(16);
 }
 EXPORT_SYMBOL(lockdep_off);
 
 void lockdep_on(void)
 {

kernel/locking/lockdep.c +8 -0

+		return false;
+
+	if (!in_nmi())
+		return false;
+
+	return true;
+}
+

Quoted 18 lines · On Fri, 21 Feb 2020 14:34:17 +0100 Peter Zijlstra <[email protected]> wrote:

kernel/locking/lockdep.c +2 -2

@@ -379,13 +379,13 @@ void lockdep_init_task(struct task_struc
 
 void lockdep_off(void)
 {
-	current->lockdep_recursion++;
+	current->lockdep_recursion += BIT(16);
 }
 EXPORT_SYMBOL(lockdep_off);
 
 void lockdep_on(void)
 {
-	current->lockdep_recursion--;
+	current->lockdep_recursion -= BIT(16);
 }
 EXPORT_SYMBOL(lockdep_on);
 

kernel/locking/lockdep.c void lock_downgrade(struct lockdep_map * · line 4487

Quoted 4 lines

kernel/locking/lockdep.c +4 -0

+
+static bool lockdep_nmi(void)
+{
+	if (current->lockdep_recursion & 0xFFFF)

Nitpick, but the association with bit 16 and this mask really should be defined as a macro somewhere and not have hard coded numbers.

Quoted 2 lines

Nitpick, but the association with bit 16 and this mask really should be defined as a macro somewhere and not have hard coded numbers.

Right, I suppose I can do something like:

#define LOCKDEP_RECURSION_BITS 16
#define LOCKDEP_OFF (1U << LOCKDEP_RECURSION_BITS) #define LOCKDEP_RECURSION_MASK (LOCKDEP_OFF - 1)

Quoted 22 lines · Steven Rostedt

Quoted 18 lines · On Fri, 21 Feb 2020 14:34:17 +0100 Peter Zijlstra <[email protected]> wrote:

kernel/locking/lockdep.c +2 -2

@@ -379,13 +379,13 @@ void lockdep_init_task(struct task_struc
 
 void lockdep_off(void)
 {
-	current->lockdep_recursion++;
+	current->lockdep_recursion += BIT(16);
 }
 EXPORT_SYMBOL(lockdep_off);
 
 void lockdep_on(void)
 {
-	current->lockdep_recursion--;
+	current->lockdep_recursion -= BIT(16);
 }
 EXPORT_SYMBOL(lockdep_on);
 

Quoted 7 lines

kernel/locking/lockdep.c void lock_downgrade(struct lockdep_map * · line 4487

Quoted 4 lines

kernel/locking/lockdep.c +4 -0

+
+static bool lockdep_nmi(void)
+{
+	if (current->lockdep_recursion & 0xFFFF)

Nitpick, but the association with bit 16 and this mask really should be defined as a macro somewhere and not have hard coded numbers.

Right, I suppose I can do something like:

#define LOCKDEP_RECURSION_BITS 16
#define LOCKDEP_OFF (1U << LOCKDEP_RECURSION_BITS) #define LOCKDEP_RECURSION_MASK (LOCKDEP_OFF - 1)

nmi_enter() does lockdep_off() and hence lockdep ignores everything.

And NMI context makes it impossible to do full IN-NMI tracking like we do IN-HARDIRQ, that could result in graph_lock recursion.

However, since look_up_lock_class() is lockless, we can find the class of a lock that has prior use and detect IN-NMI after USED, just not USED after IN-NMI.

NOTE: By shifting the lockdep_off() recursion count to bit-16, we can easily differentiate between actual recursion and off.

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

---
 kernel/locking/lockdep.c |   53 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 3 deletions(-)

kernel/locking/lockdep.c +50 -3

@@ -379,13 +379,13 @@ void lockdep_init_task(struct task_struc
 
 void lockdep_off(void)
 {
-	current->lockdep_recursion++;
+	current->lockdep_recursion += BIT(16);
 }
 EXPORT_SYMBOL(lockdep_off);
 
 void lockdep_on(void)
 {
-	current->lockdep_recursion--;
+	current->lockdep_recursion -= BIT(16);
 }
 EXPORT_SYMBOL(lockdep_on);
 

@@ -575,6 +575,7 @@ static const char *usage_str[] =
 #include "lockdep_states.h"
 #undef LOCKDEP_STATE
 	[LOCK_USED] = "INITIAL USE",
+	[LOCK_USAGE_STATES] = "IN-NMI",
 };
 #endif
 

@@ -787,6 +788,7 @@ static int count_matching_names(struct l
 	return count + 1;
 }
 
+/* used from NMI context -- must be lockless */
 static inline struct lock_class *
 look_up_lock_class(const struct lockdep_map *lock, unsigned int subclass)
 {

@@ -4463,6 +4465,34 @@ void lock_downgrade(struct lockdep_map *
 }
 EXPORT_SYMBOL_GPL(lock_downgrade);
 
+/* NMI context !!! */
+static void verify_lock_unused(struct lockdep_map *lock, struct held_lock *hlock, int subclass)
+{
+	struct lock_class *class = look_up_lock_class(lock, subclass);
+
+	/* if it doesn't have a class (yet), it certainly hasn't been used yet */
+	if (!class)
+		return;
+
+	if (!(class->usage_mask & LOCK_USED))
+		return;
+
+	hlock->class_idx = class - lock_classes;
+
+	print_usage_bug(current, hlock, LOCK_USED, LOCK_USAGE_STATES);
+}
+
+static bool lockdep_nmi(void)
+{
+	if (current->lockdep_recursion & 0xFFFF)
+		return false;
+
+	if (!in_nmi())
+		return false;
+
+	return true;
+}
+
 /*
  * We are not always called with irqs disabled - do that here,
  * and also avoid lockdep recursion:

@@ -4473,8 +4503,25 @@ void lock_acquire(struct lockdep_map *lo
 {
 	unsigned long flags;
 
-	if (unlikely(current->lockdep_recursion))
+	if (unlikely(current->lockdep_recursion)) {
+		/* XXX allow trylock from NMI ?!? */
+		if (lockdep_nmi() && !trylock) {
+			struct held_lock hlock;
+
+			hlock.acquire_ip = ip;
+			hlock.instance = lock;
+			hlock.nest_lock = nest_lock;
+			hlock.irq_context = 2; // XXX
+			hlock.trylock = trylock;
+			hlock.read = read;
+			hlock.check = check;
+			hlock.hardirqs_off = true;
+			hlock.references = 0;
+
+			verify_lock_unused(lock, &hlock, subclass);
+		}
 		return;
+	}
 
 	raw_local_irq_save(flags);
 	check_flags(flags);

Quoted 4 lines · Peter Zijlstra

nmi_enter() does lockdep_off() and hence lockdep ignores everything.

And NMI context makes it impossible to do full IN-NMI tracking like we do IN-HARDIRQ, that could result in graph_lock recursion.

The patch makes sense to me.

Reviewed-by Joel Fernandes (Google) <[email protected]>

NOTE:
Also, I was wondering if we can detect the graph_lock recursion case and avoid doing anything bad, that way we enable more of the lockdep functionality for NMI where possible. Not sure if the suggestion makes sense though!

Quoted 10 lines · Joel Fernandes

Quoted 4 lines · Peter Zijlstra

nmi_enter() does lockdep_off() and hence lockdep ignores everything.

And NMI context makes it impossible to do full IN-NMI tracking like we do IN-HARDIRQ, that could result in graph_lock recursion.

The patch makes sense to me.

Reviewed-by Joel Fernandes (Google) <[email protected]>

Quoted 5 lines

NOTE:
Also, I was wondering if we can detect the graph_lock recursion case and avoid doing anything bad, that way we enable more of the lockdep functionality for NMI where possible. Not sure if the suggestion makes sense though!

Yeah, I considered playing trylock games, but figured I shouldn't make it more complicated that it needs to be.

Since there are already a number of sites (ARM64, PowerPC) that effectively nest nmi_enter(), lets make the primitive support this before adding even more.

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

Reviewed-by Petr Mladek <[email protected]>

Acked-by Will Deacon <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/hardirq.h |    4 ++--
 arch/arm64/kernel/sdei.c         |   14 ++------------
 arch/arm64/kernel/traps.c        |    8 ++------
 arch/powerpc/kernel/traps.c      |   22 ++++++----------------
 include/linux/hardirq.h          |    5 ++++-
 include/linux/preempt.h          |    4 ++--
 kernel/printk/printk_safe.c      |    6 ++++--
 7 files changed, 22 insertions(+), 41 deletions(-)

kernel/printk/printk_safe.c +0 -1

@@ -296,12 +296,14 @@ static __printf(1, 0) int vprintk_nmi(co
 
 void notrace printk_nmi_enter(void)
 {
-	this_cpu_or(printk_context, PRINTK_NMI_CONTEXT_MASK);

Quoted 29 lines · Peter Zijlstra

Since there are already a number of sites (ARM64, PowerPC) that effectively nest nmi_enter(), lets make the primitive support this before adding even more.

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

Reviewed-by Petr Mladek <[email protected]>

Acked-by Will Deacon <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/hardirq.h |    4 ++--
 arch/arm64/kernel/sdei.c         |   14 ++------------
 arch/arm64/kernel/traps.c        |    8 ++------
 arch/powerpc/kernel/traps.c      |   22 ++++++----------------
 include/linux/hardirq.h          |    5 ++++-
 include/linux/preempt.h          |    4 ++--
 kernel/printk/printk_safe.c      |    6 ++++--
 7 files changed, 22 insertions(+), 41 deletions(-)

kernel/printk/printk_safe.c +2 -1

@@ -296,12 +296,14 @@ static __printf(1, 0) int vprintk_nmi(co
 
 void notrace printk_nmi_enter(void)
 {
-	this_cpu_or(printk_context, PRINTK_NMI_CONTEXT_MASK);
+	if (!in_nmi())
+		this_cpu_or(printk_context, PRINTK_NMI_CONTEXT_MASK);
 }
 

kernel/printk/printk_safe.c static __printf(1, 0) int vprintk_nmi(co · line 307

Quoted 6 lines

kernel/printk/printk_safe.c +2 -1

 void notrace printk_nmi_exit(void)
 {
-	this_cpu_and(printk_context, ~PRINTK_NMI_CONTEXT_MASK);
+	if (!in_nmi())
+		this_cpu_and(printk_context, ~PRINTK_NMI_CONTEXT_MASK);
 }

If the outermost NMI is interrupted while between printk_nmi_enter() and preempt_count_add(), there is still a risk that we race and clear?

Quoted 2 lines · Frederic Weisbecker

If the outermost NMI is interrupted while between printk_nmi_enter() and preempt_count_add(), there is still a risk that we race and clear?

Great catch!

There is plenty of space in the printk_context variable. I would reserve one byte there for the NMI context to be on the safe side and be done with it.

It should never overflow. The BUG_ON(in_nmi() == NMI_MASK) in nmi_enter() will trigger much earlier.

Also I hope that printk_context will get removed with the lockless printk() implementation soon anyway.

kernel/printk/internal.h +5 -3

index c8e6ab689d42..109c5ab70a0c 100644
@@ -6,9 +6,11 @@
 
 #ifdef CONFIG_PRINTK
 
-#define PRINTK_SAFE_CONTEXT_MASK	 0x3fffffff
-#define PRINTK_NMI_DIRECT_CONTEXT_MASK	 0x40000000
-#define PRINTK_NMI_CONTEXT_MASK		 0x80000000
+#define PRINTK_SAFE_CONTEXT_MASK	0x007ffffff
+#define PRINTK_NMI_DIRECT_CONTEXT_MASK	0x008000000
+#define PRINTK_NMI_CONTEXT_MASK		0xff0000000
+
+#define PRINTK_NMI_CONTEXT_OFFSET	0x010000000
 
 extern raw_spinlock_t logbuf_lock;
 

kernel/printk/printk_safe.c +2 -2

index b4045e782743..e8989418a139 100644
@@ -296,12 +296,12 @@ static __printf(1, 0) int vprintk_nmi(const char *fmt, va_list args)
 
 void notrace printk_nmi_enter(void)
 {
-	this_cpu_or(printk_context, PRINTK_NMI_CONTEXT_MASK);
+	this_cpu_add(printk_context, PRINTK_NMI_CONTEXT_OFFSET);
 }
 
 void notrace printk_nmi_exit(void)
 {
-	this_cpu_and(printk_context, ~PRINTK_NMI_CONTEXT_MASK);
+	this_cpu_sub(printk_context, PRINTK_NMI_CONTEXT_OFFSET);
 }
 
 /*

Best Regards,
Petr

Quoted 29 lines · Peter Zijlstra

Since there are already a number of sites (ARM64, PowerPC) that effectively nest nmi_enter(), lets make the primitive support this before adding even more.

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

Reviewed-by Petr Mladek <[email protected]>

Acked-by Will Deacon <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/hardirq.h |    4 ++--
 arch/arm64/kernel/sdei.c         |   14 ++------------
 arch/arm64/kernel/traps.c        |    8 ++------
 arch/powerpc/kernel/traps.c      |   22 ++++++----------------
 include/linux/hardirq.h          |    5 ++++-
 include/linux/preempt.h          |    4 ++--
 kernel/printk/printk_safe.c      |    6 ++++--
 7 files changed, 22 insertions(+), 41 deletions(-)

kernel/printk/printk_safe.c +2 -1

@@ -296,12 +296,14 @@ static __printf(1, 0) int vprintk_nmi(co
 
 void notrace printk_nmi_enter(void)
 {
-	this_cpu_or(printk_context, PRINTK_NMI_CONTEXT_MASK);
+	if (!in_nmi())
+		this_cpu_or(printk_context, PRINTK_NMI_CONTEXT_MASK);
 }
 

kernel/printk/printk_safe.c static __printf(1, 0) int vprintk_nmi(co · line 307

Quoted 6 lines

kernel/printk/printk_safe.c +2 -1

 void notrace printk_nmi_exit(void)
 {
-	this_cpu_and(printk_context, ~PRINTK_NMI_CONTEXT_MASK);
+	if (!in_nmi())
+		this_cpu_and(printk_context, ~PRINTK_NMI_CONTEXT_MASK);
 }

If the outermost NMI is interrupted while between printk_nmi_enter() and preempt_count_add(), there is still a risk that we race and clear?

Damn, true. That also means I need to fix the arm64 bits, and that's a little more tricky.

Something like so perhaps.. hmm?

---

arch/arm64/include/asm/hardirq.h +14 -11

@@ -32,30 +32,52 @@ u64 smp_irq_stat_cpu(unsigned int cpu);
 
 struct nmi_ctx {
 	u64 hcr;
+	unsigned int cnt;
 };
 
 DECLARE_PER_CPU(struct nmi_ctx, nmi_contexts);
 
-#define arch_nmi_enter()							\
-	do {									\
-		if (is_kernel_in_hyp_mode() && !in_nmi()) {			\
-			struct nmi_ctx *nmi_ctx = this_cpu_ptr(&nmi_contexts);	\
-			nmi_ctx->hcr = read_sysreg(hcr_el2);			\
-			if (!(nmi_ctx->hcr & HCR_TGE)) {			\
-				write_sysreg(nmi_ctx->hcr | HCR_TGE, hcr_el2);	\
-				isb();						\
-			}							\
-		}								\
-	} while (0)
+#define arch_nmi_enter()						\
+do {									\
+	struct nmi_ctx *___ctx;						\
+	unsigned int ___cnt;						\
+									\
+	if (!is_kernel_in_hyp_mode() || in_nmi())			\
+		break;							\
+									\
+	___ctx = this_cpu_ptr(&nmi_contexts);				\
+	___cnt = ___ctx->cnt;						\
+	if (!(___cnt & 1) && __cnt) {					\
+		___ctx->cnt += 2;					\
+		break;							\

Quoted 13 lines · Peter Zijlstra

arch/arm64/include/asm/hardirq.h +13 -0

+#define arch_nmi_enter()						\
+do {									\
+	struct nmi_ctx *___ctx;						\
+	unsigned int ___cnt;						\
+									\
+	if (!is_kernel_in_hyp_mode() || in_nmi())			\
+		break;							\
+									\
+	___ctx = this_cpu_ptr(&nmi_contexts);				\
+	___cnt = ___ctx->cnt;						\
+	if (!(___cnt & 1) && __cnt) {					\
+		___ctx->cnt += 2;					\
+		break;							\

arch/arm64/include/asm/hardirq.h u64 smp_irq_stat_cpu(unsigned int cpu); · line 62

Quoted 10 lines

arch/arm64/include/asm/hardirq.h +10 -0

+	}								\
+									\
+	___ctx->cnt |= 1;						\
+	barrier();							\
+	nmi_ctx->hcr = read_sysreg(hcr_el2);				\
+	if (!(nmi_ctx->hcr & HCR_TGE)) {				\
+		write_sysreg(nmi_ctx->hcr | HCR_TGE, hcr_el2);		\
+		isb();							\
+	}								\
+	barrier();							\

Suppose the first NMI is interrupted here. nmi_ctx->hcr has HCR_TGE unset. The new NMI is going to overwrite nmi_ctx->hcr with HCR_TGE set. Then the first NMI will not restore the correct value upon arch_nmi_exit().

So perhaps the below, but I bet I overlooked something obvious.

Quoted 30 lines · Frederic Weisbecker

Quoted 13 lines · Peter Zijlstra

arch/arm64/include/asm/hardirq.h +13 -0

+#define arch_nmi_enter()						\
+do {									\
+	struct nmi_ctx *___ctx;						\
+	unsigned int ___cnt;						\
+									\
+	if (!is_kernel_in_hyp_mode() || in_nmi())			\
+		break;							\
+									\
+	___ctx = this_cpu_ptr(&nmi_contexts);				\
+	___cnt = ___ctx->cnt;						\
+	if (!(___cnt & 1) && __cnt) {					\
+		___ctx->cnt += 2;					\
+		break;							\

arch/arm64/include/asm/hardirq.h u64 smp_irq_stat_cpu(unsigned int cpu); · line 62

Quoted 10 lines

arch/arm64/include/asm/hardirq.h +10 -0

+	}								\
+									\
+	___ctx->cnt |= 1;						\
+	barrier();							\
+	nmi_ctx->hcr = read_sysreg(hcr_el2);				\
+	if (!(nmi_ctx->hcr & HCR_TGE)) {				\
+		write_sysreg(nmi_ctx->hcr | HCR_TGE, hcr_el2);		\
+		isb();							\
+	}								\
+	barrier();							\

Suppose the first NMI is interrupted here. nmi_ctx->hcr has HCR_TGE unset. The new NMI is going to overwrite nmi_ctx->hcr with HCR_TGE set. Then the first NMI will not restore the correct value upon arch_nmi_exit().

Quoted 1 line

So perhaps the below, but I bet I overlooked something obvious.

Well, none of this is obvious :/

The basic idea was that the LSB signifies 'pending/in-progress' and when that is set, nobody else touches no nothing. Enter will unconditionally (re) write_sysreg(), exit will nothing.

Obviously I messed that up.

How's this?

#define arch_nmi_enter() \

do {									\
	struct nmi_ctx *___ctx;						\
	unsigned int ___cnt;						\
									\
	if (!is_kernel_in_hyp_mode() || in_nmi())			\
		break;							\
									\
	___ctx = this_cpu_ptr(&nmi_contexts);				\
	___cnt = ___ctx->cnt;						\
	if (!(___cnt & 1)) { /* !IN-PROGRESS */				\
		if (___cnt) {						\
			___ctx->cnt += 2;				\
			break;						\
		}							\
									\
		___ctx->hcr = read_sysreg(hcr_el2);			\
		barrier();						\
		___ctx->cnt |= 1; /* IN-PROGRESS */			\
		barrier();						\
	}								\
									\
	if (!(___ctx->hcr & HCR_TGE)) {					\
		write_sysreg(___ctx->hcr | HCR_TGE, hcr_el2);		\
		isb();							\
	}								\
	barrier();							\
	if (!(___cnt & 1))						\
		___ctx->cnt++; /* COMPLETE */				\
} while (0)

#define arch_nmi_exit() \

do {									\
	struct nmi_ctx *___ctx;						\
									\
	if (!is_kernel_in_hyp_mode() || in_nmi())			\
		break;							\
									\
	___ctx = this_cpu_ptr(&nmi_contexts);				\
	if ((___ctx->cnt & 1) || (___ctx->cnt -= 2))			\
		break;							\

	if (!(___ctx->hcr & HCR_TGE))					\
		write_sysreg(___ctx->hcr, hcr_el2);			\
} while (0)

Quoted 30 lines · Frederic Weisbecker

Quoted 13 lines · Peter Zijlstra

arch/arm64/include/asm/hardirq.h +13 -0

+#define arch_nmi_enter()						\
+do {									\
+	struct nmi_ctx *___ctx;						\
+	unsigned int ___cnt;						\
+									\
+	if (!is_kernel_in_hyp_mode() || in_nmi())			\
+		break;							\
+									\
+	___ctx = this_cpu_ptr(&nmi_contexts);				\
+	___cnt = ___ctx->cnt;						\
+	if (!(___cnt & 1) && __cnt) {					\
+		___ctx->cnt += 2;					\
+		break;							\

arch/arm64/include/asm/hardirq.h u64 smp_irq_stat_cpu(unsigned int cpu); · line 62

Quoted 10 lines

arch/arm64/include/asm/hardirq.h +10 -0

+	}								\
+									\
+	___ctx->cnt |= 1;						\
+	barrier();							\
+	nmi_ctx->hcr = read_sysreg(hcr_el2);				\
+	if (!(nmi_ctx->hcr & HCR_TGE)) {				\
+		write_sysreg(nmi_ctx->hcr | HCR_TGE, hcr_el2);		\
+		isb();							\
+	}								\
+	barrier();							\

Suppose the first NMI is interrupted here. nmi_ctx->hcr has HCR_TGE unset. The new NMI is going to overwrite nmi_ctx->hcr with HCR_TGE set. Then the first NMI will not restore the correct value upon arch_nmi_exit().

Quoted 1 line

So perhaps the below, but I bet I overlooked something obvious.

Well, none of this is obvious :/

The basic idea was that the LSB signifies 'pending/in-progress' and when that is set, nobody else touches no nothing. Enter will unconditionally (re) write_sysreg(), exit will nothing.

So here is my previous proposal, based on a simple counter, this time with comments and a few fixes:

Quoted 2 lines · Frederic Weisbecker

So here is my previous proposal, based on a simple counter, this time with comments and a few fixes:

I've presumed your SoB and made this your patch.

Make sure we run task_work before we hit any kind of userspace -- very much including signals.

Suggested-by Andy Lutomirski <[email protected]>

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

---
 arch/x86/entry/common.c                   |    8 
 usr/src/linux-2.6/arch/x86/entry/common.c |  440 ------------------------------
 2 files changed, 4 insertions(+), 444 deletions(-)

arch/x86/entry/common.c +0 -4

@@ -155,16 +155,16 @@ static void exit_to_usermode_loop(struct
 		if (cached_flags & _TIF_PATCH_PENDING)
 			klp_update_patch_state(current);
 
-		/* deal with pending signal delivery */
-		if (cached_flags & _TIF_SIGPENDING)
-			do_signal(regs);
-

It is an abomination; and in prepration of removing the whole ist_enter() thing, it needs to go.

Convert #MC over to using task_work_add() instead; it will run the same code slightly later, on the return to user path of the same exception.

It is an abomination; and in prepration of removing the whole ist_enter() thing, it needs to go.

Convert #MC over to using task_work_add() instead; it will run the same code slightly later, on the return to user path of the same exception.

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

A few exceptions (like #DB and #BP) can happen at any location in the code, this then means that tracers should treat events from these exceptions as NMI-like. We could be holding locks with interrupts disabled for instance.

Similarly, #MC is an actual NMI-like exception.

arch/x86/kernel/traps.c +1 -1

-dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
+dotraplinkage notrace void do_int3(struct pt_regs *regs, long error_code)
 {
        if (poke_int3_handler(regs))
                return;

arch/x86/kernel/traps.c dotraplinkage void do_general_protection · line 525

Quoted 9 lines

arch/x86/kernel/traps.c +1 -8

-       /*
-        * Use ist_enter despite the fact that we don't use an IST stack.
-        * We can be called from a kprobe in non-CONTEXT_KERNEL kernel
-        * mode or even during context tracking state changes.
-        *
-        * This means that we can't schedule.  That's okay.
-        */
-       ist_enter(regs);
+       nmi_enter();

I agree with the change, but some commentary might be nice. Maybe copy from here:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/idtentry&id=061eaa900b4f63601ab6381ab431fcef8dfd84be

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

arch/x86/kernel/traps.c +1 -8

-       /*
-        * Use ist_enter despite the fact that we don't use an IST stack.
-        * We can be called from a kprobe in non-CONTEXT_KERNEL kernel
-        * mode or even during context tracking state changes.
-        *
-        * This means that we can't schedule.  That's okay.
-        */
-       ist_enter(regs);
+       nmi_enter();

Quoted 4 lines

I agree with the change, but some commentary might be nice. Maybe copy from here:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/idtentry&id=061eaa900b4f63601ab6381ab431fcef8dfd84be

Fair enough; I'll add something to #DB and #BP for that.

arch/x86/kernel/traps.c +1 -3

-dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
+dotraplinkage notrace void do_int3(struct pt_regs *regs, long error_code)
 {
 	if (poke_int3_handler(regs))
 		return;
 
 	/*
-	 * Use ist_enter despite the fact that we don't use an IST stack.
-	 * We can be called from a kprobe in non-CONTEXT_KERNEL kernel

arch/x86/kernel/traps.c +1 -1

 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
 #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
 	if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP,

@@ -563,7 +530,7 @@ dotraplinkage void notrace do_int3(struc
 	cond_local_irq_disable(regs);
 
 exit:
-	ist_exit(regs);
+	nmi_exit();
 }
 NOKPROBE_SYMBOL(do_int3);

Quoted 17 lines · On Mon, 24 Feb 2020 11:43:46 +0100 Peter Zijlstra <[email protected]> wrote:

arch/x86/kernel/traps.c +5 -6

-dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
+dotraplinkage notrace void do_int3(struct pt_regs *regs, long error_code)
 {
 	if (poke_int3_handler(regs))
 		return;
 
 	/*
-	 * Use ist_enter despite the fact that we don't use an IST stack.
-	 * We can be called from a kprobe in non-CONTEXT_KERNEL kernel
-	 * mode or even during context tracking state changes.
-	 *
-	 * This means that we can't schedule.  That's okay.
+	 * Unlike any other non-IST entry, we can be called from pretty much
+	 * any location in the kernel through kprobes -- text_poke() will most
+	 * likely be handled by poke_int3_handler() above. This means this
+	 * handler is effectively NMI-like.
 	 */

Review

Quoted 2 lines

(quoted diff) +1 -1

-	ist_enter(regs);
+	nmi_enter();

Hmm, note that nmi_enter() calls other functions. Did you make sure all of them are not able to be kprobed. This is different than just being "NMI like", it's that if they are kprobed, then this will go into an infinite loop because nothing can have a kprobe before the kprobe int3 handler is called here.

Looking at nmi_enter(), that leaves trace_hardirq_enter(), since we know we marked rcu_nmi_enter() as NOKPROBES, per the patches elsewhere in this series.

Quoted 3 lines · On Mon, 24 Feb 2020 17:34:09 +0100 Peter Zijlstra <[email protected]> wrote:

Looking at nmi_enter(), that leaves trace_hardirq_enter(), since we know we marked rcu_nmi_enter() as NOKPROBES, per the patches elsewhere in this series.

Maybe this was addressed already in the series, but I'm just looking at Linus's master branch we have:

#define nmi_enter()                                             \
        do {                                                    \
                arch_nmi_enter();                               \
                printk_nmi_enter();                             \
                lockdep_off();                                  \
                ftrace_nmi_enter();                             \
                BUG_ON(in_nmi());                               \
                preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET); \
                rcu_nmi_enter();                                \
                trace_hardirq_enter();                          \
        } while (0)

Just want to confirm that printk_nmi_enter(), lockdep_off(), and ftrace_nmi_enter() are all marked fully with NOKPROBE.

Quoted 2 lines

Just want to confirm that printk_nmi_enter(), lockdep_off(), and ftrace_nmi_enter() are all marked fully with NOKPROBE.

*sigh*, right you are, I only looked at notrace, not nokprobe.

In particular the ftrace one is a bit off a mess, let me sort through that.

Quoted 3 lines · On Mon, 24 Feb 2020 22:31:39 +0100 Peter Zijlstra <[email protected]> wrote:

Quoted 2 lines

Just want to confirm that printk_nmi_enter(), lockdep_off(), and ftrace_nmi_enter() are all marked fully with NOKPROBE.

Quoted 4 lines

*sigh*, right you are, I only looked at notrace, not nokprobe.

In particular the ftrace one is a bit off a mess, let me sort through that.

ftrace_nmi_enter() has two purposes. One, for archs that need to deal with NMIs while they still use stop machine. Although, it appears sh is the only arch that does that. I can look to see if it can be ripped out.

Subject: sh/ftrace: Move arch_ftrace_nmi_{enter,exit} into nmi exception
From: Peter Zijlstra <[email protected]>
Date: Mon Feb 24 22:26:21 CET 2020

SuperH is the last remaining user of arch_ftrace_nmi_{enter,exit}(), remove it from the generic code and into the SuperH code.

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

---

arch/sh/kernel/traps.c +6 -0

+extern void arch_ftrace_nmi_enter(void);
+extern void arch_ftrace_nmi_exit(void);
+#else
+static inline void arch_ftrace_nmi_enter(void) { }
+static inline void arch_ftrace_nmi_exit(void) { }
+#endif

The other is for the hwlat detector that measures the time it was in an NMI, as NMIs appear as a hardware latency too.

Quoted 2 lines · Steven Rostedt

The other is for the hwlat detector that measures the time it was in an NMI, as NMIs appear as a hardware latency too.

Yeah,.. I hate that one. But I ended up with this patch.

And yes, I know some of those notrace annotations are strictly unnessecary due to Makefile crap, but having them is _SO_ much easier.

---
Subject: x86,tracing: Robustify ftrace_nmi_enter()
From: Peter Zijlstra <[email protected]>
Date: Mon Feb 24 23:40:29 CET 2020

ftrace_nmi_enter()
trace_hwlat_callback()
trace_clock_local()
sched_clock()
paravirt_sched_clock()
native_sched_clock()

All must not be traced or kprobed, it will be called from do_debug() before the kprobe handler.

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

---
 arch/x86/include/asm/paravirt.h |    2 +-
 arch/x86/kernel/tsc.c           |    7 +++++--
 include/linux/ftrace_irq.h      |    4 ++--
 kernel/trace/trace_clock.c      |    2 ++
 kernel/trace/trace_hwlat.c      |    4 +++-
 5 files changed, 13 insertions(+), 6 deletions(-)

Quoted 2 lines · Steven Rostedt

The other is for the hwlat detector that measures the time it was in an NMI, as NMIs appear as a hardware latency too.

Yeah,.. I hate that one. But I ended up with this patch.

And yes, I know some of those notrace annotations are strictly unnessecary due to Makefile crap, but having them is _SO_ much easier.

---
Subject: x86,tracing: Robustify ftrace_nmi_enter()
From: Peter Zijlstra <[email protected]>
Date: Mon Feb 24 23:40:29 CET 2020

ftrace_nmi_enter()
trace_hwlat_callback()
trace_clock_local()
sched_clock()
paravirt_sched_clock()
native_sched_clock()

All must not be traced or kprobed, it will be called from do_debug() before the kprobe handler.

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

---
 arch/x86/include/asm/paravirt.h |    2 +-
 arch/x86/kernel/tsc.c           |    7 +++++--
 include/linux/ftrace_irq.h      |    4 ++--
 kernel/trace/trace_clock.c      |    2 ++
 kernel/trace/trace_hwlat.c      |    4 +++-
 5 files changed, 13 insertions(+), 6 deletions(-)

arch/x86/include/asm/paravirt.h +1 -1

@@ -17,7 +17,7 @@
 #include <linux/cpumask.h>
 #include <asm/frame.h>
 
-static inline unsigned long long paravirt_sched_clock(void)
+static __always_inline unsigned long long paravirt_sched_clock(void)
 {
 	return PVOP_CALL0(unsigned long long, time.sched_clock);
 }

arch/x86/kernel/tsc.c +5 -2

@@ -14,6 +14,7 @@
 #include <linux/percpu.h>
 #include <linux/timex.h>
 #include <linux/static_key.h>
+#include <linux/kprobes.h>
 
 #include <asm/hpet.h>
 #include <asm/timer.h>

@@ -207,7 +208,7 @@ static void __init cyc2ns_init_secondary
 /*
  * Scheduler clock - returns current time in nanosec units.
  */
-u64 native_sched_clock(void)
+notrace u64 native_sched_clock(void)
 {
 	if (static_branch_likely(&__use_tsc)) {
 		u64 tsc_now = rdtsc();

@@ -228,6 +229,7 @@ u64 native_sched_clock(void)
 	/* No locking but a rare wrong value is not a big deal: */
 	return (jiffies_64 - INITIAL_JIFFIES) * (1000000000 / HZ);
 }
+NOKPROBE_SYMBOL(native_sched_clock);
 
 /*
  * Generate a sched_clock if you already have a TSC value.

@@ -240,10 +242,11 @@ u64 native_sched_clock_from_tsc(u64 tsc)
 /* We need to define a real function for sched_clock, to override the
    weak default version */
 #ifdef CONFIG_PARAVIRT
-unsigned long long sched_clock(void)
+notrace unsigned long long sched_clock(void)
 {
 	return paravirt_sched_clock();
 }
+NOKPROBE_SYMBOL(sched_clock);
 
 bool using_native_sched_clock(void)
 {

include/linux/ftrace_irq.h +2 -2

@@ -7,7 +7,7 @@ extern bool trace_hwlat_callback_enabled
 extern void trace_hwlat_callback(bool enter);
 #endif
 
-static inline void ftrace_nmi_enter(void)
+static __always_inline void ftrace_nmi_enter(void)
 {
 #ifdef CONFIG_HWLAT_TRACER
 	if (trace_hwlat_callback_enabled)

@@ -15,7 +15,7 @@ static inline void ftrace_nmi_enter(void
 #endif
 }
 
-static inline void ftrace_nmi_exit(void)
+static __always_inline void ftrace_nmi_exit(void)
 {
 #ifdef CONFIG_HWLAT_TRACER
 	if (trace_hwlat_callback_enabled)

kernel/trace/trace_clock.c +2 -0

@@ -22,6 +22,7 @@
 #include <linux/sched/clock.h>
 #include <linux/ktime.h>
 #include <linux/trace_clock.h>
+#include <linux/kprobes.h>
 
 /*
  * trace_clock_local(): the simplest and least coherent tracing clock.

@@ -44,6 +45,7 @@ u64 notrace trace_clock_local(void)
 
 	return clock;
 }
+NOKPROBE_SYMBOL(trace_clock_local);
 EXPORT_SYMBOL_GPL(trace_clock_local);
 
 /*

kernel/trace/trace_hwlat.c +3 -1

@@ -43,6 +43,7 @@
 #include <linux/cpumask.h>
 #include <linux/delay.h>
 #include <linux/sched/clock.h>
+#include <linux/kprobes.h>
 #include "trace.h"
 
 static struct trace_array	*hwlat_trace;

@@ -137,7 +138,7 @@ static void trace_hwlat_sample(struct hw
 #define init_time(a, b)	(a = b)
 #define time_u64(a)	a
 
-void trace_hwlat_callback(bool enter)
+notrace void trace_hwlat_callback(bool enter)
 {
 	if (smp_processor_id() != nmi_cpu)
 		return;

@@ -156,6 +157,7 @@ void trace_hwlat_callback(bool enter)
 	if (enter)
 		nmi_count++;
 }
+NOKPROBE_SYMBOL(trace_hwlat_callback);
 
 /**
  * get_sample - sample the CPU TSC and look for likely hardware latencies

Use of memmove() in #DF is problematic when you consider tracing and other instrumentation.

Remove the memmove() call and simply write out what need doing; Boris argues the ranges should not overlap.

Survives selftests/x86, specifically sigreturn_64.

(Andy ?!)

From: Paul E. McKenney <[email protected]>

The rcu_nmi_enter_common() and rcu_nmi_exit_common() functions take an "irq" parameter that indicates whether these functions are invoked from an irq handler (irq==true) or an NMI handler (irq==false). However, recent changes have applied notrace to a few critical functions such that rcu_nmi_enter_common() and rcu_nmi_exit_common() many now rely on in_nmi(). Note that in_nmi() works no differently than before, but rather that tracing is now prohibited in code regions where in_nmi() would incorrectly report NMI state.

This commit therefore removes the "irq" parameter and inlines rcu_nmi_enter_common() and rcu_nmi_exit_common() into rcu_nmi_enter() and rcu_nmi_exit(), respectively.

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

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

From: Steven Rostedt (VMware) <[email protected]>

It's confusing that rcu_nmi_enter() is marked NOKPROBE and rcu_nmi_exit() is not. One may think that the exit needs to be marked for the same reason the enter is, as rcu_nmi_exit() reverts the RCU state back to what it was before rcu_nmi_enter(). But the reason has nothing to do with the state of RCU.

The breakpoint handler (int3 on x86) must not have any kprobe on it until the kprobe handler is called. Otherwise, it can cause an infinite recursion and crash the machine. It just so happens that rcu_nmi_enter() is called by the int3 handler before the kprobe handler can run, and therefore needs to be marked as NOKPROBE.

Comment this to remove the confusion to why rcu_nmi_enter() is marked NOKPROBE but rcu_nmi_exit() is not.

Reported-by Joel Fernandes (Google) <[email protected]>

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

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

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

Reviewed-by Masami Hiramatsu <[email protected]>

Acked-by Joel Fernandes (Google) <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

kernel/rcu/tree.c +2 -2

  *
- * 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/rcu/tree.c void rcu_irq_exit(void) · line 700

Quoted 5 lines · On Fri, 21 Feb 2020 14:34:25 +0100 Peter Zijlstra <[email protected]> wrote:

kernel/rcu/tree.c +0 -0

@@ -699,10 +699,10 @@ void rcu_irq_exit(void)
 /*
  * Wrapper for rcu_irq_exit() where interrupts are enabled.

where interrupts may be enabled.

Quoted 10 lines

kernel/rcu/tree.c +2 -2

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

kernel/rcu/tree.c void rcu_irq_enter(void) · line 876

Quoted 3 lines

kernel/rcu/tree.c +0 -0

@@ -875,10 +875,10 @@ void rcu_irq_enter(void)
 /*
  * Wrapper for rcu_irq_enter() where interrupts are enabled.

where interrupts may be enabled.

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

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

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.

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

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

---
 include/linux/rcupdate.h |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

include/linux/rcupdate.h +22 -0

@@ -175,6 +175,35 @@ do { \
 #error "Unknown RCU implementation specified to kernel configuration"
 #endif
 
+/**
+ * trace_rcu_enter - Force RCU to be active, for code that needs RCU readers
+ *
+ * Very similar to RCU_NONIDLE() above.
+ *
+ * Tracing can happen while RCU isn't active yet, for instance in the idle loop
+ * between rcu_idle_enter() and rcu_idle_exit(), or early in exception entry.
+ * RCU will happily ignore any read-side critical sections in this case.
+ *
+ * This function ensures that RCU is aware hereafter and the code can readily
+ * rely on RCU read-side critical sections working as expected.
+ *
+ * This function is NMI safe -- provided in_nmi() is correct and will nest up-to
+ * INT_MAX/2 times.
+ */
+static inline int trace_rcu_enter(void)
+{
+	int state = !rcu_is_watching();
+	if (state)
+		rcu_irq_enter_irqsave();
+	return state;
+}

Quoted 9 lines

(quoted diff) +9 -0

+static inline int trace_rcu_enter(void)
+{
+	int state = !rcu_is_watching();
+	if (state)
+		rcu_irq_enter_irqsave();
+	return state;
+}
+
+static inline void trace_rcu_exit(int state)

Review

Quoted 8 lines

(quoted diff) +5 -0

+{
+	if (state)
+		rcu_irq_exit_irqsave();
+}
+
 /*
  * The init_rcu_head_on_stack() and destroy_rcu_head_on_stack() calls
  * are needed for dynamic initialization and destruction of rcu_head

Effectively revert commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") now that we've taught perf how to deal with not having an RCU context provided.

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

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

---
 include/linux/tracepoint.h |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

include/linux/tracepoint.h +1 -3

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo
 		 * For rcuidle callers, use srcu since sched-rcu	\
 		 * doesn't work from the idle path.			\
 		 */							\
-		if (rcuidle) {						\
+		if (rcuidle)						\
 			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-			rcu_irq_enter_irqsave();			\
-		}							\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
 									\

@@ -194,10 +192,8 @@ static inline struct tracepoint *tracepo
 			} while ((++it_func_ptr)->func);		\

Quoted 31 lines · Peter Zijlstra

Effectively revert commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") now that we've taught perf how to deal with not having an RCU context provided.

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

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

---
 include/linux/tracepoint.h |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

include/linux/tracepoint.h +2 -5

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo
 		 * For rcuidle callers, use srcu since sched-rcu	\
 		 * doesn't work from the idle path.			\
 		 */							\
-		if (rcuidle) {						\
+		if (rcuidle)						\
 			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-			rcu_irq_enter_irqsave();			\
-		}							\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
 									\

@@ -194,10 +192,8 @@ static inline struct tracepoint *tracepo
 			} while ((++it_func_ptr)->func);		\
 		}							\
 									\
-		if (rcuidle) {						\
-			rcu_irq_exit_irqsave();				\
+		if (rcuidle)						\

include/linux/tracepoint.h static inline struct tracepoint *tracepo · line 199

Quoted 5 lines

include/linux/tracepoint.h +0 -1

 			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
-		}							\
 									\
 		preempt_enable_notrace();				\
 	} while (0)

So what happens when BPF registers for these tracepoints? BPF very much wants RCU on AFAIU.

Quoted 32 lines · Peter Zijlstra

Quoted 31 lines · Peter Zijlstra

Effectively revert commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") now that we've taught perf how to deal with not having an RCU context provided.

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

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

---
 include/linux/tracepoint.h |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

include/linux/tracepoint.h +2 -5

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo
             * For rcuidle callers, use srcu since sched-rcu        \
             * doesn't work from the idle path.                     \
             */                                                     \
-           if (rcuidle) {                                          \
+           if (rcuidle)                                            \
                    __idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-                   rcu_irq_enter_irqsave();                        \
-           }                                                       \
                                                                    \
            it_func_ptr = rcu_dereference_raw((tp)->funcs);         \
                                                                    \

@@ -194,10 +192,8 @@ static inline struct tracepoint *tracepo
                    } while ((++it_func_ptr)->func);                \
            }                                                       \
                                                                    \
-           if (rcuidle) {                                          \
-                   rcu_irq_exit_irqsave();                         \
+           if (rcuidle)                                            \

Quoted 8 lines

include/linux/tracepoint.h static inline struct tracepoint *tracepo · line 199

Quoted 5 lines

include/linux/tracepoint.h +0 -1

                    srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
-           }                                                       \
                                                                    \
            preempt_enable_notrace();                               \
    } while (0)

So what happens when BPF registers for these tracepoints? BPF very much wants RCU on AFAIU.

I suspect we needs something like this...

kernel/trace/bpf_trace.c +0 -0

index a2f15222f205..67a39dbce0ce 100644

Quoted 57 lines · Peter Zijlstra

Quoted 32 lines · Peter Zijlstra

Quoted 31 lines · Peter Zijlstra

Effectively revert commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") now that we've taught perf how to deal with not having an RCU context provided.

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

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

---
 include/linux/tracepoint.h |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

include/linux/tracepoint.h +2 -5

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo
             * For rcuidle callers, use srcu since sched-rcu        \
             * doesn't work from the idle path.                     \
             */                                                     \
-           if (rcuidle) {                                          \
+           if (rcuidle)                                            \
                    __idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-                   rcu_irq_enter_irqsave();                        \
-           }                                                       \
                                                                    \
            it_func_ptr = rcu_dereference_raw((tp)->funcs);         \
                                                                    \

@@ -194,10 +192,8 @@ static inline struct tracepoint *tracepo
                    } while ((++it_func_ptr)->func);                \
            }                                                       \
                                                                    \
-           if (rcuidle) {                                          \
-                   rcu_irq_exit_irqsave();                         \
+           if (rcuidle)                                            \

Quoted 8 lines

include/linux/tracepoint.h static inline struct tracepoint *tracepo · line 199

Quoted 5 lines

include/linux/tracepoint.h +0 -1

                    srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
-           }                                                       \
                                                                    \
            preempt_enable_notrace();                               \
    } while (0)

So what happens when BPF registers for these tracepoints? BPF very much wants RCU on AFAIU.

I suspect we needs something like this...

kernel/trace/bpf_trace.c +1 -0

index a2f15222f205..67a39dbce0ce 100644
@@ -1475,11 +1475,13 @@ void bpf_put_raw_tracepoint(struct bpf_raw_event_map
 *btp)
 static __always_inline
 void __bpf_trace_run(struct bpf_prog *prog, u64 *args)
 {
+       int rcu_flags = trace_rcu_enter();
        rcu_read_lock();
        preempt_disable();

kernel/trace/bpf_trace.c void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp) · line 1484

Quoted 4 lines

kernel/trace/bpf_trace.c +1 -0

        (void) BPF_PROG_RUN(prog, args);
        preempt_enable();
        rcu_read_unlock();
+       trace_rcu_exit(rcu_flags);

One big NACK.
I will not slowdown 99% of cases because of one dumb user. Absolutely no way.

If we care about not adding those extra branches on the fast-path, there is an alternative way to do things: BPF could provide two distinct probe callbacks, one meant for rcuidle tracepoints (which would have the trace_rcu_enter/exit), and the other for the for 99% of the other callsites which have RCU watching.

I would recommend performing benchmarks justifying the choice of one approach over the other though.