Re: [PATCH v9 4/4] ring-buffer: Prevent truncation of nr_pages / nr_subbufs

[Vincent Donnefort <vdonnefort@google.com>]

On Thu, Sep 03, 2026 at 01:16:01PM -0400, Steven Rostedt wrote:
> On Tue,  1 Sep 2026 16:54:45 +0100
> Vincent Donnefort <vdonnefort@google.com> wrote:
> 
> > -static void *rb_range_buffer(struct ring_buffer_per_cpu *cpu_buffer, int idx)
> > +static void *rb_range_buffer(struct ring_buffer_per_cpu *cpu_buffer, unsigned int idx)
> >  {
> >  	struct ring_buffer_cpu_meta *meta;
> > +	unsigned int subbuf_size;
> >  	unsigned long ptr;
> > -	int subbuf_size;
> >  
> >  	meta = rb_range_meta(cpu_buffer->buffer, 0, cpu_buffer->cpu);
> >  	if (!meta)
> > @@ -1777,7 +1776,7 @@ static void *rb_range_buffer(struct ring_buffer_per_cpu *cpu_buffer, int idx)
> >  
> >  	ptr = (unsigned long)rb_subbufs_from_meta(meta);
> >  
> > -	ptr += subbuf_size * idx;
> > +	ptr += (unsigned long)subbuf_size * idx;
> 
> Really, it looks like the idx should be typecasted, as it is the number of
> subbuffers. Maybe even pass it in as unsigned long?

__rb_allocate_pages() is passing idx as a number pages, so yeah that'd make
more sense, even though a persistent buffer is capped to a 30-bits nr_pages.

> 
> >  	if (ptr + subbuf_size > cpu_buffer->buffer->range_addr_end)
> >  		return NULL;
> >  
> > @@ -1854,13 +1853,13 @@ static bool rb_meta_init(struct trace_buffer *buffer, int scratch_size)
> >   * must be the same.
> >   */
> >  static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> > -			      struct trace_buffer *buffer, int nr_pages,
> > +			      struct trace_buffer *buffer, unsigned long nr_pages,
> >  			      unsigned long *subbuf_mask)
> >  {
> > -	int subbuf_size = PAGE_SIZE;
> > +	unsigned long subbuf_size = PAGE_SIZE;
> 
> Why the long? Shouldn't it be unsigned int?

That is to cheat to not have to add a cast in

  buffers_end = meta->first_buffer + (subbuf_size * meta->nr_subbufs);

> 
> >  	unsigned long buffers_start;
> >  	unsigned long buffers_end;
> > -	int i;
> > +	unsigned long i;
> >  
> >  	if (!subbuf_mask)
> >  		return false;
> > @@ -2109,8 +2108,8 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
> >  	struct buffer_page *head_page, *orig_head, *orig_reader;
> >  	struct rb_validation_state state = { 0 };
> >  	bool skip = false;
> > +	unsigned long i;
> >  	int ret;
> > -	int i;
> >  
> >  	if (!meta || !meta->head_buffer)
> >  		return;
> > @@ -2161,7 +2160,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
> >  		rb_validate_buffer(head_page, cpu_buffer, meta, &state, 0, state.ts);
> >  	}
> >  	if (i)
> > -		pr_info("Ring buffer [%d] rewound %d pages\n", cpu_buffer->cpu, i);
> > +		pr_info("Ring buffer [%d] rewound %lu pages\n", cpu_buffer->cpu, i);
> >  
> >  	/* The last rewound page must be skipped. */
> >  	if (head_page != orig_head)
> > @@ -2245,7 +2244,8 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
> >  	}
> >  }
> >  
> > -static void rb_range_meta_init(struct trace_buffer *buffer, int nr_pages, int scratch_size)
> > +static void rb_range_meta_init(struct trace_buffer *buffer,
> > +			       unsigned long nr_pages, int scratch_size)
> 
> static void rb_range_meta_init(struct trace_buffer *buffer, unsigned long nr_pages,
> 			       int scratch_size)
> 
> looks better ;-)

ack

-- 
Vincent

> 
> >  {
> >  	struct ring_buffer_cpu_meta *meta;
> >  	unsigned long *subbuf_mask;
> > @@ -2345,8 +2345,8 @@ static int rbm_show(struct seq_file *m, void *v)
> >  			   rb_meta_subbuf_idx(meta, (void *)meta->head_buffer));
> >  		seq_printf(m, "commit_buffer: %d\n",
> >  			   rb_meta_subbuf_idx(meta, (void *)meta->commit_buffer));
> > -		seq_printf(m, "subbuf_size:   %d\n", meta->subbuf_size);
> > -		seq_printf(m, "nr_subbufs:    %d\n", meta->nr_subbufs);
> > +		seq_printf(m, "subbuf_size:   %u\n", meta->subbuf_size);
> > +		seq_printf(m, "nr_subbufs:    %u\n", meta->nr_subbufs);
> >  		return 0;
> >  	}
> >  
> 
> 
> -- Steve