io_uring: io_fail_links() should only consider first linked timeout

io_uring: io_fail_links() should only consider first linked timeout

[Jens Axboe]

We currently clear the linked timeout field if we cancel such a timeout,
but we should only attempt to cancel if it's the first one we see.
Others should simply be freed like other requests, as they haven't
been started yet.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/fs/io_uring.c b/fs/io_uring.c
index a79ef43367b1..d1085e4e8ae9 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -937,12 +937,12 @@ static void io_fail_links(struct io_kiocb *req)
 		if ((req->flags & REQ_F_LINK_TIMEOUT) &&
 		    link->submit.sqe->opcode == IORING_OP_LINK_TIMEOUT) {
 			io_link_cancel_timeout(link);
-			req->flags &= ~REQ_F_LINK_TIMEOUT;
 		} else {
 			io_cqring_fill_event(link, -ECANCELED);
 			__io_double_put_req(link);
 		}
 		kfree(sqe_to_free);
+		req->flags &= ~REQ_F_LINK_TIMEOUT;
 	}
 
 	io_commit_cqring(ctx);

-- 
Jens Axboe

Re: io_uring: io_fail_links() should only consider first linked timeout

[Pavel Begunkov]

On 11/20/2019 1:33 AM, Jens Axboe wrote:
> We currently clear the linked timeout field if we cancel such a timeout,
> but we should only attempt to cancel if it's the first one we see.
> Others should simply be freed like other requests, as they haven't
> been started yet.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index a79ef43367b1..d1085e4e8ae9 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -937,12 +937,12 @@ static void io_fail_links(struct io_kiocb *req)
>  		if ((req->flags & REQ_F_LINK_TIMEOUT) &&
>  		    link->submit.sqe->opcode == IORING_OP_LINK_TIMEOUT) {
>  			io_link_cancel_timeout(link);
> -			req->flags &= ~REQ_F_LINK_TIMEOUT;
>  		} else {
>  			io_cqring_fill_event(link, -ECANCELED);
>  			__io_double_put_req(link);
>  		}
>  		kfree(sqe_to_free);
> +		req->flags &= ~REQ_F_LINK_TIMEOUT;

That's not necessary, but maybe would safer to keep. If
REQ_F_LINK_TIMEOUT is set, than there was a link timeout request,
and for it and only for it io_link_cancel_timeout() will be called.

However, this is only true if linked timeout isn't fired. Otherwise,
there is another bug, which isn't fixed by either of the patches. We
need to clear REQ_F_LINK_TIMEOUT in io_link_timeout_fn() as well.

Let: REQ -> L_TIMEOUT1 -> L_TIMEOUT2
1. L_TIMEOUT1 fired before REQ is completed

2. io_link_timeout_fn() removes L_TIMEOUT1 from the list:
REQ|REQ_F_LINK_TIMEOUT -> L_TIMEOUT2

3. free_req(REQ) then call io_link_cancel_timeout(L_TIMEOUT2)
leaking it (as described in my patch).

P.S. haven't tried to test nor reproduce it yet.

>  	}
>  
>  	io_commit_cqring(ctx);
> 

-- 
Pavel Begunkov

Re: io_uring: io_fail_links() should only consider first linked timeout

[Bob Liu]

