Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ring-buffer: duplicate RB_PAGE_HEAD race in rb_handle_head_page() --
commit 06572f261acc9f999487d8e50d79b22b59309351 Author: Breno Leitao <[email protected]> ring-buffer: Handle a reader taking the new head in rb_handle_head_page() This patch handles RB_PAGE_MOVED returned from rb_head_page_set_head() in rb_handle_head_page() when a nested interrupt writer advances the head page and a concurrent reader swaps out new_head before the outer writer completes its second cmpxchg. > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 1fd28a72e6ee1..7a60fa175e2cc 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c > @@ -3789,6 +3789,15 @@ rb_handle_head_page(struct ring_buffer_per_cpu > *cpu_buffer, > case RB_PAGE_NORMAL: > /* OK */ > break; > + case RB_PAGE_MOVED: > + if (type == RB_PAGE_HEAD) { > + ret = rb_head_page_set_normal(cpu_buffer, next_page, > + tail_page, > + RB_PAGE_UPDATE); > + if (RB_WARN_ON(cpu_buffer, ret != RB_PAGE_UPDATE)) > + return -1; > + } > + return 1; [Severity: High] Can the same race with a nested writer and concurrent reader leave two pages in the ring buffer with RB_PAGE_HEAD set simultaneously if the reader swap happens before new_head is loaded, or when an interrupting writer sees type == RB_PAGE_UPDATE? In kernel/trace/ring_buffer.c:rb_handle_head_page(), the outer writer sets tail_page->list.next to RB_PAGE_UPDATE before computing new_head: type = rb_head_page_set_update(cpu_buffer, next_page, tail_page, RB_PAGE_HEAD); switch (type) { case RB_PAGE_HEAD: local_add(entries, &cpu_buffer->overrun); local_sub(rb_page_commit(next_page), &cpu_buffer->entries_bytes); local_inc(&cpu_buffer->pages_lost); if (cpu_buffer->ring_meta) rb_update_meta_head(cpu_buffer, next_page); break; ... } new_head = next_page; rb_inc_page(&new_head); ret = rb_head_page_set_head(cpu_buffer, new_head, next_page, RB_PAGE_NORMAL); Once rb_head_page_set_update() sets tail_page->list.next to RB_PAGE_UPDATE, if the nested interrupt writer and concurrent reader swap happen before rb_inc_page(&new_head) reads next_page->list.next: 1. The nested interrupt writer sees RB_PAGE_UPDATE, sets next_page->list.next (pointing to the old new_head) to RB_PAGE_HEAD, and advances cpu_buffer->tail_page from tail_page to next_page in rb_move_tail() -> rb_tail_page_update(). 2. The concurrent reader in rb_get_reader_page() -> __rb_get_reader_page() swaps the old new_head out for its empty reader_page, setting reader_page->list.next to the following page with RB_PAGE_HEAD and setting next_page->list.next to &reader_page->list with RB_PAGE_NORMAL. 3. The outer writer resumes and executes rb_inc_page(&new_head), which loads the newly spliced reader_page from next_page->list.next. 4. rb_head_page_set_head() then matches &reader_page->list | RB_PAGE_NORMAL, sets next_page->list.next to &reader_page->list | RB_PAGE_HEAD, and returns RB_PAGE_NORMAL instead of RB_PAGE_MOVED. 5. The subsequent tail check does not clear the extra RB_PAGE_HEAD flag because the single nested interrupt only moved cpu_buffer->tail_page to next_page: if (ret == RB_PAGE_NORMAL) { struct buffer_page *buffer_tail_page; buffer_tail_page = READ_ONCE(cpu_buffer->tail_page); if (buffer_tail_page != tail_page && buffer_tail_page != next_page) rb_head_page_set_normal(cpu_buffer, new_head, next_page, RB_PAGE_HEAD); } A similar sequence also appears possible when the outer writer sets next_page->list.next to RB_PAGE_HEAD, the reader swaps that page out for reader_page, and an interrupting writer arrives before the outer writer updates cpu_buffer->tail_page. The interrupting writer enters rb_handle_head_page() with type == RB_PAGE_UPDATE (or hits the new RB_PAGE_MOVED path and returns 1 without advancing cpu_buffer->tail_page, immediately retrying on the same tail_page), loads new_head as the spliced reader_page, and marks next_page->list.next as RB_PAGE_HEAD while buffer_tail_page is still tail_page. In both cases, wouldn't both next_page->list.next and reader_page->list.next be left marked with RB_PAGE_HEAD, corrupting the ring buffer list state and triggering RB_WARN_ON() in rb_set_head_page() or __rb_get_reader_page()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
