block: del_gendisk() vs blk_queue_enter() race condition

blk_queue_enter() sleeps forever, under ->open_mutex, there is no way for it to be woken up and to detect blk_queue_dying(). del_gendisk() sleeps forever because it attempts to grab ->open_mutex before it calls __blk_mark_disk_dead(), which would mark the queue QUEUE_FLAG_DYING and wake up ->mq_freeze_wq (which is blk_queue_enter() in this case).

I wonder how to fix it. My current "patch" is to set QUEUE_FLAG_DYING and "kick" ->mq_freeze_wq early on in del_gendisk(), before it attempts to grab ->open_mutex for the first time.

Quoted 9 lines · Sergey Senozhatsky

blk_queue_enter() sleeps forever, under ->open_mutex, there is no way for it to be woken up and to detect blk_queue_dying(). del_gendisk() sleeps forever because it attempts to grab ->open_mutex before it calls __blk_mark_disk_dead(), which would mark the queue QUEUE_FLAG_DYING and wake up ->mq_freeze_wq (which is blk_queue_enter() in this case).

I wonder how to fix it. My current "patch" is to set QUEUE_FLAG_DYING and "kick" ->mq_freeze_wq early on in del_gendisk(), before it attempts to grab ->open_mutex for the first time.

We split blk_queue_enter further to distinguish between file system requests and passthrough ones.

The file system request should be using bio_queue_enter, which only checks GD_DEAD, instead of QUEUE_FLAG_DYING. Passthrough requests like the cdrom door lock are using blk_queue_enter that checks QUEUE_FLAG_DYING which only gets set in blk_mq_destroy_queue.

So AFAICS your trace should not happen with the current kernel, but probably could happen with older stable version unless I'm missing something. What kernel version did you see this on?

.. actually, we still clear QUEUE_FLAG_DYING early. Something like the pathc below (plus proper comments) should sort it out:

block/genhd.c +0 -3

index 1c05dd4c6980b5..9a1e18fbb136cf 100644
@@ -589,9 +589,6 @@ static void __blk_mark_disk_dead(struct gendisk *disk)
 	if (test_and_set_bit(GD_DEAD, &disk->state))
 		return;
 
-	if (test_bit(GD_OWNS_QUEUE, &disk->state))
-		blk_queue_flag_set(QUEUE_FLAG_DYING, disk->queue);
-
 	/*
 	 * Stop buffered writers from dirtying pages that can't be written out.
 	 */

block/genhd.c +0 -0

 	if (!(disk->flags & GENHD_FL_HIDDEN)) {
 		sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");

On (24/10/03 06:43), Christoph Hellwig wrote:

So that mutex_lock(&disk->open_mutex) right before it potentially can deadlock (I think it will).

My idea, thus far, was to

	if (test_bit(GD_OWNS_QUEUE, &disk->state)) }
		blk_queue_flag_set(QUEUE_FLAG_DYING, disk->queue);
		blk_kick_queue_enter(disk->queue);  // this one simply wake_up_all(&q->mq_freeze_wq);
											// if the queue has QUEUE_FLAG_DYING
	}

in del_gendisk() before the very first time del_gendisk() attempts to mutex_lock(&disk->open_mutex), because that mutex is already locked forever.

Quoted 1 line · Sergey Senozhatsky

On (24/10/03 06:43), Christoph Hellwig wrote:

[..]

Quoted 11 lines

So that mutex_lock(&disk->open_mutex) right before it potentially can deadlock (I think it will).

My idea, thus far, was to

	if (test_bit(GD_OWNS_QUEUE, &disk->state)) }
		blk_queue_flag_set(QUEUE_FLAG_DYING, disk->queue);
		blk_kick_queue_enter(disk->queue);  // this one simply wake_up_all(&q->mq_freeze_wq);
											// if the queue has QUEUE_FLAG_DYING
	}

Quoted 3 lines

in del_gendisk() before the very first time del_gendisk() attempts to mutex_lock(&disk->open_mutex), because that mutex is already locked forever.

Dunno. Is something like this completely silly?