On 11/20/19 4:44 PM, Pavel Begunkov wrote:
> On 11/20/2019 1:33 AM, Jens Axboe wrote:
>> We currently clear the linked timeout field if we cancel such a timeout,
>> but we should only attempt to cancel if it's the first one we see.
>> Others should simply be freed like other requests, as they haven't
>> been started yet.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index a79ef43367b1..d1085e4e8ae9 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -937,12 +937,12 @@ static void io_fail_links(struct io_kiocb *req)
>>  		if ((req->flags & REQ_F_LINK_TIMEOUT) &&
>>  		    link->submit.sqe->opcode == IORING_OP_LINK_TIMEOUT) {
>>  			io_link_cancel_timeout(link);
>> -			req->flags &= ~REQ_F_LINK_TIMEOUT;
>>  		} else {
>>  			io_cqring_fill_event(link, -ECANCELED);
>>  			__io_double_put_req(link);
>>  		}
>>  		kfree(sqe_to_free);
>> +		req->flags &= ~REQ_F_LINK_TIMEOUT;
> 
> That's not necessary, but maybe would safer to keep. If
> REQ_F_LINK_TIMEOUT is set, than there was a link timeout request,
> and for it and only for it io_link_cancel_timeout() will be called.
> 
> However, this is only true if linked timeout isn't fired. Otherwise,
> there is another bug, which isn't fixed by either of the patches. We
> need to clear REQ_F_LINK_TIMEOUT in io_link_timeout_fn() as well.
> 
> Let: REQ -> L_TIMEOUT1 -> L_TIMEOUT2
> 1. L_TIMEOUT1 fired before REQ is completed
> 
> 2. io_link_timeout_fn() removes L_TIMEOUT1 from the list:
> REQ|REQ_F_LINK_TIMEOUT -> L_TIMEOUT2
> 
> 3. free_req(REQ) then call io_link_cancel_timeout(L_TIMEOUT2)
> leaking it (as described in my patch).
> 
> P.S. haven't tried to test nor reproduce it yet.
> 

Off topic... I'm reading the code regarding IORING_OP_LINK_TIMEOUT.
But confused by what's going to happen if userspace submit a request with IORING_OP_LINK_TIMEOUT but not IOSQE_IO_LINK.

Re: io_uring: io_fail_links() should only consider first linked timeout

[Pavel Begunkov]

On 11/20/2019 1:22 PM, Bob Liu wrote:
> On 11/20/19 4:44 PM, Pavel Begunkov wrote:
>> On 11/20/2019 1:33 AM, Jens Axboe wrote:
>>> We currently clear the linked timeout field if we cancel such a timeout,
>>> but we should only attempt to cancel if it's the first one we see.
>>> Others should simply be freed like other requests, as they haven't
>>> been started yet.
>>>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>
>>> ---
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index a79ef43367b1..d1085e4e8ae9 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -937,12 +937,12 @@ static void io_fail_links(struct io_kiocb *req)
>>>  		if ((req->flags & REQ_F_LINK_TIMEOUT) &&
>>>  		    link->submit.sqe->opcode == IORING_OP_LINK_TIMEOUT) {
>>>  			io_link_cancel_timeout(link);
>>> -			req->flags &= ~REQ_F_LINK_TIMEOUT;
>>>  		} else {
>>>  			io_cqring_fill_event(link, -ECANCELED);
>>>  			__io_double_put_req(link);
>>>  		}
>>>  		kfree(sqe_to_free);
>>> +		req->flags &= ~REQ_F_LINK_TIMEOUT;
>>
>> That's not necessary, but maybe would safer to keep. If
>> REQ_F_LINK_TIMEOUT is set, than there was a link timeout request,
>> and for it and only for it io_link_cancel_timeout() will be called.
>>
>> However, this is only true if linked timeout isn't fired. Otherwise,
>> there is another bug, which isn't fixed by either of the patches. We
>> need to clear REQ_F_LINK_TIMEOUT in io_link_timeout_fn() as well.
>>
>> Let: REQ -> L_TIMEOUT1 -> L_TIMEOUT2
>> 1. L_TIMEOUT1 fired before REQ is completed
>>
>> 2. io_link_timeout_fn() removes L_TIMEOUT1 from the list:
>> REQ|REQ_F_LINK_TIMEOUT -> L_TIMEOUT2
>>
>> 3. free_req(REQ) then call io_link_cancel_timeout(L_TIMEOUT2)
>> leaking it (as described in my patch).
>>
>> P.S. haven't tried to test nor reproduce it yet.
>>
> 
> Off topic... I'm reading the code regarding IORING_OP_LINK_TIMEOUT.
> But confused by what's going to happen if userspace submit a request with IORING_OP_LINK_TIMEOUT but not IOSQE_IO_LINK.
> 
It fails in __io_submit_sqe() with -EINVAL. (see default branch in the
switch). As for me, it's better to do it late, as it will generically
handle dependant links (e.g. fail them properly).

-- 
Pavel Begunkov

Re: io_uring: io_fail_links() should only consider first linked timeout

[Bob Liu]

