tracing: Have all levels of checks prevent recursion

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

While writing an email explaining the "bit = 0" logic for a discussion on

bit = trace_get_context_bit() + start;

Quoted 5 lines · Steven Rostedt

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

While writing an email explaining the "bit = 0" logic for a discussion on

bit = trace_get_context_bit() + start;

While there, you were also going to update that function to match/use get_recursion_context(). Because your version is still branch hell.

Quoted 7 lines · On Fri, 15 Oct 2021 18:17:02 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 5 lines · Steven Rostedt

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

While writing an email explaining the "bit = 0" logic for a discussion on

bit = trace_get_context_bit() + start;

Quoted 2 lines

While there, you were also going to update that function to match/use get_recursion_context(). Because your version is still branch hell.

That would probably be a separate patch. This is just a fix to a design flaw, changing the context tests would be performance enhancement.

Thanks for the reminder (as it was on my todo list to update that code).

Something like this:

Quoted 1 line · Steven Rostedt

Something like this:

I think having one copy of that in a header is better than having 3 copies. But yes, something along them lines.

Quoted 2 lines

I think having one copy of that in a header is better than having 3 copies. But yes, something along them lines.

I was just about to ask you about this patch ;-)

Quoted 4 lines · On Fri, 15 Oct 2021 14:20:33 -0400 Steven Rostedt <[email protected]> wrote:

Quoted 2 lines

I think having one copy of that in a header is better than having 3 copies. But yes, something along them lines.

I was just about to ask you about this patch ;-)

Except it doesn't build :-p (need to move the inlined function down a bit)

include/linux/preempt.h +7 -0

index 4d244e295e85..b32e3dabe28b 100644
@@ -77,6 +77,27 @@
 /* preempt_count() and related functions, depends on PREEMPT_NEED_RESCHED */
 #include <asm/preempt.h>
 
+/**
+ * interrupt_context_level - return interrupt context level
+ *
+ * Returns the current interrupt context level.
+ *  0 - normal context
+ *  1 - softirq context
+ *  2 - hardirq context

From: Steven Rostedt <[email protected]>
Date: Fri, 15 Oct 2021 14:25:41 -0400

Sorry for such a necroposting :z
Just wanted to know if this is a bug, so that I could send a fix, or intended behaviour.

Quoted 30 lines

Quoted 4 lines · On Fri, 15 Oct 2021 14:20:33 -0400 Steven Rostedt <[email protected]> wrote:

Quoted 2 lines

I think having one copy of that in a header is better than having 3 copies. But yes, something along them lines.

I was just about to ask you about this patch ;-)

Except it doesn't build :-p (need to move the inlined function down a bit)

include/linux/preempt.h +12 -0

index 4d244e295e85..b32e3dabe28b 100644
@@ -77,6 +77,27 @@
 /* preempt_count() and related functions, depends on PREEMPT_NEED_RESCHED */
 #include <asm/preempt.h>
 
+/**
+ * interrupt_context_level - return interrupt context level
+ *
+ * Returns the current interrupt context level.
+ *  0 - normal context
+ *  1 - softirq context
+ *  2 - hardirq context
+ *  3 - NMI context
+ */
+static __always_inline unsigned char interrupt_context_level(void)
+{
+	unsigned long pc = preempt_count();

include/linux/preempt.h line 96

Quoted 5 lines

include/linux/preempt.h +5 -0

+	unsigned char level = 0;
+
+	level += !!(pc & (NMI_MASK));
+	level += !!(pc & (NMI_MASK | HARDIRQ_MASK));
+	level += !!(pc & (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET));  

This doesn't take into account that we can switch the context manually via local_bh_disable() / local_irq_save() etc. During the testing of the

separate issue[0], I've found that the function returns 1 in both just softirq and softirq under local_irq_save().
Is this intended? Shouldn't that be

	level += !!(pc & (NMI_MASK));
	level += !!(pc * (NMI_MASK | HARDIRQ_MASK)) || irqs_disabled();
	level += !!(pc * (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET)) ||
		 in_atomic();

?
Otherwise, the result it returns is not really "context level".

include/linux/preempt.h line 100

Quoted 5 lines

include/linux/preempt.h +4 -0

+
+	return level;
+}
+  

[0]
https://lore.kernel.org/netdev/[email protected]

Thanks,
Olek

