Re: [PATCH] io_uring/poll: fix multishot recv missing EOF on wakeup race

[Jens Axboe <axboe@kernel.dk>]

On 3/16/26 7:14 PM, Jens Axboe wrote:
> On 3/16/26 5:08 PM, Pavel Begunkov wrote:
>> On 3/16/26 22:31, Jens Axboe wrote:
>>> On 3/16/26 4:24 PM, Pavel Begunkov wrote:
>>>> On 3/16/26 18:40, Jens Axboe wrote:
>>>>> On 3/16/26 9:16 AM, Jens Axboe wrote:
>>>>>> On 3/16/26 8:44 AM, Pavel Begunkov wrote:
>>>>>>> On 3/16/26 14:40, Pavel Begunkov wrote:
>>>>>>>> On 3/16/26 14:28, Jens Axboe wrote:
>>>>>>>>> On 3/16/26 8:17 AM, Pavel Begunkov wrote:
>>>>>>>>>> On 3/15/26 16:19, Jens Axboe wrote:
>>>>>>>>>>> When a socket send and shutdown() happen back-to-back, both fire
>>>>>>>>>>> wake-ups before the receiver's task_work has a chance to run. The first
>>>>>>>>>>> wake gets poll ownership (poll_refs=1), and the second bumps it to 2.
>>>>>>>>>>> When io_poll_check_events() runs, it calls io_poll_issue() which does a
>>>>>>>>>>> recv that reads the data and returns IOU_RETRY. The loop then drains all
>>>>>>>>>>> accumulated refs (atomic_sub_return(2) -> 0) and exits, even though only
>>>>>>>>>>> the first event was consumed. Since the shutdown is a persistent state
>>>>>>>>>>> change, no further wakeups will happen, and the multishot recv can hang
>>>>>>>>>>> forever.
>>>>>>>>>>>
>>>>>>>>>>> Fix this by only draining a single poll ref after io_poll_issue()
>>>>>>>>>>> returns IOU_RETRY for the APOLL_MULTISHOT path. If additional wakes
>>>>>>>>>>> raced in (poll_refs was > 1), the loop iterates again, vfs_poll()
>>>>>>>>>>> discovers the remaining state.
>>>>>>>>>>
>>>>>>>>>> How often will iterate with no effect for normal execution (i.e.
>>>>>>>>>> no shutdown)? And how costly it'll be? Why not handle HUP instead?
>>>>>>>>>
>>>>>>>>> That is my worry too. I spent a bit of time on it this morning to figure
>>>>>>>>> out why this is a new issue, and traced it down to 6.16..6.17, and this
>>>>>>>>> commit in particular:
>>>>>>>>>
>>>>>>>>> commit df30285b3670bf52e1e5512e4d4482bec5e93c16
>>>>>>>>> Author: Kuniyuki Iwashima <kuniyu@google.com>
>>>>>>>>> Date:   Wed Jul 2 22:35:18 2025 +0000
>>>>>>>>>
>>>>>>>>>        af_unix: Introduce SO_INQ.
>>>>>>>>>
>>>>>>>>> which is then not the first time I've had to fix fallout from that
>>>>>>>>> commit. Need to dig a bit deeper. That said, I do also worry a bit about
>>>>>>>>> missing events. Yes if both poll triggers are of the same type, eg
>>>>>>>>> POLLIN, then we don't need to iterate again. IN + HUP is problematic, as
>>>>>>>>> would anything else where you'd need separate handling for the trigger.
>>>>>>>>
>>>>>>>> Thinking more, I don't think the patch is correct either. Seems you
>>>>>>>> expect the last recv to return 0, but let's say you have 2 refs and
>>>>>>>> 8K in the rx queue. The first recv call gets 4K b/c some allocation
>>>>>>>> fails. The 2nd recv call returns another 4K, and now you're in the
>>>>>>>> same situation as before.
>>>>>>>>
>>>>>>>> You're trying to rely on a too specific behaviour. HUP handling should
>>>>>>>> be better.
>>>>>>>
>>>>>>> Some variation on, if HUP'ed, it spins until the opcode give up.
>>>>>>
>>>>>> Took a quick look, and we don't even get a HUP, the hangup side
>>>>>> ends up with a 0 mask. Which is less than useful... I'll keep
>>>>>> digging.
>>>>>
>>>>> How about something like this? Will only retry if hup was seen, and
>>>>> there are multiple refs. Avoids re-iterating for eg multiple POLLIN
>>>>> wakes, which should be the common hot path if v & IO_POLL_REF_MASK != 1.
>>>>> Keeps it local too.
>>>>
>>>> HUP handling is just a hack, it'd be best to avoid complicating
>>>
>>> It is, that's why I wasn't a huge fan of gating on that in the first
>>> place!
>>>
>>>> the pool loop logic for that (and those continue do).
>>>>
>>>> io_poll_loop_retry() {
>>>>      ...
>>>>      atomic_or(IO_POLL_RETRY_FLAG, &req->poll_refs);
>>>> }
>>>> if (req->cqe.res & (POLLHUP | POLLRDHUP))
>>>>      io_poll_loop_retry();
>>>>
>>>>
>>>> Can we isolate it like this? Nobody should care about extra
>>>> atomics for this case.
>>>
>>> It's not the extra atomic, I agree it doesn't matter for this non-hot
>>> case. It's more that setting the retry flag isn't enough, you need to
>>> have it do another loop at that point. And if you just set that, then
>>> it'll drop all refs and happily return and wait for the next poll
>>> trigger that won't happen past HUP.
>>
>> It was actually supposed to force it into another iteration, but
>> I see what you're saying. I'll take a look at this part tomorrow
> 
> I mean, we can just reuse 'v' for this, and set IO_POLL_RETRY_FLAG.
> There's no reason to set it in ->poll_refs as it's all local anyway.
> Then that at least gets rid of the 'retry' variable.

