Re: [PATCH 1/1] io_uring/poll: fix POLLERR handling

[Jens Axboe <axboe@kernel.dk>]

On 7/15/25 12:13 PM, Jens Axboe wrote:
> On 7/15/25 3:06 AM, Pavel Begunkov wrote:
>> On 7/14/25 21:45, Jens Axboe wrote:
>>> On 7/14/25 11:51 AM, Jens Axboe wrote:
>>>> On 7/14/25 9:30 AM, Pavel Begunkov wrote:
>>>>> On 7/14/25 15:56, Jens Axboe wrote:
>>>>>> On 7/14/25 4:59 AM, Pavel Begunkov wrote:
>>>>>>> 8c8492ca64e7 ("io_uring/net: don't retry connect operation on EPOLLERR")
>>>>>>> is a little dirty hack that
>>>>>>> 1) wrongfully assumes that POLLERR equals to a failed request, which
>>>>>>> breaks all POLLERR users, e.g. all error queue recv interfaces.
>>>>>>> 2) deviates the connection request behaviour from connect(2), and
>>>>>>> 3) racy and solved at a wrong level.
>>>>>>>
>>>>>>> Nothing can be done with 2) now, and 3) is beyond the scope of the
>>>>>>> patch. At least solve 1) by moving the hack out of generic poll handling
>>>>>>> into io_connect().
>>>>>>>
>>>>>>> Cc: stable@vger.kernel.org
>>>>>>> Fixes: 8c8492ca64e79 ("io_uring/net: don't retry connect operation on EPOLLERR")
>>>>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>>>>> ---
>>>>>>>    io_uring/net.c  | 4 +++-
>>>>>>>    io_uring/poll.c | 2 --
>>>>>>>    2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/io_uring/net.c b/io_uring/net.c
>>>>>>> index 43a43522f406..e2213e4d9420 100644
>>>>>>> --- a/io_uring/net.c
>>>>>>> +++ b/io_uring/net.c
>>>>>>> @@ -1732,13 +1732,15 @@ int io_connect_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>>>>>      int io_connect(struct io_kiocb *req, unsigned int issue_flags)
>>>>>>>    {
>>>>>>> +    struct poll_table_struct pt = { ._key = EPOLLERR };
>>>>>>>        struct io_connect *connect = io_kiocb_to_cmd(req, struct io_connect);
>>>>>>>        struct io_async_msghdr *io = req->async_data;
>>>>>>>        unsigned file_flags;
>>>>>>>        int ret;
>>>>>>>        bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
>>>>>>>    -    if (unlikely(req->flags & REQ_F_FAIL)) {
>>>>>>> +    ret = vfs_poll(req->file, &pt) & req->apoll_events;
>>>>>>> +    if (ret & EPOLLERR) {
>>>>>>>            ret = -ECONNRESET;
>>>>>>>            goto out;
>>>>>>
>>>>>> Is this req->apoll_events masking necessary or useful?
>>>>>
>>>>> good point, shouldn't be here
>>>>
>>>> Do you want to send a v2?
>>>
>>> Actually I think it can be improved/fixed further. If POLLIN is set, we
>>
>> How is it related to POLLIN?
> 
> Gah POLLOUT of course.
> 
>>> should let it go through. And there should not be a need to call
>>> vfs_poll() unless ->in_progress is already set. Something ala:
>>
>> In any case, v1 doesn't seem to work, so needs to be done differently.
> 
> RIght, that's what I aluded to in the "improved/fixed" further. FWIW, I
> did dig out the old test case I wrote for this and added it to liburing
> as well. So should have somewhat better coverage now.

How about:

if (connect->in_progress) {
	struct poll_table_struct pt = { ._key = EPOLLERR };

	if (vfs_poll(req->file, &pt) & EPOLLERR)
		goto get_sock_err;
}
 
with get_sock_err being where we do sock_error()?

-- 
Jens Axboe