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. > + > + 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? > } > EXPORT_SYMBOL_GPL(simple_ring_buffer_reserve); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
