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
