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: <20250304123457.GA25281@redhat.com>
Date: Tue, 4 Mar 2025 13:34:57 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: Hillf Danton <hdanton@...a.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 03/04, Hillf Danton wrote:
>
> On Tue, 4 Mar 2025 11:05:57 +0530 K Prateek Nayak <kprateek.nayak@....com>
> >> @@ -573,11 +573,13 @@ pipe_write(struct kiocb *iocb, struct io
> >>   		 * after waiting we need to re-check whether the pipe
> >>   		 * become empty while we dropped the lock.
> >>   		 */
> >> +		tail = pipe->tail;
> >>   		mutex_unlock(&pipe->mutex);
> >>   		if (was_empty)
> >>   			wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM);
> >>   		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
> >> -		wait_event_interruptible_exclusive(pipe->wr_wait, pipe_writable(pipe));
> >> +		wait_event_interruptible_exclusive(pipe->wr_wait,
> >> +				!READ_ONCE(pipe->readers) || tail != READ_ONCE(pipe->tail));
> >
> >That could work too for the case highlighted but in case the head too
> >has moved by the time the writer wakes up, it'll lead to an extra
> >wakeup.
> >
> Note wakeup can occur even if pipe is full,

Perhaps I misunderstood you, but I don't think pipe_read() can ever do
wake_up(pipe->wr_wait) if pipe is full...

> 		 * So we still need to wake up any pending writers in the
> 		 * _very_ unlikely case that the pipe was full, but we got
> 		 * no data.
> 		 */

Only if wake_writer is true,

		if (unlikely(wake_writer))
			wake_up_interruptible_sync_poll(...);

and in this case the pipe is no longer full. A zero-sized buffer was
removed.

Of course this pipe can be full again when the woken writer checks the
condition, but this is another story. And in this case, with your
proposed change, the woken writer will take pipe->mutex for no reason.

Note also that the comment and code above was already removed by
https://lore.kernel.org/all/20250210114039.GA3588@redhat.com/

Oleg.