The pipe_poll function does not use locks, and adding an entry
to the waitqueue is not guaranteed to happen before pipe->nrbufs
(or other fields) are read, leading to missed wakeups.
Looking at Ruby CI build logs and backtraces, I've noticed
occasional instances where processes are stuck in select(2) or
ppoll(2) with a pipe.
I don't have access to the systems where this is happening to
test/reproduce the problem, and haven't been able to reproduce
it locally on less-powerful hardware, either. However, it seems
like a problem based on similar comments in
fs/eventfd.c::eventfd_poll made by Paolo.
Signed-off-by: Eric Wong <normalperson@xxxxxxxx>
Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
---
fs/pipe.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/fs/pipe.c b/fs/pipe.c
index 39d6f431da83..1a904d941cf1 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -509,7 +509,7 @@ static long pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
}
}
-/* No kernel lock held - fine */
+/* No kernel lock held - fine, but a compiler barrier is required */
static __poll_t
pipe_poll(struct file *filp, poll_table *wait)
{
@@ -519,7 +519,35 @@ pipe_poll(struct file *filp, poll_table *wait)
poll_wait(filp, &pipe->wait, wait);
- /* Reading only -- no need for acquiring the semaphore. */
+ /*
+ * Reading only -- no need for acquiring the semaphore, but
+ * we need a compiler barrier to ensure the compiler does
+ * not reorder reads to pipe->nrbufs, pipe->writers,
+ * pipe->readers, filp->f_version, pipe->w_counter, and
+ * pipe->buffers before poll_wait to avoid missing wakeups
+ * from compiler reordering. In other words, we need to
+ * prevent the following situation:
+ *
+ * pipe_poll pipe_write
+ * ----------------- ------------
+ * nrbufs = pipe->nrbufs (INVALID!)
+ *
+ * __pipe_lock
+ * pipe->nrbufs = ++bufs;
+ * __pipe_unlock
+ * wake_up_interruptible_sync_poll
+ * pipe->wait is empty, no wakeup
+ *
+ * lock pipe->wait.lock (in poll_wait)
+ * __add_wait_queue
+ * unlock pipe->wait.lock
+ *
+ * // pipe->nrbufs should be read here, NOT above
+ *
+ * pipe_poll returns 0 (WRONG)
+ */
+ barrier();
+
nrbufs = pipe->nrbufs;
mask = 0;
if (filp->f_mode & FMODE_READ) {
--
EW
