poll_wait: add mb() to fix theoretical race between waitqueue_active() and .poll()

Linus,

I misread fs/eventpoll.c, it has the same problem. And more __pollwait()-like functions, for example p9_pollwait(). So 1/5 adds mb() into poll_wait(), not into __pollwait().

WangYuli, after 1/5 we can reconsider your patch.

Oleg.
---

 include/linux/poll.h | 26 ++++++++++++--------------
 include/net/sock.h   | 17 +++++++----------
 io_uring/io_uring.c  |  9 ++++-----
 3 files changed, 23 insertions(+), 29 deletions(-)

As the comment above waitqueue_active() explains, it can only be used if both waker and waiter have mb()'s that pair with each other. However __pollwait() is broken in this respect.

This is not pipe-specific, but let's look at pipe_poll() for example:

	poll_wait(...); // -> __pollwait() -> add_wait_queue()

	LOAD(pipe->head);
	LOAD(pipe->head);

In theory these LOAD()'s can leak into the critical section inside add_wait_queue() and can happen before list_add(entry, wq_head), in this case pipe_poll() can race with wakeup_pipe_readers/writers which do

	smp_mb();
	if (waitqueue_active(wq_head))
		wake_up_interruptible(wq_head);

There are more __pollwait()-like functions (grep init_poll_funcptr), and it seems that at least ep_ptable_queue_proc() has the same problem, so the patch adds smp_mb() into poll_wait().

---
 include/linux/poll.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

index d1ea4f3714a8..fc641b50f129 100644
@@ -41,8 +41,16 @@ typedef struct poll_table_struct {
 
 static inline void poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
 {
-	if (p && p->_qproc && wait_address)
+	if (p && p->_qproc && wait_address) {
 		p->_qproc(filp, wait_address, p);
+		/*
+		 * This memory barrier is paired in the wq_has_sleeper().
+		 * See the comment above prepare_to_wait(), we need to
+		 * ensure that subsequent tests in this thread can't be
+		 * reordered with __add_wait_queue() in _qproc() paths.
+		 */
+		smp_mb();
+	}
 }
 
 /*

2.25.1.362.g51ebf55

This check is historical and no longer needed, wait_address is never NULL. These days we rely on the poll_table->_qproc check. NULL if select/poll is not going to sleep, or it already has a data to report, or all waiters have already been registered after the 1st iteration.

However, poll_table *p can be NULL, see p9_fd_poll() for example, so we can't remove the "p != NULL" check.

---
 include/linux/poll.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index fc641b50f129..57b6d1ccd8bf 100644
@@ -41,7 +41,7 @@ typedef struct poll_table_struct {
 
 static inline void poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
 {
-	if (p && p->_qproc && wait_address) {
+	if (p && p->_qproc) {
 		p->_qproc(filp, wait_address, p);
 		/*
 		 * This memory barrier is paired in the wq_has_sleeper().

2.25.1.362.g51ebf55

Now that poll_wait() provides a full barrier we can remove smp_rmb() from io_uring_poll().

In fact I don't think smp_rmb() was correct, it can't serialize LOADs and STOREs.

---
 io_uring/io_uring.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index 06ff41484e29..a64a82b93b86 100644
@@ -2809,13 +2809,12 @@ static __poll_t io_uring_poll(struct file *file, poll_table *wait)
 
 	if (unlikely(!ctx->poll_activated))
 		io_activate_pollwq(ctx);
-
-	poll_wait(file, &ctx->poll_wq, wait);
 	/*
-	 * synchronizes with barrier from wq_has_sleeper call in
-	 * io_commit_cqring
+	 * provides mb() which pairs with barrier from wq_has_sleeper
+	 * call in io_commit_cqring
 	 */
-	smp_rmb();
+	poll_wait(file, &ctx->poll_wq, wait);
+
 	if (!io_sqring_full(ctx))
 		mask |= EPOLLOUT | EPOLLWRNORM;
 

2.25.1.362.g51ebf55

Now that poll_wait() provides a full barrier we can remove smp_mb() from sock_poll_wait().

Also, the poll_does_not_wait() check before poll_wait() just adds the unnecessary confusion, kill it. poll_wait() does the same "p && p->_qproc" check.

---
 include/net/sock.h | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

index 7464e9f9f47c..305f3ae5edc2 100644
@@ -2291,7 +2291,7 @@ static inline bool skwq_has_sleeper(struct socket_wq *wq)
 }
 
 /**
- * sock_poll_wait - place memory barrier behind the poll_wait call.
+ * sock_poll_wait - wrapper for the poll_wait call.
  * @filp:           file
  * @sock:           socket to wait on
  * @p:              poll_table

@@ -2301,15 +2301,12 @@ static inline bool skwq_has_sleeper(struct socket_wq *wq)
 static inline void sock_poll_wait(struct file *filp, struct socket *sock,
 				  poll_table *p)
 {
-	if (!poll_does_not_wait(p)) {
-		poll_wait(filp, &sock->wq.wait, p);
-		/* We need to be sure we are in sync with the
-		 * socket flags modification.
-		 *
-		 * This memory barrier is paired in the wq_has_sleeper.
-		 */
-		smp_mb();
-	}
+	/* Provides a barrier we need to be sure we are in sync
+	 * with the socket flags modification.
+	 *
+	 * This memory barrier is paired in the wq_has_sleeper.
+	 */
+	poll_wait(filp, &sock->wq.wait, p);
 }
 
 static inline void skb_set_hash_from_sk(struct sk_buff *skb, struct sock *sk)