Date: Thu, 3 Oct 2024 23:45:10 -0700
From: Christoph Hellwig <[email protected]>
To: Sergey Senozhatsky <[email protected]>
Cc: Christoph Hellwig <[email protected]>, Jens Axboe <[email protected]>,
[email protected], Yang Yang <[email protected]>
Subject: Re: block: del_gendisk() vs blk_queue_enter() race condition
Message-ID: <[email protected]>

Quoted 1 line · Sergey Senozhatsky

Dunno. Is something like this completely silly?

__blk_mark_disk_dead got moved into the lock by: 7e04da2dc701 ("block: fix deadlock between sd_remove & sd_release"), which has a trace that looks very similar to the one your reported.

And that commit also points out something I missed - we do not set QUEUE_FLAG_DYING here because the gendisk does not own the queue for SCSI. Because of that allocating the request in sd/sr will not fail, and it will deadlock.

So I think the short term fix is to also fail passthrough request here - either by clearing and resurrecting QUEUE_FLAG_DYING or by also checking q->disk for GD_DEAD if it exists. Both of these are a bit ugly because they will fail passthrough through /dev/sg during the removal which is unexpected (although probably not happening for usual workloads).

The proper fix would be to split the freezing mechanism for file system vs passthrough I/O, but that's going to be a huge change.

block/genhd.c +2 -0

+	blk_queue_disk_dead(disk->queue);
+

block/blk-core.c +0 -0

index bc5e8c5eaac9..ccd36cb5ada7 100644
@@ -292,6 +292,16 @@ void blk_queue_start_drain(struct request_queue *q)
 	wake_up_all(&q->mq_freeze_wq);
 }

Quoted 8 lines · Sergey Senozhatsky

block/blk-core.c +0 -0

index bc5e8c5eaac9..ccd36cb5ada7 100644
@@ -292,6 +292,16 @@ void blk_queue_start_drain(struct request_queue *q)
 	wake_up_all(&q->mq_freeze_wq);
 }
 

block/blk-core.c void blk_queue_start_drain(struct request_queue *q) · line 303

Quoted 9 lines

block/blk-core.c +9 -0

+void blk_queue_disk_dead(struct request_queue *q)
+{
+	struct gendisk *disk = q->disk;
+
+	if (WARN_ON_ONCE(!test_bit(GD_DEAD, &disk->state)))
+		return;
+	/* Make blk_queue_enter() reexamine the GD_DEAD flag. */
+	wake_up_all(&q->mq_freeze_wq);
+}

Why is this a separate helper vs just doing the wake_up_all in the only caller that sets (with the suggested fixup anyway) GD_DEAD?

block/blk-core.c int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) · line 332

Quoted 2 lines

block/blk-core.c +2 -0

+			   blk_queue_dying(q) ||
+			   test_bit(GD_DEAD, &disk->state));

This needs to check for a NULL disk.

And now that I'm looking at the code a bit more this makes me worried that checking for q->disk here sounds like a good way to hit a race with clearing it. So I fear we need the other hack variant that sets QUEUE_FLAG_DYING unconditionally in __blk_mark_disk_dead and then clears it again (for GD_OWNS_QUEUE only) toward the end of del_gendisk.

Hmm, setting QUEUE_FLAG_DYING unconditionally in __blk_mark_disk_dead() implies moving it up, to the very top of del_gendisk(), before the first time we grab ->open_mutex, because that's the issue that we are having. Does this sound like re-introducing the previous deadlock scenario (the one you pointed at previously) because of that "don't acquire ->open_mutex after freezing the queue" thing?

Quoted 6 lines · Sergey Senozhatsky

Hmm, setting QUEUE_FLAG_DYING unconditionally in __blk_mark_disk_dead() implies moving it up, to the very top of del_gendisk(), before the first time we grab ->open_mutex, because that's the issue that we are having. Does this sound like re-introducing the previous deadlock scenario (the one you pointed at previously) because of that "don't acquire ->open_mutex after freezing the queue" thing?

So the trace of that one is literally the same as the one you reported, and I'm still wondering how they are related (I hope Yang Yang can chime in)

I suspect that if we mark both the disk and queue dead early that will error out everything and should fix it.

block/genhd.c +10 -8

