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

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

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

> It's not impossible we can make this work, but I don't see a great way
> of doing it without messing with the loop like I did :/. Or checking
> ->poll_refs again, and at that point the separate variable is better
> imho.
-- 
Pavel Begunkov