linux-kernel - Re: [PATCH v2 1/2] ring-buffer: Fix full_waiters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240312113254.78455352@gandalf.local.home>
Date: Tue, 12 Mar 2024 11:32:54 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, Mark
 Rutland <mark.rutland@....com>, Mathieu Desnoyers
 <mathieu.desnoyers@...icios.com>, Andrew Morton
 <akpm@...ux-foundation.org>, stable@...r.kernel.org
Subject: Re: [PATCH v2 1/2] ring-buffer: Fix full_waiters_pending in poll

On Wed, 13 Mar 2024 00:22:10 +0900
Masami Hiramatsu (Google) <mhiramat@...nel.org> wrote:

> On Tue, 12 Mar 2024 09:19:20 -0400
> Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> > From: "Steven Rostedt (Google)" <rostedt@...dmis.org>
> > 
> > If a reader of the ring buffer is doing a poll, and waiting for the ring
> > buffer to hit a specific watermark, there could be a case where it gets
> > into an infinite ping-pong loop.
> > 
> > The poll code has:
> > 
> >   rbwork->full_waiters_pending = true;
> >   if (!cpu_buffer->shortest_full ||
> >       cpu_buffer->shortest_full > full)
> >          cpu_buffer->shortest_full = full;
> > 
> > The writer will see full_waiters_pending and check if the ring buffer is
> > filled over the percentage of the shortest_full value. If it is, it calls
> > an irq_work to wake up all the waiters.
> > 
> > But the code could get into a circular loop:
> > 
> > 	CPU 0					CPU 1
> > 	-----					-----
> >  [ Poll ]
> >    [ shortest_full = 0 ]
> >    rbwork->full_waiters_pending = true;
> > 					  if (rbwork->full_waiters_pending &&
> > 					      [ buffer percent ] > shortest_full) {
> > 					         rbwork->wakeup_full = true;
> > 					         [ queue_irqwork ]  
> 
> Oh, so `[ buffer percent ] > shortest_full` does not work because
> if this happens in this order, shortest_full may be 0.

Exactly!

> 
> > 
> >    cpu_buffer->shortest_full = full;
> > 
> > 					  [ IRQ work ]
> > 					  if (rbwork->wakeup_full) {
> > 					        cpu_buffer->shortest_full = 0;

And here shortest_full gets set back to zero! (But that's not the bug).

> > 					        wakeup poll waiters;
> >   [woken]
> >    if ([ buffer percent ] > full)
> >       break;
> >    rbwork->full_waiters_pending = true;

The bug is setting full_waiters_pending before updating the shortest_full.

> > 					  if (rbwork->full_waiters_pending &&
> > 					      [ buffer percent ] > shortest_full) {
> > 					         rbwork->wakeup_full = true;
> > 					         [ queue_irqwork ]
> > 
> >    cpu_buffer->shortest_full = full;
> > 
> > 					  [ IRQ work ]
> > 					  if (rbwork->wakeup_full) {
> > 					        cpu_buffer->shortest_full = 0;
> > 					        wakeup poll waiters;
> >   [woken]
> > 
> >  [ Wash, rinse, repeat! ]
> > 
> > In the poll, the shortest_full needs to be set before the
> > full_pending_waiters, as once that is set, the writer will compare the
> > current shortest_full (which is incorrect) to decide to call the irq_work,
> > which will reset the shortest_full (expecting the readers to update it).
> > 
> > Also move the setting of full_waiters_pending after the check if the ring
> > buffer has the required percentage filled. There's no reason to tell the
> > writer to wake up waiters if there are no waiters.
> >   
> 
> Looks good to me.
> 
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>

Thanks!

I'm running it through my tests and when they finish, I'll be posting the
for-linus patches.

-- Steve