2.25.1.362.g51ebf55

It no longer has users.

---
 include/linux/poll.h | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

index 57b6d1ccd8bf..12bb18e8b978 100644
@@ -25,14 +25,14 @@
 
 struct poll_table_struct;
 
-/* 
+/*
  * structures and helpers for f_op->poll implementations
  */
 typedef void (*poll_queue_proc)(struct file *, wait_queue_head_t *, struct poll_table_struct *);
 
 /*
- * Do not touch the structure directly, use the access functions
- * poll_does_not_wait() and poll_requested_events() instead.
+ * Do not touch the structure directly, use the access function
+ * poll_requested_events() instead.
  */
 typedef struct poll_table_struct {
 	poll_queue_proc _qproc;

@@ -53,16 +53,6 @@ static inline void poll_wait(struct file * filp, wait_queue_head_t * wait_addres
 	}
 }
 
-/*
- * Return true if it is guaranteed that poll will not wait. This is the case
- * if the poll() of another file descriptor in the set got an event, so there
- * is no need for waiting.
- */
-static inline bool poll_does_not_wait(const poll_table *p)
-{
-	return p == NULL || p->_qproc == NULL;
-}
-
 /*
  * Return the set of events that the application wants to poll for.
  * This is useful for drivers that need to know whether a DMA transfer has

2.25.1.362.g51ebf55

Ack on all five patches, looks sane to me.

Christian, I'm assuming this goes through your tree? If not, holler, and I can take it directly.

Linus

Same, series looks good.

Jens Axboe

Yes, on it. Sorry, this took a bit. I'm trying to work downwards through all the mail.

Applied to the vfs-6.14.poll branch of the vfs/vfs.git tree. Patches in the vfs-6.14.poll branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.14.poll

[1/5] poll_wait: add mb() to fix theoretical race between waitqueue_active() and .poll()

      https://git.kernel.org/vfs/vfs/c/cacd9ae4bf80
[2/5] poll_wait: kill the obsolete wait_address check
      https://git.kernel.org/vfs/vfs/c/10b02a2cfec2
[3/5] io_uring_poll: kill the no longer necessary barrier after poll_wait()
      https://git.kernel.org/vfs/vfs/c/4e15fa8305de
[4/5] sock_poll_wait: kill the no longer necessary barrier after poll_wait()
      https://git.kernel.org/vfs/vfs/c/b2849867b3a7
[5/5] poll: kill poll_does_not_wait()
      https://git.kernel.org/vfs/vfs/c/f005bf18a57a