Hi,
We have an issue with drains not working due to missing copy of some state, it's affecting 5.2/5.3/5.4. I'm attaching the patch for 5.4, however the patch should apply to 5.2 and 5.3 as well by just removing the last hunk. The last hunk is touching the linked code, which was introduced with 5.4.
Can we get this queued up for stable? Thanks! Don't have an email for Tomáš, assuming the reported-by is fine with just his name. Want to ensure I include attribution I do have.
From: Jens Axboe axboe@kernel.dk Subject: [PATCH] io_uring: ensure req->submit is copied when req is deferred
There's an issue with deferred requests through drain, where if we do need to defer, we're not copying over the sqe_submit state correctly. This can result in using uninitialized data when we then later go and submit the deferred request, like this check in __io_submit_sqe():
if (unlikely(s->index >= ctx->sq_entries)) return -EINVAL;
with 's' being uninitialized, we can randomly fail this check. Fix this by copying sqe_submit state when we defer a request.
Reported-by: Andres Freund andres@anarazel.de Reported-by: Tomáš Chaloupka Signed-off-by: Jens Axboe axboe@kernel.dk
---
diff --git a/fs/io_uring.c b/fs/io_uring.c index 2c819c3c855d..0393545a39a7 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2016,7 +2017,7 @@ static int io_timeout(struct io_kiocb *req, const struct io_uring_sqe *sqe) }
static int io_req_defer(struct io_ring_ctx *ctx, struct io_kiocb *req, - const struct io_uring_sqe *sqe) + struct sqe_submit *s) { struct io_uring_sqe *sqe_copy;
@@ -2034,7 +2035,8 @@ static int io_req_defer(struct io_ring_ctx *ctx, struct io_kiocb *req, return 0; }
- memcpy(sqe_copy, sqe, sizeof(*sqe_copy)); + memcpy(&req->submit, s, sizeof(*s)); + memcpy(sqe_copy, s->sqe, sizeof(*sqe_copy)); req->submit.sqe = sqe_copy;
INIT_WORK(&req->work, io_sq_wq_submit_work); @@ -2399,7 +2401,7 @@ static int io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req, { int ret;
- ret = io_req_defer(ctx, req, s->sqe); + ret = io_req_defer(ctx, req, s); if (ret) { if (ret != -EIOCBQUEUED) { io_free_req(req); @@ -2426,7 +2428,7 @@ static int io_queue_link_head(struct io_ring_ctx *ctx, struct io_kiocb *req, * list. */ req->flags |= REQ_F_IO_DRAIN; - ret = io_req_defer(ctx, req, s->sqe); + ret = io_req_defer(ctx, req, s); if (ret) { if (ret != -EIOCBQUEUED) { io_free_req(req);
On Wed, Dec 04, 2019 at 08:53:43AM -0700, Jens Axboe wrote:
Hi,
We have an issue with drains not working due to missing copy of some state, it's affecting 5.2/5.3/5.4. I'm attaching the patch for 5.4, however the patch should apply to 5.2 and 5.3 as well by just removing the last hunk. The last hunk is touching the linked code, which was introduced with 5.4.
Does this match up with a patch in Linus's tree anywhere? Why is this a stable-only thing?
thanks,
greg k-h
On 12/7/19 5:01 AM, Greg KH wrote:
On Wed, Dec 04, 2019 at 08:53:43AM -0700, Jens Axboe wrote:
Hi,
We have an issue with drains not working due to missing copy of some state, it's affecting 5.2/5.3/5.4. I'm attaching the patch for 5.4, however the patch should apply to 5.2 and 5.3 as well by just removing the last hunk. The last hunk is touching the linked code, which was introduced with 5.4.
Does this match up with a patch in Linus's tree anywhere? Why is this a stable-only thing?
Because it was fixed as part of a cleanup series in mainline, before anyone realized we had this issue. That removed the separate states of ->index vs ->submit.sqe. That series is not something I was comfortable putting into stable, hence the much simpler addition. Here's the patch in the series that fixes the same issue:
commit cf6fd4bd559ee61a4454b161863c8de6f30f8dca Author: Pavel Begunkov asml.silence@gmail.com Date: Mon Nov 25 23:14:39 2019 +0300
io_uring: inline struct sqe_submit
On Sat, Dec 07, 2019 at 08:19:18AM -0700, Jens Axboe wrote:
On 12/7/19 5:01 AM, Greg KH wrote:
On Wed, Dec 04, 2019 at 08:53:43AM -0700, Jens Axboe wrote:
Hi,
We have an issue with drains not working due to missing copy of some state, it's affecting 5.2/5.3/5.4. I'm attaching the patch for 5.4, however the patch should apply to 5.2 and 5.3 as well by just removing the last hunk. The last hunk is touching the linked code, which was introduced with 5.4.
Does this match up with a patch in Linus's tree anywhere? Why is this a stable-only thing?
Because it was fixed as part of a cleanup series in mainline, before anyone realized we had this issue. That removed the separate states of ->index vs ->submit.sqe. That series is not something I was comfortable putting into stable, hence the much simpler addition. Here's the patch in the series that fixes the same issue:
commit cf6fd4bd559ee61a4454b161863c8de6f30f8dca Author: Pavel Begunkov asml.silence@gmail.com Date: Mon Nov 25 23:14:39 2019 +0300
io_uring: inline struct sqe_submit
Thanks for the info, now queued up!
greg k-h
linux-stable-mirror@lists.linaro.org