• Zheng Yejian's avatar
    tracing: Fix race issue between cpu buffer write and swap · 3163f635
    Zheng Yejian authored
    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: f1affcaa ("tracing: Add snapshot in the per_cpu trace directories")
    Signed-off-by: default avatarZheng Yejian <zhengyejian1@huawei.com>
    Signed-off-by: default avatarSteven Rostedt (Google) <rostedt@goodmis.org>
    3163f635
trace.c 258 KB