On Fri, 21 Jul 2023 17:34:41 +0200
Alexander Lobakin <[email protected]> wrote:

Quoted 6 lines

include/linux/preempt.h line 96

Quoted 3 lines

include/linux/preempt.h +3 -0

+	level += !!(pc & (NMI_MASK));
+	level += !!(pc & (NMI_MASK | HARDIRQ_MASK));
+	level += !!(pc & (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET));  

This doesn't take into account that we can switch the context manually via local_bh_disable() / local_irq_save() etc. During the testing of the

You cannot manually switch interrupt context.

Quoted 3 lines

separate issue[0], I've found that the function returns 1 in both just softirq and softirq under local_irq_save().
Is this intended? Shouldn't that be

That is intended behavior.

local_bh_disable() and local_irq_save() is not a context switch. It is just preventing that context from happening. The interrupt_context_level() is to tell us what context we are running in, not what context is disabled.

Quoted 8 lines

	level += !!(pc & (NMI_MASK));
	level += !!(pc * (NMI_MASK | HARDIRQ_MASK)) || irqs_disabled();
	level += !!(pc * (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET)) ||
		 in_atomic();

?
Otherwise, the result it returns is not really "context level".

local_bh_disable() use to (and perhaps still does in some configurations) confuse things. But read the comment in kernel/softirq.c

/*
* SOFTIRQ_OFFSET usage:
*
* On !RT kernels 'count' is the preempt counter, on RT kernels this applies
* to a per CPU counter and to task::softirqs_disabled_cnt.
*
* - count is changed by SOFTIRQ_OFFSET on entering or leaving softirq
* processing.
*
* - count is changed by SOFTIRQ_DISABLE_OFFSET (= 2 * SOFTIRQ_OFFSET)
* on local_bh_disable or local_bh_enable.
*
* This lets us distinguish between whether we are currently processing
* softirq and whether we just have bh disabled.
*/

Just because you disable interrupts does not mean you are in interrupt context.

-- Steve

Quoted 10 lines

include/linux/preempt.h line 100

Quoted 5 lines

include/linux/preempt.h +4 -0

+
+	return level;
+}
+  

[0]
https://lore.kernel.org/netdev/[email protected]

Thanks,
Olek

Quoted 2 lines

Just because you disable interrupts does not mean you are in interrupt context.

Ah okay, thanks! IOW, if we want to check in some code that we're certainly have interrupts enabled and are not in the interrupt context, we must always do

if (!(in_hardirq() || irqs_disabled()))

, nothing more elegant / already existing / ...?

Quoted 2 lines

if (!(in_hardirq() || irqs_disabled()))

Yeah, probably.

Quoted 1 line

, nothing more elegant / already existing / ...?

It's not a common check. What would you call that?

Quoted 5 lines · On Fri, 15 Oct 2021 20:04:29 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 1 line · Steven Rostedt

Something like this:

I think having one copy of that in a header is better than having 3 copies. But yes, something along them lines.

I was just about to ask you about this patch ;-)

kernel/events/internal.h DEFINE_OUTPUT_COPY(__output_copy_user, arch_perf_out_copy_user) · line 208

Quoted 4 lines

kernel/events/internal.h +0 -0

@@ -206,11 +206,7 @@ DEFINE_OUTPUT_COPY(__output_copy_user, arch_perf_out_copy_user)
 static inline int get_recursion_context(int *recursion)
 {
 	unsigned int pc = preempt_count();  

Although I think we can do without that ^ line as well :-)

Quoted 2 lines

kernel/events/internal.h +0 -1

-	unsigned char rctx = 0;

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

While writing an email explaining the "bit = 0" logic for a discussion on making ftrace_test_recursion_trylock() disable preemption, I discovered a path that makes the "not do the logic if bit is zero" unsafe.

Since we want to encourage architectures to implement all ftrace features, having them slow down due to this extra logic may encourage the maintainers to update to the latest ftrace features. And because this logic is only safe for them, remove it completely.

[*] There is on layer of recursion that is allowed, and that is to allow

     for the transition between interrupt context (normal -> softirq ->
     irq -> NMI), because a trace may occur before the context update is
     visible to the trace recursion logic.

include/linux/trace_recursion.h +0 -0

index a9f9c5714e65..168fdf07419a 100644

