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

Reply via email to