Re: [PATCH v2] blk-mq: take quiesce_lock when removing a queue from the tag set

[Nilay Shroff]

On 11/6/26 10:23 PM, Mani Tofigh via B4 Relay wrote:
> From: Mani Tofigh<mt4022@xxxxxxxxxxxx>
>
> Unquiescing a tag set can run the hw queues of a queue whose hctxs were
> already freed.  blk_mq_unquiesce_tagset() walks the tag set's queues
> under quiesce_lock and rcu_read_lock().  Commit 71ec3c7d6db8 ("blk-mq:
> quiesce a queue that joins a quiesced tag set") added quiesce_lock to
> the walks and to adding a queue, but blk_mq_del_queue_tag_set() still
> removes a queue without it.  The hctxs are then freed right away since
> RCU only keeps the request_queue itself alive:
>
>    Task A (controller reset)            Task B (namespace removal)
>    blk_mq_unquiesce_tagset()
>      /* finds q */
>                                         blk_mq_del_queue_tag_set(q)
>                                         blk_mq_release(q)
>                                           /* frees the hctxs */
>      blk_mq_run_hw_queues(q)
>        /* reads freed hctxs */
>
> The window is short.  On PREEMPT_RT quiesce_lock is a sleeping lock and
> the walk can be preempted inside it, which is how it's hit here:
>
>    [  399.851277] BUG: KASAN: slab-use-after-free in blk_mq_run_hw_queues+0x4bd/0x550
>    [  399.851328] Read of size 8 at addr ff11000003014148 by task kworker/u9:2/116
>    [  399.851352] CPU: 1 UID: 0 PID: 116 Comm: kworker/u9:2 Not tainted 7.3.0-16299-g81cb9aa43246 #1 PREEMPT_RT
>    [  399.851425] Workqueue: nvme-reset-wq nvme_loop_reset_ctrl_work
>    [  399.851441] Call Trace:
>    [  399.851457]  <TASK>
>    [  399.851551]  blk_mq_run_hw_queues+0x4bd/0x550
>    [  399.851564]  blk_mq_unquiesce_tagset+0x128/0x210
>    [  399.851576]  nvme_loop_reset_ctrl_work+0x78/0x4c0
>    [  399.851600]  process_one_work+0x92b/0x16d0
>    [  399.851660]  worker_thread+0x4e6/0xd60
>    [  399.851707]  kthread+0x300/0x3e0
>    [  399.851727]  ret_from_fork+0x4f0/0x6f0
>    [  399.851776]  ret_from_fork_asm+0x1a/0x30
>    [  399.851793]  </TASK>
>    ...
>    [  399.851988] Freed by task 111:
>    [  399.852040]  kfree+0x1b3/0x4b0
>    [  399.852050]  blk_mq_release+0x400/0x550
>    [  399.852060]  blk_put_queue+0x94/0x120
>    [  399.852068]  disk_release+0x1bd/0x3f0
>    [  399.852102]  nvme_free_ns+0x39/0x270
>    [  399.852113]  nvme_remove_nsid_range+0x2d1/0x4a0
>    [  399.852123]  nvme_scan_ns_list+0x2cc/0x390
>    [  399.852134]  nvme_scan_work+0x421/0x9f0
>    ...
>
> With BLK_CGROUP the disk's root blkg holds the queue until a grace
> period later, which hides this for NVMe namespaces.  It's still open
> without BLK_CGROUP, and from reading the code also for queues freed
> without a disk (e.g., SCSI device queues on a UFS host).
>
> Take quiesce_lock around the list_del_rcu() as well so that the walk
> either finishes with a queue before it leaves the list or never sees
> it, and say so in the comment of quiesce_lock.  With the fix, KASAN
> doesn't report the use-after-free anymore.
>
> This bug was identified and confirmed with the help of a deterministic
> hypervisor currently under development.  The C reproducer below however
> hits it about once every 20 minutes on a PREEMPT_RT kernel.
>
> Fixes: 59e25ef2b413 ("block: Use RCU in blk_mq_[un]quiesce_tagset() instead of set->tag_list_lock")
> Cc:stable@xxxxxxxxxxxxxxx
> Reviewed-by: Mohamed Khalfella<mkhalfella@xxxxxxxxxxxxxxx>
> Signed-off-by: Mani Tofigh<mt4022@xxxxxxxxxxxx>

Looks good to me:

Reviewed-by: Nilay Shroff <nilay@xxxxxxxxxxxxx>