The first point does not hold. trace_rb_cpu_prepare() allocates
buffer->buffers[cpu] and IPIs via rb_cpu_sync() before setting the cpumask
bit, so observing the bit implies observing the pointer.
owever, The second is real but predates this patch:
ring_buffer_subbuf_order_set()
never took cpus_read_lock(), we can fix as follow :
--- a/kernel/trace/ring_buffer.c
(/home/dcarlier/Contribs/linux-trace/kernel/trace/ring_buffer.c)
+++ b/kernel/trace/ring_buffer.c
@@ -7465,6 +7465,12 @@ int ring_buffer_subbuf_order_set(struct
trace_buffer *buffer, int order)
if (psize > RB_WRITE_MASK + 1)
return -EINVAL;
+ /*
+ * Keep CPUs from coming online while changing the order to
+ * synchronize with new per CPU buffers being created.
+ */
+ guard(cpus_read_lock)();
+
/* prevent another thread from changing buffer sizes */
guard(mutex)(&buffer->mutex);
Cheers.
On Sat, 12 Sept 2026 at 11:56, <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] Missing cpus_read_lock() allows concurrent CPU hotplug events to
> crash ring_buffer_subbuf_order_set() via uninitialized list dereference or
> NULL pointer dereference.
> --
>
> commit d554518f86e022de0069fc76033b9d71e9046148
> Author: David Carlier <[email protected]>
>
> ring-buffer: Check resize_disabled before publishing the new subbuf order
>
> This commit checks if resizing is disabled on any CPU before applying
> the new sub-buffer order. This aims to prevent a race condition with
> mmap fast paths mapping already-mapped CPUs with the new order.
>
> Cc: [email protected]
>
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 9c03a555a6ba2..d7e5e4620d096 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -7474,6 +7474,14 @@ int ring_buffer_subbuf_order_set(struct trace_buffer
> > *buffer, int order)
> >
> > old_capacity = rb_subbuf_capacity(buffer);
> >
> > + /* The mmap fast path reads subbuf_order without buffer->mutex. */
> > + for_each_buffer_cpu(buffer, cpu) {
> > + if (!cpumask_test_cpu(cpu, buffer->cpumask))
> > + continue;
> > + if (atomic_read(&buffer->buffers[cpu]->resize_disabled))
> > + return -EBUSY;
> > + }
> > +
>
> [Severity: High]
> Can this new loop race with concurrent CPU hotplug events and trigger a NULL
> pointer dereference?
>
> Unlike ring_buffer_resize(), ring_buffer_subbuf_order_set() doesn't appear
> to take cpus_read_lock(). Meanwhile, trace_rb_cpu_prepare() can update
> buffer->cpumask without holding buffer->mutex.
>
> On weakly ordered architectures, could the lack of an smp_rmb() here cause
> the reader to observe a newly added CPU in buffer->cpumask but load a NULL
> buffer->buffers[cpu]?
>
> Additionally, without cpus_read_lock(), if a CPU is added to the cpumask
> between the allocation loop and the final page update loop later in this
> function, new_pages will be an empty list head:
>
> ring_buffer_subbuf_order_set() {
> ...
> /* One page was allocated for the reader page */
> cpu_buffer->reader_page = list_entry(cpu_buffer->new_pages.next,
> struct buffer_page, list);
> list_del_init(&cpu_buffer->reader_page->list);
> ...
> }
>
> Could this calculate a fake pointer overlapping the cpu_buffer struct and
> corrupt memory when modified?
>
> > atomic_inc(&buffer->record_disabled);
> >
> > /* Make sure all commits have finished */
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=1