+	/*
+	 * Tell the file system to write back all dirty data and shut down if
+	 * it hasn't been notified earlier.
+	 */
+	if (!test_bit(GD_DEAD, &disk->state))
+		blk_report_disk_dead(disk, false);
+	/* TODO: big fat comment */
+	blk_queue_flag_set(QUEUE_FLAG_DYING, disk->queue);
+	__blk_mark_disk_dead(disk);
+
 	/*
 	 * Prevent new openers by unlinked the bdev inode.
 	 */

@@ -657,18 +667,10 @@ void del_gendisk(struct gendisk *disk)
 		bdev_unhash(part);
 	mutex_unlock(&disk->open_mutex);
 
-	/*
-	 * Tell the file system to write back all dirty data and shut down if
-	 * it hasn't been notified earlier.
-	 */
-	if (!test_bit(GD_DEAD, &disk->state))
-		blk_report_disk_dead(disk, false);
-
 	/*
 	 * Drop all partitions now that the disk is marked dead.
 	 */
 	mutex_lock(&disk->open_mutex);
-	__blk_mark_disk_dead(disk);
 	xa_for_each_start(&disk->part_tbl, idx, part, 1)
 		drop_partition(part);
 	mutex_unlock(&disk->open_mutex);

block/genhd.c +1 -0

+		blk_queue_flag_clear(QUEUE_FLAG_DYING, disk->queue);

This needs to check for a NULL disk. And now that I'm looking at the code a bit more this makes me worried that checking for q->disk here sounds like a good way to hit a race with clearing it. So I fear we need the other hack variant that sets QUEUE_FLAG_DYING unconditionally in __blk_mark_disk_dead and then clears it again (for GD_OWNS_QUEUE only) toward the end of del_gendisk.

What kernel version did you see this on?

Hello,

I'm looking at a report from the fleet (don't have a reproducer) and wondering what you and block folks might think / suggest.

The problem is basically as follows

CPU0

do_syscall
sys_close
__fput

    blkdev_release
     blkdev_put              grabs ->open_mutex
      sr_block_release
       scsi_set_medium_removal
        ioctl_internal_command
         scsi_execute_cmd
          scsi_alloc_request
           blk_mq_alloc_request
            blk_queue_enter
             schedule

at the same time:

CPU1

usb_disconnect
usb_disable_device
device_del

    usb_unbind_interface
     usb_stor_disconnect
      scsi_remove_host
       scsi_forget_host
        __scsi_remove_device
         device_del
          bus_remove_device
           device_release_driver_internal
            sr_remove
             del_gendisk
              mutex_lock     attempts to grab ->open_mutex
               schedule

Quoted 30 lines · Sergey Senozhatsky

Hello,

I'm looking at a report from the fleet (don't have a reproducer) and wondering what you and block folks might think / suggest.

The problem is basically as follows

CPU0

do_syscall
sys_close
__fput

    blkdev_release
     blkdev_put              grabs ->open_mutex
      sr_block_release
       scsi_set_medium_removal
        ioctl_internal_command
         scsi_execute_cmd
          scsi_alloc_request
           blk_mq_alloc_request
            blk_queue_enter
             schedule

at the same time:

CPU1

usb_disconnect
usb_disable_device
device_del

Quoted 13 lines

    usb_unbind_interface
     usb_stor_disconnect
      scsi_remove_host
       scsi_forget_host
        __scsi_remove_device
         device_del
          bus_remove_device
           device_release_driver_internal
            sr_remove
             del_gendisk
              mutex_lock     attempts to grab ->open_mutex
               schedule

I'm a little confused here. How is the queue getting frozen in this scenario?

Or another report:

