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: <20250227135240.GB25639@redhat.com>
Date: Thu, 27 Feb 2025 14:52:41 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: "Sapkal, Swapnil" <swapnil.sapkal@....com>,
	Mateusz Guzik <mjguzik@...il.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Manfred Spraul <manfred@...orfullife.com>,
	Christian Brauner <brauner@...nel.org>,
	David Howells <dhowells@...hat.com>,
	WangYuli <wangyuli@...ontech.com>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	K Prateek Nayak <kprateek.nayak@....com>,
	"Shenoy, Gautham Ranjal" <gautham.shenoy@....com>,
	Neeraj.Upadhyay@....com
Subject: Re: [PATCH] pipe_read: don't wake up the writer if the pipe is still
 full

Forgot to mention...

Even if this patch is likely "offtopic", it probably makes sense.
However, it is "incomplete" in that there are other scenarious.

Again, the pipe is full. A writer W1 tries to write 4096 bytes and
sleeps. A writer W2 tries to write 1 byte and sleeps too.

A reader reads 4096 bytes, updates pipe->tail and wakes W1.

Another writer comes, writes 1 byte and "steals" the buffer released
by the reader.

W1 sleeps again, this is correct. But nobody will wake W2 which could
succeed.

This (and the previous more simple scenario) means that

	if (pipe_full(pipe->head, pipe->tail, pipe->max_usage))
		wake_next_writer = false;

before return in anon_pipe_write() is not really right in this sense.

Oleg.

On 02/27, Oleg Nesterov wrote:
>
> Hmm...
> 
> Suppose that pipe is full, a writer W tries to write a single byte
> and sleeps on pipe->wr_wait.
> 
> A reader reads PAGE_SIZE bytes, updates pipe->tail, and wakes W up.
> 
> But, before the woken W takes pipe->mutex, another writer comes and
> writes 1 byte. This updates ->head and makes pipe_full() true again.
> 
> Now, W could happily merge its "small" write into the last buffer,
> but it will sleep again, despite the fact the last buffer has room
> for 4095 bytes.
> 
> Sapkal, I don't think this can explain the hang, receiver()->read()
> should wake this writer later anyway. But could you please retest
> with the patch below?
> 
> Thanks,
> 
> Oleg.
> ---
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index b0641f75b1ba..222881559c30 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -455,6 +455,7 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
>  	 * page-aligns the rest of the writes for large writes
>  	 * spanning multiple pages.
>  	 */
> +again:
>  	head = pipe->head;
>  	was_empty = pipe_empty(head, pipe->tail);
>  	chars = total_len & (PAGE_SIZE-1);
> @@ -559,8 +560,8 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
>  		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
>  		wait_event_interruptible_exclusive(pipe->wr_wait, pipe_writable(pipe));
>  		mutex_lock(&pipe->mutex);
> -		was_empty = pipe_empty(pipe->head, pipe->tail);
>  		wake_next_writer = true;
> +		goto again;
>  	}
>  out:
>  	if (pipe_full(pipe->head, pipe->tail, pipe->max_usage))