[RFC PATCH] ftrace: Fix lost recursion records in ftrace_record_recursion()

[RFC PATCH] ftrace: Fix lost recursion records in ftrace_record_recursion()

[Andrea Parri]

cached_function is set to the current ip before the cmpxchg() that
claims a slot in recursed_functions[]. When that cmpxchg() loses a
race for the same slot, the code bumps index and retries via "goto
again", but the retry immediately matches its own cached_function
write and returns without ever reaching the bumped-index cmpxchg().

Concretely, for two writers racing on the same index:

  CPU 0 (ip = A)                    CPU 1 (ip = B)
  --------------                    --------------
                                     cmpxchg(&recursed_functions[index].ip,
                                             0, B);      // succeeds
  cached_function = A;
  cmpxchg(&recursed_functions[index].ip,
          0, A);
    // fails, old == B
  index++;
  goto again;
  if (A == cached_function)         // true: matches A's own store
          return;                   // A is dropped, not retried

Once dropped this way, cached_function stays set to A, so every later
recursion of A also hits the fast path and returns before reaching the
retry, until some unrelated ip overwrites the cache.

Only set cached_function once the record is confirmed present, either
because a concurrent writer already added it or because this writer
just claimed the slot. The retry path leaves it untouched, so it no
longer matches against a value it just wrote for itself.

Fixes: 773c16705058e ("ftrace: Add recording of functions that caused recursion")
Assisted-by: LLM
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
Can't claim to fully understand the logic behind ftrace_record_recursion().
Notably, its smp_mb__after_atomic() calls and the lack of barrier comments
both look suspicious to my LKMM-trained eyes.  ;)  Hence the RFC.
---
 kernel/trace/trace_recursion_record.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace_recursion_record.c b/kernel/trace/trace_recursion_record.c
index bac4bc844ccd8..f42089c30f53c 100644
--- a/kernel/trace/trace_recursion_record.c
+++ b/kernel/trace/trace_recursion_record.c
@@ -17,8 +17,8 @@ static struct recursed_functions recursed_functions[CONFIG_FTRACE_RECORD_RECURSI
 static atomic_t nr_records;
 
 /*
- * Cache the last found function. Yes, updates to this is racey, but
- * so is memory cache ;-)
+ * Cache the last function confirmed present in recursed_functions[].
+ * Updates to this are racy, but this is only a best-effort cache.
  */
 static unsigned long cached_function;
 
@@ -67,24 +67,27 @@ void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip)
 		}
 	}
 
-	cached_function = ip;
-
 	/*
 	 * We only want to add a function if it hasn't been added before.
-	 * Add to the current location before incrementing the count.
-	 * If it fails to add, then increment the index (save in i)
-	 * and try again.
+	 * Claim the current slot before incrementing the count. If the slot
+	 * is occupied by another function, advance to the next slot and retry.
+	 *
+	 * Do not update cached_function until ip is known to be present;
+	 * otherwise the retry would match its own cache update.
 	 */
 	old = cmpxchg(&recursed_functions[index].ip, 0, ip);
 	if (old != 0) {
 		/* Did something else already added this for us? */
-		if (old == ip)
+		if (old == ip) {
+			cached_function = ip;
 			return;
+		}
 		/* Try the next location (use i for the next index) */
 		index++;
 		goto again;
 	}
 
