From: Paolo Bonzini <pbonzini@redhat.com>
To: Eric Wong <normalperson@yhbt.net>, Al Viro <viro@ZenIV.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC] pipe: prevent compiler reordering in pipe_poll
Date: Mon, 10 Sep 2018 10:55:47 +0200 [thread overview]
Message-ID: <817dd145-fba5-d5e8-26b8-746b9bab4dd9@redhat.com> (raw)
In-Reply-To: <20180824225431.tpaxuck7idgnj3b7@dcvr>
On 25/08/2018 00:54, Eric Wong wrote:
> 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.
The documentation change can be useful, but if you add a compiler
barrier you should also mention why reordering at the processor level is
okay. In this case, processor-level reordering is okay because (just
like in fs/eventfd.c) poll_wait acts as an acquire barrier.
*However* I would be surprised if the scenario (even the one in
fs/eventfd.c) can actually happen, and I don't think the compiler
barrier is useful; there's no reason why the compiler should think that
it can hoist the reads above poll_wait.
In fact, there is a big difference between READ_ONCE() and barrier() for
whoever reads the code, which makes the code after your patch worse than
before. READ_ONCE() means "I know I am accessing this variable outside
a lock". barrier() means one of two things: 1) "I know what I am doing
can trick the compiler, and I don't want that to happen"; 2) "I am
synchronizing against other things happening on this CPU" such as
interrupts. In this case you are not doing any of the two.
Paolo
> Signed-off-by: Eric Wong <normalperson@yhbt.net>
> Cc: Paolo Bonzini <pbonzini@redhat.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) {
>
prev parent reply other threads:[~2018-09-10 13:48 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-24 22:54 [RFC] pipe: prevent compiler reordering in pipe_poll Eric Wong
2018-08-24 23:05 ` Al Viro
2018-09-10 8:55 ` Paolo Bonzini [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=817dd145-fba5-d5e8-26b8-746b9bab4dd9@redhat.com \
--to=pbonzini@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=normalperson@yhbt.net \
--cc=viro@ZenIV.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).