Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Missing CPU hotplug lock in ring_buffer_subbuf_order_set() causes NULL/poison pointer dereference on concurrent hotplug. -- commit 6a990a7dda41a1ef54e4e559d824ba3e1166dfe2 Author: Vincent Donnefort <[email protected]> ring-buffer: Clean up resize_disabled checks This patch creates a common helper rb_resize_disabled() to refactor duplicate checks in ring_buffer_subbuf_order_set() and ring_buffer_resize(). It also removes an unnecessary cpumask_test_cpu() in ring_buffer_subbuf_order_set(). > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 04bb94c29f587..37801ac5e92e2 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c [ ... ] > @@ -7473,13 +7475,9 @@ 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; > - } > + /* Check it is resizable before we touch subbuf_order */ > + if (rb_resize_disabled(buffer, RING_BUFFER_ALL_CPUS)) > + return -EBUSY; > > atomic_inc(&buffer->record_disabled); [Severity: High] This is a pre-existing issue, but since we are modifying ring_buffer_subbuf_order_set(), is there a missing CPU hotplug lock here that could cause a crash? Unlike ring_buffer_resize() which takes cpus_read_lock(), this function takes buffer->mutex but fails to protect buffer->cpumask against concurrent hotplug modifications. > > @@ -7490,17 +7488,8 @@ int ring_buffer_subbuf_order_set(struct trace_buffer > *buffer, int order) > > /* Make sure all new buffers are allocated, before deleting the old > ones */ > for_each_buffer_cpu(buffer, cpu) { > - > - if (!cpumask_test_cpu(cpu, buffer->cpumask)) > - continue; > - > cpu_buffer = buffer->buffers[cpu]; > > - if (atomic_read(&cpu_buffer->resize_disabled)) { > - err = -EBUSY; > - goto error; > - } > - [Severity: High] This is a pre-existing issue related to the missing hotplug lock above. In this first loop of ring_buffer_subbuf_order_set(), new pages are allocated only for CPUs present in the mask at this point: for_each_buffer_cpu(buffer, cpu) { ... /* Allocate the new size buffer */ INIT_LIST_HEAD(&cpu_buffer->new_pages); if (__rb_allocate_pages(cpu_buffer, nr_pages, &cpu_buffer->new_pages)) { ... } } Later, the second loop iterates over the cpumask again: for_each_buffer_cpu(buffer, cpu) { ... /* 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); If the cpumask expands between the two loops due to concurrent hotplug, the second loop will process the newly added CPU. However, for that new CPU, the new_pages list was not populated by the first loop. Instead, trace_rb_cpu_prepare() will have left it in a LIST_POISON1 state (via list_del()), causing a deterministic kernel crash when dereferencing new_pages.next. Could cpus_read_lock() be added at the beginning of ring_buffer_subbuf_order_set() to protect against this? > /* Update the number of pages to match the new size */ > nr_pages = old_capacity * buffer->buffers[cpu]->nr_pages; > nr_pages = DIV_ROUND_UP(nr_pages, rb_subbuf_capacity(buffer)); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
