[PATCH] io_uring: fix error handling for io_uring_cmd

[PATCH] io_uring: fix error handling for io_uring_cmd

[Anuj Gupta]

Commit 97b388d70b53 ("io_uring: handle completions in the core") moved the
error handling from handler to core. But for io_uring_cmd handler we end
up completing more than once (both in handler and in core) leading to
use_after_free.
Change io_uring_cmd handler to avoid calling io_uring_cmd_done in case
of error.

Fixes: 97b388d70b53 ("io_uring: handle completions in the core")
Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 io_uring/uring_cmd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
index 0a421ed51e7e..d5972864009e 100644
--- a/io_uring/uring_cmd.c
+++ b/io_uring/uring_cmd.c
@@ -106,7 +106,9 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
 	}
 
 	if (ret != -EIOCBQUEUED) {
-		io_uring_cmd_done(ioucmd, ret, 0);
+		if (ret < 0)
+			req_set_fail(ret);
+		io_req_set_res(req, ret, 0);
 		return IOU_OK;
 	}
 
-- 
2.25.1

Re: [PATCH] io_uring: fix error handling for io_uring_cmd

[Jens Axboe]

On Thu, 11 Aug 2022 14:44:59 +0530, Anuj Gupta wrote:
> Commit 97b388d70b53 ("io_uring: handle completions in the core") moved the
> error handling from handler to core. But for io_uring_cmd handler we end
> up completing more than once (both in handler and in core) leading to
> use_after_free.
> Change io_uring_cmd handler to avoid calling io_uring_cmd_done in case
> of error.
> 
> [...]

Applied, thanks!

[1/1] io_uring: fix error handling for io_uring_cmd
      commit: f1bb0fd63c374e1410ff05fb434aa78e1ce09ae4

Best regards,
-- 
Jens Axboe

Re: [PATCH] io_uring: fix error handling for io_uring_cmd

[Jens Axboe]

On 8/11/22 9:38 AM, Jens Axboe wrote:
> On Thu, 11 Aug 2022 14:44:59 +0530, Anuj Gupta wrote:
>> Commit 97b388d70b53 ("io_uring: handle completions in the core") moved the
>> error handling from handler to core. But for io_uring_cmd handler we end
>> up completing more than once (both in handler and in core) leading to
>> use_after_free.
>> Change io_uring_cmd handler to avoid calling io_uring_cmd_done in case
>> of error.
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] io_uring: fix error handling for io_uring_cmd
>       commit: f1bb0fd63c374e1410ff05fb434aa78e1ce09ae4

Ehm, did you compile this:

> io_uring/uring_cmd.c: In function ?io_uring_cmd?:
io_uring/uring_cmd.c:113:38: warning: passing argument 1 of ?req_set_fail? makes pointer from integer without a cast [-Wint-conversion]
  113 |                         req_set_fail(ret);
      |                                      ^~~
      |                                      |
      |                                      int
In file included from io_uring/uring_cmd.c:9:
io_uring/io_uring.h:144:50: note: expected ?struct io_kiocb *? but argument is of type ?int?
  144 | static inline void req_set_fail(struct io_kiocb *req)
      |                                 ~~~~~~~~~~~~~~~~~^~~

s/ret/req obviously.

-- 
Jens Axboe

Re: [PATCH] io_uring: fix error handling for io_uring_cmd

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

On Thu, Aug 11, 2022 at 10:55:29AM -0600, Jens Axboe wrote:
>On 8/11/22 9:38 AM, Jens Axboe wrote:
>> On Thu, 11 Aug 2022 14:44:59 +0530, Anuj Gupta wrote:
>>> Commit 97b388d70b53 ("io_uring: handle completions in the core") moved the
>>> error handling from handler to core. But for io_uring_cmd handler we end
>>> up completing more than once (both in handler and in core) leading to
>>> use_after_free.
>>> Change io_uring_cmd handler to avoid calling io_uring_cmd_done in case
>>> of error.
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [1/1] io_uring: fix error handling for io_uring_cmd
>>       commit: f1bb0fd63c374e1410ff05fb434aa78e1ce09ae4
>
>Ehm, did you compile this:
Sorry. Version that landed here got a upgrade in
commit-description but downgrade in this part :-(

BTW, we noticed the original issue while testing fixedbufs support.
Thinking to add a liburing test that involves sending a command which
nvme will fail during submission. Can come in handy.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] io_uring: fix error handling for io_uring_cmd

