Re: [PATCH v5 05/18] tracing/simple_ring_buffer: Add support for compressed length

On Wed, Aug 12, 2026 at 04:42:41PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] The patch unconditionally introduces `type_len` compression for small events and hardcodes 4-byte alignment, ignoring `RB_FORCE_8BYTE_ALIGNMENT` and `RB_ARCH_ALIGNMENT`. This causes payload truncation and unaligned accesses on architectures requiring 8-byte alignment.
> 
> Pre-existing issues:
> - [High] `simple_ring_buffer_reserve` lacks a bounds check to prevent `event_size` from exceeding the maximum page capacity (`PAGE_SIZE - BUF_PAGE_HDR_SIZE`).
> --
> 
> --- Patch [5]: [PATCH v5 05/18] tracing/simple_ring_buffer: Add support for compressed length ---
> commit 661f4dfe638155c632fb481b814114210260a54c
> Author: Vincent Donnefort <[email protected]>
> 
> tracing/simple_ring_buffer: Add support for compressed length
> 
> This patch adds support for compressing the event length into the event
> header type for small events in the simple ring buffer, saving 32 bits
> per event. It enforces a 4-byte alignment for the compressed length.
> 
> > diff --git a/kernel/trace/simple_ring_buffer.c b/kernel/trace/simple_ring_buffer.c
> > index f4642f5adda33..1a97d17cca246 100644
> > --- a/kernel/trace/simple_ring_buffer.c
> > +++ b/kernel/trace/simple_ring_buffer.c
> > @@ -207,7 +207,15 @@ static unsigned long rb_event_size(unsigned long length)
> >  {
> >  	struct ring_buffer_event *event;
> >  
> > -	return length + RB_EVNT_HDR_SIZE + sizeof(event->array[0]);
> > +	if (!length)
> > +		length++;
> > +
> > +	length = ALIGN(length, RB_ALIGNMENT);
> 
> [Severity: High]
> Does this hardcoded 4-byte alignment cause unaligned access exceptions on
> architectures that require 8-byte alignment (where RB_FORCE_8BYTE_ALIGNMENT
> is 1)?
> 
> It looks like using RB_ARCH_ALIGNMENT instead of RB_ALIGNMENT would prevent
> these exceptions.

I will add support for RB_FORCE_8BYTE_ALIGNMENT in v6.

> 
> > +
> > +	if (length > RB_MAX_SMALL_DATA)
> > +		length += sizeof(event->array[0]);
> > +
> > +	return length + RB_EVNT_HDR_SIZE;
> >  }
> 
> [ ... ]
> 
> > @@ -259,9 +270,13 @@ simple_rb_reserve_next(struct simple_rb_per_cpu *cpu_buffer, unsigned long lengt
> >  		time_delta = 0;
> >  	}
> >  
> > -	event->type_len = 0;
> > +	if (length > RB_MAX_SMALL_DATA) {
> > +		event->type_len = 0;
> > +		event->array[0] = array_size;
> > +	} else {
> > +		event->type_len = DIV_ROUND_UP(array_size, RB_ALIGNMENT);
> > +	}
> 
> [Severity: High]
> Can this unconditional compression cause payload truncation on architectures
> requiring 8-byte alignment?
> 
> If RB_FORCE_8BYTE_ALIGNMENT is set, it seems the core ring buffer reader
> (ring_buffer_event_length) might subsequently subtract 4 bytes erroneously,
> leading to data loss and potential out-of-bounds reads when parsing the event.
> 
> >  	event->time_delta = time_delta;
> > -	event->array[0] = event_size - RB_EVNT_HDR_SIZE;
> >  
> >  	return event;
> >  }
> > @@ -284,7 +299,7 @@ void *simple_ring_buffer_reserve(struct simple_rb_per_cpu *cpu_buffer, unsigned
> >  
> >  	rb_event = simple_rb_reserve_next(cpu_buffer, length, timestamp);
> >  
> > -	return &rb_event->array[1];
> > +	return rb_event->type_len ? &rb_event->array[0] : &rb_event->array[1];
> 
> [Severity: High]
> This is a pre-existing issue, but does simple_ring_buffer_reserve() lack a
> bounds check for events larger than the maximum page capacity?
> 
> If a caller attempts to reserve an event with a length exceeding
> PAGE_SIZE - BUF_PAGE_HDR_SIZE, simple_rb_reserve_next() will move to a new
> page and set the write index beyond the page boundary. This would return a
> pointer that allows the caller to write out of bounds.
> 
> Would it be safer to validate that length is smaller than the maximum buffer
> page data size before proceeding with the reservation?

Right now we just do not consider such huge events as possible.

-- 
Vincent

> 
> >  }
> >  EXPORT_SYMBOL_GPL(simple_ring_buffer_reserve);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5