include/linux/trace_recursion.h +1 -4

 		if (val & (1 << bit)) {
 			do_ftrace_record_recursion(ip, pip);
 			return -1;
 		}
-	} else {
-		/* Normal check passed, clear the transition to allow it again */
-		val &= ~(1 << TRACE_TRANSITION_BIT);
 	}
 
 	val |= 1 << bit;
 	current->trace_recursion = val;
 	barrier();
 
-	return bit + 1;
+	return bit;
 }

Quoted 24 lines · Steven Rostedt

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

While writing an email explaining the "bit = 0" logic for a discussion on making ftrace_test_recursion_trylock() disable preemption, I discovered a path that makes the "not do the logic if bit is zero" unsafe.

Since we want to encourage architectures to implement all ftrace features, having them slow down due to this extra logic may encourage the maintainers to update to the latest ftrace features. And because this logic is only safe for them, remove it completely.

[*] There is on layer of recursion that is allowed, and that is to allow

     for the transition between interrupt context (normal -> softirq ->
     irq -> NMI), because a trace may occur before the context update is
     visible to the trace recursion logic.

include/linux/trace_recursion.h +0 -0

index a9f9c5714e65..168fdf07419a 100644
@@ -165,40 +147,29 @@ static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign
 	unsigned int val = READ_ONCE(current->trace_recursion);
 	int bit;
 

include/linux/trace_recursion.h static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign · line 170

Quoted 3 lines

include/linux/trace_recursion.h +0 -3

-	/* A previous recursion check was made */
-	if ((val & TRACE_CONTEXT_MASK) > max)
-		return 0;  

@max parameter is no longer used.

include/linux/trace_recursion.h static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign · line 156

Quoted 10 lines

include/linux/trace_recursion.h +1 -2

-
 	bit = trace_get_context_bit() + start;
 	if (unlikely(val & (1 << bit))) {
 		/*
 		 * It could be that preempt_count has not been updated during
 		 * a switch between contexts. Allow for a single recursion.
 		 */
-		bit = TRACE_TRANSITION_BIT;
+		bit = TRACE_CTX_TRANSITION + start;  

I just want to be sure that I understand it correctly.

The transition bit is the same for all contexts. It will allow one recursion only in one context.

IMHO, the same problem (not-yet-updated preempt_count) might happen in any transition between contexts: normal -> soft IRQ -> IRQ -> NMI.

Well, I am not sure what exacly it means "preempt_count has not been updated during a switch between contexts."

Is it that a function in the interrupt entry code is traced before preempt_count is updated?

Or that an interrupt entry is interrupted by a higher level interrupt, e.g. IRQ entry code interrupted by NMI?

I guess that it is the first case. It would mean that the above function allows one mistake (one traced funtion in an interrupt entry code before the entry code updates preempt_count).

Do I get it correctly?
Is this what we want?

Could we please update the comment? I mean to say if it is a race or if we trace a function that should not get traced.

Quoted 18 lines

include/linux/trace_recursion.h +1 -4

 		if (val & (1 << bit)) {
 			do_ftrace_record_recursion(ip, pip);
 			return -1;
 		}
-	} else {
-		/* Normal check passed, clear the transition to allow it again */
-		val &= ~(1 << TRACE_TRANSITION_BIT);
 	}
 
 	val |= 1 << bit;
 	current->trace_recursion = val;
 	barrier();
 
-	return bit + 1;
+	return bit;
 }
 

include/linux/trace_recursion.h static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign · line 174

Quoted 9 lines

include/linux/trace_recursion.h +0 -4

 static __always_inline void trace_clear_recursion(int bit)
 {
-	if (!bit)
-		return;
-
 	barrier();
-	bit--;
 	trace_recursion_clear(bit);
 }  

Otherwise, the change looks great. It simplifies that logic a lot. I think that I start understanding it ;-)

Quoted 25 lines · On Mon, 18 Oct 2021 12:19:20 +0200 Petr Mladek <[email protected]> wrote:

Quoted 24 lines · Steven Rostedt

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

While writing an email explaining the "bit = 0" logic for a discussion on making ftrace_test_recursion_trylock() disable preemption, I discovered a path that makes the "not do the logic if bit is zero" unsafe.

Since we want to encourage architectures to implement all ftrace features, having them slow down due to this extra logic may encourage the maintainers to update to the latest ftrace features. And because this logic is only safe for them, remove it completely.

