Re: [PATCH] ring-buffer: Fixes for subbuf resizing

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calculating the total ring buffer size in `ring_buffer_subbuf_order_set` truncates large sizes because `nr_pages` is declared as a 32-bit `int`, causing large ring buffers (>4GB) to silently shrink.
--

--- Patch [1]: [PATCH] ring-buffer: Fixes for subbuf resizing ---
commit 2e24224b70e061b34a4388f3a630382a4b8c1c44
Author: Vincent Donnefort <[email protected]>

ring-buffer: Fixes for subbuf resizing

This series addresses multiple issues discovered with the dynamic ring
buffer resizing.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -7320,7 +7376,8 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  {
>  	struct ring_buffer_per_cpu *cpu_buffer;
>  	struct buffer_page *bpage, *tmp;
> -	int old_order, old_size;
> +	unsigned int old_capacity;
> +	int old_order;
>  	int nr_pages;
>  	int psize;
>  	int err;

[ ... ]

> @@ -7367,8 +7424,8 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
>  		}
>  
>  		/* Update the number of pages to match the new size */
> -		nr_pages = old_size * buffer->buffers[cpu]->nr_pages;
> -		nr_pages = DIV_ROUND_UP(nr_pages, buffer->subbuf_size);
> +		nr_pages = old_capacity * buffer->buffers[cpu]->nr_pages;
> +		nr_pages = DIV_ROUND_UP(nr_pages, rb_subbuf_capacity(buffer));

[Severity: High]
This is a pre-existing issue, but does this size calculation truncate when
the ring buffer is larger than 4GB?

Because nr_pages is declared as a 32-bit int at the top of the function,
assigning the total byte size (old_capacity * buffer->buffers[cpu]->nr_pages)
to it truncates the upper bits of the 64-bit result before calculating the
new number of pages.

For example, a 5GB buffer would get its byte size silently truncated to
around 705MB before DIV_ROUND_UP computes the new number of pages, leading
to an unexpectedly shrunken ring buffer. Should nr_pages or a temporary
variable be declared as unsigned long to hold the intermediate byte size?

>  
>  		/* we need a minimum of two pages */
>  		if (nr_pages < 2)
>  			nr_pages = 2;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1