On 11/20/19 7:07 PM, Pavel Begunkov wrote:
> On 11/20/2019 1:22 PM, Bob Liu wrote:
>> On 11/20/19 4:44 PM, Pavel Begunkov wrote:
>>> On 11/20/2019 1:33 AM, Jens Axboe wrote:
>>>> We currently clear the linked timeout field if we cancel such a timeout,
>>>> but we should only attempt to cancel if it's the first one we see.
>>>> Others should simply be freed like other requests, as they haven't
>>>> been started yet.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>
>>>> ---
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index a79ef43367b1..d1085e4e8ae9 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -937,12 +937,12 @@ static void io_fail_links(struct io_kiocb *req)
>>>>  		if ((req->flags & REQ_F_LINK_TIMEOUT) &&
>>>>  		    link->submit.sqe->opcode == IORING_OP_LINK_TIMEOUT) {
>>>>  			io_link_cancel_timeout(link);
>>>> -			req->flags &= ~REQ_F_LINK_TIMEOUT;
>>>>  		} else {
>>>>  			io_cqring_fill_event(link, -ECANCELED);
>>>>  			__io_double_put_req(link);
>>>>  		}
>>>>  		kfree(sqe_to_free);
>>>> +		req->flags &= ~REQ_F_LINK_TIMEOUT;
>>>
>>> That's not necessary, but maybe would safer to keep. If
>>> REQ_F_LINK_TIMEOUT is set, than there was a link timeout request,
>>> and for it and only for it io_link_cancel_timeout() will be called.
>>>
>>> However, this is only true if linked timeout isn't fired. Otherwise,
>>> there is another bug, which isn't fixed by either of the patches. We
>>> need to clear REQ_F_LINK_TIMEOUT in io_link_timeout_fn() as well.
>>>
>>> Let: REQ -> L_TIMEOUT1 -> L_TIMEOUT2
>>> 1. L_TIMEOUT1 fired before REQ is completed
>>>
>>> 2. io_link_timeout_fn() removes L_TIMEOUT1 from the list:
>>> REQ|REQ_F_LINK_TIMEOUT -> L_TIMEOUT2
>>>
>>> 3. free_req(REQ) then call io_link_cancel_timeout(L_TIMEOUT2)
>>> leaking it (as described in my patch).
>>>
>>> P.S. haven't tried to test nor reproduce it yet.
>>>
>>
>> Off topic... I'm reading the code regarding IORING_OP_LINK_TIMEOUT.
>> But confused by what's going to happen if userspace submit a request with IORING_OP_LINK_TIMEOUT but not IOSQE_IO_LINK.
>>
> It fails in __io_submit_sqe() with -EINVAL. (see default branch in the
> switch). As for me, it's better to do it late, as it will generically
> handle dependant links (e.g. fail them properly).
> 

I see, thanks.
As for me, it may better return -EINVAL in advance so as to skip a lot unnecessary code for those reqs.

@@ -3176,6 +3176,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,

                if (!io_get_sqring(ctx, &req->submit)) {
		}
...
+               if (unlikely(req_is_invalid(req)))
+                       return -EINVAL;

Re: io_uring: io_fail_links() should only consider first linked timeout

[Jens Axboe]

