[PATCH] block: Prevent potential deadlock in blk_revalidate_disk_zones()

[PATCH] block: Prevent potential deadlock in blk_revalidate_disk_zones()

[Damien Le Moal]

The function blk_revalidate_disk_zones() calls the function
disk_update_zone_resources() after freezing the device queue. In turn,
disk_update_zone_resources() calls queue_limits_start_update() which
takes a queue limits mutex lock, resulting in the ordering:
q->q_usage_counter check -> q->limits_lock. However, the usual ordering
is to always take a queue limit lock before freezing the queue to commit
the limits updates, e.g., the code pattern:

lim = queue_limits_start_update(q);
...
blk_mq_freeze_queue(q);
ret = queue_limits_commit_update(q, &lim);
blk_mq_unfreeze_queue(q);

Thus, blk_revalidate_disk_zones() introduces a potential circular
locking dependency deadlock that lockdep sometimes catches with the
splat:

[   51.934109] ======================================================
[   51.935916] WARNING: possible circular locking dependency detected
[   51.937561] 6.12.0+ #2107 Not tainted
[   51.938648] ------------------------------------------------------
[   51.940351] kworker/u16:4/157 is trying to acquire lock:
[   51.941805] ffff9fff0aa0bea8 (&q->limits_lock){+.+.}-{4:4}, at: disk_update_zone_resources+0x86/0x170
[   51.944314]
               but task is already holding lock:
