Re: [PATCH 6.12 166/181] ring-buffer: Use current_context for safe per-CPU buffer swap

Hi Greg/Sasha

On 17/08/26 7:04 pm, Greg Kroah-Hartman wrote:
> 6.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Tengda Wu <[email protected]>
> 
> commit f27bdc43077e4fcb5557dfc315ee8d91e741f483 upstream.
> 
> The ring_buffer_swap_cpu() function currently checks the per-CPU
> committing counter to determine if a buffer is actively being written to
> before performing the swap. However, there exists a race window where
> this check can be bypassed:
> 
>      ring_buffer_lock_reserve
>          cpu_buffer = buffer->buffers[cpu];       // cpu_buffer_a
>          rb_reserve_next_event
>              rb_start_commit // inc committing
>              if (unlikely(READ_ONCE(cpu_buffer->buffer) != buffer)) {...}
>              __rb_reserve_next
>                  rb_move_tail
>                      rb_end_commit(cpu_buffer);   // dec committing => 0
>                      /* interrupt hits here, successfully swaps! */
>                      local_inc(&cpu_buffer->committing);
> 
>      ring_buffer_unlock_commit
>          cpu_buffer = buffer->buffers[cpu];      // cpu_buffer_b
>          rb_commit
>              rb_end_commit
>              RB_WARN_ON(cpu_buffer, !local_read(&cpu_buffer->committing))
>                                                  // triggers warning
> 
> The committing counter can temporarily drop to 0 during a single write
> operation (within rb_move_tail), creating a window where swap can
> succeed even though the write is still in progress. This leads to
> inconsistent buffer state and triggers the RB_WARN_ON in rb_commit().
> 
> Replace the committing counter check with current_context checks, which
> are set at the entry of ring_buffer_lock_reserve() and remain valid
> throughout the entire write operation, providing a reliable indicator of
> buffer busy state during swap.
> 
> Cc: [email protected]
> Fixes: 4239c38fe0b3 ("ring-buffer: Process commits whenever moving to a new page.")
> Link: https://patch.msgid.link/[email protected]
> Signed-off-by: Tengda Wu <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>   kernel/trace/ring_buffer.c |    8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -6253,7 +6253,7 @@ int ring_buffer_swap_cpu(struct trace_bu
>   {
>   	struct ring_buffer_per_cpu *cpu_buffer_a;
>   	struct ring_buffer_per_cpu *cpu_buffer_b;
> -	int ret = -EINVAL;
> +	int ret = -EBUSY;
>   
>   	if (!cpumask_test_cpu(cpu, buffer_a->cpumask) ||
>   	    !cpumask_test_cpu(cpu, buffer_b->cpumask))
> @@ -6298,10 +6298,10 @@ int ring_buffer_swap_cpu(struct trace_bu
>   	atomic_inc(&cpu_buffer_a->record_disabled);
>   	atomic_inc(&cpu_buffer_b->record_disabled);
>   
> -	ret = -EBUSY;
> -	if (local_read(&cpu_buffer_a->committing))
> +	/* Do not swap if either buffer is in the process of writing */
> +	if (cpu_buffer_a->current_context)
>   		goto out_dec;
> -	if (local_read(&cpu_buffer_b->committing))
> +	if (cpu_buffer_b->current_context)
>   		goto out_dec;

I have run an AI-assisted backport review and it spotted an issue. The
current_context checks themselves are correct, but the return-value
change does not fit the older 6.12.y control flow.

Upstream has direct validation returns:

	int ret = -EBUSY;
	...
	if (cpu_buffer_a->nr_pages != cpu_buffer_b->nr_pages)
		return -EINVAL;

6.12.y instead has:

	int ret = -EBUSY;
	...
	if (cpu_buffer_a->nr_pages != cpu_buffer_b->nr_pages)
		goto out;

This changes invalid CPU-mask, page-count, and sub-buffer-order results
from -EINVAL to -EBUSY. I think update_max_tr_single() treats -EBUSY as 
an expected commit/resize collision.

Maybe we could be retaining ret = -EINVAL at declaration and assigning
ret = -EBUSY immediately before the new current_context checks.

thanks,
Harshit



>   
>   	/*
> 
> 
>