IO-Uring Archive on lore.kernel.org
 help / color / Atom feed
* io_uring: io_fail_links() should only consider first linked timeout
@ 2019-11-19 22:33 Jens Axboe
  2019-11-20  8:44 ` Pavel Begunkov
  0 siblings, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2019-11-19 22:33 UTC (permalink / raw)
  To: io-uring

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: io_uring: io_fail_links() should only consider first linked timeout
  2019-11-19 22:33 io_uring: io_fail_links() should only consider first linked timeout Jens Axboe
@ 2019-11-20  8:44 ` Pavel Begunkov
  2019-11-20 10:22   ` Bob Liu
  2019-11-20 14:22   ` Jens Axboe
  0 siblings, 2 replies; 10+ messages in thread
From: Pavel Begunkov @ 2019-11-20  8:44 UTC (permalink / raw)
  To: Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: io_uring: io_fail_links() should only consider first linked timeout
  2019-11-20  8:44 ` Pavel Begunkov
@ 2019-11-20 10:22   ` Bob Liu
  2019-11-20 11:07     ` Pavel Begunkov
  2019-11-20 14:22   ` Jens Axboe
  1 sibling, 1 reply; 10+ messages in thread
From: Bob Liu @ 2019-11-20 10:22 UTC (permalink / raw)
  To: Pavel Begunkov, Jens Axboe, io-uring

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: io_uring: io_fail_links() should only consider first linked timeout
  2019-11-20 10:22   ` Bob Liu
@ 2019-11-20 11:07     ` Pavel Begunkov
  2019-11-20 14:03       ` Bob Liu
  0 siblings, 1 reply; 10+ messages in thread
From: Pavel Begunkov @ 2019-11-20 11:07 UTC (permalink / raw)
  To: Bob Liu, Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: io_uring: io_fail_links() should only consider first linked timeout
  2019-11-20 11:07     ` Pavel Begunkov
@ 2019-11-20 14:03       ` Bob Liu
  2019-11-20 14:23         ` Jens Axboe
  2019-11-20 14:28         ` Pavel Begunkov
  0 siblings, 2 replies; 10+ messages in thread
From: Bob Liu @ 2019-11-20 14:03 UTC (permalink / raw)
  To: Pavel Begunkov, Jens Axboe, io-uring

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;


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: io_uring: io_fail_links() should only consider first linked timeout
  2019-11-20  8:44 ` Pavel Begunkov
  2019-11-20 10:22   ` Bob Liu
@ 2019-11-20 14:22   ` Jens Axboe
  2019-11-20 15:02     ` Pavel Begunkov
  1 sibling, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2019-11-20 14:22 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: io_uring: io_fail_links() should only consider first linked timeout
  2019-11-20 14:03       ` Bob Liu
@ 2019-11-20 14:23         ` Jens Axboe
  2019-11-20 14:28         ` Pavel Begunkov
  1 sibling, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2019-11-20 14:23 UTC (permalink / raw)
  To: Bob Liu, Pavel Begunkov, io-uring

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: io_uring: io_fail_links() should only consider first linked timeout
  2019-11-20 14:03       ` Bob Liu
  2019-11-20 14:23         ` Jens Axboe
@ 2019-11-20 14:28         ` Pavel Begunkov
  1 sibling, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2019-11-20 14:28 UTC (permalink / raw)
  To: Bob Liu, Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: io_uring: io_fail_links() should only consider first linked timeout
  2019-11-20 14:22   ` Jens Axboe
@ 2019-11-20 15:02     ` Pavel Begunkov
  2019-11-20 15:06       ` Jens Axboe
  0 siblings, 1 reply; 10+ messages in thread
From: Pavel Begunkov @ 2019-11-20 15:02 UTC (permalink / raw)
  To: Jens Axboe, io-uring

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: io_uring: io_fail_links() should only consider first linked timeout
  2019-11-20 15:02     ` Pavel Begunkov
@ 2019-11-20 15:06       ` Jens Axboe
  0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2019-11-20 15:06 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19 22:33 io_uring: io_fail_links() should only consider first linked timeout Jens Axboe
2019-11-20  8:44 ` Pavel Begunkov
2019-11-20 10:22   ` Bob Liu
2019-11-20 11:07     ` Pavel Begunkov
2019-11-20 14:03       ` Bob Liu
2019-11-20 14:23         ` Jens Axboe
2019-11-20 14:28         ` Pavel Begunkov
2019-11-20 14:22   ` Jens Axboe
2019-11-20 15:02     ` Pavel Begunkov
2019-11-20 15:06       ` Jens Axboe

IO-Uring Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/io-uring/0 io-uring/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 io-uring io-uring/ https://lore.kernel.org/io-uring \
		io-uring@vger.kernel.org
	public-inbox-index io-uring

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.io-uring


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git