* [PATCH] io_uring: Fix LINK_TIMEOUT checks @ 2019-11-14 21:20 Pavel Begunkov 2019-11-14 21:25 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Pavel Begunkov @ 2019-11-14 21:20 UTC (permalink / raw) To: Jens Axboe, io-uring If IORING_OP_LINK_TIMEOUT request is a head of a link or an individual request, pass it further through the submission path, where it will eventually fail in __io_submit_sqe(). So respecting links and drains. The case, which is really need to be checked, is if a IORING_OP_LINK_TIMEOUT request is 3rd or later in a link, that is invalid from the user API perspective (judging by the code). Moreover, put/free and friends will try to io_link_cancel_timeout() such request, even though it wasn't initialised. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 55f8b1d378df..fd2ba35f53e3 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2952,6 +2952,12 @@ static void io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, if (*link) { struct io_kiocb *prev = *link; + if (unlikely(READ_ONCE(s->sqe->opcode) == IORING_OP_LINK_TIMEOUT + && !list_empty(&prev->link_list))) { + ret = -EINVAL; + goto err_req; + } + sqe_copy = kmemdup(s->sqe, sizeof(*sqe_copy), GFP_KERNEL); if (!sqe_copy) { ret = -EAGAIN; @@ -2966,10 +2972,6 @@ static void io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, INIT_LIST_HEAD(&req->link_list); *link = req; - } else if (READ_ONCE(s->sqe->opcode) == IORING_OP_LINK_TIMEOUT) { - /* Only valid as a linked SQE */ - ret = -EINVAL; - goto err_req; } else { io_queue_sqe(req); } -- 2.24.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: Fix LINK_TIMEOUT checks 2019-11-14 21:20 [PATCH] io_uring: Fix LINK_TIMEOUT checks Pavel Begunkov @ 2019-11-14 21:25 ` Jens Axboe 2019-11-14 21:31 ` Pavel Begunkov 2019-11-15 0:12 ` Jens Axboe 0 siblings, 2 replies; 6+ messages in thread From: Jens Axboe @ 2019-11-14 21:25 UTC (permalink / raw) To: Pavel Begunkov, io-uring On 11/14/19 2:20 PM, Pavel Begunkov wrote: > If IORING_OP_LINK_TIMEOUT request is a head of a link or an individual > request, pass it further through the submission path, where it will > eventually fail in __io_submit_sqe(). So respecting links and drains. > > The case, which is really need to be checked, is if a > IORING_OP_LINK_TIMEOUT request is 3rd or later in a link, that is > invalid from the user API perspective (judging by the code). Moreover, > put/free and friends will try to io_link_cancel_timeout() such request, > even though it wasn't initialised. Care to add a test case for these to liburings test/link-timeout.c? -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: Fix LINK_TIMEOUT checks 2019-11-14 21:25 ` Jens Axboe @ 2019-11-14 21:31 ` Pavel Begunkov 2019-11-15 0:12 ` Jens Axboe 1 sibling, 0 replies; 6+ messages in thread From: Pavel Begunkov @ 2019-11-14 21:31 UTC (permalink / raw) To: Jens Axboe, io-uring [-- Attachment #1.1: Type: text/plain, Size: 758 bytes --] On 15/11/2019 00:25, Jens Axboe wrote: > On 11/14/19 2:20 PM, Pavel Begunkov wrote: >> If IORING_OP_LINK_TIMEOUT request is a head of a link or an individual >> request, pass it further through the submission path, where it will >> eventually fail in __io_submit_sqe(). So respecting links and drains. >> >> The case, which is really need to be checked, is if a >> IORING_OP_LINK_TIMEOUT request is 3rd or later in a link, that is >> invalid from the user API perspective (judging by the code). Moreover, >> put/free and friends will try to io_link_cancel_timeout() such request, >> even though it wasn't initialised. > > Care to add a test case for these to liburings test/link-timeout.c? > I'll add it a bit later -- Pavel Begunkov [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: Fix LINK_TIMEOUT checks 2019-11-14 21:25 ` Jens Axboe 2019-11-14 21:31 ` Pavel Begunkov @ 2019-11-15 0:12 ` Jens Axboe 2019-11-15 2:36 ` Jens Axboe 1 sibling, 1 reply; 6+ messages in thread From: Jens Axboe @ 2019-11-15 0:12 UTC (permalink / raw) To: Pavel Begunkov, io-uring On 11/14/19 2:25 PM, Jens Axboe wrote: > On 11/14/19 2:20 PM, Pavel Begunkov wrote: >> If IORING_OP_LINK_TIMEOUT request is a head of a link or an individual >> request, pass it further through the submission path, where it will >> eventually fail in __io_submit_sqe(). So respecting links and drains. >> >> The case, which is really need to be checked, is if a >> IORING_OP_LINK_TIMEOUT request is 3rd or later in a link, that is >> invalid from the user API perspective (judging by the code). Moreover, >> put/free and friends will try to io_link_cancel_timeout() such request, >> even though it wasn't initialised. > > Care to add a test case for these to liburings test/link-timeout.c? Wrote some test cases, I think that io_req_link_next() is just wrong. The below should correct it. We shouldn't loop here at all, just find the first one. That'll start that guy, sequence will continue, etc. diff --git a/fs/io_uring.c b/fs/io_uring.c index 5ad652fa24b8..3230da399681 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -858,7 +858,7 @@ static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr) * safe side. */ nxt = list_first_entry_or_null(&req->link_list, struct io_kiocb, list); - while (nxt) { + if (nxt) { list_del_init(&nxt->list); if (!list_empty(&req->link_list)) { INIT_LIST_HEAD(&nxt->link_list); @@ -876,12 +876,12 @@ static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr) /* we dropped this link, get next */ nxt = list_first_entry_or_null(&req->link_list, struct io_kiocb, list); - } else if (nxtptr && io_wq_current_is_worker()) { - *nxtptr = nxt; - break; - } else { - io_queue_async_work(nxt); - break; + } + if (nxt) { + if (nxtptr && io_wq_current_is_worker()) + *nxtptr = nxt; + else + io_queue_async_work(nxt); } } -- Jens Axboe ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: Fix LINK_TIMEOUT checks 2019-11-15 0:12 ` Jens Axboe @ 2019-11-15 2:36 ` Jens Axboe 2019-11-15 3:22 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Jens Axboe @ 2019-11-15 2:36 UTC (permalink / raw) To: Pavel Begunkov, io-uring On 11/14/19 5:12 PM, Jens Axboe wrote: > On 11/14/19 2:25 PM, Jens Axboe wrote: >> On 11/14/19 2:20 PM, Pavel Begunkov wrote: >>> If IORING_OP_LINK_TIMEOUT request is a head of a link or an individual >>> request, pass it further through the submission path, where it will >>> eventually fail in __io_submit_sqe(). So respecting links and drains. >>> >>> The case, which is really need to be checked, is if a >>> IORING_OP_LINK_TIMEOUT request is 3rd or later in a link, that is >>> invalid from the user API perspective (judging by the code). Moreover, >>> put/free and friends will try to io_link_cancel_timeout() such request, >>> even though it wasn't initialised. >> >> Care to add a test case for these to liburings test/link-timeout.c? > > Wrote some test cases, I think that io_req_link_next() is just wrong. > The below should correct it. We shouldn't loop here at all, just find > the first one. That'll start that guy, sequence will continue, etc. Well that was crap, I sent an earlier unfinished version. Here's the right one: diff --git a/fs/io_uring.c b/fs/io_uring.c index 5ad652fa24b8..31adee55e153 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -858,30 +858,26 @@ static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr) * safe side. */ nxt = list_first_entry_or_null(&req->link_list, struct io_kiocb, list); - while (nxt) { + if (nxt) { list_del_init(&nxt->list); if (!list_empty(&req->link_list)) { INIT_LIST_HEAD(&nxt->link_list); list_splice(&req->link_list, &nxt->link_list); nxt->flags |= REQ_F_LINK; + } else if (req->flags & REQ_F_LINK_TIMEOUT) { + wake_ev = io_link_cancel_timeout(nxt); + nxt = NULL; } /* * If we're in async work, we can continue processing the chain * in this context instead of having to queue up new async work. */ - if (req->flags & REQ_F_LINK_TIMEOUT) { - wake_ev = io_link_cancel_timeout(nxt); - - /* we dropped this link, get next */ - nxt = list_first_entry_or_null(&req->link_list, - struct io_kiocb, list); - } else if (nxtptr && io_wq_current_is_worker()) { - *nxtptr = nxt; - break; - } else { - io_queue_async_work(nxt); - break; + if (nxt) { + if (nxtptr && io_wq_current_is_worker()) + *nxtptr = nxt; + else + io_queue_async_work(nxt); } } @@ -2465,7 +2461,7 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe, sqe->cancel_flags) return -EINVAL; - io_async_find_and_cancel(ctx, req, READ_ONCE(sqe->addr), NULL); + io_async_find_and_cancel(ctx, req, READ_ONCE(sqe->addr), nxt); return 0; } @@ -2741,10 +2737,12 @@ static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer) */ if (!list_empty(&req->list)) { prev = list_entry(req->list.prev, struct io_kiocb, link_list); - if (refcount_inc_not_zero(&prev->refs)) + if (refcount_inc_not_zero(&prev->refs)) { + prev->flags &= ~REQ_F_LINK_TIMEOUT; list_del_init(&req->list); - else + } else { prev = NULL; + } } spin_unlock_irqrestore(&ctx->completion_lock, flags); -- Jens Axboe ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: Fix LINK_TIMEOUT checks 2019-11-15 2:36 ` Jens Axboe @ 2019-11-15 3:22 ` Jens Axboe 0 siblings, 0 replies; 6+ messages in thread From: Jens Axboe @ 2019-11-15 3:22 UTC (permalink / raw) To: Pavel Begunkov, io-uring On 11/14/19 7:36 PM, Jens Axboe wrote: > On 11/14/19 5:12 PM, Jens Axboe wrote: >> On 11/14/19 2:25 PM, Jens Axboe wrote: >>> On 11/14/19 2:20 PM, Pavel Begunkov wrote: >>>> If IORING_OP_LINK_TIMEOUT request is a head of a link or an individual >>>> request, pass it further through the submission path, where it will >>>> eventually fail in __io_submit_sqe(). So respecting links and drains. >>>> >>>> The case, which is really need to be checked, is if a >>>> IORING_OP_LINK_TIMEOUT request is 3rd or later in a link, that is >>>> invalid from the user API perspective (judging by the code). Moreover, >>>> put/free and friends will try to io_link_cancel_timeout() such request, >>>> even though it wasn't initialised. >>> >>> Care to add a test case for these to liburings test/link-timeout.c? >> >> Wrote some test cases, I think that io_req_link_next() is just wrong. >> The below should correct it. We shouldn't loop here at all, just find >> the first one. That'll start that guy, sequence will continue, etc. > > Well that was crap, I sent an earlier unfinished version. Here's the > right one: I think the only thing missing after this is handling multiple linked timeouts in a sequence. I'll do that now. -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-15 3:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-14 21:20 [PATCH] io_uring: Fix LINK_TIMEOUT checks Pavel Begunkov 2019-11-14 21:25 ` Jens Axboe 2019-11-14 21:31 ` Pavel Begunkov 2019-11-15 0:12 ` Jens Axboe 2019-11-15 2:36 ` Jens Axboe 2019-11-15 3:22 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).