Thread (25 messages) 25 messages, 2 authors, 2022-08-29

Re: [RFC PATCH 2/9] ublk_drv: refactor ublk_cancel_queue()

From: Ming Lei <hidden>
Date: 2022-08-29 03:01:37
Also in: lkml

On Wed, Aug 24, 2022 at 01:47:37PM +0800, ZiyangZhang wrote:
quoted hunk ↗ jump to hunk
Assume only a few FETCH_REQ ioucmds are sent to ublk_drv, then the
ubq_daemon exits, We have to call io_uring_cmd_done() for all ioucmds
received so that io_uring ctx will not leak.

ublk_cancel_queue() may be called before START_DEV or after STOP_DEV,
we decrease ubq->nr_io_ready and clear UBLK_IO_FLAG_ACTIVE so that we
won't call io_uring_cmd_done() twice for one ioucmd to avoid UAF. Also
clearing UBLK_IO_FLAG_ACTIVE makes the code more reasonable.

Signed-off-by: ZiyangZhang <redacted>
---
 drivers/block/ublk_drv.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index c39b67d7133d..e08f636b0b9d 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -967,18 +967,23 @@ static void ublk_cancel_queue(struct ublk_queue *ubq)
 {
 	int i;
 
-	if (!ublk_queue_ready(ubq))
+	if (!ubq->nr_io_ready)
 		return;
 
 	for (i = 0; i < ubq->q_depth; i++) {
 		struct ublk_io *io = &ubq->ios[i];
 
-		if (io->flags & UBLK_IO_FLAG_ACTIVE)
+		if (io->flags & UBLK_IO_FLAG_ACTIVE) {
+			pr_devel("%s: done old cmd: qid %d tag %d\n",
+					__func__, ubq->q_id, i);
 			io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0);
+			io->flags &= ~UBLK_IO_FLAG_ACTIVE;
+			ubq->nr_io_ready--;
+		}
 	}
 
 	/* all io commands are canceled */
-	ubq->nr_io_ready = 0;
+	WARN_ON_ONCE(ubq->nr_io_ready);
The change looks fine, but suggest to add comment like the
following given the above WARN_ON_ONCE() change isn't obvious.
1) ublk_cancel_dev() is called before sending START_DEV(), ->mutex
provides protection on above update.

2) ublk_cancel_dev() is called after sending START_DEV(), disk is
deleted first, UBLK_IO_RES_ABORT is returned so that any new io
command can't be issued to driver, so updating on io flags and
nr_io_ready is safe here

Also ->nr_io_ready is guaranteed to become zero after ublk_cance_queue
returns since request queue is either frozen or not present in both two cases.

Thanks,
Ming
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help