io_uring/net: fix a multishot termination case for recv

Commit Message

Jens Axboe Sept. 28, 2024, 12:18 p.m. UTC

If the recv returns zero, or an error, then it doesn't matter if more
data has already been received for this buffer. A condition like that
should terminate the multishot receive. Rather than pass in the
collected return value, pass in whether to terminate or keep the recv
going separately.

Link: https://github.com/axboe/liburing/issues/1246
Cc: [e-mail]
Fixes: b3fdea6ecb55 ("io_uring: multishot recv")
Signed-off-by: Jens Axboe <[e-mail]>

---

Comments

Jens Axboe Sept. 28, 2024, 12:40 p.m. UTC | #1

On 9/28/24 6:18 AM, Jens Axboe wrote:
> diff --git a/io_uring/net.c b/io_uring/net.c
> index f10f5a22d66a..18507658a921 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -1133,6 +1133,7 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>  	int ret, min_ret = 0;
>  	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
>  	size_t len = sr->len;
> +	bool mshot_finished;
>  
>  	if (!(req->flags & REQ_F_POLLED) &&
>  	    (sr->flags & IORING_RECVSEND_POLL_FIRST))
> @@ -1187,6 +1188,7 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>  		req_set_fail(req);
>  	}
>  
> +	mshot_finished = ret <= 0;
>  	if (ret > 0)
>  		ret += sr->done_io;
>  	else if (sr->done_io)
> @@ -1194,7 +1196,7 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>  	else
>  		io_kbuf_recycle(req, issue_flags);
>  
> -	if (!io_recv_finish(req, &ret, kmsg, ret <= 0, issue_flags))
> +	if (!io_recv_finish(req, &ret, kmsg, mshot_finished, issue_flags))
>  		goto retry_multishot;
>  
>  	return ret;

On second thought, I don't think we can get into this situation -
sr->done_io is only ever used for recv if we had to retry after getting
some data. And that only happens if MSG_WAITALL is set, which is not
legal for multishot and will result in -EINVAL. So don't quite see how
we can run into this issue. But I could be missing something...

Comments?

Pavel Begunkov Sept. 29, 2024, 7:25 p.m. UTC | #2

On 9/28/24 13:40, Jens Axboe wrote:
> On 9/28/24 6:18 AM, Jens Axboe wrote:
>> diff --git a/io_uring/net.c b/io_uring/net.c
>> index f10f5a22d66a..18507658a921 100644
>> --- a/io_uring/net.c
>> +++ b/io_uring/net.c
>> @@ -1133,6 +1133,7 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>>   	int ret, min_ret = 0;
>>   	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
>>   	size_t len = sr->len;
>> +	bool mshot_finished;
>>   
>>   	if (!(req->flags & REQ_F_POLLED) &&
>>   	    (sr->flags & IORING_RECVSEND_POLL_FIRST))
>> @@ -1187,6 +1188,7 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>>   		req_set_fail(req);
>>   	}
>>   
>> +	mshot_finished = ret <= 0;
>>   	if (ret > 0)
>>   		ret += sr->done_io;
>>   	else if (sr->done_io)
>> @@ -1194,7 +1196,7 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>>   	else
>>   		io_kbuf_recycle(req, issue_flags);
>>   
>> -	if (!io_recv_finish(req, &ret, kmsg, ret <= 0, issue_flags))
>> +	if (!io_recv_finish(req, &ret, kmsg, mshot_finished, issue_flags))
>>   		goto retry_multishot;
>>   
>>   	return ret;
> 
> On second thought, I don't think we can get into this situation -
> sr->done_io is only ever used for recv if we had to retry after getting
> some data. And that only happens if MSG_WAITALL is set, which is not
> legal for multishot and will result in -EINVAL. So don't quite see how
> we can run into this issue. But I could be missing something...
> 
> Comments?

I noticed the chunk months ago, it's definitely a sloppy one, but I
deemed it not to be an actual problem after trying to exploit it at
the moment.

Jens Axboe Sept. 30, 2024, 1:58 a.m. UTC | #3

On 9/29/24 1:25 PM, Pavel Begunkov wrote:
> On 9/28/24 13:40, Jens Axboe wrote:
>> On 9/28/24 6:18 AM, Jens Axboe wrote:
>>> diff --git a/io_uring/net.c b/io_uring/net.c
>>> index f10f5a22d66a..18507658a921 100644
>>> --- a/io_uring/net.c
>>> +++ b/io_uring/net.c
>>> @@ -1133,6 +1133,7 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>>>       int ret, min_ret = 0;
>>>       bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
>>>       size_t len = sr->len;
>>> +    bool mshot_finished;
>>>         if (!(req->flags & REQ_F_POLLED) &&
>>>           (sr->flags & IORING_RECVSEND_POLL_FIRST))
>>> @@ -1187,6 +1188,7 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>>>           req_set_fail(req);
>>>       }
>>>   +    mshot_finished = ret <= 0;
>>>       if (ret > 0)
>>>           ret += sr->done_io;
>>>       else if (sr->done_io)
>>> @@ -1194,7 +1196,7 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>>>       else
>>>           io_kbuf_recycle(req, issue_flags);
>>>   -    if (!io_recv_finish(req, &ret, kmsg, ret <= 0, issue_flags))
>>> +    if (!io_recv_finish(req, &ret, kmsg, mshot_finished, issue_flags))
>>>           goto retry_multishot;
>>>         return ret;
>>
>> On second thought, I don't think we can get into this situation -
>> sr->done_io is only ever used for recv if we had to retry after getting
>> some data. And that only happens if MSG_WAITALL is set, which is not
>> legal for multishot and will result in -EINVAL. So don't quite see how
>> we can run into this issue. But I could be missing something...
>>
>> Comments?
> 
> I noticed the chunk months ago, it's definitely a sloppy one, but I
> deemed it not to be an actual problem after trying to exploit it at
> the moment.

Yes, might not be a bad idea to still do the tweak. Not because we can
_currently_ hit it, but because it could be possible in the future and
just get overlooked.

Thanks for confirming :-)

Patch

diff --git a/io_uring/net.c b/io_uring/net.c
index f10f5a22d66a..18507658a921 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -1133,6 +1133,7 @@  int io_recv(struct io_kiocb *req, unsigned int issue_flags)
 	int ret, min_ret = 0;
 	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
 	size_t len = sr->len;
+	bool mshot_finished;
 
 	if (!(req->flags & REQ_F_POLLED) &&
 	    (sr->flags & IORING_RECVSEND_POLL_FIRST))
@@ -1187,6 +1188,7 @@  int io_recv(struct io_kiocb *req, unsigned int issue_flags)
 		req_set_fail(req);
 	}
 
+	mshot_finished = ret <= 0;
 	if (ret > 0)
 		ret += sr->done_io;
 	else if (sr->done_io)
@@ -1194,7 +1196,7 @@  int io_recv(struct io_kiocb *req, unsigned int issue_flags)
 	else
 		io_kbuf_recycle(req, issue_flags);
 
-	if (!io_recv_finish(req, &ret, kmsg, ret <= 0, issue_flags))
+	if (!io_recv_finish(req, &ret, kmsg, mshot_finished, issue_flags))
 		goto retry_multishot;
 
 	return ret;