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

[Steven Rostedt <rostedt@goodmis.org>]

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