[Jens Axboe]

On 8/11/22 11:35 AM, Kanchan Joshi wrote:
> On Thu, Aug 11, 2022 at 10:55:29AM -0600, Jens Axboe wrote:
>> On 8/11/22 9:38 AM, Jens Axboe wrote:
>>> On Thu, 11 Aug 2022 14:44:59 +0530, Anuj Gupta wrote:
>>>> Commit 97b388d70b53 ("io_uring: handle completions in the core") moved the
>>>> error handling from handler to core. But for io_uring_cmd handler we end
>>>> up completing more than once (both in handler and in core) leading to
>>>> use_after_free.
>>>> Change io_uring_cmd handler to avoid calling io_uring_cmd_done in case
>>>> of error.
>>>>
>>>> [...]
>>>
>>> Applied, thanks!
>>>
>>> [1/1] io_uring: fix error handling for io_uring_cmd
>>>       commit: f1bb0fd63c374e1410ff05fb434aa78e1ce09ae4
>>
>> Ehm, did you compile this:
> Sorry. Version that landed here got a upgrade in
> commit-description but downgrade in this part :-(

I fixed it up.

> BTW, we noticed the original issue while testing fixedbufs support.
> Thinking to add a liburing test that involves sending a command which
> nvme will fail during submission. Can come in handy.

I think that's a good idea - if you had eg a NOP linked after a passthru
command that failed, then that would catch this case.

-- 
Jens Axboe

Re: [PATCH] io_uring: fix error handling for io_uring_cmd

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

On Thu, Aug 11, 2022 at 11:51:29AM -0600, Jens Axboe wrote:
>On 8/11/22 11:35 AM, Kanchan Joshi wrote:
>> On Thu, Aug 11, 2022 at 10:55:29AM -0600, Jens Axboe wrote:
>>> On 8/11/22 9:38 AM, Jens Axboe wrote:
>>>> On Thu, 11 Aug 2022 14:44:59 +0530, Anuj Gupta wrote:
>>>>> Commit 97b388d70b53 ("io_uring: handle completions in the core") moved the
>>>>> error handling from handler to core. But for io_uring_cmd handler we end
>>>>> up completing more than once (both in handler and in core) leading to
>>>>> use_after_free.
>>>>> Change io_uring_cmd handler to avoid calling io_uring_cmd_done in case
>>>>> of error.
>>>>>
>>>>> [...]
>>>>
>>>> Applied, thanks!
>>>>
>>>> [1/1] io_uring: fix error handling for io_uring_cmd
>>>>       commit: f1bb0fd63c374e1410ff05fb434aa78e1ce09ae4
>>>
>>> Ehm, did you compile this:
>> Sorry. Version that landed here got a upgrade in
>> commit-description but downgrade in this part :-(
>
>I fixed it up.

noticed, thanks.

>> BTW, we noticed the original issue while testing fixedbufs support.
>> Thinking to add a liburing test that involves sending a command which
>> nvme will fail during submission. Can come in handy.
>
>I think that's a good idea - if you had eg a NOP linked after a passthru
>command that failed, then that would catch this case.

Right. For now in liburing test we don't do anything that is guranteed
to fail from nvme-side. Test issues iopoll (that fails) but that failure
comes from io_uring itself (as .iopoll is not set). So another test that
will form a bad passthru command (e.g. wrong nsid) which only nvme can
(and will) fail.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] io_uring: fix error handling for io_uring_cmd

[Jens Axboe]

On 8/11/22 11:57 AM, Kanchan Joshi wrote:
>>> BTW, we noticed the original issue while testing fixedbufs support.
>>> Thinking to add a liburing test that involves sending a command which
>>> nvme will fail during submission. Can come in handy.
>>
>> I think that's a good idea - if you had eg a NOP linked after a passthru
>> command that failed, then that would catch this case.
> 
> Right. For now in liburing test we don't do anything that is guranteed
> to fail from nvme-side. Test issues iopoll (that fails) but that failure
> comes from io_uring itself (as .iopoll is not set). So another test that
> will form a bad passthru command (e.g. wrong nsid) which only nvme can
> (and will) fail.

Yes, that's a good idea in general, testing only successful completions
doesn't really give full coverage.

-- 
Jens Axboe