linux-kernel - Re: [PATCH] pipe_read: don't wake up the writer if the pipe is still full

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250310233350.3301-1-hdanton@sina.com>
Date: Tue, 11 Mar 2025 07:33:49 +0800
From: Hillf Danton <hdanton@...a.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: K Prateek Nayak <kprateek.nayak@....com>,
	Mateusz Guzik <mjguzik@...il.com>,
	"Sapkal, Swapnil" <swapnil.sapkal@....com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pipe_read: don't wake up the writer if the pipe is still full

On Mon, 10 Mar 2025 13:43:42 +0100 Oleg Nesterov
> On 03/10, Hillf Danton wrote:
> > On Mon, 10 Mar 2025 12:09:15 +0100 Oleg Nesterov
> > > On 03/10, Hillf Danton wrote:
> > > > On Sun, 9 Mar 2025 18:02:55 +0100 Oleg Nesterov
> > > > >
> > > > > So (again, in this particular case) we could apply the patch below
> > > > > on top of Linus's tree.
> > > > >
> > > > > So, with or without these changes, the writer should be woken up at
> > > > > step-03 in your scenario.
> > > > >
> > > > Fine, before checking my scenario once more, feel free to pinpoint the
> > > > line number where writer is woken up, with the change below applied.
> > >
> > >     381          if (wake_writer)
> > > ==> 382                  wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM);
> > >     383          if (wake_next_reader)
> > >     384                  wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM);
> > >     385          kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
> > >     386          if (ret > 0)
> > >     387                  file_accessed(filp);
> > >     388          return ret;
> > >
> > > line 382, no?
> > >
> > Yes, but how is the wait loop at line-370 broken?
> >
> >  360                 }
> >  361                 mutex_unlock(&pipe->mutex);
> >  362
> >  363                 BUG_ON(wake_writer);
> >  364                 /*
> >  365                  * But because we didn't read anything, at this point we can
> >  366                  * just return directly with -ERESTARTSYS if we're interrupted,
> >  367                  * since we've done any required wakeups and there's no need
> >  368                  * to mark anything accessed. And we've dropped the lock.
> >  369                  */
> >  370                 if (wait_event_interruptible_exclusive(pipe->rd_wait, pipe_readable(pipe)) < 0)
> >  371                         return -ERESTARTSYS;
> 
> Hmm. I don't understand you, again.
> 
> OK, once some writer writes at least one byte (this will make the
> pipe_empty() condition false) and wakes this reader up.
> 
> If you meant something else, say, if you referred to you previous
> scenario, please clarify your question.
> 
The step-03 in my scenario [1] shows a reader sleeps at line-370 after
making the pipe empty, so after your change that cuts the chance for
waking up writer, who will wake up the sleeping reader? Nobody.

Feel free to check my scenario again.

step-03
	task-118766 new reader
	makes pipe empty
	sleeps

[1] https://lore.kernel.org/lkml/20250307060827.3083-1-hdanton@sina.com/