diff mbox series

[2/7] io_uring/net: don't overflow multishot recv

Message ID 0b295634e8f1b71aa764c984608c22d85f88f75c.1691757663.git.asml.silence@gmail.com (mailing list archive)
State New
Headers show
Series random fixes and cleanups | expand

Commit Message

Pavel Begunkov Aug. 11, 2023, 12:53 p.m. UTC
Don't allow overflowing multishot recv CQEs, it might get out of
hand, hurt performanece, and in the worst case scenario OOM the task.

Cc: stable@vger.kernel.org
Fixes: b3fdea6ecb55c ("io_uring: multishot recv")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jiri Slaby Sept. 14, 2023, 8:34 a.m. UTC | #1
On 11. 08. 23, 14:53, Pavel Begunkov wrote:
> Don't allow overflowing multishot recv CQEs, it might get out of
> hand, hurt performanece, and in the worst case scenario OOM the task.
> 
> Cc: stable@vger.kernel.org
> Fixes: b3fdea6ecb55c ("io_uring: multishot recv")
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>   io_uring/net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/io_uring/net.c b/io_uring/net.c
> index 1599493544a5..8c419c01a5db 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -642,7 +642,7 @@ static inline bool io_recv_finish(struct io_kiocb *req, int *ret,
>   
>   	if (!mshot_finished) {
>   		if (io_aux_cqe(req, issue_flags & IO_URING_F_COMPLETE_DEFER,
> -			       *ret, cflags | IORING_CQE_F_MORE, true)) {
> +			       *ret, cflags | IORING_CQE_F_MORE, false)) {

This one breaks iouring's recv-multishot.t test:
Running test recv-multishot.t                                       MORE 
flag not set
test stream=0 wait_each=0 recvmsg=0 early_error=4  defer=0 failed
Test recv-multishot.t failed with ret 1

Is the commit or the test broken ;)?

>   			io_recv_prep_retry(req);
>   			/* Known not-empty or unknown state, retry */
>   			if (cflags & IORING_CQE_F_SOCK_NONEMPTY ||

thanks,
Pavel Begunkov Sept. 14, 2023, 1:02 p.m. UTC | #2
On 9/14/23 09:34, Jiri Slaby wrote:
> On 11. 08. 23, 14:53, Pavel Begunkov wrote:
>> Don't allow overflowing multishot recv CQEs, it might get out of
>> hand, hurt performanece, and in the worst case scenario OOM the task.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: b3fdea6ecb55c ("io_uring: multishot recv")
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>   io_uring/net.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/io_uring/net.c b/io_uring/net.c
>> index 1599493544a5..8c419c01a5db 100644
>> --- a/io_uring/net.c
>> +++ b/io_uring/net.c
>> @@ -642,7 +642,7 @@ static inline bool io_recv_finish(struct io_kiocb *req, int *ret,
>>       if (!mshot_finished) {
>>           if (io_aux_cqe(req, issue_flags & IO_URING_F_COMPLETE_DEFER,
>> -                   *ret, cflags | IORING_CQE_F_MORE, true)) {
>> +                   *ret, cflags | IORING_CQE_F_MORE, false)) {
> 
> This one breaks iouring's recv-multishot.t test:
> Running test recv-multishot.t                                       MORE flag not set
> test stream=0 wait_each=0 recvmsg=0 early_error=4  defer=0 failed
> Test recv-multishot.t failed with ret 1
> 
> Is the commit or the test broken ;)?

The test is not right. I fixed it up while sending the kernel patch,
but a new liburing version hasn't been cut yet and I assume you're
not using upstream version?

> 
>>               io_recv_prep_retry(req);
>>               /* Known not-empty or unknown state, retry */
>>               if (cflags & IORING_CQE_F_SOCK_NONEMPTY ||
> 
> thanks,
Jens Axboe Sept. 14, 2023, 2:15 p.m. UTC | #3
On 9/14/23 7:02 AM, Pavel Begunkov wrote:
> On 9/14/23 09:34, Jiri Slaby wrote:
>> On 11. 08. 23, 14:53, Pavel Begunkov wrote:
>>> Don't allow overflowing multishot recv CQEs, it might get out of
>>> hand, hurt performanece, and in the worst case scenario OOM the task.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: b3fdea6ecb55c ("io_uring: multishot recv")
>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>> ---
>>>   io_uring/net.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/io_uring/net.c b/io_uring/net.c
>>> index 1599493544a5..8c419c01a5db 100644
>>> --- a/io_uring/net.c
>>> +++ b/io_uring/net.c
>>> @@ -642,7 +642,7 @@ static inline bool io_recv_finish(struct io_kiocb *req, int *ret,
>>>       if (!mshot_finished) {
>>>           if (io_aux_cqe(req, issue_flags & IO_URING_F_COMPLETE_DEFER,
>>> -                   *ret, cflags | IORING_CQE_F_MORE, true)) {
>>> +                   *ret, cflags | IORING_CQE_F_MORE, false)) {
>>
>> This one breaks iouring's recv-multishot.t test:
>> Running test recv-multishot.t                                       MORE flag not set
>> test stream=0 wait_each=0 recvmsg=0 early_error=4  defer=0 failed
>> Test recv-multishot.t failed with ret 1
>>
>> Is the commit or the test broken ;)?
> 
> The test is not right. I fixed it up while sending the kernel patch,
> but a new liburing version hasn't been cut yet and I assume you're
> not using upstream version?

I'll probably cut a new release soonish, we're after the merge window
now so whatever went into 6.6-rc should be solid/sane API wise.
diff mbox series

Patch

diff --git a/io_uring/net.c b/io_uring/net.c
index 1599493544a5..8c419c01a5db 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -642,7 +642,7 @@  static inline bool io_recv_finish(struct io_kiocb *req, int *ret,
 
 	if (!mshot_finished) {
 		if (io_aux_cqe(req, issue_flags & IO_URING_F_COMPLETE_DEFER,
-			       *ret, cflags | IORING_CQE_F_MORE, true)) {
+			       *ret, cflags | IORING_CQE_F_MORE, false)) {
 			io_recv_prep_retry(req);
 			/* Known not-empty or unknown state, retry */
 			if (cflags & IORING_CQE_F_SOCK_NONEMPTY ||