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

[Jens Axboe <axboe@kernel.dk>]

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
should let it go through. And there should not be a need to call
vfs_poll() unless ->in_progress is already set. Something ala:

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

		mask = vfs_poll(req->file, &pt);
		if ((mask & (EPOLLERR | EPOLLIN)) == EPOLLERR) {
			ret = -ECONNRESET;
			goto out;
		}
 	}

avoiding what I'm also guessing will be a sparse complaint on the 'ret'
vs __poll_t use as well. Not that poll.c is free of those already, but
new ones will surely bother me via email.

-- 
Jens Axboe