[PATCH] pipe: keep per-write wakeups for EPOLLEXCLUSIVE consumers

[PATCH] pipe: keep per-write wakeups for EPOLLEXCLUSIVE consumers

[Mani Tofigh via B4 Relay]

From: Mani Tofigh <mt4022@columbia.edu>

A task blocked in read() on a pipe can sleep while the pipe holds data,
when an epoll(EPOLLEXCLUSIVE) watcher of the read end was added before
it.  The watcher's entry on ->rd_wait is exclusive and sits ahead of the
reader, so a write into an empty pipe wakes only the watcher.  The
watcher reads without sleeping in anon_pipe_read() and doesn't pass the
wakeup on.  Since commit f1cebdec9739 ("pipe: only enable the extra
wake_up(rd_wait) for EPOLLET consumers") the next writes into the
non-empty pipe don't wake anyone anymore:

  Task E                   Task A                 Task C
  epoll_wait(ep, -1)
    /* EPOLLEXCLUSIVE */
                           read(p[0])
                                                  write(p[1], "1")
                                                    /* wakes E only */
                                                  write(p[1], "2")
                                                    /* no wakeup */
  read(p[0], 1 byte)
  exit()
                           /* still asleep, "2" is in the pipe */

A then sleeps until the next write into an empty pipe, if ever.  Before
that commit any pipe_poll() enabled the per-write wakeups.  Those
reached A because an EPOLLEXCLUSIVE entry only takes a wakeup while a
task sleeps in epoll_wait().

So EPOLLEXCLUSIVE consumers depend on the per-write wakeups too, for a
different reason than EPOLLET.  Enable them for EPOLLEXCLUSIVE as well
and say why in the comment.  With the fix, A reads the second byte.

This bug was identified and confirmed with the help of a deterministic
hypervisor currently under development.  The C reproducer below however
hits it within about 2s.

Fixes: f1cebdec9739 ("pipe: only enable the extra wake_up(rd_wait) for EPOLLET consumers")
Cc: stable@vger.kernel.org
Signed-off-by: Mani Tofigh <mt4022@columbia.edu>
---
Reproducer: https://github.com/manitofigh/files/blob/main/linux-repros/pipeexcl.c

There are a couple of older gaps like this (a single write, and the
write side with EPOLLOUT|EPOLLEXCLUSIVE).  I left both alone here though.
---
 fs/pipe.c                 | 6 +++++-
 include/linux/pipe_fs_i.h | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 4c1122597a3c..8ad792d300c9 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -700,9 +700,13 @@ pipe_poll(struct file *filp, poll_table *wait)
 	 * and the ->pseudo_edgetrigger check in anon_pipe_write().
 	 * Currently io_uring sets EPOLLET for multishot polls, so it gets the
 	 * same behaviour.
+	 *
+	 * EPOLLEXCLUSIVE needs them too.  Its entry is an exclusive waiter on
+	 * ->rd_wait that can take the wakeup meant for a blocked reader, and
+	 * unlike a woken reader in anon_pipe_read() it doesn't pass it on.
 	 */
 	if ((filp->f_mode & FMODE_READ) &&
-	    wait && (wait->_key & EPOLLET) &&
+	    wait && (wait->_key & (EPOLLET | EPOLLEXCLUSIVE)) &&
 	    unlikely(!READ_ONCE(pipe->pseudo_edgetrigger)))
 		WRITE_ONCE(pipe->pseudo_edgetrigger, true);
 
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 95f6236c5c5b..6b2e14ec6ef8 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -90,7 +90,7 @@ struct anon_pipe_prealloc {
  *	@files: number of struct file referring this pipe (protected by ->i_lock)
  *	@r_counter: reader counter
  *	@w_counter: writer counter
- *	@pseudo_edgetrigger: has an EPOLLET consumer, enable per-write wakeups
+ *	@pseudo_edgetrigger: enable per-write wakeups for EPOLLET/EPOLLEXCLUSIVE
  *	@fasync_readers: reader side fasync
  *	@fasync_writers: writer side fasync
  *	@bufs: the circular array of pipe buffers

---
base-commit: b0993210e420f2f1d61e6719fbcd3966b96ff9ed
change-id: 20261111-pipe-epollexclusive-wakeup-981528041b4d

Best regards,
--  
Mani Tofigh <mt4022@columbia.edu>

Re: [PATCH] pipe: keep per-write wakeups for EPOLLEXCLUSIVE consumers

[Oleg Nesterov]

On 11/11, Mani Tofigh via B4 Relay wrote:
>
> From: Mani Tofigh <mt4022@columbia.edu>
>
> A task blocked in read() on a pipe can sleep while the pipe holds data,
> when an epoll(EPOLLEXCLUSIVE) watcher of the read end was added before
> it.  The watcher's entry on ->rd_wait is exclusive and sits ahead of the
> reader, so a write into an empty pipe wakes only the watcher.

I am already sleeping, but at first glance this is already wrong?

I mean, a WQ_FLAG_EXCLUSIVE waiter should not sit ahead of non-exclusive
waiter, no?

Oleg.

Re: [PATCH] pipe: keep per-write wakeups for EPOLLEXCLUSIVE consumers

[Oleg Nesterov]

On 11/12, Oleg Nesterov wrote:
>
> On 11/11, Mani Tofigh via B4 Relay wrote:
> >
> > From: Mani Tofigh <mt4022@columbia.edu>
> >
> > A task blocked in read() on a pipe can sleep while the pipe holds data,
> > when an epoll(EPOLLEXCLUSIVE) watcher of the read end was added before
> > it.  The watcher's entry on ->rd_wait is exclusive and sits ahead of the
> > reader, so a write into an empty pipe wakes only the watcher.
>
> I am already sleeping, but at first glance this is already wrong?
>
> I mean, a WQ_FLAG_EXCLUSIVE waiter should not sit ahead of non-exclusive
> waiter, no?

Ah, I forgot that anon_pipe_read() uses wait_event_exclusive(), and it
assumes that this wait-event is the only reason to set wake_next_reader...

I seem to understand, let me think about it.

Oleg.

Re: [PATCH] pipe: keep per-write wakeups for EPOLLEXCLUSIVE consumers

[Oleg Nesterov]

This probably needs more discussion... I will write another email.
But so far I think we don't really care.

On 11/11, Mani Tofigh via B4 Relay wrote:
>
> A task blocked in read() on a pipe can sleep while the pipe holds data,
> when an epoll(EPOLLEXCLUSIVE) watcher of the read end was added before
> it.  The watcher's entry on ->rd_wait is exclusive and sits ahead of the
> reader, so a write into an empty pipe wakes only the watcher.  The
> watcher reads without sleeping in anon_pipe_read() and doesn't pass the
> wakeup on.  Since commit f1cebdec9739 ("pipe: only enable the extra
> wake_up(rd_wait) for EPOLLET consumers") the next writes into the
> non-empty pipe don't wake anyone anymore:
>
>   Task E                   Task A                 Task C
>   epoll_wait(ep, -1)
>     /* EPOLLEXCLUSIVE */
>                            read(p[0])
>                                                   write(p[1], "1")
>                                                     /* wakes E only */
>                                                   write(p[1], "2")
>                                                     /* no wakeup */
>   read(p[0], 1 byte)
>   exit()
>                            /* still asleep, "2" is in the pipe */

Yes, in this case f1cebdec9739 ("pipe: only enable the extra wake_up(rd_wait)
for EPOLLET consumers") makes a difference.

However, this trivial test-case

	#include <unistd.h>
	#include <sys/epoll.h>
	#include <sys/wait.h>
	#include <assert.h>
	#include <stdio.h>

	int main(void)
	{
		int pfd[2], efd;
		struct epoll_event evt = { .events = EPOLLIN | EPOLLEXCLUSIVE };
		char c;

		pipe(pfd);
		efd = epoll_create1(0);
		epoll_ctl(efd, EPOLL_CTL_ADD, pfd[0], &evt);

		if (!fork()) {
			epoll_wait(efd, &evt, 1, -1);
			read(pfd[0], &c, 1);
			printf("1st: %c\n", c);
			return 0;
		}

		sleep(1);

		if (!fork()) {
			read(pfd[0], &c, 1);
			printf("2nd: %c\n", c);
			return 0;
		}

		sleep(1);

		write(pfd[1], "12", 2);
		assert(wait(NULL) > 0);
		assert(wait(NULL) > 0);

		return 0;
	}

hangs even with this patch

	+++ b/fs/pipe.c
	@@ -749,6 +749,7 @@ pipe_poll(struct file *filp, poll_table *wait)
		    unlikely(!READ_ONCE(pipe->pseudo_edgetrigger)))
			WRITE_ONCE(pipe->pseudo_edgetrigger, true);
	 
	+WRITE_ONCE(pipe->pseudo_edgetrigger, true);
		/*
		 * Reading pipe state only -- no need for acquiring the semaphore.
		 *

which reverts f1cebdec9739.

The 2nd child sleeps in read(pfd[0]) forever even if the pipe is not empty.
So I am not sure we want to enforce another oddity in pipe_poll/read ...

Oleg.