[*] There is on layer of recursion that is allowed, and that is to allow

     for the transition between interrupt context (normal -> softirq ->
     irq -> NMI), because a trace may occur before the context update is
     visible to the trace recursion logic.

include/linux/trace_recursion.h +0 -0

index a9f9c5714e65..168fdf07419a 100644
@@ -165,40 +147,29 @@ static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign
 	unsigned int val = READ_ONCE(current->trace_recursion);
 	int bit;
 

Quoted 5 lines

include/linux/trace_recursion.h static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign · line 170

Quoted 3 lines

include/linux/trace_recursion.h +0 -3

-	/* A previous recursion check was made */
-	if ((val & TRACE_CONTEXT_MASK) > max)
-		return 0;  

@max parameter is no longer used.

Thanks for noticing!

Quoted 13 lines

include/linux/trace_recursion.h static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign · line 156

Quoted 10 lines

include/linux/trace_recursion.h +1 -2

-
 	bit = trace_get_context_bit() + start;
 	if (unlikely(val & (1 << bit))) {
 		/*
 		 * It could be that preempt_count has not been updated during
 		 * a switch between contexts. Allow for a single recursion.
 		 */
-		bit = TRACE_TRANSITION_BIT;
+		bit = TRACE_CTX_TRANSITION + start;  

I just want to be sure that I understand it correctly.

Quoted 2 lines

The transition bit is the same for all contexts. It will allow one recursion only in one context.

Right.

Quoted 1 line

Quoted 2 lines

IMHO, the same problem (not-yet-updated preempt_count) might happen in any transition between contexts: normal -> soft IRQ -> IRQ -> NMI.

Yes, and then we will drop the event if it happens twice, otherwise, we will need to have a 4 layer transition bit mask, and allow 4 recursions, which is more than I want to allow.

Quoted 5 lines

Well, I am not sure what exacly it means "preempt_count has not been updated during a switch between contexts."

Quoted 5 lines

Is it that a function in the interrupt entry code is traced before preempt_count is updated?

Or that an interrupt entry is interrupted by a higher level interrupt, e.g. IRQ entry code interrupted by NMI?

Both actually ;-)

There are places that can trigger a trace between the time the interrupt is triggered, and the time the preempt_count updates the interrupt context it is in.

Thus the tracer will still think it is in the previous context. But that is OK, unless, that interrupt happened while the previous context was in the middle of tracing:

trace() {
context = get_context(preempt_count);
test_and_set_bit(context)

      <<--- interrupt --->>>
      trace() {
          context = get_context(preempt_count);
          test_and_set_bit(context); <-- detects recursion!
      }
      update_preempt_count(irq_context);

By allowing a single recursion, it still does the above trace.

Quoted 9 lines

(quoted diff) +1 -2

-
 	bit = trace_get_context_bit() + start;
 	if (unlikely(val & (1 << bit))) {
 		/*
 		 * It could be that preempt_count has not been updated during
 		 * a switch between contexts. Allow for a single recursion.
 		 */
-		bit = TRACE_TRANSITION_BIT;
+		bit = TRACE_CTX_TRANSITION + start;  

Could we please update the comment? I mean to say if it is a race or if we trace a function that should not get traced.

Quoted 10 lines · On Mon, 18 Oct 2021 12:19:20 +0200 Petr Mladek <[email protected]> wrote:

Quoted 9 lines

(quoted diff) +1 -2

-
 	bit = trace_get_context_bit() + start;
 	if (unlikely(val & (1 << bit))) {
 		/*
 		 * It could be that preempt_count has not been updated during
 		 * a switch between contexts. Allow for a single recursion.
 		 */
-		bit = TRACE_TRANSITION_BIT;
+		bit = TRACE_CTX_TRANSITION + start;  

[..]

Quoted 2 lines

Could we please update the comment? I mean to say if it is a race or if we trace a function that should not get traced.

What do you think of this change?

include/linux/trace_recursion.h +0 -1

index 1d8cce02c3fb..24f284eb55a7 100644
@@ -168,8 +168,12 @@ static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign
 	bit = trace_get_context_bit() + start;
 	if (unlikely(val & (1 << bit))) {
 		/*
-		 * It could be that preempt_count has not been updated during

Feel free to postpone this change. I do not want to complicate upstreaming the fix for stable. I am sorry if I already complicated it.