This is as clean/simple as I can get it, less text too for this one.

diff --git a/io_uring/poll.c b/io_uring/poll.c
index aac4b3b881fb..06183cf60201 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -228,6 +228,22 @@ static inline void io_poll_execute(struct io_kiocb *req, int res)
 		__io_poll_execute(req, res);
 }
 
+static inline void io_poll_check_retry(struct io_kiocb *req, int *v)
+{
+	if (req->opcode == IORING_OP_POLL_ADD)
+		return;
+	if (!(req->cqe.res & (POLLHUP | POLLRDHUP)))
+		return;
+	/*
+	 * Release all references, retry if someone tried to restart
+	 * task_work while we were executing it.
+	 */
+	*v &= IO_POLL_REF_MASK;
+
+	/* multiple refs and HUP, ensure we loop once more */
+	(*v)--;
+}
+
 /*
  * All poll tw should go through this. Checks for poll events, manages
  * references, does rewait, etc.
@@ -294,6 +310,8 @@ static int io_poll_check_events(struct io_kiocb *req, io_tw_token_t tw)
 		if (req->apoll_events & EPOLLONESHOT)
 			return IOU_POLL_DONE;
 
+		io_poll_check_retry(req, &v);
+
 		/* multishot, just fill a CQE and proceed */
 		if (!(req->flags & REQ_F_APOLL_MULTISHOT)) {
 			__poll_t mask = mangle_poll(req->cqe.res &
@@ -316,12 +334,6 @@ static int io_poll_check_events(struct io_kiocb *req, io_tw_token_t tw)
 
 		/* force the next iteration to vfs_poll() */
 		req->cqe.res = 0;
-
-		/*
-		 * Release all references, retry if someone tried to restart
-		 * task_work while we were executing it.
-		 */
-		v &= IO_POLL_REF_MASK;
 	} while (atomic_sub_return(v, &req->poll_refs) & IO_POLL_REF_MASK);
 
 	io_napi_add(req);

-- 
Jens Axboe