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
