| Message ID | 20250401182813.1115909-1-dw@davidwei.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring/zcrx: return early from io_zcrx_recv_skb if readlen is 0 | expand |
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.
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.
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.
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;
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(+)