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: <CAGudoHHKf_FXrrNJQCqvC50QSV87u+8sRaPQwm6rWvPeirO2_A@mail.gmail.com>
Date: Thu, 27 Feb 2025 16:59:59 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: "Sapkal, Swapnil" <swapnil.sapkal@....com>, Linus Torvalds <torvalds@...ux-foundation.org>, 
	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

On Thu, Feb 27, 2025 at 1:51 PM Oleg Nesterov <oleg@...hat.com> 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))
>

I think this is buggy.

You get wakeups also when the last reader goes away. The for loop you
are jumping out of makes sure to check for the condition, same for the
first mutex acquire. With this goto you can get a successful write
instead of getting SIGPIPE. iow this should goto few lines higher.

I am not sure about the return value. The for loop bumps ret with each
write, but the section you are jumping to overwrites it. So if the
thread wrote some data within the loop, went to sleep and woke up to a
state where it can do a write in the section you are jumping to, it is
going to return the wrong number of bytes.

Unless I'm misreading something.

However, I do think something may be going on with the "split" ops,
which is why I suggested going from 100 bytes where the bug was
encountered to 128 for testing purposes. If that cleared it, that
would be nice for sure. :>

-- 
Mateusz Guzik <mjguzik gmail.com>