+	cached_function = ip;
 	recursed_functions[index].parent_ip = parent_ip;
 
 	/*
-- 
2.53.0

Re: [RFC PATCH] ftrace: Fix lost recursion records in ftrace_record_recursion()

[Steven Rostedt]

On Thu, 17 Sep 2026 11:02:17 +0200
Andrea Parri <parri.andrea@gmail.com> wrote:

> cached_function is set to the current ip before the cmpxchg() that
> claims a slot in recursed_functions[]. When that cmpxchg() loses a
> race for the same slot, the code bumps index and retries via "goto
> again", but the retry immediately matches its own cached_function
> write and returns without ever reaching the bumped-index cmpxchg().
> 
> Concretely, for two writers racing on the same index:
> 
>   CPU 0 (ip = A)                    CPU 1 (ip = B)
>   --------------                    --------------
>                                      cmpxchg(&recursed_functions[index].ip,
>                                              0, B);      // succeeds
>   cached_function = A;
>   cmpxchg(&recursed_functions[index].ip,
>           0, A);
>     // fails, old == B
>   index++;
>   goto again;
>   if (A == cached_function)         // true: matches A's own store
>           return;                   // A is dropped, not retried
> 
> Once dropped this way, cached_function stays set to A, so every later
> recursion of A also hits the fast path and returns before reaching the
> retry, until some unrelated ip overwrites the cache.
> 
> Only set cached_function once the record is confirmed present, either
> because a concurrent writer already added it or because this writer
> just claimed the slot. The retry path leaves it untouched, so it no
> longer matches against a value it just wrote for itself.
> 

Good catch!

> Fixes: 773c16705058e ("ftrace: Add recording of functions that caused recursion")
> Assisted-by: LLM
> Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
> ---
> Can't claim to fully understand the logic behind ftrace_record_recursion().
> Notably, its smp_mb__after_atomic() calls and the lack of barrier comments
> both look suspicious to my LKMM-trained eyes.  ;)  Hence the RFC.

Well, the nr_records is updated via a cmpxchg() which itself is a full
memory barrier. The smp_mb__after_atomic() calls are to make sure the value
read is in sync with other CPUs before doing the compares. It also syncs
with the clearing function that is disabling and the writes, clears it, and
enables it (the -1 updates).

Otherwise, IIUC, the atomic_read() itself doesn't guarantee that what you
read is what is in memory without explicitly using the smp_mb__after_atomic().


> ---
>  kernel/trace/trace_recursion_record.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/trace/trace_recursion_record.c b/kernel/trace/trace_recursion_record.c
> index bac4bc844ccd8..f42089c30f53c 100644
> --- a/kernel/trace/trace_recursion_record.c
> +++ b/kernel/trace/trace_recursion_record.c
> @@ -17,8 +17,8 @@ static struct recursed_functions recursed_functions[CONFIG_FTRACE_RECORD_RECURSI
>  static atomic_t nr_records;
>  
>  /*
> - * Cache the last found function. Yes, updates to this is racey, but
> - * so is memory cache ;-)
> + * Cache the last function confirmed present in recursed_functions[].
> + * Updates to this are racy, but this is only a best-effort cache.
>   */
>  static unsigned long cached_function;
>  
> @@ -67,24 +67,27 @@ void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip)
>  		}
>  	}
>  
> -	cached_function = ip;
> -
>  	/*
>  	 * We only want to add a function if it hasn't been added before.
> -	 * Add to the current location before incrementing the count.
> -	 * If it fails to add, then increment the index (save in i)
> -	 * and try again.
> +	 * Claim the current slot before incrementing the count. If the slot
> +	 * is occupied by another function, advance to the next slot and retry.
> +	 *
> +	 * Do not update cached_function until ip is known to be present;
> +	 * otherwise the retry would match its own cache update.
>  	 */
>  	old = cmpxchg(&recursed_functions[index].ip, 0, ip);
>  	if (old != 0) {
>  		/* Did something else already added this for us? */
> -		if (old == ip)
> +		if (old == ip) {
> +			cached_function = ip;
>  			return;
> +		}

I think we do not need the above. If old == ip, then whatever set it before
should have updated the cached_function.

-- Steve


>  		/* Try the next location (use i for the next index) */
>  		index++;
>  		goto again;
>  	}
>  
> +	cached_function = ip;
>  	recursed_functions[index].parent_ip = parent_ip;
>  
>  	/*

Re: [RFC PATCH] ftrace: Fix lost recursion records in ftrace_record_recursion()

[Andrea Parri]

On Wed, Oct 14, 2026 at 05:28:21PM -0400, Steven Rostedt wrote:
> > Can't claim to fully understand the logic behind ftrace_record_recursion().
> > Notably, its smp_mb__after_atomic() calls and the lack of barrier comments
> > both look suspicious to my LKMM-trained eyes.  ;)  Hence the RFC.
>
> Well, the nr_records is updated via a cmpxchg() which itself is a full
> memory barrier. The smp_mb__after_atomic() calls are to make sure the value
> read is in sync with other CPUs before doing the compares. It also syncs
> with the clearing function that is disabling and the writes, clears it, and
> enables it (the -1 updates).
>
> Otherwise, IIUC, the atomic_read() itself doesn't guarantee that what you
> read is what is in memory without explicitly using the smp_mb__after_atomic().

Thanks for the explanation.  I don't think smp_mb__after_atomic() gives
us that, though.  Per Documentation/atomic_t.txt, the barriers
smp_mb__{before,after}_atomic() "only apply to the RMW atomic ops";
atomic_read() and atomic_set() are not RMW ops, so after them the
barrier is not guaranteed to order anything.  Most architectures
implement it as smp_mb(), which happens to cover these uses, but on x86
it is not even a compiler barrier, so the compiler is free to move the
memset() before atomic_set(-1); LoongArch still defines it as
barrier(), though a fix for that is pending [1].
And no barrier makes a load return a more recent value; barriers only
order accesses against each other.

What I think the code wants is: on the recording side, the load from
nr_records ordered before the loads from recursed_functions[], e.g.
atomic_read_acquire(); on the clearing side, the store of -1 ordered
before the memset(), for which smp_wmb() is enough.  (The memset()
itself still races with the recorders.  Marking its stores would make
that a well-defined race, but clearing would stay best-effort, as the
existing comment says, unless the clear waits for in-flight recorders,
e.g. with a grace period.)

This is independent of the cached_function bug, so I'll leave it out of
v1 and send it as a separate patch, with comments, if that's OK with you.

> > @@ -67,24 +67,27 @@ void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip)

[...]

> >  	old = cmpxchg(&recursed_functions[index].ip, 0, ip);
> >  	if (old != 0) {
> >  		/* Did something else already added this for us? */
> > -		if (old == ip)
> > +		if (old == ip) {
> > +			cached_function = ip;
> >  			return;
> > +		}
>
> I think we do not need the above. If old == ip, then whatever set it before
> should have updated the cached_function.

Makes sense, I'll drop it in v1.  (The cache only holds one entry, so
another ip could have replaced it in the meantime, but that just means a
later call takes the slow path.)

Thanks,
  Andrea

[1] https://lore.kernel.org/r/20260908073906.31781-1-yangtiezhu@loongson.cn