Skip to content

Commit 3163f63

Browse files
Zheng Yejianrostedt
authored andcommitted
tracing: Fix race issue between cpu buffer write and swap
Warning happened in rb_end_commit() at code: if (RB_WARN_ON(cpu_buffer, !local_read(&cpu_buffer->committing))) WARNING: CPU: 0 PID: 139 at kernel/trace/ring_buffer.c:3142 rb_commit+0x402/0x4a0 Call Trace: ring_buffer_unlock_commit+0x42/0x250 trace_buffer_unlock_commit_regs+0x3b/0x250 trace_event_buffer_commit+0xe5/0x440 trace_event_buffer_reserve+0x11c/0x150 trace_event_raw_event_sched_switch+0x23c/0x2c0 __traceiter_sched_switch+0x59/0x80 __schedule+0x72b/0x1580 schedule+0x92/0x120 worker_thread+0xa0/0x6f0 It is because the race between writing event into cpu buffer and swapping cpu buffer through file per_cpu/cpu0/snapshot: Write on CPU 0 Swap buffer by per_cpu/cpu0/snapshot on CPU 1 -------- -------- tracing_snapshot_write() [...] ring_buffer_lock_reserve() cpu_buffer = buffer->buffers[cpu]; // 1. Suppose find 'cpu_buffer_a'; [...] rb_reserve_next_event() [...] ring_buffer_swap_cpu() if (local_read(&cpu_buffer_a->committing)) goto out_dec; if (local_read(&cpu_buffer_b->committing)) goto out_dec; buffer_a->buffers[cpu] = cpu_buffer_b; buffer_b->buffers[cpu] = cpu_buffer_a; // 2. cpu_buffer has swapped here. rb_start_commit(cpu_buffer); if (unlikely(READ_ONCE(cpu_buffer->buffer) != buffer)) { // 3. This check passed due to 'cpu_buffer->buffer' [...] // has not changed here. return NULL; } cpu_buffer_b->buffer = buffer_a; cpu_buffer_a->buffer = buffer_b; [...] // 4. Reserve event from 'cpu_buffer_a'. ring_buffer_unlock_commit() [...] cpu_buffer = buffer->buffers[cpu]; // 5. Now find 'cpu_buffer_b' !!! rb_commit(cpu_buffer) rb_end_commit() // 6. WARN for the wrong 'committing' state !!! Based on above analysis, we can easily reproduce by following testcase: ``` bash #!/bin/bash dmesg -n 7 sysctl -w kernel.panic_on_warn=1 TR=/sys/kernel/tracing echo 7 > ${TR}/buffer_size_kb echo "sched:sched_switch" > ${TR}/set_event while [ true ]; do echo 1 > ${TR}/per_cpu/cpu0/snapshot done & while [ true ]; do echo 1 > ${TR}/per_cpu/cpu0/snapshot done & while [ true ]; do echo 1 > ${TR}/per_cpu/cpu0/snapshot done & ``` To fix it, IIUC, we can use smp_call_function_single() to do the swap on the target cpu where the buffer is located, so that above race would be avoided. Link: https://lore.kernel.org/linux-trace-kernel/20230831132739.4070878-1-zhengyejian1@huawei.com Cc: <mhiramat@kernel.org> Fixes: f1affca ("tracing: Add snapshot in the per_cpu trace directories") Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
1 parent 2cf0dee commit 3163f63

File tree

1 file changed

+12
-5
lines changed

1 file changed

+12
-5
lines changed

kernel/trace/trace.c

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7599,6 +7599,11 @@ static int tracing_snapshot_open(struct inode *inode, struct file *file)
75997599
return ret;
76007600
}
76017601

7602+
static void tracing_swap_cpu_buffer(void *tr)
7603+
{
7604+
update_max_tr_single((struct trace_array *)tr, current, smp_processor_id());
7605+
}
7606+
76027607
static ssize_t
76037608
tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
76047609
loff_t *ppos)
@@ -7657,13 +7662,15 @@ tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
76577662
ret = tracing_alloc_snapshot_instance(tr);
76587663
if (ret < 0)
76597664
break;
7660-
local_irq_disable();
76617665
/* Now, we're going to swap */
7662-
if (iter->cpu_file == RING_BUFFER_ALL_CPUS)
7666+
if (iter->cpu_file == RING_BUFFER_ALL_CPUS) {
7667+
local_irq_disable();
76637668
update_max_tr(tr, current, smp_processor_id(), NULL);
7664-
else
7665-
update_max_tr_single(tr, current, iter->cpu_file);
7666-
local_irq_enable();
7669+
local_irq_enable();
7670+
} else {
7671+
smp_call_function_single(iter->cpu_file, tracing_swap_cpu_buffer,
7672+
(void *)tr, 1);
7673+
}
76677674
break;
76687675
default:
76697676
if (tr->allocated_snapshot) {

0 commit comments

Comments
 (0)