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 <[email protected]>
---
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
