| 0/0 |
2026/07/31 14:12 |
flow |
assessment-kcsan |
20m
Results: map[Benign:true Explanation:### Race Summary
* **Function A:** `rb_wakeups` (called via `ring_buffer_unlock_commit` from a
tracepoint triggered during a system call like `do_readlinkat`)
* **Function B:** `rb_wakeups` (called via `ring_buffer_unlock_commit` from a
concurrent event on another CPU)
* **Variable/Field:** `buffer->irq_work.waiters_pending` (or
`cpu_buffer->irq_work.waiters_pending`)
---
### Synchronization Context
The ring buffer's fast path (`ring_buffer_unlock_commit`) is designed to be
extremely fast and lockless. When committing an event, `rb_wakeups` is called to
check if there are any pending waiters that need to be woken up via `irq_work`.
Because events can be committed concurrently on different CPUs, multiple threads
can execute `rb_wakeups` concurrently without mutual exclusion.
---
### Final Classification: **BENIGN**
---
### Detailed Reasoning
The data race on `waiters_pending` is **completely benign** and structurally
tolerant to concurrent reads and writes due to the following reasons:
1. **No Torn Accesses:**
`waiters_pending` is a 1-byte `bool` variable. On all supported
architectures, 1-byte reads and writes are inherently atomic, meaning there
is no risk of reading a partially written or "torn" value.
2. **No Livelocks or Fused Accesses:**
`rb_wakeups` is a non-looping function called once per event commit. There
are no polling loops reading `waiters_pending`, so there is no risk of
compiler optimizations (such as hoisting a load out of a loop) causing a
thread to get stuck in an infinite loop.
3. **Redundant Wakeups are Safe:**
If CPU 0 and CPU 1 concurrently execute `rb_wakeups` and both read
`waiters_pending` as `true`, both will write `false` to it and call
`rb_irq_work_queue()`. The `irq_work` subsystem is designed to safely handle
concurrent and redundant queuing.
4. **No Lost Wakeups:**
If CPU 0 writes `false` and CPU 1 reads `false`, CPU 1 will skip queuing the
work. This is correct because CPU 0 has already queued the `irq_work` to wake
up the waiters.
5. **Explicit Design Intent:**
The kernel source code explicitly documents this design in `rb_wait_cond`:
```c
/*
* There's no reason to protect this flag either, as
* the work queue and irq_work logic will do the necessary
* synchronization for the wake ups. The only thing
* that is necessary is that the wake up happens after
* a task has been queued. It's OK for spurious wake ups.
*/
```
---
### Recommended Annotations
To document the race and silence KCSAN, the accesses to `waiters_pending` and
`full_waiters_pending` can be wrapped in `READ_ONCE()` / `WRITE_ONCE()` or
marked with `data_race()`.
For example, in `rb_wakeups`:
```c
if (data_race(buffer->irq_work.waiters_pending)) {
data_race(buffer->irq_work.waiters_pending = false);
rb_irq_work_queue(&buffer->irq_work);
}
```]
|
| 1/1 |
2026/07/31 14:12 |
action |
kernel-checkouter |
0m
Results: map[KernelSrc:/app/workdir/cache/src/d8c60cecff4c452a1f8b661b13d5b4d032f7121a]
|
| 2/1 |
2026/07/31 14:12 |
action |
kernel-builder |
0m
Results: map[KernelObj:/app/workdir/cache/build/b552e108765add6f8b31de83c90e1485b1c920c4]
|
| 3/1 |
2026/07/31 14:12 |
action |
codesearch-prepare |
0m
Results: map[Index:codesearch-index]
|
| 4/1 |
2026/07/31 14:12 |
agent |
expert |
20m
Model:
Results:
Instruction: # KCSAN Data Race Severity Analysis Protocol
You are an expert Linux kernel concurrency engineer analyzing a Kernel
Concurrency Sanitizer (KCSAN) data race report to determine if it is
**BENIGN** or **HARMFUL**.
## 1. CLASSIFICATIONS
### **BENIGN (Truly Benign)**
The logic is sound and structurally tolerant to compiler optimizations or
stale/torn reads.
- **Diagnostics/Stats:** Reads used only for `/proc`, `/sys`, counters, or
`pr_info`.
- **Heuristic Hints:** A "hint" flag where an old value only causes a
slightly delayed update or a sub-optimal but safe fast-path.
- **Single-Writer Flag Updates:** A single writer updating flags where the
concurrent read is a simple bitwise check (e.g., `flags & MASK`). These are
historically tolerated, assuming neither "Fused Accesses" nor "Ordering
Violations" are relevant in this context.
- **Marked Reloads:** A load feeding into a `cmpxchg()` loop or checked
against a later `READ_ONCE()` reload.
- **Safe Overwrites:** Writing the same value already present.
### **HARMFUL (Logic Bug or Marking Required)**
The race causes incorrect behavior due to a synchronization failure or
because missing annotations allow the compiler to break the algorithm.
**Marking Required for Correctness:**
The algorithm is logically sound but requires annotations (`READ_ONCE()`,
`WRITE_ONCE()`, `smp_load_acquire()`, `smp_store_release()`, etc.) to be safe.
- **Fused Accesses:** The compiler might merge accesses or hoist a load out
of a loop, breaking polling/wait loops (livelocks).
- **Torn Accesses:** A large access (e.g., 64-bit on 32-bit arch) might be
split into multiple non-atomic accesses. Note that `READ_ONCE()` does **not**
guarantee atomicity for 64-bit variables on 32-bit architectures.
- **Ordering Violations:** The race breaks a "happens-before" relationship
(requires primitives with implied or explicit memory barriers).
**Logic Bugs:**
A fundamental synchronization failure. Marking accesses will **not** fix it;
the logic itself must change.
- **Pointers/Lifecycle:** The racing variable is a pointer being dereferenced
or a refcount governing object lifecycle (Use-After-Free risk).
- **Control Flow:** The variable guards a critical section, memory allocation,
or hardware command.
- **Bitfields:** Concurrent writes to different bits in the same word.
Compilers often use non-atomic read-modify-write sequences, meaning a
write to `bit_A` can "clobber" a concurrent write to `bit_B`. However,
do not blindly assume all bitfield accesses are harmful; you must prove
that a concurrent write actually clobbers another in a way that breaks
logic.
- **Complex Structures:** Races on shared lists, trees, or hashmaps.
- **Lossy Updates:** Concurrent plain RMW operations (e.g., `var++`) on
non-diagnostic variables where every increment must be preserved.
- **State Machines:** Races allowing a state machine to bypass transitions
or enter an invalid state.
- **Adjacent Unsynchronized Operations:** Consider races happening at the
same time. For example, if both threads execute `struct->has_elements = true;
list_add(node, &struct->list);`, the race on `has_elements` implies an
adjacent race on `list_head`, which is HARMFUL.
## 2. RESEARCH & ANALYSIS WORKFLOW
1. **Locate the Race:** Find the exact variables and functions in the stack
traces. Do not speculate about hypothetical compiler
behaviors or theoretical dangers (e.g., dismissing something as
"fundamentally unsafe") without tracing the actual data flow to a crash.
2. **Contextualize:** Identify held locks, RCU sections, or interrupt
contexts. Explain why the race is possible (e.g., "Thread A holds `lock`,
but Thread B is a lockless reader").
3. **Data Flow:** Follow the racing variable into its subsequent uses. If
the reader sees a stale or torn value, what branch is taken? Does it lead
to a pointer dereference or `BUG_ON()`? **You must prove harm via this
data flow.**
4. **Failure Sequencing (if HARMFUL):** Construct a two-column execution trace
demonstrating the failure.
*Example format:*
```text
CPU0 CPU1
function_a()
// lockless read
ptr = global_ptr
<PREEMPT>
function_b()
lock(&my_lock)
global_ptr = NULL
unlock(&my_lock)
kfree(ptr)
if (ptr)
*ptr = 1; // -> Use-After-Free!
```
## 3. OUTPUT FORMAT
- **Race Summary:** `[Function A]` vs `[Function B]` on `[Variable/Field]`.
- **Synchronization context:** List held locks and explain the lack of mutual
exclusion.
- **Final Classification:** **BENIGN** or **HARMFUL**.
**If BENIGN:**
- **Reasoning:** Briefly explain why the race is structurally tolerant to stale
or torn reads.
- **Recommended Annotations:** Suggest appropriate annotations (e.g.,
`READ_ONCE()`, `data_race()`).
**If HARMFUL:**
- **Failure Reasoning:** Explain and provide a two-column interleaving showing
exactly how the race leads to a failure such as corruption or kernel crash.
- **Recommended Fix (ONLY if fix is trivial):** Suggest a structural fix (e.g.,
"Hold `mapping->i_pages` lock", "Convert to `atomic_t`") or required memory
ordering annotations (e.g., "Wrap in `READ_ONCE()`", "Use
`smp_load_acquire()`").
Don't make assumptions about the kernel source code (it may be different from what you assume it is).
Extensively use the provided code access tools (codesearch-*, git-*, grepper, etc)
to examine the actual source code, and confirm any assumptions.
Prefer calling several tools at the same time to save round-trips.
Use set-results tool to provide results of the analysis.
It must be called exactly once before the final reply.
Ignore results of this tool.
Prompt:
The data race report is:
==================================================================
BUG: KCSAN: data-race in do_readlinkat / ring_buffer_unlock_commit
read to 0xffff888100074cb4 of 1 bytes by task 14450 on cpu 1:
write to 0xffff888100074cb4 of 1 bytes by task 14466 on cpu 0:
rb_wakeups kernel/trace/ring_buffer.c:4294 [inline]
ring_buffer_unlock_commit+0x6a/0x440 kernel/trace/ring_buffer.c:4494
__buffer_unlock_commit kernel/trace/trace.h:1697 [inline]
trace_buffer_unlock_commit_regs+0x43/0x310 kernel/trace/trace.c:2049
trace_event_buffer_commit+0x149/0x590 kernel/trace/trace.c:2024
do_trace_event_raw_event_x86_fpu arch/x86/include/asm/trace/fpu.h:10 [inline]
trace_event_raw_event_x86_fpu+0x165/0x200 arch/x86/include/asm/trace/fpu.h:10
__do_trace_x86_fpu_regs_activated arch/x86/include/asm/trace/fpu.h:47 [inline]
trace_x86_fpu_regs_activated arch/x86/include/asm/trace/fpu.h:47 [inline]
fpregs_activate arch/x86/kernel/fpu/context.h:50 [inline]
fpregs_mark_activate+0xff/0x120 arch/x86/kernel/fpu/core.c:924
restore_fpregs_from_user arch/x86/kernel/fpu/signal.c:322 [inline]
__fpu_restore_sig arch/x86/kernel/fpu/signal.c:351 [inline]
fpu__restore_sig+0x5a0/0xad0 arch/x86/kernel/fpu/signal.c:485
restore_sigcontext arch/x86/kernel/signal_64.c:95 [inline]
__ia32_sys_rt_sigreturn+0x29b/0x350 arch/x86/kernel/signal_64.c:268
x64_sys_call+0x2823/0x3020 arch/x86/include/generated/asm/syscalls_64.h:16
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x136/0x3c0 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
value changed: 0x01 -> 0x00
Reported by Kernel Concurrency Sanitizer on:
CPU: 0 UID: 0 PID: 14466 Comm: syz.5.4072 Tainted: G W syzkaller #0 PREEMPT(lazy)
Tainted: [W]=WARN
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/24/2026
==================================================================
Reply: ### Race Summary
* **Function A:** `rb_wakeups` (called via `ring_buffer_unlock_commit` from a tracepoint triggered during a system call like `do_readlinkat`)
* **Function B:** `rb_wakeups` (called via `ring_buffer_unlock_commit` from a concurrent event on another CPU)
* **Variable/Field:** `buffer->irq_work.waiters_pending` (or `cpu_buffer->irq_work.waiters_pending`)
---
### Synchronization Context
The ring buffer's fast path (`ring_buffer_unlock_commit`) is designed to be extremely fast and lockless. When committing an event, `rb_wakeups` is called to check if there are any pending waiters that need to be woken up via `irq_work`. Because events can be committed concurrently on different CPUs, multiple threads can execute `rb_wakeups` concurrently without mutual exclusion.
---
### Final Classification: **BENIGN**
---
### Detailed Reasoning
The data race on `waiters_pending` is **completely benign** and structurally tolerant to concurrent reads and writes due to the following reasons:
1. **No Torn Accesses:**
`waiters_pending` is a 1-byte `bool` variable. On all supported architectures, 1-byte reads and writes are inherently atomic, meaning there is no risk of reading a partially written or "torn" value.
2. **No Livelocks or Fused Accesses:**
`rb_wakeups` is a non-looping function called once per event commit. There are no polling loops reading `waiters_pending`, so there is no risk of compiler optimizations (such as hoisting a load out of a loop) causing a thread to get stuck in an infinite loop.
3. **Redundant Wakeups are Safe:**
If CPU 0 and CPU 1 concurrently execute `rb_wakeups` and both read `waiters_pending` as `true`, both will write `false` to it and call `rb_irq_work_queue()`. The `irq_work` subsystem is designed to safely handle concurrent and redundant queuing.
4. **No Lost Wakeups:**
If CPU 0 writes `false` and CPU 1 reads `false`, CPU 1 will skip queuing the work. This is correct because CPU 0 has already queued the `irq_work` to wake up the waiters.
5. **Explicit Design Intent:**
The kernel source code explicitly documents this design in `rb_wait_cond`:
```c
/*
* There's no reason to protect this flag either, as
* the work queue and irq_work logic will do the necessary
* synchronization for the wake ups. The only thing
* that is necessary is that the wake up happens after
* a task has been queued. It's OK for spurious wake ups.
*/
```
---
### Recommended Annotations
To document the race and silence KCSAN, the accesses to `waiters_pending` and `full_waiters_pending` can be wrapped in `READ_ONCE()` / `WRITE_ONCE()` or marked with `data_race()`.
For example, in `rb_wakeups`:
```c
if (data_race(buffer->irq_work.waiters_pending)) {
data_race(buffer->irq_work.waiters_pending = false);
rb_irq_work_queue(&buffer->irq_work);
}
```
|
| 5/2 |
2026/07/31 14:12 |
llm |
expert |
0m
Model:
Tokens:
input: 6002
output: 37
thoughts: 211
Thoughts: **Analyzing a KCSAN Data Race Report**
Okay, so the KCSAN report highlights a data race between `do_readlinkat` and `ring_buffer_unlock_commit`. The core of the issue is that a read operation is occurring within `do_readlinkat`. The report itself doesn't provide a stack trace for the read itself, or maybe it's just `do_readlinkat` on the trace. That is a bit annoying.
Hmm, let's see, it says: `read to 0xffff888100074cb4 of 1 bytes by task 14450 on cpu 1:`. A raw memory address; this reinforces my suspicion that the stack trace is truncated or nonexistent for the specific read. To understand the context better, I need to dig deeper into how these functions interact. I'll need to see how data is shared or synchronized.
My next step is to search the kernel source code for relationships between `do_readlinkat`, `ring_buffer_unlock_commit`, and the `rb_wakeups` variable, which I suspect is a key synchronization point. I'll need to figure out what data is being shared. I think the fastest way to do that is to use something like `codesearch-definition-source` or `grepper` and look for `rb_wakeups` in `kernel/trace/ring_buffer.c`. That should give me the necessary insights to understand the read operation.
|
| 6/2 |
2026/07/31 14:12 |
tool |
codesearch-definition-source |
0m
Args: map[ContextFile:kernel/trace/ring_buffer.c Name:rb_wakeups]
Results: map[SourceCode:4290: static __always_inline void
4291: rb_wakeups(struct trace_buffer *buffer, struct ring_buffer_per_cpu *cpu_buffer)
4292: {
4293: if (buffer->irq_work.waiters_pending) {
4294: buffer->irq_work.waiters_pending = false;
4295: /* irq_work_queue() supplies it's own memory barriers */
4296: rb_irq_work_queue(&buffer->irq_work);
4297: }
4298:
4299: if (cpu_buffer->irq_work.waiters_pending) {
4300: cpu_buffer->irq_work.waiters_pending = false;
4301: /* irq_work_queue() supplies it's own memory barriers */
4302: rb_irq_work_queue(&cpu_buffer->irq_work);
4303: }
4304:
4305: if (cpu_buffer->last_pages_touch == local_read(&cpu_buffer->pages_touched))
4306: return;
4307:
4308: if (cpu_buffer->reader_page == cpu_buffer->commit_page)
4309: return;
4310:
4311: if (!cpu_buffer->irq_work.full_waiters_pending)
4312: return;
4313:
4314: cpu_buffer->last_pages_touch = local_read(&cpu_buffer->pages_touched);
4315:
4316: if (!full_hit(buffer, cpu_buffer->cpu, cpu_buffer->shortest_full))
4317: return;
4318:
4319: cpu_buffer->irq_work.wakeup_full = true;
4320: cpu_buffer->irq_work.full_waiters_pending = false;
4321: /* irq_work_queue() supplies it's own memory barriers */
4322: rb_irq_work_queue(&cpu_buffer->irq_work);
4323: }
SourceFile:kernel/trace/ring_buffer.c]
|
| 7/2 |
2026/07/31 14:12 |
llm |
expert |
0m
Model:
Tokens:
input: 7184
output: 18
thoughts: 186
Thoughts: **Investigating a Suspect Read**
Okay, so I'm looking at this write operation: it's setting `buffer->irq_work.waiters_pending = false;` (line 4294) or maybe `cpu_buffer->irq_work.waiters_pending = false;` (line 4300). The specific memory address of this write is `0xffff888100074cb4`. That's the key. Now, the interesting bit is the read that's associated with it. Why am I seeing this read in `do_readlinkat`? That seems completely unrelated. Why would reading a symbolic link involve checking if some "waiters" are pending on interrupt work? It doesn't make sense immediately.
I need to dig deeper. Is `do_readlinkat` *directly* reading `waiters_pending`, which is unlikely? Or, more probably, is `do_readlinkat` calling some other function that *itself* reads this flag? Or, is it perhaps a tracepoint being triggered inside `do_readlinkat` that's the culprit? I have to consider all the possibilities.
My next step is to search the codebase for `waiters_pending`. I'll start with `kernel/trace/ring_buffer.c`, since that's where my initial observation seems to be pointing, but I'll expand the search to other relevant files as needed. I need to understand the connection between `do_readlinkat` and this potentially unexpected read. This requires careful tracing and analysis.
|
| 8/2 |
2026/07/31 14:13 |
tool |
grepper |
0m
Args: map[Expression:waiters_pending]
Results: map[Output:kernel/futex/futex.h=360=static inline void futex_hb_waiters_dec(struct futex_hash_bucket *hb)
--
kernel/futex/futex.h-366-
kernel/futex/futex.h:367:static inline int futex_hb_waiters_pending(struct futex_hash_bucket *hb)
kernel/futex/futex.h-368-{
--
kernel/futex/waitwake.c-83- * atomic operations (see futex_hb_waiters_inc) and where (B) orders the write
kernel/futex/waitwake.c:84: * to futex and the waiters read (see futex_hb_waiters_pending()).
kernel/futex/waitwake.c-85- *
--
kernel/futex/waitwake.c=178=int futex_wake(u32 __user *uaddr, unsigned int flags, void __user *pop, int nr_wake, u32 bitset)
--
kernel/futex/waitwake.c-201- /* Make sure we really have tasks to wakeup */
kernel/futex/waitwake.c:202: if (!futex_hb_waiters_pending(hb))
kernel/futex/waitwake.c-203- return ret;
--
kernel/trace/ring_buffer.c=437=struct rb_irq_work {
--
kernel/trace/ring_buffer.c-441- atomic_t seq;
kernel/trace/ring_buffer.c:442: bool waiters_pending;
kernel/trace/ring_buffer.c:443: bool full_waiters_pending;
kernel/trace/ring_buffer.c-444- bool wakeup_full;
--
kernel/trace/ring_buffer.c=816=static void rb_wake_up_waiters(struct irq_work *work)
--
kernel/trace/ring_buffer.c-823- wake_up_all(&rbwork->waiters);
kernel/trace/ring_buffer.c:824: if (rbwork->full_waiters_pending || rbwork->wakeup_full) {
kernel/trace/ring_buffer.c-825- /* Only cpu_buffer sets the above flags */
--
kernel/trace/ring_buffer.c-831- rbwork->wakeup_full = false;
kernel/trace/ring_buffer.c:832: rbwork->full_waiters_pending = false;
kernel/trace/ring_buffer.c-833-
--
kernel/trace/ring_buffer.c=914=rb_wait_cond(struct rb_irq_work *rbwork, struct trace_buffer *buffer,
--
kernel/trace/ring_buffer.c-943- if (full)
kernel/trace/ring_buffer.c:944: rbwork->full_waiters_pending = true;
kernel/trace/ring_buffer.c-945- else
kernel/trace/ring_buffer.c:946: rbwork->waiters_pending = true;
kernel/trace/ring_buffer.c-947-
--
kernel/trace/ring_buffer.c=1039=__poll_t ring_buffer_poll_wait(struct trace_buffer *buffer, int cpu,
--
kernel/trace/ring_buffer.c-1061- /*
kernel/trace/ring_buffer.c:1062: * Only allow full_waiters_pending update to be seen after
kernel/trace/ring_buffer.c-1063- * the shortest_full is set (in rb_watermark_hit). If the
kernel/trace/ring_buffer.c:1064: * writer sees the full_waiters_pending flag set, it will
kernel/trace/ring_buffer.c-1065- * compare the amount in the ring buffer to shortest_full.
--
kernel/trace/ring_buffer.c-1070- * the below smp_mb() makes sure that the update to
kernel/trace/ring_buffer.c:1071: * full_waiters_pending doesn't leak up into the above.
kernel/trace/ring_buffer.c-1072- */
kernel/trace/ring_buffer.c-1073- smp_mb();
kernel/trace/ring_buffer.c:1074: rbwork->full_waiters_pending = true;
kernel/trace/ring_buffer.c-1075- return 0;
--
kernel/trace/ring_buffer.c-1078- poll_wait(filp, &rbwork->waiters, poll_table);
kernel/trace/ring_buffer.c:1079: rbwork->waiters_pending = true;
kernel/trace/ring_buffer.c-1080-
kernel/trace/ring_buffer.c-1081- /*
kernel/trace/ring_buffer.c:1082: * There's a tight race between setting the waiters_pending and
kernel/trace/ring_buffer.c:1083: * checking if the ring buffer is empty. Once the waiters_pending bit
kernel/trace/ring_buffer.c-1084- * is set, the next event will wake the task up, but we can get stuck
--
kernel/trace/ring_buffer.c=4291=rb_wakeups(struct trace_buffer *buffer, struct ring_buffer_per_cpu *cpu_buffer)
kernel/trace/ring_buffer.c-4292-{
kernel/trace/ring_buffer.c:4293: if (buffer->irq_work.waiters_pending) {
kernel/trace/ring_buffer.c:4294: buffer->irq_work.waiters_pending = false;
kernel/trace/ring_buffer.c-4295- /* irq_work_queue() supplies it's own memory barriers */
--
kernel/trace/ring_buffer.c-4298-
kernel/trace/ring_buffer.c:4299: if (cpu_buffer->irq_work.waiters_pending) {
kernel/trace/ring_buffer.c:4300: cpu_buffer->irq_work.waiters_pending = false;
kernel/trace/ring_buffer.c-4301- /* irq_work_queue() supplies it's own memory barriers */
--
kernel/trace/ring_buffer.c-4310-
kernel/trace/ring_buffer.c:4311: if (!cpu_buffer->irq_work.full_waiters_pending)
kernel/trace/ring_buffer.c-4312- return;
--
kernel/trace/ring_buffer.c-4319- cpu_buffer->irq_work.wakeup_full = true;
kernel/trace/ring_buffer.c:4320: cpu_buffer->irq_work.full_waiters_pending = false;
kernel/trace/ring_buffer.c-4321- /* irq_work_queue() supplies it's own memory barriers */
]
|
| 9/2 |
2026/07/31 14:13 |
llm |
expert |
0m
Model:
Tokens:
input: 9523
output: 23
thoughts: 253
Thoughts: **Analyzing a Kernel KCSAN Report**
Okay, let's dive into this KCSAN report. The initial focus is on `do_readlinkat`, so I need to understand why it's flagged. My first thought is to check if it's directly reading `waiters_pending`. That's a good starting point, but wait... why is `do_readlinkat` the culprit in the report? Maybe it calls a tracepoint, or perhaps the report is simply attributing the read to the *task name* or the function where it occurred?
The report itself shows a read of one byte to `0xffff888100074cb4` by task `14450` on CPU `1`. A critical issue is that there's no stack trace for the read! This is unusual. I need to consider why. It's possible that KCSAN sometimes omits stack traces for simple reads, when the stack wasn't captured or perhaps in this case it really *is* `do_readlinkat`.
Alright, `do_readlinkat` is a system call, which means it likely performs a significant amount of work. If the read is happening within `do_readlinkat`, I need to scrutinize what it actually does. The next step is clear: I need to search the kernel source for `do_readlinkat` and trace its operations.
|
| 10/2 |
2026/07/31 14:14 |
tool |
codesearch-find-references |
0m
Args:
Results: map[References:[map[ReferenceKind:calls ReferencingEntityKind:function ReferencingEntityName:__do_sys_readlink SourceFile:fs/stat.c SourceLine:605] map[ReferenceKind:calls ReferencingEntityKind:function ReferencingEntityName:__do_sys_readlinkat SourceFile:fs/stat.c SourceLine:599]] TruncatedOutput:false]
|
| 11/2 |
2026/07/31 14:14 |
llm |
expert |
0m
Model:
Tokens:
input: 10219
output: 33
thoughts: 29
Thoughts: Okay, let's dive into this. Here's my thought process on the matter:
**Dissecting `do_readlinkat` in `fs/stat.c`**
Alright, I need to get a handle on what `do_readlinkat` is actually *doing*. My usual approach is to go straight to the source, so let's navigate to the `fs/stat.c` file. Given what I know about the kernel's file system implementation, a likely area of interest would be around line 599. That's a reasonable starting point to begin to understand how the system handles symbolic links, especially since we are using `readlinkat`. I need to understand the control flow, any potential error conditions that are being handled, and ultimately, what actions are taken to resolve this system call. I will probably look at the function signature, the arguments being passed, and any calls to other kernel functions within that function.
|
| 12/2 |
2026/07/31 14:14 |
tool |
read-file |
0m
Args: map[File:fs/stat.c FirstLine:580 LineCount:40]
Results: map[Contents: 580: error = security_inode_readlink(path.dentry);
581: if (!error) {
582: touch_atime(&path);
583: error = vfs_readlink(path.dentry, buf, bufsiz);
584: }
585: } else {
586: error = (name->name[0] == '\0') ? -ENOENT : -EINVAL;
587: }
588: path_put(&path);
589: if (retry_estale(error, lookup_flags)) {
590: lookup_flags |= LOOKUP_REVAL;
591: goto retry;
592: }
593: return error;
594: }
595:
596: SYSCALL_DEFINE4(readlinkat, int, dfd, const char __user *, pathname,
597: char __user *, buf, int, bufsiz)
598: {
599: return do_readlinkat(dfd, pathname, buf, bufsiz);
600: }
601:
602: SYSCALL_DEFINE3(readlink, const char __user *, path, char __user *, buf,
603: int, bufsiz)
604: {
605: return do_readlinkat(AT_FDCWD, path, buf, bufsiz);
606: }
607:
608:
609: /* ---------- LFS-64 ----------- */
610: #if defined(__ARCH_WANT_STAT64) || defined(__ARCH_WANT_COMPAT_STAT64)
611:
612: #ifndef INIT_STRUCT_STAT64_PADDING
613: # define INIT_STRUCT_STAT64_PADDING(st) memset(&st, 0, sizeof(st))
614: #endif
615:
616: static long cp_new_stat64(struct kstat *stat, struct stat64 __user *statbuf)
617: {
618: struct stat64 tmp;
619:
]
|
| 13/2 |
2026/07/31 14:14 |
llm |
expert |
1m
Model:
Tokens:
input: 11101
output: 23
thoughts: 1101
Thoughts: **Analyzing the Concurrency Issue in `waiters_pending`**
Okay, so I'm looking at this trace and trying to figure out where this read of `waiters_pending` is happening. Initially, I thought `do_readlinkat` itself was directly reading `waiters_pending`, but that doesn't seem right. It doesn't appear in the function's code.
My first instinct was to check for tracepoints, and perhaps the system call entry/exit, or maybe the name in the read message is misleading. Then I took a closer look at the read itself, specifically the memory address and size. The lack of a stack trace initially threw me. Could this read be happening in `ring_buffer_poll_wait`, `rb_wait_cond`, or `rb_wakeups`?
I reviewed the `rb_wakeups` code: it *reads* and then *writes* `buffer->irq_work.waiters_pending`. If a concurrent call to `rb_wakeups` or something similar happened, it could explain the read.
Then I went back to the report. Wait, why does it mention `do_readlinkat`? That's when it hit me. Task 14450 was likely executing `do_readlinkat`, which triggered a system call tracepoint, such as `sys_enter_readlinkat` or `sys_exit_readlinkat`. The tracepoint writes to the ring buffer, which eventually calls `ring_buffer_unlock_commit`. In `ring_buffer_unlock_commit` is `rb_wakeups`, which in turn reads `buffer->irq_work.waiters_pending`. That makes perfect sense!
The read and write both originate from the `rb_wakeups` functions (or related ring buffer functions), the compiler might be inlining or KCSAN might only be showing the top-level function, or maybe the read stack trace was truncated/omitted.
Now, to be thorough, I need to check for other places where `waiters_pending` is read or written. Reviewing `kernel/trace/ring_buffer.c`, I identified three more locations: `rb_wait_cond`, and `ring_buffer_poll_wait`. I will investigate how `rb_wait_cond` is called, specifically checking the references to `rb_wait_cond`.
|
| 14/2 |
2026/07/31 14:16 |
tool |
codesearch-find-references |
0m
Args:
Results: map[References:[map[ReferenceKind:calls ReferencingEntityKind:function ReferencingEntityName:ring_buffer_wait SourceFile:kernel/trace/ring_buffer.c SourceLine:1018] map[ReferenceKind:calls ReferencingEntityKind:function ReferencingEntityName:ring_buffer_wait SourceFile:kernel/trace/ring_buffer.c SourceLine:1018] map[ReferenceKind:calls ReferencingEntityKind:function ReferencingEntityName:ring_buffer_wait SourceFile:kernel/trace/ring_buffer.c SourceLine:1018]] TruncatedOutput:false]
|
| 15/2 |
2026/07/31 14:16 |
llm |
expert |
0m
Model:
Tokens:
input: 12911
output: 38
thoughts: 23
Thoughts: Here's my take on the `ring_buffer_wait` function, thinking through the code:
**Diving into Ring Buffer Waiting**
Okay, time to get my hands dirty and dissect `ring_buffer_wait` within `kernel/trace/ring_buffer.c`. I need to understand exactly how the ring buffer implementation handles waiting for data to become available. This is crucial for performance.
First, I expect to see some kind of spinlock or mutex protecting the data structure. It's almost guaranteed in any multithreaded environment. The function's name implies waiting, so Iβm looking for synchronization primitives. I'll need to see how threads are blocked when the buffer is empty, and unblocked when new data arrives. I'm guessing that some kind of wait queue, perhaps based on a completion mechanism, will be utilized. Thatβs probably the most efficient approach, allowing threads to sleep instead of busy-waiting.
My instinct says that I will find logic to handle both the producer and consumer sides of the buffer. The producer adds data, and the consumer reads it. The wait mechanism must coordinate these two. The wait condition might check flags indicating that new data is available or the buffer is becoming full. I want to see if there are any timeout mechanisms, in case the producer thread is stalled or the system is under extreme load. And I'll be really interested to see how it manages the "wake-up" signal. Is it using a broadcast or a specific "wake one" approach? The difference can greatly affect concurrency and performance in certain scenarios. Finally, I'll be looking for any potential for deadlocks or priority inversion, which is always a concern when dealing with concurrent data structures.
|
| 16/2 |
2026/07/31 14:16 |
tool |
codesearch-definition-source |
0m
Args: map[ContextFile:kernel/trace/ring_buffer.c Name:ring_buffer_wait]
Results: map[SourceCode: 968: /**
969: * ring_buffer_wait - wait for input to the ring buffer
970: * @buffer: buffer to wait on
971: * @cpu: the cpu buffer to wait on
972: * @full: wait until the percentage of pages are available, if @cpu != RING_BUFFER_ALL_CPUS
973: * @cond: condition function to break out of wait (NULL to run once)
974: * @data: the data to pass to @cond.
975: *
976: * If @cpu == RING_BUFFER_ALL_CPUS then the task will wake up as soon
977: * as data is added to any of the @buffer's cpu buffers. Otherwise
978: * it will wait for data to be added to a specific cpu buffer.
979: */
980: int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full,
981: ring_buffer_cond_fn cond, void *data)
982: {
983: struct ring_buffer_per_cpu *cpu_buffer;
984: struct wait_queue_head *waitq;
985: struct rb_irq_work *rbwork;
986: struct rb_wait_data rdata;
987: int ret = 0;
988:
989: /*
990: * Depending on what the caller is waiting for, either any
991: * data in any cpu buffer, or a specific buffer, put the
992: * caller on the appropriate wait queue.
993: */
994: if (cpu == RING_BUFFER_ALL_CPUS) {
995: rbwork = &buffer->irq_work;
996: /* Full only makes sense on per cpu reads */
997: full = 0;
998: } else {
999: if (!cpumask_test_cpu(cpu, buffer->cpumask))
1000: return -ENODEV;
1001: cpu_buffer = buffer->buffers[cpu];
1002: rbwork = &cpu_buffer->irq_work;
1003: }
1004:
1005: if (full)
1006: waitq = &rbwork->full_waiters;
1007: else
1008: waitq = &rbwork->waiters;
1009:
1010: /* Set up to exit loop as soon as it is woken */
1011: if (!cond) {
1012: cond = rb_wait_once;
1013: rdata.irq_work = rbwork;
1014: rdata.seq = atomic_read_acquire(&rbwork->seq);
1015: data = &rdata;
1016: }
1017:
1018: ret = wait_event_interruptible((*waitq),
1019: rb_wait_cond(rbwork, buffer, cpu, full, cond, data));
1020:
1021: return ret;
1022: }
SourceFile:kernel/trace/ring_buffer.c]
|
| 17/2 |
2026/07/31 14:16 |
llm |
expert |
0m
Model:
Tokens:
input: 14223
output: 38
thoughts: 14
Thoughts: Okay, let's dive into this. Here's my take on those thoughts, as if they're my own:
**Analyzing the Code: `rb_wait_cond`**
Alright, I'm thinking, "Let's dig into the `rb_wait_cond` function too." It's not enough to understand the overall architecture; I need to see the details. I know this is a critical section of code related to synchronization in Ruby, likely dealing with condition variables and thread waiting. I need to understand how threads are suspended, how the wait is managed, how the condition is checked, and what happens when the wait is signaled. I expect to see interactions with the underlying operating system's synchronization primitives, like mutexes and condition variables, and I need to parse the subtleties of the Ruby implementation to grasp any potential optimizations or, more importantly, any subtle bugs that might exist in this area. It all comes down to understanding the flow of execution and the states of threads at each point. This is going to be good.
|
| 18/2 |
2026/07/31 14:17 |
tool |
codesearch-definition-source |
0m
Args: map[ContextFile:kernel/trace/ring_buffer.c Name:rb_wait_cond]
Results: map[SourceCode: 913: static inline bool
914: rb_wait_cond(struct rb_irq_work *rbwork, struct trace_buffer *buffer,
915: int cpu, int full, ring_buffer_cond_fn cond, void *data)
916: {
917: if (rb_watermark_hit(buffer, cpu, full))
918: return true;
919:
920: if (cond(data))
921: return true;
922:
923: /*
924: * The events can happen in critical sections where
925: * checking a work queue can cause deadlocks.
926: * After adding a task to the queue, this flag is set
927: * only to notify events to try to wake up the queue
928: * using irq_work.
929: *
930: * We don't clear it even if the buffer is no longer
931: * empty. The flag only causes the next event to run
932: * irq_work to do the work queue wake up. The worse
933: * that can happen if we race with !trace_empty() is that
934: * an event will cause an irq_work to try to wake up
935: * an empty queue.
936: *
937: * There's no reason to protect this flag either, as
938: * the work queue and irq_work logic will do the necessary
939: * synchronization for the wake ups. The only thing
940: * that is necessary is that the wake up happens after
941: * a task has been queued. It's OK for spurious wake ups.
942: */
943: if (full)
944: rbwork->full_waiters_pending = true;
945: else
946: rbwork->waiters_pending = true;
947:
948: return false;
949: }
SourceFile:kernel/trace/ring_buffer.c]
|
| 19/2 |
2026/07/31 14:17 |
llm |
expert |
0m
Model:
Tokens:
input: 15084
output: 40
thoughts: 1233
Thoughts: **Analyzing a Potential Data Race in `rb_wakeups`**
Okay, so I'm digging into this `rb_wait_cond` function and immediately, the comment jumps out at me. It basically says that this flag, `waiters_pending`, doesn't need explicit protection because the work queue and `irq_work` logic handle synchronization. Spurious wake-ups are acceptable, which, in my experience, is a common optimization, but I need to be sure.
So, let's look for the race condition, and I see there's a race between reading and writing `buffer->irq_work.waiters_pending` (or `cpu_buffer->irq_work.waiters_pending`) in `rb_wakeups`. `rb_wakeups` is called by `ring_buffer_unlock_commit`. I see task 14466 on CPU 0 is calling `ring_buffer_unlock_commit`. Simultaneously, task 14450 on CPU 1 is reading that `waiters_pending` value, so that's where to focus. Now, where is task 14450 reading this? Ah! It is calling `do_readlinkat`, so if that triggers a tracepoint, it also calls `ring_buffer_unlock_commit`! Perfect, both tasks are calling `ring_buffer_unlock_commit` concurrently, so both are also executing `rb_wakeups` concurrently.
Now, let's verify if that concurrent call to `rb_wakeups` is even possible. Yes, because `ring_buffer_unlock_commit` can be called concurrently on different CPUs when events are committed on different CPUs. So, we've got CPU 0 and CPU 1 executing `rb_wakeups` at the same time.
Within `rb_wakeups`:
```c
if (buffer->irq_work.waiters_pending) {
buffer->irq_work.waiters_pending = false;
/* irq_work_queue() supplies it's own memory barriers */
rb_irq_work_queue(&buffer->irq_work);
}
```
Wait, `buffer->irq_work` is shared! That's the global buffer, so the race is: if both CPUs commit at the same time, they'll both call `rb_wakeups(buffer, cpu_buffer)`. Both will check `buffer->irq_work.waiters_pending`. If it's true, they both might see it as true, and both try to set it to false and call `rb_irq_work_queue(&buffer->irq_work)`. So, is this a data race? Definitely, one is writing and the other is either reading or writing.
Is the race harmful? If both CPUs read `waiters_pending` as true, both set it to false, and both call `rb_irq_work_queue`. That function is safe to call concurrently, since it is atomic. If one reads false and the other writes false, then the second CPU won't call `rb_irq_work_queue`, but... is it a problem? Is there a case where `waiters_pending` gets set to true, but because of this race, the write gets lost? Let's check how `waiters_pending` gets set to true. It can be set to true in `rb_wait_cond` (called from `wait_event_interruptible`) or `ring_buffer_poll_wait`. OK, I'll need to go back and look at `ring_buffer_poll_wait` again.
|
| 20/2 |
2026/07/31 14:18 |
tool |
codesearch-definition-source |
0m
Args: map[ContextFile:kernel/trace/ring_buffer.c Name:ring_buffer_poll_wait]
Results: map[SourceCode:1024: /**
1025: * ring_buffer_poll_wait - poll on buffer input
1026: * @buffer: buffer to wait on
1027: * @cpu: the cpu buffer to wait on
1028: * @filp: the file descriptor
1029: * @poll_table: The poll descriptor
1030: * @full: wait until the percentage of pages are available, if @cpu != RING_BUFFER_ALL_CPUS
1031: *
1032: * If @cpu == RING_BUFFER_ALL_CPUS then the task will wake up as soon
1033: * as data is added to any of the @buffer's cpu buffers. Otherwise
1034: * it will wait for data to be added to a specific cpu buffer.
1035: *
1036: * Returns EPOLLIN | EPOLLRDNORM if data exists in the buffers,
1037: * zero otherwise.
1038: */
1039: __poll_t ring_buffer_poll_wait(struct trace_buffer *buffer, int cpu,
1040: struct file *filp, poll_table *poll_table, int full)
1041: {
1042: struct ring_buffer_per_cpu *cpu_buffer;
1043: struct rb_irq_work *rbwork;
1044:
1045: if (cpu == RING_BUFFER_ALL_CPUS) {
1046: rbwork = &buffer->irq_work;
1047: full = 0;
1048: } else {
1049: if (!cpumask_test_cpu(cpu, buffer->cpumask))
1050: return EPOLLERR;
1051:
1052: cpu_buffer = buffer->buffers[cpu];
1053: rbwork = &cpu_buffer->irq_work;
1054: }
1055:
1056: if (full) {
1057: poll_wait(filp, &rbwork->full_waiters, poll_table);
1058:
1059: if (rb_watermark_hit(buffer, cpu, full))
1060: return EPOLLIN | EPOLLRDNORM;
1061: /*
1062: * Only allow full_waiters_pending update to be seen after
1063: * the shortest_full is set (in rb_watermark_hit). If the
1064: * writer sees the full_waiters_pending flag set, it will
1065: * compare the amount in the ring buffer to shortest_full.
1066: * If the amount in the ring buffer is greater than the
1067: * shortest_full percent, it will call the irq_work handler
1068: * to wake up this list. The irq_handler will reset shortest_full
1069: * back to zero. That's done under the reader_lock, but
1070: * the below smp_mb() makes sure that the update to
1071: * full_waiters_pending doesn't leak up into the above.
1072: */
1073: smp_mb();
1074: rbwork->full_waiters_pending = true;
1075: return 0;
1076: }
1077:
1078: poll_wait(filp, &rbwork->waiters, poll_table);
1079: rbwork->waiters_pending = true;
1080:
1081: /*
1082: * There's a tight race between setting the waiters_pending and
1083: * checking if the ring buffer is empty. Once the waiters_pending bit
1084: * is set, the next event will wake the task up, but we can get stuck
1085: * if there's only a single event in.
1086: *
1087: * FIXME: Ideally, we need a memory barrier on the writer side as well,
1088: * but adding a memory barrier to all events will cause too much of a
1089: * performance hit in the fast path. We only need a memory barrier when
1090: * the buffer goes from empty to having content. But as this race is
1091: * extremely small, and it's not a problem if another event comes in, we
1092: * will fix it later.
1093: */
1094: smp_mb();
1095:
1096: if ((cpu == RING_BUFFER_ALL_CPUS && !ring_buffer_empty(buffer)) ||
1097: (cpu != RING_BUFFER_ALL_CPUS && !ring_buffer_empty_cpu(buffer, cpu)))
1098: return EPOLLIN | EPOLLRDNORM;
1099: return 0;
1100: }
SourceFile:kernel/trace/ring_buffer.c]
|
| 21/2 |
2026/07/31 14:18 |
llm |
expert |
0m
Model:
Tokens:
input: 18490
output: 36
thoughts: 680
Thoughts: **Analyzing a Race Condition in the Ring Buffer Implementation**
Okay, let's dive into this KCSAN report. The core of the issue seems to be a data race related to `waiters_pending` within the `ring_buffer_poll_wait` function, as the comment points out. The comment itself hints at a subtle race condition between setting this `waiters_pending` flag and checking the ring buffer's emptiness. The potential fix is known, a memory barrier, but avoiding adding barriers adds performance for the majority of the time the buffer is not empty.
The KCSAN report highlights a read and write operation on the same memory location, specifically `0xffff888100074cb4` of 1 byte. The write comes from `rb_wakeups` within `ring_buffer_unlock_commit`. The write is setting the waiters_pending to false. The read, according to the report, is also from `rb_wakeups` at line 4293: `if (buffer->irq_work.waiters_pending)`. That makes sense; it's the flag used to determine if we need to wake up a waiting task.
Now, the interesting thing is that the read is triggered by a call to `do_readlinkat`, which is an indirect way of triggering tracepoints, which in turn calls `ring_buffer_unlock_commit` which calls `rb_wakeups`. Thus the read and write are both in the wake up path!
My next step would be to review the Git history of `kernel/trace/ring_buffer.c` to look for related commits involving `waiters_pending`, `rb_wakeups`, or KCSAN. This might uncover prior attempts to address this race condition or provide insights into the evolution of this code.
|
| 22/2 |
2026/07/31 14:19 |
tool |
git-log |
0m
Args: map[CodeRegexp:waiters_pending PathPrefix:kernel/trace/ring_buffer.c]
Results: map[Output:7af9ded0c2ca ring-buffer: Use wait_event_interruptible() in ring_buffer_wait()
e36f19a6457b ring-buffer: Reuse rb_watermark_hit() for the poll logic
8145f1c35fa6 ring-buffer: Fix full_waiters_pending in poll
68282dd930ea ring-buffer: Fix resetting of shortest_full
b3594573681b ring-buffer: Fix waking up ring buffer readers
42fb0a1e84ff tracing/ring-buffer: Have polling block on watermark
ec0bbc5ec566 ring-buffer: Check pending waiters when doing wake ups as well
03329f993978 tracing: Add tracefs file buffer_percentage
d90fd77402d3 ring-buffer: Reorganize function locations
1e0d6714aceb ring-buffer: Do not wake up a splice waiter when page is not full
]
|
| 23/2 |
2026/07/31 14:19 |
llm |
expert |
0m
Model:
Tokens:
input: 19845
output: 23
thoughts: 58
Thoughts: Okay, let's take a look at these commits. I need to investigate what changed in `ec0bbc5ec566`, `b3594573681b`, and `7af9ded0c2ca`. I'll use `git show` to get a clear picture of the modifications. This should give me the detailed diffs I need to quickly grasp the impact of each commit.
|
| 24/2 |
2026/07/31 14:19 |
tool |
git-show |
0m
Args:
Results: map[Output:commit ec0bbc5ec5664dcee344f79373852117dc672c86
Author: Steven Rostedt (Google) <rostedt@goodmis.org>
Date: Tue Sep 27 19:15:25 2022 -0400
ring-buffer: Check pending waiters when doing wake ups as well
The wake up waiters only checks the "wakeup_full" variable and not the
"full_waiters_pending". The full_waiters_pending is set when a waiter is
added to the wait queue. The wakeup_full is only set when an event is
triggered, and it clears the full_waiters_pending to avoid multiple calls
to irq_work_queue().
The irq_work callback really needs to check both wakeup_full as well as
full_waiters_pending such that this code can be used to wake up waiters
when a file is closed that represents the ring buffer and the waiters need
to be woken up.
Link: https://lkml.kernel.org/r/20220927231824.209460321@goodmis.org
Cc: stable@vger.kernel.org
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Fixes: 15693458c4bc0 ("tracing/ring-buffer: Move poll wake ups into ring buffer code")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 02db92c9eb1b..5a7d818ca3ea 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -917,8 +917,9 @@ static void rb_wake_up_waiters(struct irq_work *work)
struct rb_irq_work *rbwork = container_of(work, struct rb_irq_work, work);
wake_up_all(&rbwork->waiters);
- if (rbwork->wakeup_full) {
+ if (rbwork->full_waiters_pending || rbwork->wakeup_full) {
rbwork->wakeup_full = false;
+ rbwork->full_waiters_pending = false;
wake_up_all(&rbwork->full_waiters);
}
}
]
|
| 25/2 |
2026/07/31 14:19 |
llm |
expert |
1m
Model:
Tokens:
input: 20678
output: 27
thoughts: 21
Thoughts: Okay, here's my attempt at summarizing that thought process, tailored for an expert audience:
**Investigating a Potentially Relevant Identifier**
Right, I need to take a look at `b3594573681b`. Given my understanding of the current context β and I'm inferring this likely from the preceding information, though I'm not seeing it explicitly here β it seems that identifier is worth further examination. It's either directly or indirectly related to the current query. Whether it's a file ID, a process name, a database entry, or something else entirely depends on the domain, which I'm already implicitly considering. My next step is clearly to bring up whatever relevant tools I have at my disposal and dive in. I expect to confirm or reject this identifier's relevance and understand its behavior or attributes, hopefully quickly, and then move on with what I am looking at.
|
| 26/2 |
2026/07/31 14:21 |
tool |
git-show |
0m
Args:
Results: map[Output:commit b3594573681b53316ec0365332681a30463edfd6
Author: Steven Rostedt (Google) <rostedt@goodmis.org>
Date: Fri Mar 8 15:24:03 2024 -0500
ring-buffer: Fix waking up ring buffer readers
A task can wait on a ring buffer for when it fills up to a specific
watermark. The writer will check the minimum watermark that waiters are
waiting for and if the ring buffer is past that, it will wake up all the
waiters.
The waiters are in a wait loop, and will first check if a signal is
pending and then check if the ring buffer is at the desired level where it
should break out of the loop.
If a file that uses a ring buffer closes, and there's threads waiting on
the ring buffer, it needs to wake up those threads. To do this, a
"wait_index" was used.
Before entering the wait loop, the waiter will read the wait_index. On
wakeup, it will check if the wait_index is different than when it entered
the loop, and will exit the loop if it is. The waker will only need to
update the wait_index before waking up the waiters.
This had a couple of bugs. One trivial one and one broken by design.
The trivial bug was that the waiter checked the wait_index after the
schedule() call. It had to be checked between the prepare_to_wait() and
the schedule() which it was not.
The main bug is that the first check to set the default wait_index will
always be outside the prepare_to_wait() and the schedule(). That's because
the ring_buffer_wait() doesn't have enough context to know if it should
break out of the loop.
The loop itself is not needed, because all the callers to the
ring_buffer_wait() also has their own loop, as the callers have a better
sense of what the context is to decide whether to break out of the loop
or not.
Just have the ring_buffer_wait() block once, and if it gets woken up, exit
the function and let the callers decide what to do next.
Link: https://lore.kernel.org/all/CAHk-=whs5MdtNjzFkTyaUy=vHi=qwWgPi0JgTe6OYUYMNSRZfg@mail.gmail.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240308202431.792933613@goodmis.org
Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linke li <lilinke99@qq.com>
Cc: Rabin Vincent <rabin@rab.in>
Fixes: e30f53aad2202 ("tracing: Do not busy wait in buffer splice")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 0699027b4f4c..3400f11286e3 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -384,7 +384,6 @@ struct rb_irq_work {
struct irq_work work;
wait_queue_head_t waiters;
wait_queue_head_t full_waiters;
- long wait_index;
bool waiters_pending;
bool full_waiters_pending;
bool wakeup_full;
@@ -798,14 +797,40 @@ void ring_buffer_wake_waiters(struct trace_buffer *buffer, int cpu)
rbwork = &cpu_buffer->irq_work;
}
- rbwork->wait_index++;
- /* make sure the waiters see the new index */
- smp_wmb();
-
/* This can be called in any context */
irq_work_queue(&rbwork->work);
}
+static bool rb_watermark_hit(struct trace_buffer *buffer, int cpu, int full)
+{
+ struct ring_buffer_per_cpu *cpu_buffer;
+ bool ret = false;
+
+ /* Reads of all CPUs always waits for any data */
+ if (cpu == RING_BUFFER_ALL_CPUS)
+ return !ring_buffer_empty(buffer);
+
+ cpu_buffer = buffer->buffers[cpu];
+
+ if (!ring_buffer_empty_cpu(buffer, cpu)) {
+ unsigned long flags;
+ bool pagebusy;
+
+ if (!full)
+ return true;
+
+ raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
+ pagebusy = cpu_buffer->reader_page == cpu_buffer->commit_page;
+ ret = !pagebusy && full_hit(buffer, cpu, full);
+
+ if (!cpu_buffer->shortest_full ||
+ cpu_buffer->shortest_full > full)
+ cpu_buffer->shortest_full = full;
+ raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
+ }
+ return ret;
+}
+
/**
* ring_buffer_wait - wait for input to the ring buffer
* @buffer: buffer to wait on
@@ -821,7 +846,6 @@ int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full)
struct ring_buffer_per_cpu *cpu_buffer;
DEFINE_WAIT(wait);
struct rb_irq_work *work;
- long wait_index;
int ret = 0;
/*
@@ -840,81 +864,54 @@ int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full)
work = &cpu_buffer->irq_work;
}
- wait_index = READ_ONCE(work->wait_index);
-
- while (true) {
- if (full)
- prepare_to_wait(&work->full_waiters, &wait, TASK_INTERRUPTIBLE);
- else
- prepare_to_wait(&work->waiters, &wait, TASK_INTERRUPTIBLE);
-
- /*
- * The events can happen in critical sections where
- * checking a work queue can cause deadlocks.
- * After adding a task to the queue, this flag is set
- * only to notify events to try to wake up the queue
- * using irq_work.
- *
- * We don't clear it even if the buffer is no longer
- * empty. The flag only causes the next event to run
- * irq_work to do the work queue wake up. The worse
- * that can happen if we race with !trace_empty() is that
- * an event will cause an irq_work to try to wake up
- * an empty queue.
- *
- * There's no reason to protect this flag either, as
- * the work queue and irq_work logic will do the necessary
- * synchronization for the wake ups. The only thing
- * that is necessary is that the wake up happens after
- * a task has been queued. It's OK for spurious wake ups.
- */
- if (full)
- work->full_waiters_pending = true;
- else
- work->waiters_pending = true;
-
- if (signal_pending(current)) {
- ret = -EINTR;
- break;
- }
-
- if (cpu == RING_BUFFER_ALL_CPUS && !ring_buffer_empty(buffer))
- break;
-
- if (cpu != RING_BUFFER_ALL_CPUS &&
- !ring_buffer_empty_cpu(buffer, cpu)) {
- unsigned long flags;
- bool pagebusy;
- bool done;
-
- if (!full)
- break;
-
- raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
- pagebusy = cpu_buffer->reader_page == cpu_buffer->commit_page;
- done = !pagebusy && full_hit(buffer, cpu, full);
+ if (full)
+ prepare_to_wait(&work->full_waiters, &wait, TASK_INTERRUPTIBLE);
+ else
+ prepare_to_wait(&work->waiters, &wait, TASK_INTERRUPTIBLE);
- if (!cpu_buffer->shortest_full ||
- cpu_buffer->shortest_full > full)
- cpu_buffer->shortest_full = full;
- raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
- if (done)
- break;
- }
+ /*
+ * The events can happen in critical sections where
+ * checking a work queue can cause deadlocks.
+ * After adding a task to the queue, this flag is set
+ * only to notify events to try to wake up the queue
+ * using irq_work.
+ *
+ * We don't clear it even if the buffer is no longer
+ * empty. The flag only causes the next event to run
+ * irq_work to do the work queue wake up. The worse
+ * that can happen if we race with !trace_empty() is that
+ * an event will cause an irq_work to try to wake up
+ * an empty queue.
+ *
+ * There's no reason to protect this flag either, as
+ * the work queue and irq_work logic will do the necessary
+ * synchronization for the wake ups. The only thing
+ * that is necessary is that the wake up happens after
+ * a task has been queued. It's OK for spurious wake ups.
+ */
+ if (full)
+ work->full_waiters_pending = true;
+ else
+ work->waiters_pending = true;
- schedule();
+ if (rb_watermark_hit(buffer, cpu, full))
+ goto out;
- /* Make sure to see the new wait index */
- smp_rmb();
- if (wait_index != work->wait_index)
- break;
+ if (signal_pending(current)) {
+ ret = -EINTR;
+ goto out;
}
+ schedule();
+ out:
if (full)
finish_wait(&work->full_waiters, &wait);
else
finish_wait(&work->waiters, &wait);
+ if (!ret && !rb_watermark_hit(buffer, cpu, full) && signal_pending(current))
+ ret = -EINTR;
+
return ret;
}
]
|
| 27/2 |
2026/07/31 14:21 |
llm |
expert |
0m
Model:
Tokens:
input: 23804
output: 23
thoughts: 17
Thoughts: Okay, here's my interpretation of that thought process, tailored for an expert audience, and presented as if it were my own internal monologue:
**Investigating a Specific Commit Hash**
Right, let's circle back to that commit hash, `7af9ded0c2ca`. I remember seeing that one flagged during the recent incident. It's time to dig into the details. I have to verify that the issue I suspected from the surface is corroborated by direct examination of the code change, and its context. Perhaps I saw something in the automated analysis, or maybe a colleague mentioned something specifically related to that commit during the postmortem. Either way, it's prudent to review it meticulously. I need to understand its impact within the broader system, and if this specific code introduced the root cause of the current malfunction. Let's see what that revision holds. I'll start with the diff, and then move quickly to the surrounding commits to get a sense of the before and after states. Iβll be looking for potential regressions, unintended consequences, or any subtle vulnerabilities that might have been overlooked during earlier reviews.
|
| 28/2 |
2026/07/31 14:22 |
tool |
git-show |
0m
Args:
Results: map[Output:commit 7af9ded0c2caac0a95f33df5cb04706b0f502588
Author: Steven Rostedt (Google) <rostedt@goodmis.org>
Date: Tue Mar 12 08:15:07 2024 -0400
ring-buffer: Use wait_event_interruptible() in ring_buffer_wait()
Convert ring_buffer_wait() over to wait_event_interruptible(). The default
condition is to execute the wait loop inside __wait_event() just once.
This does not change the ring_buffer_wait() prototype yet, but
restructures the code so that it can take a "cond" and "data" parameter
and will call wait_event_interruptible() with a helper function as the
condition.
The helper function (rb_wait_cond) takes the cond function and data
parameters. It will first check if the buffer hit the watermark defined by
the "full" parameter and then call the passed in condition parameter. If
either are true, it returns true.
If rb_wait_cond() does not return true, it will set the appropriate
"waiters_pending" flag and returns false.
Link: https://lore.kernel.org/linux-trace-kernel/CAHk-=wgsNgewHFxZAJiAQznwPMqEtQmi1waeS2O1v6L4c_Um5A@mail.gmail.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240312121703.399598519@goodmis.org
Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linke li <lilinke99@qq.com>
Cc: Rabin Vincent <rabin@rab.in>
Fixes: f3ddb74ad0790 ("tracing: Wake up ring buffer waiters on closing of the file")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h
index fa802db216f9..338a33db1577 100644
--- a/include/linux/ring_buffer.h
+++ b/include/linux/ring_buffer.h
@@ -98,6 +98,7 @@ __ring_buffer_alloc(unsigned long size, unsigned flags, struct lock_class_key *k
__ring_buffer_alloc((size), (flags), &__key); \
})
+typedef bool (*ring_buffer_cond_fn)(void *data);
int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full);
__poll_t ring_buffer_poll_wait(struct trace_buffer *buffer, int cpu,
struct file *filp, poll_table *poll_table, int full);
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 8c3730a88662..f4c34b7c7e1e 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -843,43 +843,15 @@ static bool rb_watermark_hit(struct trace_buffer *buffer, int cpu, int full)
return ret;
}
-/**
- * ring_buffer_wait - wait for input to the ring buffer
- * @buffer: buffer to wait on
- * @cpu: the cpu buffer to wait on
- * @full: wait until the percentage of pages are available, if @cpu != RING_BUFFER_ALL_CPUS
- *
- * If @cpu == RING_BUFFER_ALL_CPUS then the task will wake up as soon
- * as data is added to any of the @buffer's cpu buffers. Otherwise
- * it will wait for data to be added to a specific cpu buffer.
- */
-int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full)
+static inline bool
+rb_wait_cond(struct rb_irq_work *rbwork, struct trace_buffer *buffer,
+ int cpu, int full, ring_buffer_cond_fn cond, void *data)
{
- struct ring_buffer_per_cpu *cpu_buffer;
- DEFINE_WAIT(wait);
- struct rb_irq_work *work;
- int ret = 0;
-
- /*
- * Depending on what the caller is waiting for, either any
- * data in any cpu buffer, or a specific buffer, put the
- * caller on the appropriate wait queue.
- */
- if (cpu == RING_BUFFER_ALL_CPUS) {
- work = &buffer->irq_work;
- /* Full only makes sense on per cpu reads */
- full = 0;
- } else {
- if (!cpumask_test_cpu(cpu, buffer->cpumask))
- return -ENODEV;
- cpu_buffer = buffer->buffers[cpu];
- work = &cpu_buffer->irq_work;
- }
+ if (rb_watermark_hit(buffer, cpu, full))
+ return true;
- if (full)
- prepare_to_wait(&work->full_waiters, &wait, TASK_INTERRUPTIBLE);
- else
- prepare_to_wait(&work->waiters, &wait, TASK_INTERRUPTIBLE);
+ if (cond(data))
+ return true;
/*
* The events can happen in critical sections where
@@ -902,27 +874,75 @@ int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full)
* a task has been queued. It's OK for spurious wake ups.
*/
if (full)
- work->full_waiters_pending = true;
+ rbwork->full_waiters_pending = true;
else
- work->waiters_pending = true;
+ rbwork->waiters_pending = true;
- if (rb_watermark_hit(buffer, cpu, full))
- goto out;
+ return false;
+}
- if (signal_pending(current)) {
- ret = -EINTR;
- goto out;
+/*
+ * The default wait condition for ring_buffer_wait() is to just to exit the
+ * wait loop the first time it is woken up.
+ */
+static bool rb_wait_once(void *data)
+{
+ long *once = data;
+
+ /* wait_event() actually calls this twice before scheduling*/
+ if (*once > 1)
+ return true;
+
+ (*once)++;
+ return false;
+}
+
+/**
+ * ring_buffer_wait - wait for input to the ring buffer
+ * @buffer: buffer to wait on
+ * @cpu: the cpu buffer to wait on
+ * @full: wait until the percentage of pages are available, if @cpu != RING_BUFFER_ALL_CPUS
+ *
+ * If @cpu == RING_BUFFER_ALL_CPUS then the task will wake up as soon
+ * as data is added to any of the @buffer's cpu buffers. Otherwise
+ * it will wait for data to be added to a specific cpu buffer.
+ */
+int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full)
+{
+ struct ring_buffer_per_cpu *cpu_buffer;
+ struct wait_queue_head *waitq;
+ ring_buffer_cond_fn cond;
+ struct rb_irq_work *rbwork;
+ void *data;
+ long once = 0;
+ int ret = 0;
+
+ cond = rb_wait_once;
+ data = &once;
+
+ /*
+ * Depending on what the caller is waiting for, either any
+ * data in any cpu buffer, or a specific buffer, put the
+ * caller on the appropriate wait queue.
+ */
+ if (cpu == RING_BUFFER_ALL_CPUS) {
+ rbwork = &buffer->irq_work;
+ /* Full only makes sense on per cpu reads */
+ full = 0;
+ } else {
+ if (!cpumask_test_cpu(cpu, buffer->cpumask))
+ return -ENODEV;
+ cpu_buffer = buffer->buffers[cpu];
+ rbwork = &cpu_buffer->irq_work;
}
- schedule();
- out:
if (full)
- finish_wait(&work->full_waiters, &wait);
+ waitq = &rbwork->full_waiters;
else
- finish_wait(&work->waiters, &wait);
+ waitq = &rbwork->waiters;
- if (!ret && !rb_watermark_hit(buffer, cpu, full) && signal_pending(current))
- ret = -EINTR;
+ ret = wait_event_interruptible((*waitq),
+ rb_wait_cond(rbwork, buffer, cpu, full, cond, data));
return ret;
}
]
|
| 29/2 |
2026/07/31 14:22 |
llm |
expert |
1m
Model:
Tokens:
input: 26501
output: 40
thoughts: 2093
Thoughts: **Analyzing the Wakeup Logic in `rb_wait_cond` and `rb_wakeups`**
Okay, let's break this down. The code's comments claim that there's no need to protect the `waiters_pending` flag, asserting the work queue and `irq_work` logic will handle synchronization for wakeups. The only essential part is that the wakeup happens *after* a task is queued, and spurious wakeups are acceptable. I need to be sure this is correct.
My initial thought is to scrutinize this claim for any compiler optimization or potential torn read/write issues. Since `waiters_pending` is a `bool` (1 byte), reads and writes are inherently atomic on all architectures. That's good, so no torn reads or writes. However, compiler optimizations can be nasty. Could the compiler reorder or optimize the read or write in a way that breaks this logic?
Looking at `rb_wakeups`, I see the critical read and write: `if (buffer->irq_work.waiters_pending) { buffer->irq_work.waiters_pending = false; rb_irq_work_queue(&buffer->irq_work);}`. The flag is read and then immediately set to false.
My immediate question is: Is there any loop that could interfere? No, `rb_wakeups` is only called once per `ring_buffer_unlock_commit`. The compiler can't merge the read and write in a way that breaks the logic, as it's a straightforward read and write. Even so, KCSAN flags this as a potential data race. But, is it harmful in practice? Let's think.
If CPU 0 sees `waiters_pending` as `true` and sets it to `false`, and simultaneously CPU 1 *also* sees it as `true` and sets it to `false`, it's still fine. Both call `rb_irq_work_queue`, which is correct.
What about a writer (CPU 0/1) setting `waiters_pending` to `false` *after* a waiter (CPU 2) sets it to `true`? No problem, because both calls `rb_irq_work_queue(&buffer->irq_work)`. What about the other way around? The same. No lost wakeups!
However, what about a lost wakeup scenario where a writer *sees* `waiters_pending` as `false` *even though* a waiter *has just* set it to `true`? Memory ordering could be the culprit, the writer might see a stale value. If the writer sees `false`, it *won't* call `rb_irq_work_queue`. This looks bad, and a bit of a race.
Let's check the `ring_buffer_poll_wait` and `rb_wait_cond` functions to verify this assumption.
In `rb_wait_cond`, the waiter sets `waiters_pending = true`, and then checks if the buffer is empty (or the watermark has been hit). If it's *not* empty, the waiter returns and doesn't sleep. The waiter must see that there *is* new data so that the wakeup logic is initiated.
So, if it is *empty*, the waiter will sleep. But if the buffer is empty, there's no new event. When a *new* event is written, the writer calls `rb_wakeups`. *Crucially*, the event write happened AFTER the waiter checked if the buffer is empty. Hence, the write MUST be visible to the next check, or the writer must see `waiters_pending = true`.
I need to confirm there is a memory barrier on the writer's side. `ring_buffer_unlock_commit`, where `rb_wakeups` is called, is the key. Let's see... looking at the source...
|
| 30/2 |
2026/07/31 14:23 |
tool |
codesearch-definition-source |
0m
Args: map[ContextFile:kernel/trace/ring_buffer.c Name:ring_buffer_unlock_commit]
Results: map[SourceCode:4477: /**
4478: * ring_buffer_unlock_commit - commit a reserved
4479: * @buffer: The buffer to commit to
4480: *
4481: * This commits the data to the ring buffer, and releases any locks held.
4482: *
4483: * Must be paired with ring_buffer_lock_reserve.
4484: */
4485: int ring_buffer_unlock_commit(struct trace_buffer *buffer)
4486: {
4487: struct ring_buffer_per_cpu *cpu_buffer;
4488: int cpu = raw_smp_processor_id();
4489:
4490: cpu_buffer = buffer->buffers[cpu];
4491:
4492: rb_commit(cpu_buffer);
4493:
4494: rb_wakeups(buffer, cpu_buffer);
4495:
4496: trace_recursive_unlock(cpu_buffer);
4497:
4498: preempt_enable_notrace();
4499:
4500: return 0;
4501: }
SourceFile:kernel/trace/ring_buffer.c]
|
| 31/2 |
2026/07/31 14:23 |
llm |
expert |
1m
Model:
Tokens:
input: 29829
output: 36
thoughts: 21
Thoughts: Okay, here's my take on those thoughts, tailored for an expert audience:
**Analyzing `rb_commit` for Memory Barriers and Locks**
Alright, let's dive into the `rb_commit` code. My immediate focus is on the possibility of memory barriers or explicit locks within that function. I need to understand how it interacts with the underlying memory model and potential concurrent access. I'm looking for clues about how it handles the synchronization between different threads or processes modifying shared data. Any memory fences, atomic operations, mutexes, reader/writer locks, or any other synchronization primitives would be of particular interest to me here. It's crucial to identify those potential bottlenecks and ensure correct behavior, especially regarding data consistency and race condition avoidance in a multi-threaded or distributed environment. Let me get into the code, and start looking.
|
| 32/2 |
2026/07/31 14:24 |
tool |
codesearch-definition-source |
0m
Args: map[ContextFile:kernel/trace/ring_buffer.c Name:rb_commit]
Results: map[SourceCode:4267: static void rb_commit(struct ring_buffer_per_cpu *cpu_buffer)
4268: {
4269: local_inc(&cpu_buffer->entries);
4270: rb_end_commit(cpu_buffer);
4271: }
SourceFile:kernel/trace/ring_buffer.c]
|
| 33/2 |
2026/07/31 14:24 |
llm |
expert |
0m
Model:
Tokens:
input: 30168
output: 38
thoughts: 12
Thoughts: Okay, let's dive into this `rb_end_commit` function. I'm going to analyze its role within the broader context of the system. Considering my experience, I'm thinking about the implications for transaction management, specifically concerning data durability and concurrency control.
I'll be zeroing in on how it interacts with the underlying storage layer; is it directly involved in syncing data to disk, managing locks, or perhaps just signalling that the transaction has reached a certain point? I'm anticipating interactions with other commit-related routines; what's the control flow? It's essential to understand its place within the overarching transactional protocol.
I'm mentally stepping through possible failure scenarios. If this function fails, what's the rollback strategy? How does it interact with the logging mechanism? I want to determine the atomicity guarantees, the consistency models employed, and the impact it has on isolation, given my understanding of the system's architecture. I need to understand its complexity relative to the whole transaction. I also want to consider how the function handles exceptional conditions, like errors during data writes or concurrency conflicts. I need a clear mental model to assess its robustness and performance characteristics.
|
| 34/2 |
2026/07/31 14:25 |
tool |
codesearch-definition-source |
0m
Args: map[ContextFile:kernel/trace/ring_buffer.c Name:rb_end_commit]
Results: map[SourceCode:4222: static __always_inline void rb_end_commit(struct ring_buffer_per_cpu *cpu_buffer)
4223: {
4224: unsigned long commits;
4225:
4226: if (RB_WARN_ON(cpu_buffer,
4227: !local_read(&cpu_buffer->committing)))
4228: return;
4229:
4230: again:
4231: commits = local_read(&cpu_buffer->commits);
4232: /* synchronize with interrupts */
4233: barrier();
4234: if (local_read(&cpu_buffer->committing) == 1)
4235: rb_set_commit_to_write(cpu_buffer);
4236:
4237: local_dec(&cpu_buffer->committing);
4238:
4239: /* synchronize with interrupts */
4240: barrier();
4241:
4242: /*
4243: * Need to account for interrupts coming in between the
4244: * updating of the commit page and the clearing of the
4245: * committing counter.
4246: */
4247: if (unlikely(local_read(&cpu_buffer->commits) != commits) &&
4248: !local_read(&cpu_buffer->committing)) {
4249: local_inc(&cpu_buffer->committing);
4250: goto again;
4251: }
4252: }
SourceFile:kernel/trace/ring_buffer.c]
|
| 35/2 |
2026/07/31 14:25 |
llm |
expert |
1m
Model:
Tokens:
input: 30916
output: 21
thoughts: 1782
Thoughts: **Data Race Analysis: ring_buffer.c:waiters_pending**
Okay, let's break this down. First, I revisited the comment in `ring_buffer_poll_wait`, which outlines a potential data race with the `waiters_pending` flag. The comment suggested the race is minor and can be fixed later because of its low probability. Then, the subsequent comment claims that no memory protection is necessary because the work queue and `irq_work` logic synchronize the wake-ups, and spurious wake-ups are acceptable as long as they occur after a task has been queued.
I then revisited the KCSAN report. It highlighted a read/write data race on `waiters_pending`. The crucial piece is that the read happens in `rb_wakeups`, which is called as part of `ring_buffer_unlock_commit`.
The core question: is this a *benign* race? I need to analyze if the logic is inherently tolerant to stale reads or concurrent writes, which the guidelines define as "BENIGN." A compiler-related problem is, on the other hand, a logic bug.
I need to confirm if we need to mark it benign or harmful. "HARMFUL" would suggest a logical bug or missing annotations, potentially due to fused accesses, torn accesses, or ordering violations.
Now, let's see if compiler optimizations like fused accesses are possible. The check involves seeing if `waiters_pending` is read inside a loop that could be optimized by a compiler and cause the loop to never terminate. After examining `rb_wait_cond`, I confirmed that it *is* called inside a loop because it is inside `wait_event_interruptible`, but `waiters_pending` is *written* in `rb_wait_cond`, and *not* read inside the loop. The `waiters_pending` is ONLY read in `rb_wakeups`, which is NOT a loop. The other instances of `waiters_pending` are assignments of the boolean value. So, no compiler hoisting, or fused accesses, are possible.
Next, could torn accesses be a problem? No, because `waiters_pending` is a `bool` (1 byte), and single-byte reads/writes are atomic across all architectures.
Finally, are there any ordering violations? The comment indicates that the `irq_work` synchronization guarantees proper ordering. The only requirement is for the wake-up to occur after the task has been queued. Spurious wakeups are acceptable.
To summarize: because `waiters_pending` is only read once per commit inside `rb_wakeups` and is not read in a loop, and because single-byte reads/writes are atomic, there's no way the compiler can break this with optimizations, or that torn accesses can cause problems. Itβs logically sound.
Based on this analysis, I'm concluding this data race is **BENIGN** because the logic is tolerant to stale reads, concurrent writes, and possible spurious wake-ups. The existing code handles it correctly without explicit memory barriers or atomics.
|
| 36/2 |
2026/07/31 14:26 |
tool |
git-log |
0m
Args: map[MessageRegexps:[waiters_pending]]
Results: map[Output:7af9ded0c2ca ring-buffer: Use wait_event_interruptible() in ring_buffer_wait()
8145f1c35fa6 ring-buffer: Fix full_waiters_pending in poll
ec0bbc5ec566 ring-buffer: Check pending waiters when doing wake ups as well
76835b0ebf8a futex: Ensure get_futex_key_refs() always implies a barrier
4ce97dbf5024 trace: Fix epoll hang when we race with new entries
11d4616bd07f futex: revert back to the explicit waiter counting code
]
|
| 37/2 |
2026/07/31 14:26 |
llm |
expert |
0m
Model:
Tokens:
input: 33533
output: 26
thoughts: 30
Thoughts: Okay, here's my interpretation of those thoughts, tailored for an expert audience, and delivered as a first-person account:
**Diving into the Hashes**
Right, let's get down to brass tacks. I need to take a look at these specific commits, `8145f1c35fa6` and `4ce97dbf5024`. My intuition is telling me that there's something specific in them that needs immediate attention. Given my understanding of the codebase and the current challenges we're facing, the presence of these particular identifiers suggests a potential area of concern or a recent change that warrants thorough review. I'll need to use my typical methods to identify what needs to be looked at. This is likely related to recent system instability or a subtle bug report. These commits are likely very recent, and the nature of the issue requires immediate attention, especially with a system as dynamic as this.
|
| 38/2 |
2026/07/31 14:27 |
tool |
git-show |
0m
Args:
Results: map[Output:commit 8145f1c35fa648da662078efab299c4467b85ad5
Author: Steven Rostedt (Google) <rostedt@goodmis.org>
Date: Tue Mar 12 09:19:20 2024 -0400
ring-buffer: Fix full_waiters_pending in poll
If a reader of the ring buffer is doing a poll, and waiting for the ring
buffer to hit a specific watermark, there could be a case where it gets
into an infinite ping-pong loop.
The poll code has:
rbwork->full_waiters_pending = true;
if (!cpu_buffer->shortest_full ||
cpu_buffer->shortest_full > full)
cpu_buffer->shortest_full = full;
The writer will see full_waiters_pending and check if the ring buffer is
filled over the percentage of the shortest_full value. If it is, it calls
an irq_work to wake up all the waiters.
But the code could get into a circular loop:
CPU 0 CPU 1
----- -----
[ Poll ]
[ shortest_full = 0 ]
rbwork->full_waiters_pending = true;
if (rbwork->full_waiters_pending &&
[ buffer percent ] > shortest_full) {
rbwork->wakeup_full = true;
[ queue_irqwork ]
cpu_buffer->shortest_full = full;
[ IRQ work ]
if (rbwork->wakeup_full) {
cpu_buffer->shortest_full = 0;
wakeup poll waiters;
[woken]
if ([ buffer percent ] > full)
break;
rbwork->full_waiters_pending = true;
if (rbwork->full_waiters_pending &&
[ buffer percent ] > shortest_full) {
rbwork->wakeup_full = true;
[ queue_irqwork ]
cpu_buffer->shortest_full = full;
[ IRQ work ]
if (rbwork->wakeup_full) {
cpu_buffer->shortest_full = 0;
wakeup poll waiters;
[woken]
[ Wash, rinse, repeat! ]
In the poll, the shortest_full needs to be set before the
full_pending_waiters, as once that is set, the writer will compare the
current shortest_full (which is incorrect) to decide to call the irq_work,
which will reset the shortest_full (expecting the readers to update it).
Also move the setting of full_waiters_pending after the check if the ring
buffer has the required percentage filled. There's no reason to tell the
writer to wake up waiters if there are no waiters.
Link: https://lore.kernel.org/linux-trace-kernel/20240312131952.630922155@goodmis.org
Cc: stable@vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Fixes: 42fb0a1e84ff5 ("tracing/ring-buffer: Have polling block on watermark")
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 6ffbccb9bcf0..99fdda29ce4e 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -965,16 +965,32 @@ __poll_t ring_buffer_poll_wait(struct trace_buffer *buffer, int cpu,
poll_wait(filp, &rbwork->full_waiters, poll_table);
raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
- rbwork->full_waiters_pending = true;
if (!cpu_buffer->shortest_full ||
cpu_buffer->shortest_full > full)
cpu_buffer->shortest_full = full;
raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
- } else {
- poll_wait(filp, &rbwork->waiters, poll_table);
- rbwork->waiters_pending = true;
+ if (full_hit(buffer, cpu, full))
+ return EPOLLIN | EPOLLRDNORM;
+ /*
+ * Only allow full_waiters_pending update to be seen after
+ * the shortest_full is set. If the writer sees the
+ * full_waiters_pending flag set, it will compare the
+ * amount in the ring buffer to shortest_full. If the amount
+ * in the ring buffer is greater than the shortest_full
+ * percent, it will call the irq_work handler to wake up
+ * this list. The irq_handler will reset shortest_full
+ * back to zero. That's done under the reader_lock, but
+ * the below smp_mb() makes sure that the update to
+ * full_waiters_pending doesn't leak up into the above.
+ */
+ smp_mb();
+ rbwork->full_waiters_pending = true;
+ return 0;
}
+ poll_wait(filp, &rbwork->waiters, poll_table);
+ rbwork->waiters_pending = true;
+
/*
* There's a tight race between setting the waiters_pending and
* checking if the ring buffer is empty. Once the waiters_pending bit
@@ -990,9 +1006,6 @@ __poll_t ring_buffer_poll_wait(struct trace_buffer *buffer, int cpu,
*/
smp_mb();
- if (full)
- return full_hit(buffer, cpu, full) ? EPOLLIN | EPOLLRDNORM : 0;
-
if ((cpu == RING_BUFFER_ALL_CPUS && !ring_buffer_empty(buffer)) ||
(cpu != RING_BUFFER_ALL_CPUS && !ring_buffer_empty_cpu(buffer, cpu)))
return EPOLLIN | EPOLLRDNORM;
]
|
| 39/2 |
2026/07/31 14:27 |
llm |
expert |
0m
Model:
Tokens:
input: 35471
output: 25
thoughts: 19
Thoughts: Okay, here's my attempt at summarizing those thoughts from a first-person, expert perspective:
**Investigating a Specific Commit**
Right, so we're looking at a potential issue, and I need to dig into it. The last time I saw something like this, it ended up being in `4ce97dbf5024`. I'm starting by remembering that commit hash and immediately jumping into an examination. Let's see if that specific commit has any relevance to the current problem. It's time to check `4ce97dbf5024` and see what it has to say for itself. Need to cross-reference it with the logs. It's a standard first step in this kind of debugging scenario.
|
| 40/2 |
2026/07/31 14:28 |
tool |
git-show |
0m
Args:
Results: map[Output:commit 4ce97dbf50245227add17c83d87dc838e7ca79d0
Author: Josef Bacik <jbacik@fb.com>
Date: Mon Aug 25 13:59:41 2014 -0400
trace: Fix epoll hang when we race with new entries
Epoll on trace_pipe can sometimes hang in a weird case. If the ring buffer is
empty when we set waiters_pending but an event shows up exactly at that moment
we can miss being woken up by the ring buffers irq work. Since
ring_buffer_empty() is inherently racey we will sometimes think that the buffer
is not empty. So we don't get woken up and we don't think there are any events
even though there were some ready when we added the watch, which makes us hang.
This patch fixes this by making sure that we are actually on the wait list
before we set waiters_pending, and add a memory barrier to make sure
ring_buffer_empty() is going to be correct.
Link: http://lkml.kernel.org/p/1408989581-23727-1-git-send-email-jbacik@fb.com
Cc: stable@vger.kernel.org # 3.10+
Cc: Martin Lau <kafai@fb.com>
Signed-off-by: Josef Bacik <jbacik@fb.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index afb04b9b818a..b38fb2b9e237 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -626,8 +626,22 @@ int ring_buffer_poll_wait(struct ring_buffer *buffer, int cpu,
work = &cpu_buffer->irq_work;
}
- work->waiters_pending = true;
poll_wait(filp, &work->waiters, poll_table);
+ work->waiters_pending = true;
+ /*
+ * There's a tight race between setting the waiters_pending and
+ * checking if the ring buffer is empty. Once the waiters_pending bit
+ * is set, the next event will wake the task up, but we can get stuck
+ * if there's only a single event in.
+ *
+ * FIXME: Ideally, we need a memory barrier on the writer side as well,
+ * but adding a memory barrier to all events will cause too much of a
+ * performance hit in the fast path. We only need a memory barrier when
+ * the buffer goes from empty to having content. But as this race is
+ * extremely small, and it's not a problem if another event comes in, we
+ * will fix it later.
+ */
+ smp_mb();
if ((cpu == RING_BUFFER_ALL_CPUS && !ring_buffer_empty(buffer)) ||
(cpu != RING_BUFFER_ALL_CPUS && !ring_buffer_empty_cpu(buffer, cpu)))
]
|
| 41/2 |
2026/07/31 14:28 |
llm |
expert |
0m
Model:
Tokens:
input: 36515
output: 15
thoughts: 1043
Thoughts: **Benign Data Race in Ring Buffer Wakeup Logic**
Okay, this is pretty clear cut. Let's revisit that KCSAN report. The core issue, as the report points out, is a data race: a read and a write to the same 1-byte memory location (`0xffff888100074cb4`). Specifically, task 14450 on CPU 1 is reading the memory, and task 14466 on CPU 0 is writing to it, both within `rb_wakeups`.
Let's quickly confirm. Reading the `rb_wakeups` code, the read is indeed on line 4293: `if (buffer->irq_work.waiters_pending)`. The write is on line 4294: `buffer->irq_work.waiters_pending = false;`. So CPU 0 is clearing the `waiters_pending` flag while CPU 1 is checking it. And we know that both CPUs are executing `rb_wakeups` concurrently.
The question then becomes: is this benign? Absolutely. Let's break it down:
1. `waiters_pending` is just a boolean. It tracks whether anyone's waiting.
2. If both CPUs read 'true' and both write 'false', both will call `rb_irq_work_queue`. That's harmless. The queue is designed to handle this redundancy safely.
3. If one CPU reads 'false' and the other writes 'false', no harm either. We just have an extra check; the queue is already queued.
4. It's a single byte, so no torn reads or writes can occur.
5. There are no repeated reads, so we don't have to worry about compiler optimization livelocks.
6. The comment in `rb_wait_cond` explicitly supports this pattern, noting the synchronization is handled correctly by the queue logic, and spurious wakeups are acceptable. This is the smoking gun!
So this *is* benign.
However, should we recommend any code changes to make this more explicit and prevent false positives from KCSAN? Yes, we should. Annotating accesses to `waiters_pending` with `READ_ONCE()` and `WRITE_ONCE()` (or even `data_race()`) would be a good practice, even though it's technically unnecessary in this instance. This will suppress the KCSAN warnings and protect against any subtle compiler optimizations, although they are highly unlikely. We should also check for similar access patterns in `rb_wait_cond` and `ring_buffer_poll_wait`, and apply the same annotations there.
Okay, I'm confident in calling this benign. Let's call `set-results` with `Benign: true`.
|
| 42/2 |
2026/07/31 14:29 |
tool |
set-results |
0m
Args:
Results:
|
| 43/2 |
2026/07/31 14:29 |
llm |
expert |
3m
Model:
Tokens:
input: 38194
output: 780
thoughts: 758
Thoughts: **My Analysis of the Data Race in ring_buffer_unlock_commit/rb_wakeups**
Okay, here's how I'm thinking through this data race analysis. My goal is to be incredibly clear and comprehensive, providing a definitive classification. I'll structure my explanation around these points:
First, I need to concisely summarize the race itself. It appears we have two concurrent calls to `rb_wakeups`. Function A is called via `ring_buffer_unlock_commit` from a tracepoint triggered within, for example, `do_readlinkat`, or potentially other system calls. Function B is also calling `rb_wakeups` via `ring_buffer_unlock_commit`, originating from another concurrent event. The variable in question is `buffer->irq_work.waiters_pending` (or potentially `cpu_buffer->irq_work.waiters_pending`). This is where the contention is happening.
Next, it's crucial to understand the synchronization context. `ring_buffer_unlock_commit` is, as I recall, called locklessly. That's the key: it's on the fast path, so the system avoids a lock to gain performance. This also means that `rb_wakeups` is called concurrently on different CPUs without any mutual exclusion. This is the root of the potential data race.
Now, the bottom line: I'm classifying this as **BENIGN**.
The reasoning needs to be meticulous. I have to explain precisely why this isn't a problem.
* `waiters_pending` is a boolean flag, which indicates whether there are waiters to be woken up.
* **No Torn Accesses**: `waiters_pending` is a 1-byte boolean. Reads and writes of 1-byte values are atomic on all architectures that matter for us.
* **No Livelocks / Fused Accesses**: `rb_wakeups` is not a loop, and there are no loops reading `waiters_pending`. There's no scenario where a compiler could hoist or merge an access to it, leading to an infinite loop, or livelock.
* **Redundant Wakeups are Safe**: If both CPUs, concurrently, read `waiters_pending` as `true` and then both write `false` and call `rb_irq_work_queue()`, this isn't a problem. A redundant call to `irq_work_queue()` is handled safely by the `irq_work` subsystem. It can handle this situation.
* **No Lost Wakeups**: If one CPU writes `false` and the other CPU reads `false`, the second CPU skips queuing the work. This is the correct behavior because the first CPU has already taken care of it.
* **Explicit Design Intent**: And a really important piece, I remember seeing the comment in `rb_wait_cond`, which explicitly states that this flag does *not* need protection, and that spurious wakeups are perfectly acceptable.
I also should think about how to best annotate this. The analysis suggests wrapping reads and writes to `waiters_pending` and also `full_waiters_pending` with `READ_ONCE()` / `WRITE_ONCE()`, or using `data_race()` to document the race and silence KCSAN and similar tools. I think this is good, even if it might seem redundant, it is better to be explicit.
Wait, the KCSAN report shows `do_readlinkat` as the reader? I need to remember why that's happening. Ah, right. Task 14450 was executing `do_readlinkat`. This triggered a tracepoint which then calls `ring_buffer_unlock_commit`, which then calls `rb_wakeups`. The `do_readlinkat` call is the top-level function that the tracepoint is associated with. That stack trace will get truncated or simplified by KCSAN and it will show that top-level function. The system call triggered the tracepoint that caused the call chain.
Okay, I'm confident in my thorough analysis. Ready to write this response.
|
| 44/1 |
2026/07/31 14:32 |
action |
format-explanation |
0m
Results: map[Explanation:### Race Summary
* **Function A:** `rb_wakeups` (called via `ring_buffer_unlock_commit` from a
tracepoint triggered during a system call like `do_readlinkat`)
* **Function B:** `rb_wakeups` (called via `ring_buffer_unlock_commit` from a
concurrent event on another CPU)
* **Variable/Field:** `buffer->irq_work.waiters_pending` (or
`cpu_buffer->irq_work.waiters_pending`)
---
### Synchronization Context
The ring buffer's fast path (`ring_buffer_unlock_commit`) is designed to be
extremely fast and lockless. When committing an event, `rb_wakeups` is called to
check if there are any pending waiters that need to be woken up via `irq_work`.
Because events can be committed concurrently on different CPUs, multiple threads
can execute `rb_wakeups` concurrently without mutual exclusion.
---
### Final Classification: **BENIGN**
---
### Detailed Reasoning
The data race on `waiters_pending` is **completely benign** and structurally
tolerant to concurrent reads and writes due to the following reasons:
1. **No Torn Accesses:**
`waiters_pending` is a 1-byte `bool` variable. On all supported
architectures, 1-byte reads and writes are inherently atomic, meaning there
is no risk of reading a partially written or "torn" value.
2. **No Livelocks or Fused Accesses:**
`rb_wakeups` is a non-looping function called once per event commit. There
are no polling loops reading `waiters_pending`, so there is no risk of
compiler optimizations (such as hoisting a load out of a loop) causing a
thread to get stuck in an infinite loop.
3. **Redundant Wakeups are Safe:**
If CPU 0 and CPU 1 concurrently execute `rb_wakeups` and both read
`waiters_pending` as `true`, both will write `false` to it and call
`rb_irq_work_queue()`. The `irq_work` subsystem is designed to safely handle
concurrent and redundant queuing.
4. **No Lost Wakeups:**
If CPU 0 writes `false` and CPU 1 reads `false`, CPU 1 will skip queuing the
work. This is correct because CPU 0 has already queued the `irq_work` to wake
up the waiters.
5. **Explicit Design Intent:**
The kernel source code explicitly documents this design in `rb_wait_cond`:
```c
/*
* There's no reason to protect this flag either, as
* the work queue and irq_work logic will do the necessary
* synchronization for the wake ups. The only thing
* that is necessary is that the wake up happens after
* a task has been queued. It's OK for spurious wake ups.
*/
```
---
### Recommended Annotations
To document the race and silence KCSAN, the accesses to `waiters_pending` and
`full_waiters_pending` can be wrapped in `READ_ONCE()` / `WRITE_ONCE()` or
marked with `data_race()`.
For example, in `rb_wakeups`:
```c
if (data_race(buffer->irq_work.waiters_pending)) {
data_race(buffer->irq_work.waiters_pending = false);
rb_irq_work_queue(&buffer->irq_work);
}
```]
|