linux-kernel - [RFC] pipe: prevent compiler reordering in pipe

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-next>] [day] [month] [year] [list]

Message-ID: <20180824225431.tpaxuck7idgnj3b7@dcvr>
Date:   Fri, 24 Aug 2018 22:54:31 +0000
From:   Eric Wong <normalperson@...t.net>
To:     Al Viro <viro@...IV.linux.org.uk>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: [RFC] pipe: prevent compiler reordering in pipe_poll

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@...t.net>
Cc: Paolo Bonzini <pbonzini@...hat.com>
---
 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