[PATCH] io_uring/zcrx: return early from io_zcrx_recv_skb if readlen is 0

[PATCH] io_uring/zcrx: return early from io_zcrx_recv_skb if readlen is 0

[David Wei]

When readlen is set for a recvzc request, tcp_read_sock() will call
io_zcrx_recv_skb() one final time with len == desc->count == 0. This is
caused by the !desc->count check happening too late. The offset + 1 !=
skb->len happens earlier and causes the while loop to continue.

Fix this in io_zcrx_recv_skb() instead of tcp_read_sock(). Return early
if len is 0 i.e. the read is done.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 io_uring/zcrx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
index 9c95b5b6ec4e..d1dd25e7cf4a 100644
--- a/io_uring/zcrx.c
+++ b/io_uring/zcrx.c
@@ -818,6 +818,8 @@ io_zcrx_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
 	int ret = 0;
 
 	len = min_t(size_t, len, desc->count);
+	if (!len)
+		goto out;
 	if (unlikely(args->nr_skbs++ > IO_SKBS_PER_CALL_LIMIT))
 		return -EAGAIN;
 
-- 
2.47.1

Re: [PATCH] io_uring/zcrx: return early from io_zcrx_recv_skb if readlen is 0

[Jens Axboe]

On 4/1/25 12:28 PM, David Wei wrote:
> When readlen is set for a recvzc request, tcp_read_sock() will call
> io_zcrx_recv_skb() one final time with len == desc->count == 0. This is
> caused by the !desc->count check happening too late. The offset + 1 !=
> skb->len happens earlier and causes the while loop to continue.
> 
> Fix this in io_zcrx_recv_skb() instead of tcp_read_sock(). Return early
> if len is 0 i.e. the read is done.

Needs a Fixes tag, which looks like it should be:

Fixes: 6699ec9a23f8 ("io_uring/zcrx: add a read limit to recvzc requests")

?

> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  io_uring/zcrx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
> index 9c95b5b6ec4e..d1dd25e7cf4a 100644
> --- a/io_uring/zcrx.c
> +++ b/io_uring/zcrx.c
> @@ -818,6 +818,8 @@ io_zcrx_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
>  	int ret = 0;
>  
>  	len = min_t(size_t, len, desc->count);
> +	if (!len)
> +		goto out;

just return 0 here? Jumping to out would make more sense if there
are things to fixup/account at this point, but it's just going
to find offset == start_off and return 'ret', which is 0 anyway.

-- 
Jens Axboe

Re: [PATCH] io_uring/zcrx: return early from io_zcrx_recv_skb if readlen is 0

[David Wei]

On 2025-04-01 11:56, Jens Axboe wrote:
> On 4/1/25 12:28 PM, David Wei wrote:
>> When readlen is set for a recvzc request, tcp_read_sock() will call
>> io_zcrx_recv_skb() one final time with len == desc->count == 0. This is
>> caused by the !desc->count check happening too late. The offset + 1 !=
>> skb->len happens earlier and causes the while loop to continue.
>>
>> Fix this in io_zcrx_recv_skb() instead of tcp_read_sock(). Return early
>> if len is 0 i.e. the read is done.
> 
> Needs a Fixes tag, which looks like it should be:
> 
> Fixes: 6699ec9a23f8 ("io_uring/zcrx: add a read limit to recvzc requests")
> 
> ?

Sorry I missed that, will add the tag.

> 
>> Signed-off-by: David Wei <dw@davidwei.uk>
>> ---
>>  io_uring/zcrx.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
>> index 9c95b5b6ec4e..d1dd25e7cf4a 100644
>> --- a/io_uring/zcrx.c
>> +++ b/io_uring/zcrx.c
>> @@ -818,6 +818,8 @@ io_zcrx_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
>>  	int ret = 0;
>>  
>>  	len = min_t(size_t, len, desc->count);
>> +	if (!len)
>> +		goto out;
> 
> just return 0 here? Jumping to out would make more sense if there
> are things to fixup/account at this point, but it's just going
> to find offset == start_off and return 'ret', which is 0 anyway.
> 

Makes sense, yeah. I'll return 0 here early.

Re: [PATCH] io_uring/zcrx: return early from io_zcrx_recv_skb if readlen is 0

[Jens Axboe]

On 4/1/25 1:10 PM, David Wei wrote:
> On 2025-04-01 11:56, Jens Axboe wrote:
>> On 4/1/25 12:28 PM, David Wei wrote:
>>> When readlen is set for a recvzc request, tcp_read_sock() will call
>>> io_zcrx_recv_skb() one final time with len == desc->count == 0. This is
>>> caused by the !desc->count check happening too late. The offset + 1 !=
>>> skb->len happens earlier and causes the while loop to continue.
>>>
>>> Fix this in io_zcrx_recv_skb() instead of tcp_read_sock(). Return early
>>> if len is 0 i.e. the read is done.
>>
>> Needs a Fixes tag, which looks like it should be:
>>
>> Fixes: 6699ec9a23f8 ("io_uring/zcrx: add a read limit to recvzc requests")
>>
>> ?
> 
> Sorry I missed that, will add the tag.
> 
>>
>>> Signed-off-by: David Wei <dw@davidwei.uk>
>>> ---
>>>  io_uring/zcrx.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
>>> index 9c95b5b6ec4e..d1dd25e7cf4a 100644
>>> --- a/io_uring/zcrx.c
>>> +++ b/io_uring/zcrx.c
>>> @@ -818,6 +818,8 @@ io_zcrx_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
>>>  	int ret = 0;
>>>  
>>>  	len = min_t(size_t, len, desc->count);
>>> +	if (!len)
>>> +		goto out;
>>
>> just return 0 here? Jumping to out would make more sense if there
>> are things to fixup/account at this point, but it's just going
>> to find offset == start_off and return 'ret', which is 0 anyway.
>>
> 
> Makes sense, yeah. I'll return 0 here early.

Probably augment that with an unlikely() as well for v2, it's definitely
an error/unexpected case.

-- 
Jens Axboe