On 11/20/19 7:03 AM, Bob Liu wrote:
> On 11/20/19 7:07 PM, Pavel Begunkov wrote:
>> On 11/20/2019 1:22 PM, Bob Liu wrote:
>>> On 11/20/19 4:44 PM, Pavel Begunkov wrote:
>>>> On 11/20/2019 1:33 AM, Jens Axboe wrote:
>>>>> We currently clear the linked timeout field if we cancel such a timeout,
>>>>> but we should only attempt to cancel if it's the first one we see.
>>>>> Others should simply be freed like other requests, as they haven't
>>>>> been started yet.
>>>>>
>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>>
>>>>> ---
>>>>>
>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>> index a79ef43367b1..d1085e4e8ae9 100644
>>>>> --- a/fs/io_uring.c
>>>>> +++ b/fs/io_uring.c
>>>>> @@ -937,12 +937,12 @@ static void io_fail_links(struct io_kiocb *req)
>>>>>   		if ((req->flags & REQ_F_LINK_TIMEOUT) &&
>>>>>   		    link->submit.sqe->opcode == IORING_OP_LINK_TIMEOUT) {
>>>>>   			io_link_cancel_timeout(link);
>>>>> -			req->flags &= ~REQ_F_LINK_TIMEOUT;
>>>>>   		} else {
>>>>>   			io_cqring_fill_event(link, -ECANCELED);
>>>>>   			__io_double_put_req(link);
>>>>>   		}
>>>>>   		kfree(sqe_to_free);
>>>>> +		req->flags &= ~REQ_F_LINK_TIMEOUT;
>>>>
>>>> That's not necessary, but maybe would safer to keep. If
>>>> REQ_F_LINK_TIMEOUT is set, than there was a link timeout request,
>>>> and for it and only for it io_link_cancel_timeout() will be called.
>>>>
>>>> However, this is only true if linked timeout isn't fired. Otherwise,
>>>> there is another bug, which isn't fixed by either of the patches. We
>>>> need to clear REQ_F_LINK_TIMEOUT in io_link_timeout_fn() as well.
>>>>
>>>> Let: REQ -> L_TIMEOUT1 -> L_TIMEOUT2
>>>> 1. L_TIMEOUT1 fired before REQ is completed
>>>>
>>>> 2. io_link_timeout_fn() removes L_TIMEOUT1 from the list:
>>>> REQ|REQ_F_LINK_TIMEOUT -> L_TIMEOUT2
>>>>
>>>> 3. free_req(REQ) then call io_link_cancel_timeout(L_TIMEOUT2)
>>>> leaking it (as described in my patch).
>>>>
>>>> P.S. haven't tried to test nor reproduce it yet.
>>>>
>>>
>>> Off topic... I'm reading the code regarding IORING_OP_LINK_TIMEOUT.
>>> But confused by what's going to happen if userspace submit a request with IORING_OP_LINK_TIMEOUT but not IOSQE_IO_LINK.
>>>
>> It fails in __io_submit_sqe() with -EINVAL. (see default branch in the
>> switch). As for me, it's better to do it late, as it will generically
>> handle dependant links (e.g. fail them properly).
>>
> 
> I see, thanks.
> As for me, it may better return -EINVAL in advance so as to skip a lot
> unnecessary code for those reqs.

It's an error case, hence unnecessary code isn't an issue. It's much
more important to just let it unwind naturally for that, imho.

-- 
Jens Axboe

Re: io_uring: io_fail_links() should only consider first linked timeout

[Pavel Begunkov]