[   51.945688] ffff9fff0aa0b890 (&q->q_usage_counter(queue)#3){++++}-{0:0}, at: blk_revalidate_disk_zones+0x15f/0x340
[   51.948527]
               which lock already depends on the new lock.

[   51.951296]
               the existing dependency chain (in reverse order) is:
[   51.953708]
               -> #1 (&q->q_usage_counter(queue)#3){++++}-{0:0}:
[   51.956131]        blk_queue_enter+0x1c9/0x1e0
[   51.957290]        blk_mq_alloc_request+0x187/0x2a0
[   51.958365]        scsi_execute_cmd+0x78/0x490 [scsi_mod]
[   51.959514]        read_capacity_16+0x111/0x410 [sd_mod]
[   51.960693]        sd_revalidate_disk.isra.0+0x872/0x3240 [sd_mod]
[   51.962004]        sd_probe+0x2d7/0x520 [sd_mod]
[   51.962993]        really_probe+0xd5/0x330
[   51.963898]        __driver_probe_device+0x78/0x110
[   51.964925]        driver_probe_device+0x1f/0xa0
[   51.965916]        __driver_attach_async_helper+0x60/0xe0
[   51.967017]        async_run_entry_fn+0x2e/0x140
[   51.968004]        process_one_work+0x21f/0x5a0
[   51.968987]        worker_thread+0x1dc/0x3c0
[   51.969868]        kthread+0xe0/0x110
[   51.970377]        ret_from_fork+0x31/0x50
[   51.970983]        ret_from_fork_asm+0x11/0x20
[   51.971587]
               -> #0 (&q->limits_lock){+.+.}-{4:4}:
[   51.972479]        __lock_acquire+0x1337/0x2130
[   51.973133]        lock_acquire+0xc5/0x2d0
[   51.973691]        __mutex_lock+0xda/0xcf0
[   51.974300]        disk_update_zone_resources+0x86/0x170
[   51.975032]        blk_revalidate_disk_zones+0x16c/0x340
[   51.975740]        sd_zbc_revalidate_zones+0x73/0x160 [sd_mod]
[   51.976524]        sd_revalidate_disk.isra.0+0x465/0x3240 [sd_mod]
[   51.977824]        sd_probe+0x2d7/0x520 [sd_mod]
[   51.978917]        really_probe+0xd5/0x330
[   51.979915]        __driver_probe_device+0x78/0x110
[   51.981047]        driver_probe_device+0x1f/0xa0
[   51.982143]        __driver_attach_async_helper+0x60/0xe0
[   51.983282]        async_run_entry_fn+0x2e/0x140
[   51.984319]        process_one_work+0x21f/0x5a0
[   51.985873]        worker_thread+0x1dc/0x3c0
[   51.987289]        kthread+0xe0/0x110
[   51.988546]        ret_from_fork+0x31/0x50
[   51.989926]        ret_from_fork_asm+0x11/0x20
[   51.991376]
               other info that might help us debug this:

[   51.994127]  Possible unsafe locking scenario:

[   51.995651]        CPU0                    CPU1
[   51.996694]        ----                    ----
[   51.997716]   lock(&q->q_usage_counter(queue)#3);
[   51.998817]                                lock(&q->limits_lock);
[   52.000043]                                lock(&q->q_usage_counter(queue)#3);
[   52.001638]   lock(&q->limits_lock);
[   52.002485]
                *** DEADLOCK ***

Prevent this issue by moving the calls to blk_mq_freeze_queue() and
blk_mq_unfreeze_queue() around the call to queue_limits_commit_update()
in disk_update_zone_resources(). disk_free_zone_resources() is also
modified to operate with the queue frozen as before by adding calls to
blk_mq_freeze_queue() and blk_mq_unfreeze_queue().

Fixes: 843283e96e5a ("block: Fake max open zones limit when there is no limit")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-zoned.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 70211751df16..1f961088b813 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -1468,9 +1468,13 @@ static unsigned int disk_set_conv_zones_bitmap(struct gendisk *disk,
 
 void disk_free_zone_resources(struct gendisk *disk)
 {
+	struct request_queue *q = disk->queue;
+
 	if (!disk->zone_wplugs_pool)
 		return;
 
+	blk_mq_freeze_queue(q);
+
 	cancel_work_sync(&disk->zone_wplugs_work);
 
 	if (disk->zone_wplugs_wq) {
@@ -1493,6 +1497,8 @@ void disk_free_zone_resources(struct gendisk *disk)
 	disk->zone_capacity = 0;
 	disk->last_zone_capacity = 0;
 	disk->nr_zones = 0;
+
+	blk_mq_unfreeze_queue(q);
 }
 
 static inline bool disk_need_zone_resources(struct gendisk *disk)
@@ -1551,6 +1557,7 @@ static int disk_update_zone_resources(struct gendisk *disk,
 	unsigned int nr_seq_zones, nr_conv_zones;
 	unsigned int pool_size;
 	struct queue_limits lim;
+	int ret;
 
 	disk->nr_zones = args->nr_zones;
 	disk->zone_capacity = args->zone_capacity;
@@ -1601,7 +1608,11 @@ static int disk_update_zone_resources(struct gendisk *disk,
 	}
 
 commit:
-	return queue_limits_commit_update(q, &lim);
+	blk_mq_freeze_queue(q);
+	ret = queue_limits_commit_update(q, &lim);
+	blk_mq_unfreeze_queue(q);
+
+	return ret;
 }
 
 static int blk_revalidate_conv_zone(struct blk_zone *zone, unsigned int idx,
@@ -1816,14 +1827,12 @@ int blk_revalidate_disk_zones(struct gendisk *disk)
 	 * Set the new disk zone parameters only once the queue is frozen and
 	 * all I/Os are completed.
 	 */
-	blk_mq_freeze_queue(q);
 	if (ret > 0)
 		ret = disk_update_zone_resources(disk, &args);
 	else
 		pr_warn("%s: failed to revalidate zones\n", disk->disk_name);
 	if (ret)
 		disk_free_zone_resources(disk);
-	blk_mq_unfreeze_queue(q);
 
 	return ret;
 }
-- 
2.47.0

Re: [PATCH] block: Prevent potential deadlock in blk_revalidate_disk_zones()

[Christoph Hellwig]

On Tue, Nov 26, 2024 at 05:53:42PM +0900, Damien Le Moal wrote:
> in disk_update_zone_resources(). disk_free_zone_resources() is also
> modified to operate with the queue frozen as before by adding calls to
> blk_mq_freeze_queue() and blk_mq_unfreeze_queue().

This now adds a queue freeze to disk_release for zoned device, which
previously didn't have it.  Given that at this point no I/O on the
disk is possible, and the freezes are quite expensive that's probably
not a good idea.

> -	blk_mq_freeze_queue(q);
>  	if (ret > 0)
>  		ret = disk_update_zone_resources(disk, &args);
>  	else
>  		pr_warn("%s: failed to revalidate zones\n", disk->disk_name);
>  	if (ret)
>  		disk_free_zone_resources(disk);
> -	blk_mq_unfreeze_queue(q);

So for a minimal version you could keep the freezing here.

Re: [PATCH] block: Prevent potential deadlock in blk_revalidate_disk_zones()

[Damien Le Moal]

On 11/26/24 19:33, Christoph Hellwig wrote:
> On Tue, Nov 26, 2024 at 05:53:42PM +0900, Damien Le Moal wrote:
>> in disk_update_zone_resources(). disk_free_zone_resources() is also
>> modified to operate with the queue frozen as before by adding calls to
>> blk_mq_freeze_queue() and blk_mq_unfreeze_queue().
> 
> This now adds a queue freeze to disk_release for zoned device, which
> previously didn't have it.  Given that at this point no I/O on the
> disk is possible, and the freezes are quite expensive that's probably
> not a good idea.
> 
>> -	blk_mq_freeze_queue(q);
>>  	if (ret > 0)
>>  		ret = disk_update_zone_resources(disk, &args);
>>  	else
>>  		pr_warn("%s: failed to revalidate zones\n", disk->disk_name);
>>  	if (ret)
>>  		disk_free_zone_resources(disk);
>> -	blk_mq_unfreeze_queue(q);
> 
> So for a minimal version you could keep the freezing here.

Good point. Sending V2 with that fixed.


-- 
Damien Le Moal
Western Digital Research