Re: [PATCH v1 1/9] ring-buffer: Always read buffer->buffers after testing the cpumask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] ring-buffer: atomic counter underflow in ring_buffer_reset()
  [1]
--

commit fbf1784e6b7f3202f4d6898704fb8180140b6da5
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Always read buffer->buffers after testing the cpumask

This commit moves the load of buffer->buffers[cpu] after the
cpumask_test_cpu() check in ring_buffer_peek(), ring_buffer_reset_cpu(),
and ring_buffer_read_page() to avoid reading a stale per-CPU buffer
pointer when racing with trace_rb_cpu_prepare().

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 1fd28a72e6ee1..bd8126f6353b7 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c

[ ... ]

> @@ -6740,11 +6742,13 @@ static void reset_disabled_cpu_buffer(struct ring_buffer_per_cpu *cpu_buffer)
>   */
>  void ring_buffer_reset_cpu(struct trace_buffer *buffer, int cpu)
>  {
> -	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
> +	struct ring_buffer_per_cpu *cpu_buffer;
>  
>  	if (!cpumask_test_cpu(cpu, buffer->cpumask))
>  		return;
>  
> +	cpu_buffer = buffer->buffers[cpu];
> +
>  	/* prevent another thread from changing buffer sizes */
>  	mutex_lock(&buffer->mutex);

[Severity: High]
This problem wasn't introduced by this patch, but can a concurrent
trace_rb_cpu_prepare() race with ring_buffer_reset() [1] and
underflow cpu_buffer->record_disabled and cpu_buffer->resize_disabled?

Unlike ring_buffer_reset_online_cpus(), which marks each disabled
cpu_buffer with RESET_BIT before sleeping in synchronize_rcu(),
ring_buffer_reset() iterates over buffer->cpumask twice without tracking
which CPUs were disabled in the first loop or holding cpus_read_lock():

CPU0                                  CPU1
ring_buffer_reset()
  for_each_buffer_cpu(buffer, cpu)
    atomic_inc(&cpu_buffer->resize_disabled);
    atomic_inc(&cpu_buffer->record_disabled);
  synchronize_rcu();
                                      trace_rb_cpu_prepare()
                                        rb_allocate_cpu_buffer()
                                        cpumask_set_cpu(cpu, buffer->cpumask);
  for_each_buffer_cpu(buffer, cpu)
    reset_disabled_cpu_buffer(cpu_buffer);
    atomic_dec(&cpu_buffer->record_disabled);  <-- underflows to -1
    atomic_dec(&cpu_buffer->resize_disabled);  <-- underflows to -1

For the newly prepared CPU, reset_disabled_cpu_buffer() runs while
recording is still enabled and both atomic counters underflow from 0 to
-1, corrupting the per-CPU ring buffer state.

[1] https://sashiko.dev/#/bug/linux-2557ac4d-25af-4832-a2c9-c12af32c7a04

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112141440.3050124-1-vdonnefort@google.com?part=1