On 11/20/2019 5:03 PM, Bob Liu wrote:
> I see, thanks.
> As for me, it may better return -EINVAL in advance so as to skip a lot unnecessary code for those reqs.
> 
Hah, I removed similar check just yesterday (see [PATCH 3/4] "io_uring:
remove redundant check"). We don't care about performance of invalid
requests, and this one in hot-path. The fewer edge cases, the better.

Also, for the example below, I'd want to fail INVALID_REQ and cancel
valid ones, but not exit after processing invalid, or even worse
processing the rest without it.

INVALID_REQ -> VALID_REQ -> VALID_REQ.

> @@ -3176,6 +3176,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
> 
>                 if (!io_get_sqring(ctx, &req->submit)) {
> 		}
> ...
> +               if (unlikely(req_is_invalid(req)))
> +                       return -EINVAL;
> 

-- 
Pavel Begunkov

Re: io_uring: io_fail_links() should only consider first linked timeout

[Jens Axboe]

On 11/20/19 1:44 AM, Pavel Begunkov wrote:
> On 11/20/2019 1:33 AM, Jens Axboe wrote:
>> We currently clear the linked timeout field if we cancel such a timeout,
>> but we should only attempt to cancel if it's the first one we see.
>> Others should simply be freed like other requests, as they haven't
>> been started yet.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index a79ef43367b1..d1085e4e8ae9 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -937,12 +937,12 @@ static void io_fail_links(struct io_kiocb *req)
>>   		if ((req->flags & REQ_F_LINK_TIMEOUT) &&
>>   		    link->submit.sqe->opcode == IORING_OP_LINK_TIMEOUT) {
>>   			io_link_cancel_timeout(link);
>> -			req->flags &= ~REQ_F_LINK_TIMEOUT;
>>   		} else {
>>   			io_cqring_fill_event(link, -ECANCELED);
>>   			__io_double_put_req(link);
>>   		}
>>   		kfree(sqe_to_free);
>> +		req->flags &= ~REQ_F_LINK_TIMEOUT;
> 
> That's not necessary, but maybe would safer to keep. If
> REQ_F_LINK_TIMEOUT is set, than there was a link timeout request,
> and for it and only for it io_link_cancel_timeout() will be called.
> 
> However, this is only true if linked timeout isn't fired. Otherwise,
> there is another bug, which isn't fixed by either of the patches. We
> need to clear REQ_F_LINK_TIMEOUT in io_link_timeout_fn() as well.
> 
> Let: REQ -> L_TIMEOUT1 -> L_TIMEOUT2
> 1. L_TIMEOUT1 fired before REQ is completed
> 
> 2. io_link_timeout_fn() removes L_TIMEOUT1 from the list:
> REQ|REQ_F_LINK_TIMEOUT -> L_TIMEOUT2
> 
> 3. free_req(REQ) then call io_link_cancel_timeout(L_TIMEOUT2)
> leaking it (as described in my patch).
> 
> P.S. haven't tried to test nor reproduce it yet.

That's exactly the case I was worried about. In any case, it seems
prudent to handle it defensively.

-- 
Jens Axboe

Re: io_uring: io_fail_links() should only consider first linked timeout

[Pavel Begunkov]

On 11/20/2019 5:22 PM, Jens Axboe wrote:
>> However, this is only true if linked timeout isn't fired. Otherwise,
>> there is another bug, which isn't fixed by either of the patches. We
>> need to clear REQ_F_LINK_TIMEOUT in io_link_timeout_fn() as well.
>>
>> Let: REQ -> L_TIMEOUT1 -> L_TIMEOUT2
>> 1. L_TIMEOUT1 fired before REQ is completed
>>
>> 2. io_link_timeout_fn() removes L_TIMEOUT1 from the list:
>> REQ|REQ_F_LINK_TIMEOUT -> L_TIMEOUT2
>>
>> 3. free_req(REQ) then call io_link_cancel_timeout(L_TIMEOUT2)
>> leaking it (as described in my patch).
>>
>> P.S. haven't tried to test nor reproduce it yet.
> 
> That's exactly the case I was worried about. In any case, it seems
> prudent to handle it defensively.
> 
Yeah, I've got from the patch that may have missed something.

Will you add this "should clear REQ_F_LINK_TIMEOUT in
io_link_timeout_fn()" (e.g. in v2), or should I?

-- 
Pavel Begunkov

Re: io_uring: io_fail_links() should only consider first linked timeout

[Jens Axboe]

On 11/20/19 8:02 AM, Pavel Begunkov wrote:
> On 11/20/2019 5:22 PM, Jens Axboe wrote:
>>> However, this is only true if linked timeout isn't fired. Otherwise,
>>> there is another bug, which isn't fixed by either of the patches. We
>>> need to clear REQ_F_LINK_TIMEOUT in io_link_timeout_fn() as well.
>>>
>>> Let: REQ -> L_TIMEOUT1 -> L_TIMEOUT2
>>> 1. L_TIMEOUT1 fired before REQ is completed
>>>
>>> 2. io_link_timeout_fn() removes L_TIMEOUT1 from the list:
>>> REQ|REQ_F_LINK_TIMEOUT -> L_TIMEOUT2
>>>
>>> 3. free_req(REQ) then call io_link_cancel_timeout(L_TIMEOUT2)
>>> leaking it (as described in my patch).
>>>
>>> P.S. haven't tried to test nor reproduce it yet.
>>
>> That's exactly the case I was worried about. In any case, it seems
>> prudent to handle it defensively.
>>
> Yeah, I've got from the patch that may have missed something.
> 
> Will you add this "should clear REQ_F_LINK_TIMEOUT in
> io_link_timeout_fn()" (e.g. in v2), or should I?

I'll add it in the v2.

-- 
Jens Axboe