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

[sashiko-bot@kernel.org]

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 <vdonnefort@google.com>

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/20260812161340.2417322-1-vdonnefort@google.com?part=5