sr 1:0:0:0: Power-on or device reset occurred
sr 1:0:0:0: [sr0] scsi3-mmc drive: 8x/24x writer dvd-ram cd/rw xa/form2 cdda tray usb 1-1.3.1: USB disconnect, device number 27

  schedule+0x554/0x1218
  schedule_preempt_disabled+0x30/0x50
  mutex_lock+0x3c/0x70
  del_gendisk+0xe8/0x370
  sr_remove+0x30/0x58 [sr_mod (HASH:d5f2 4)]
  device_release_driver_internal+0x1a0/0x278
  device_release_driver+0x24/0x38
  bus_remove_device+0x150/0x170
  device_del+0x1d0/0x348
  __scsi_remove_device+0xb4/0x198
  scsi_forget_host+0x5c/0x80
  scsi_remove_host+0x98/0x1c8
  usb_stor_disconnect+0x74/0x110
  usb_unbind_interface+0xcc/0x250
  device_release_driver_internal+0x1a0/0x278
  device_release_driver+0x24/0x38
  bus_remove_device+0x150/0x170
  device_del+0x1d0/0x348
  usb_disable_device+0x88/0x190
  usb_disconnect+0xf8/0x318

  schedule+0x554/0x1218
  blk_queue_enter+0xd0/0x170

  blk_mq_alloc_request+0x138/0x1e8
  scsi_execute_cmd+0x88/0x258
  scsi_test_unit_ready+0x88/0x118
  sr_drive_status+0x5c/0x160 [sr_mod (HASH:d5f2 4)]
  cdrom_ioctl+0x7d4/0x2730 [cdrom (HASH:37c3 5)]
  sr_block_ioctl+0xa8/0x110 [sr_mod (HASH:d5f2 4)]
  blkdev_ioctl+0x468/0xbf0
  __arm64_sys_ioctl+0x254/0x6d0

Didn't copy one more backtrace here, there are two mutexes involved.

  schedule+0x554/0x1218
  schedule_preempt_disabled+0x30/0x50
  mutex_lock+0x3c/0x70
  sr_block_release+0x2c/0x60 [sr_mod (HASH:d5f2 4)]
  blkdev_put+0x184/0x290
  blkdev_release+0x34/0x50
  __fput_sync+0xa8/0x2d8
  __arm64_sys_close+0x6c/0xd8
  invoke_syscall+0x78/0xf0

So process A holds cd->lock and sleeps in blk_queue_enter()
process B holds ->open_mutex and sleeps on cd->lock, which is owned by A process C sleeps on ->open_mutex, which is owned by B.

CD layer code this can be relatively easily avoided while cleaning a lot of the code up. Give me a little time to cook something up.

Quoted 12 lines · Sergey Senozhatsky

Didn't copy one more backtrace here, there are two mutexes involved.

  schedule+0x554/0x1218
  schedule_preempt_disabled+0x30/0x50
  mutex_lock+0x3c/0x70
  sr_block_release+0x2c/0x60 [sr_mod (HASH:d5f2 4)]
  blkdev_put+0x184/0x290
  blkdev_release+0x34/0x50
  __fput_sync+0xa8/0x2d8
  __arm64_sys_close+0x6c/0xd8
  invoke_syscall+0x78/0xf0

Quoted 3 lines

So process A holds cd->lock and sleeps in blk_queue_enter()
process B holds ->open_mutex and sleeps on cd->lock, which is owned by A process C sleeps on ->open_mutex, which is owned by B.

Oh, cd->mutex is a bit of a problem. And looking into the generic CD layer code this can be relatively easily avoided while cleaning a lot of the code up. Give me a little time to cook something up.

I also wonder if simulating a cdrom removal might be possible using qemu to help reproducing some of this.

cd->lock still falls a victim of
"blk_queue_enter() and blk_queue_start_drain() are both called under ->open_mutex" thingy, which seems like a primary problem here. No matter why blk_queue_enter() sleeps, draining under ->open_mutex, given that what we want to drain can hold ->open_mutex, sometimes isn't going to drain.

Quoted 5 lines · Sergey Senozhatsky

cd->lock still falls a victim of
"blk_queue_enter() and blk_queue_start_drain() are both called under ->open_mutex" thingy, which seems like a primary problem here. No matter why blk_queue_enter() sleeps, draining under ->open_mutex, given that what we want to drain can hold ->open_mutex, sometimes isn't going to drain.

Yes. So I think we'll need to move __blk_mark_disk_dead out of ->open_mutex again

it also isn't protected when calling blk_mark_disk_dead, but we'll also need to stop the SCSI LLDs from submitting new commands from
their ->relase routines. Let me cook up a little series..