Skip to contentSkip to conversation

blk-iocost: fix the lock order in ioc_cost_model_write()

PATCH In review Tao Cui 3 participants 3 messages Nov 11

Subsystems (3)
Block layer cgroup cgroup blkio
lore
Share & tools
lore

Tao Cui

author
#0 lore raw

PATCH 1/1 blk-iocost: fix the lock order in ioc_cost_model_write()

ioc_cost_model_write() holds rq_qos_mutex, which is acquired by blkg_conf_open_bdev(), across blk_mq_freeze_queue(). ioc_qos_write() freezes the queue before taking the mutex instead.

The inversion cannot currently deadlock because blk_mq_freeze_queue() is in a noio scope since 1e1a9cecfa ("block: force noio scope in blk_mq_freeze_queue"), so a caller freezing a queue cannot recurse into the frozen queue through reclaim. Holding the mutex across the freeze needlessly serializes config writers behind it and leaves two different orderings in the same file. Release the mutex around the freeze to match ioc_qos_write(), mirroring the fix there in 9730763f4756 ("block: correct locking order for protecting blk-wbt parameters").

Reported-by [email protected]
Link https://sashiko.dev/#/patchset/[email protected]?part=1
Signed-off-by Tao Cui <[email protected]>
---
 block/blk-iocost.c | 7 +++++++
 1 file changed, 7 insertions(+)
block/blk-iocost.c +7 -0
index 1c1025157d8e..7fe0a6fa146e 100644
@@ -3605,7 +3605,14 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 		ioc = q_to_ioc(q);
 	}
 
+	/*
+	 * Nothing in the cost model is protected yet; release rq_qos_mutex
+	 * around the freeze to match ioc_qos_write().
+	 */
+	mutex_unlock(&q->rq_qos_mutex);
 	memflags = blk_mq_freeze_queue(q);
+	mutex_lock(&q->rq_qos_mutex);
+
 	blk_mq_quiesce_queue(q);
 
 	spin_lock_irq(&ioc->lock);
Signature · 1 line
2.53.0
#1 lore raw
Quoted 26 lines · Tao Cui

ioc_cost_model_write() holds rq_qos_mutex, which is acquired by blkg_conf_open_bdev(), across blk_mq_freeze_queue(). ioc_qos_write() freezes the queue before taking the mutex instead.

The inversion cannot currently deadlock because blk_mq_freeze_queue() is in a noio scope since 1e1a9cecfa ("block: force noio scope in blk_mq_freeze_queue"), so a caller freezing a queue cannot recurse into the frozen queue through reclaim. Holding the mutex across the freeze needlessly serializes config writers behind it and leaves two different orderings in the same file. Release the mutex around the freeze to match ioc_qos_write(), mirroring the fix there in 9730763f4756 ("block: correct locking order for protecting blk-wbt parameters").

Reported-by [email protected]
Link https://sashiko.dev/#/patchset/[email protected]?part=1
Signed-off-by Tao Cui <[email protected]>
---
 block/blk-iocost.c | 7 +++++++
 1 file changed, 7 insertions(+)
block/blk-iocost.c +0 -0
index 1c1025157d8e..7fe0a6fa146e 100644

block/blk-iocost.c static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input, · line 3614

Quoted 10 lines
block/blk-iocost.c +6 -0
@@ -3605,7 +3605,14 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 		ioc = q_to_ioc(q);
 	}
 
+	/*
+	 * Nothing in the cost model is protected yet; release rq_qos_mutex
+	 * around the freeze to match ioc_qos_write().
+	 */
+	mutex_unlock(&q->rq_qos_mutex);
 	memflags = blk_mq_freeze_queue(q);
+	mutex_lock(&q->rq_qos_mutex);

blkg_conf_open_bdev takes rq_qos_mutex aronud the disk_live call. So you'll have to redo it here. Or maybe we should just move that check and the locking into the callers of blkg_conf_open_bdev in general to better support the need for freezing and other potentially nested locking?

maintainer
#2 lore raw
In reply to Tao Cui

Hello, Tao.

Quoted 6 lines · Tao Cui

The inversion cannot currently deadlock because blk_mq_freeze_queue() is in a noio scope since 1e1a9cecfa ("block: force noio scope in blk_mq_freeze_queue"), so a caller freezing a queue cannot recurse into the frozen queue through reclaim. Holding the mutex across the freeze needlessly serializes config writers behind it and leaves two different orderings in the same file.

There is no deadlock here to begin with, and the noio scope isn't why. Freezing is counted and doesn't exclude other freezers, and a freeze only waits for in-flight IOs to drain, none of which takes rq_qos_mutex. The patch doesn't make the ordering consistent either. blk_iocost_init() still freezes under the mutex through rq_qos_add() and blkcg_activate_policy(), and so do blk_throtl_init() and blk_iolatency_init().

block/blk-iocost.c static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input, · line 3614

Quoted 3 lines
block/blk-iocost.c +2 -0
+	mutex_unlock(&q->rq_qos_mutex);
 	memflags = blk_mq_freeze_queue(q);
+	mutex_lock(&q->rq_qos_mutex);

This lets the disk be deleted in the middle. @ioc was looked up before the unlock and rq_qos_exit() can tear it down before the mutex is retaken. With a delay added after the unlock and a null_blk device deleted during it, the write goes on to use the ioc after ioc_rqos_exit() and KASAN reports use-after-free in blk_mq_quiesce_queue() called from ioc_cost_model_write().

So, this fixes nothing and adds a bug.

Separately, ioc_qos_write() already has the same window. It retakes the mutex without rechecking disk_live(), so with the disk deleted in between, it initializes iocost on the dead disk and hits the same use-after-free. That's the part which needs fixing.

Thanks.

--
tejun