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

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

[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: sashiko-bot@kernel.org
Link: https://sashiko.dev/#/patchset/20261103132613.1952975-1-cui.tao@linux.dev?part=1
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/blk-iocost.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 1c1025157d8e..7fe0a6fa146e 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -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);
-- 
2.53.0

Re: [PATCH] blk-iocost: fix the lock order in ioc_cost_model_write()

[Christoph Hellwig]

On Thu, Nov 12, 2026 at 12:21:37AM +0800, Tao Cui wrote:
> 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: sashiko-bot@kernel.org
> Link: https://sashiko.dev/#/patchset/20261103132613.1952975-1-cui.tao@linux.dev?part=1
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> ---
>  block/blk-iocost.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 1c1025157d8e..7fe0a6fa146e 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -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?

Re: [PATCH] blk-iocost: fix the lock order in ioc_cost_model_write()

[Tejun Heo]

Hello, Tao.

On Thu, Nov 12, 2026 at 12:21:37AM +0800, Tao Cui wrote:
> 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().

> +	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

Re: [PATCH] blk-iocost: fix the lock order in ioc_cost_model_write()

[Tao Cui]

Hello, Tejun, Christoph,

Thanks for the reviews.


于 2026年11月12日 GMT+08:00 01:31:38，Tejun Heo <tj@kernel.org> 写道：
>Hello, Tao.
>
>On Thu, Nov 12, 2026 at 12:21:37AM +0800, Tao Cui wrote:
>> 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

I got this wrong. I treated the freeze as something that could deadlock
against the mutex, but it is a counted wait for in-flight I/O to drain,
and the drain paths don't take rq_qos_mutex, so there was never a
deadlock here to fix. The noio scope of blk_mq_freeze_queue() addresses
a different concern: preventing reclaim from recursing into the frozen
queue. I conflated the two.

>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().
>
I also missed that holding rq_qos_mutex across a freeze is the normal
pattern in these paths, so the "two different orderings in the same
file" premise of the commit message was wrong to begin with.

>> +	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 for the reproducer. This is what I missed: the mutex also
protects the lifetime of @ioc against rq_qos_exit(), and releasing it
around the freeze drops that protection.

On Wed, Nov 11, 2026 at 05:33:06PM +0100, Christoph Hellwig wrote:
> blkg_conf_open_bdev takes rq_qos_mutex aronud the disk_live call.  So
> you'll have to redo it here.

You're right, releasing the mutex voids the disk_live() check made under
it in blkg_conf_open_bdev().  I'll withdraw this version.

Sorry for the noise, and thank you both for the explanations. I'll
take a closer look at these locking and lifetime semantics and come
back with a new version once it's properly tested.

Thanks,
Tao


>Thanks.
>
>--
>tejun