Re: [PATCH v1 1/2] tracing/remotes: Account for ring buffer page header in size calculation

[Vincent Donnefort]

On Wed, Sep 09, 2026 at 06:13:16PM -0400, Steven Rostedt wrote:
> On Mon,  7 Sep 2026 20:26:42 +0100
> Vincent Donnefort <vdonnefort@xxxxxxxxxx> wrote:
> 
> > trace_buffer_desc_size() and trace_remote_alloc_buffer undercount the
> > required pages because every ring buffer page contains a header
> > (BUF_PAGE_HDR_SIZE). Account for that header to ensure allocated remote
> > ring buffers aren't smaller than requested by the user.
> > 
> > While at it, ensure those functions catch nr_pages overflow.
> 
> Fixes patches should never have "while at it". That means it belongs as a
> separate patch. The only "while at it" that is acceptable is white space
> fixes or other formatting changes along with new code that is not a fix.

ack.

> 
> > 
> > Fixes: 2e67fabd8b77 ("ring-buffer: Introduce ring-buffer remotes")
> > Signed-off-by: Vincent Donnefort <vdonnefort@xxxxxxxxxx>
> > 
> > diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h
> > index afc7daa6ee7d..7a1a92f87650 100644
> > --- a/include/linux/ring_buffer.h
> > +++ b/include/linux/ring_buffer.h
> > @@ -8,6 +8,8 @@
> >  
> >  #include <uapi/linux/trace_mmap.h>
> >  
> > +#include <linux/ring_buffer_types.h>
> 
> Hmm, why is this after the uapi header? It should be part of the linux/
> headers.
> 
> > +
> >  struct trace_buffer;
> >  struct ring_buffer_iter;
> >  
> > @@ -281,9 +283,14 @@ static inline struct ring_buffer_desc *__first_ring_buffer_desc(struct trace_buf
> >  
> >  static inline size_t trace_buffer_desc_size(size_t buffer_size, unsigned int nr_cpus)
> >  {
> > -	unsigned int nr_pages = max(DIV_ROUND_UP(buffer_size, PAGE_SIZE), 2UL) + 1;
> > +	unsigned long nr_pages =
> > +		max(DIV_ROUND_UP(buffer_size, PAGE_SIZE - BUF_PAGE_HDR_SIZE), 2UL) + 1;
> 
> This is getting a little unwieldy. Let's break it up:
> 
> 	unsigned long pages_req = DIV_ROUND_UP(buffer_size, PAGE_SIZE - BUF_PAGE_HDR_SIZE);
> 	unsigned long nr_pages = max(pages_req, 2UL) + 1;
> 
> Or better yet, let's add a new helper function:
> 
> static inline unsigned long calculate_nr_pages(size_t buffer_size)
> {
> 	unsigned long req_pages = DIV_ROUND_UP(buffer_size, PAGE_SIZE - BUF_PAGE_HDR_SIZE);
> 	return max(pages_req, 2UL) + 1;
> }

I'll introduce the helper from this patch as suggested.

> 
> Then this could be simply:
> 
> 	unsigned long nr_pages = calculate_nr_pages(buffer_size);
> 
> >  	struct ring_buffer_desc *rbdesc;
> >  
> > +	/* Capped by ring_buffer_desc::nr_page_va */
> > +	if (nr_pages > UINT_MAX)
> > +		return SIZE_MAX;
> 
> Separate patch.
> 
> > +
> >  	return size_add(offsetof(struct trace_buffer_desc, __data),
> >  			size_mul(nr_cpus, struct_size(rbdesc, page_va, nr_pages)));
> >  }
> > diff --git a/kernel/trace/trace_remote.c b/kernel/trace/trace_remote.c
> > index 75fa1ffc4c96..2e0fdbb730b7 100644
> > --- a/kernel/trace/trace_remote.c
> > +++ b/kernel/trace/trace_remote.c
> > @@ -980,9 +980,12 @@ int trace_remote_alloc_buffer(struct trace_buffer_desc *desc, size_t desc_size,
> >  			      const struct cpumask *cpumask)
> >  {
> >  	size_t min_desc_size = trace_buffer_desc_size(buffer_size, cpumask_weight(cpumask));
> > -	unsigned int nr_pages = max(DIV_ROUND_UP(buffer_size, PAGE_SIZE), 2UL) + 1;
> >  	struct ring_buffer_desc *rb_desc;
> >  	int cpu, ret = -ENOMEM;
> > +	unsigned int nr_pages;
> > +
> 
> 
> > +	if (min_desc_size == SIZE_MAX)
> > +		return -E2BIG;
> 
> Looks like this should be a separate patch too.
> 
> >  
> >  	if (desc_size < min_desc_size)
> >  		return -EINVAL;
> > @@ -991,6 +994,7 @@ int trace_remote_alloc_buffer(struct trace_buffer_desc *desc, size_t desc_size,
> >  	desc->struct_len = min_desc_size;
> >  
> >  	rb_desc = __first_ring_buffer_desc(desc);
> > +	nr_pages = max(DIV_ROUND_UP(buffer_size, PAGE_SIZE - BUF_PAGE_HDR_SIZE), 2UL) + 1;
> 
> And here we can have;
> 
> 	nr_pages = calculate_nr_pages(buffer_size);
> 
> -- Steve
> 
> >  
> >  	for_each_cpu(cpu, cpumask) {
> >  		unsigned int id;
> 

-- 
Vincent