Re: [PATCH] ring-buffer: Fix false warning in ring_buffer_map_get_reader()

[Krystian Kaniewski]

On 9/21/2026 10:12 AM, Vincent Donnefort wrote:
> On Fri, Sep 18, 2026 at 02:34:25PM +0000, syzbot wrote:
> > From: Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx>
> >
> > In ring_buffer_map_get_reader(), an unconditional WARN_ON(!reader) is
> > triggered when rb_get_reader_page() returns NULL:
> >
> > WARNING: CPU: 1 PID: 5906 at kernel/trace/ring_buffer.c:7998
> > ring_buffer_map_get_reader+0x940/0x9d0
> > CPU: 1 UID: 0 PID: 5906 Comm: task Not tainted
> > RIP: 0010:ring_buffer_map_get_reader+0x940/0x9d0
> > kernel/trace/ring_buffer.c:7998
> > Call Trace:
> >   <TASK>
> >   tracing_buffers_ioctl+0x258/0x300 kernel/trace/trace.c:7381
> >   __se_sys_ioctl+0xfc/0x170 fs/ioctl.c:583
> >   do_syscall_64+0x166/0x520 arch/x86/entry/syscall_64.c:84
> >   entry_SYSCALL_64_after_hwframe+0x77/0x7f
> >   </TASK>
> >
> > This warning is triggered due to a race between a writer committing events
> > and a reader mapping the ring buffer via TRACE_MMAP_IOCTL_GET_READER. When
> > a writer commits an event in rb_set_commit_to_write(), it advances
> > cpu_buffer->commit_page to cpu_buffer->tail_page in its first loop before
> > updating the commit counter (commit_page->page->commit) in the second loop.
> > If a reader invokes ring_buffer_map_get_reader() at this moment, the
> > initial check cpu_buffer->reader_page == cpu_buffer->commit_page is false,
> > and it calls rb_get_reader_page(). Inside __rb_get_reader_page(), the
> > reader swaps reader_page with the head page (which is the new commit_page).
> > Because the writer has not yet updated the commit count on the new page,
> > rb_page_size() is zero and reader_page->read < rb_page_size() evaluates to
> > false. __rb_get_reader_page() then checks if cpu_buffer->commit_page ==
> > cpu_buffer->reader_page. Since both now point to the swapped page, the
> > condition evaluates to true and rb_get_reader_page() legitimately returns
> > NULL to indicate the reader caught up to the writer.
>
> This seems to make the check above reader_page == commit_page redundant. Doesn't
> it?
>
> > Furthermore, rb_get_reader_page() can legitimately return NULL when there
> > is no data to read. Re-checking mutable writer state such as
>
> We are checking rb_per_cpu_empty() with the reader_lock held few lines above. I
> don't believe we expect NULL here for that reason.
>
> > cpu_buffer->reader_page != cpu_buffer->commit_page is insufficient because
> > a writer on another CPU can advance commit_page before the check is
> > evaluated without being stopped by reader_lock.
> >
> > Because WARN_ON must not be used for conditions that can legitimately
> > happen, and pr_err should be used instead if necessary, remove the
> > WARN_ON() entirely since returning NULL here is an expected condition.
> >
> > Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions")
> > Assisted-by: Gemini:gemini-3.8-flash syzbot
> > Reported-by: syzbot+de3d7f9bcc9212f3fae1@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=de3d7f9bcc9212f3fae1
> > Link: https://syzkaller.appspot.com/ai_job?id=488adc18-a10e-42d2-a205-25327c38837f
> > Signed-off-by: Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx>
> >
> > ---
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 9c03a555a..1a4da3d47 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -7995,7 +7995,7 @@ int ring_buffer_map_get_reader(struct trace_buffer *buffer, int cpu)
> >   		goto out;
> >   
> >   	reader = rb_get_reader_page(cpu_buffer);
> > -	if (WARN_ON(!reader))
> > +	if (!reader)
> >   		goto out;
> >   
> >   	/* Check if any events were dropped */
> >
> >
> > base-commit: df2908090cda368b01ff43709f51890076c56157
> > --
> > See https://goo.gle/syzbot-ai-patches for information about AI-generated patches.
> > The person who has signed off on the patch is responsible for
> > addressing comments.
> > syzbot engineers can be reached at syzkaller@xxxxxxxxxxxxxxxx.

Thanks for taking a look into my patch!

>
> This seems to make the check above reader_page == commit_page 
redundant. Doesn't
> it?
>

Yes, it is no longer needed for correctness once a NULL result is 
handled without a warning. It remains a fast path when the reader has 
already caught up, avoiding a call to rb_get_reader_page(). I can remove 
it if you prefer in v2 patch.

>>
>> Furthermore, rb_get_reader_page() can legitimately return NULL when 
there
>> is no data to read. Re-checking mutable writer state such as
>
> We are checking rb_per_cpu_empty() with the reader_lock held few 
lines above. I
> don't believe we expect NULL here for that reason.
>

Yes, you are right, an empty result exits before the helper. But 
reader_lock does not stop the writer and a nonempty entry count does not 
guarantee a committed page. After swapping the reader page, the helper 
can catch up to commit_page and return NULL. I can clarify the commit 
message as part of v2 patch if needed.