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

[Tao Cui <cui.tao@linux.dev>]

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