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

[Pavel Begunkov <asml.silence@gmail.com>]

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
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.


> diff --git a/io_uring/poll.c b/io_uring/poll.c
> index b671b84657d9..bd79a04a2c59 100644
> --- a/io_uring/poll.c
> +++ b/io_uring/poll.c
> @@ -228,6 +228,16 @@ static inline void io_poll_execute(struct io_kiocb *req, int res)
>   		__io_poll_execute(req, res);
>   }
>   
> +static inline bool io_poll_loop_retry(struct io_kiocb *req, int v)
> +{
> +	if (req->opcode == IORING_OP_POLL_ADD)
> +		return false;
> +	/* multiple refs and HUP, ensure we loop once more */
> +	if (v != 1 && req->cqe.res & (POLLHUP | POLLRDHUP))

v != 1 looks suspicious, at this stage it's hard to trace what
io_recv_finish() is really doing, but better to drop the check.
  
req->cqe.res should already be in a register, makes more sense
to gate on that first.

-- 
Pavel Begunkov