io_uring: io_fail_links() should only consider first linked timeout

PATCH Jens Axboe 3 participants 10 messages Nov 20, 2019

Subsystems (2)

VFS io_uring

lore

Share & tools

lore

Jens Axboe

author maintainer

2019-11-20 03:33 UTC #0 lore raw

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 <[email protected]>

---

fs/io_uring.c +1 -1

index a79ef43367b1..d1085e4e8ae9 100644
@@ -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);

Signature · 1 line

Jens Axboe

Pavel Begunkov

2019-11-20 08:44 UTC #1 lore raw

Quoted 14 lines · 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 <[email protected]>
---
fs/io_uring.c +0 -0
index a79ef43367b1..d1085e4e8ae9 100644

fs/io_uring.c static void io_fail_links(struct io_kiocb *req) · line 945

Quoted 10 lines

fs/io_uring.c +1 -1

@@ -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.

Quoted 4 lines

fs/io_uring.c +0 -0

 	}
 
 	io_commit_cqring(ctx);

Signature · 1 line

Pavel Begunkov

Bob Liu

2019-11-20 10:22 UTC #2 lore raw

Quoted 37 lines · Pavel Begunkov

Quoted 14 lines · 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 <[email protected]>
---
fs/io_uring.c +0 -0
index a79ef43367b1..d1085e4e8ae9 100644
fs/io_uring.c static void io_fail_links(struct io_kiocb *req) · line 945
Quoted 10 lines
fs/io_uring.c +1 -1
@@ -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

Quoted 8 lines

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.

Pavel Begunkov

2019-11-20 11:07 UTC #3 lore raw

Quoted 41 lines · Bob Liu

Quoted 40 lines · Pavel Begunkov
Quoted 14 lines · 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 <[email protected]>
---
fs/io_uring.c +0 -0
index a79ef43367b1..d1085e4e8ae9 100644
fs/io_uring.c static void io_fail_links(struct io_kiocb *req) · line 945
Quoted 10 lines
fs/io_uring.c +1 -1
@@ -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

Quoted 9 lines

Quoted 5 lines
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).

Signature · 1 line

Pavel Begunkov

Bob Liu

2019-11-20 14:03 UTC #4 lore raw

Quoted 48 lines · Pavel Begunkov

Quoted 47 lines · Bob Liu
Quoted 45 lines · Pavel Begunkov
Quoted 14 lines · 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 <[email protected]>
---
fs/io_uring.c +0 -0
index a79ef43367b1..d1085e4e8ae9 100644
fs/io_uring.c static void io_fail_links(struct io_kiocb *req) · line 945
Quoted 10 lines
fs/io_uring.c +1 -1
@@ -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.

Quoted 7 lines

Quoted 3 lines
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;

Jens Axboe

author maintainer

2019-11-20 14:23 UTC #5 lore raw

Quoted 52 lines · Bob Liu

Quoted 51 lines · Pavel Begunkov
Quoted 50 lines · Bob Liu
Quoted 37 lines · Pavel Begunkov
Quoted 14 lines · 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 <[email protected]>
---
fs/io_uring.c +0 -0
index a79ef43367b1..d1085e4e8ae9 100644
fs/io_uring.c static void io_fail_links(struct io_kiocb *req) · line 945
Quoted 10 lines
fs/io_uring.c +1 -1
@@ -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
Quoted 8 lines
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.

Quoted 8 lines

Quoted 4 lines
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.

Signature · 1 line

Jens Axboe

Pavel Begunkov

2019-11-20 14:28 UTC #6 lore raw

In reply to Bob Liu

Quoted 3 lines · Bob Liu

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.

Quoted 8 lines

(quoted diff) +2 -0

@@ -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;

Signature · 1 line

Pavel Begunkov

Jens Axboe

author maintainer

2019-11-20 14:22 UTC #7 lore raw

In reply to Pavel Begunkov

Quoted 37 lines · Pavel Begunkov

Quoted 14 lines · 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 <[email protected]>
---
fs/io_uring.c +0 -0
index a79ef43367b1..d1085e4e8ae9 100644
fs/io_uring.c static void io_fail_links(struct io_kiocb *req) · line 945
Quoted 10 lines
fs/io_uring.c +1 -1
@@ -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

Quoted 7 lines

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.

Signature · 1 line

Jens Axboe

Pavel Begunkov

2019-11-20 15:02 UTC #8 lore raw

Quoted 10 lines · Jens Axboe

Quoted 10 lines
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

Quoted 8 lines

Quoted 4 lines
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?

Signature · 1 line

Pavel Begunkov

Jens Axboe

author maintainer

2019-11-20 15:06 UTC #9 lore raw

Quoted 16 lines · Pavel Begunkov

Quoted 15 lines · Jens Axboe
Quoted 14 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Signature · 1 line

Jens Axboe