Re: block: del_gendisk() vs blk_queue_enter() race condition

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (24/10/03 23:45), Christoph Hellwig wrote:
> Date: Thu, 3 Oct 2024 23:45:10 -0700
> From: Christoph Hellwig <hch@infradead.org>
> To: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Christoph Hellwig <hch@infradead.org>, Jens Axboe <axboe@kernel.dk>,
>  linux-block@vger.kernel.org, Yang Yang <yang.yang@vivo.com>
> Subject: Re: block: del_gendisk() vs blk_queue_enter() race condition
> Message-ID: <Zv-O9tldIzPfD8ju@infradead.org>
> 
> On Fri, Oct 04, 2024 at 01:21:27PM +0900, Sergey Senozhatsky wrote:
> > 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.

Hmm, okay, a deadlock one way or another.

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

I see.  Thanks for the pointers.

> 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).

You are way ahead of me.  Does the below diff look like "checking for
GD_DEAD"?

---

diff --git a/block/blk-core.c b/block/blk-core.c
index bc5e8c5eaac9..ccd36cb5ada7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -292,6 +292,16 @@ void blk_queue_start_drain(struct request_queue *q)
 	wake_up_all(&q->mq_freeze_wq);
 }
 
+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);
+}
+
 /**
  * blk_queue_enter() - try to increase q->q_usage_counter
  * @q: request queue pointer
@@ -302,6 +312,8 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
 	const bool pm = flags & BLK_MQ_REQ_PM;
 
 	while (!blk_try_enter_queue(q, pm)) {
+		struct gendisk *disk = q->disk;
+
 		if (flags & BLK_MQ_REQ_NOWAIT)
 			return -EAGAIN;
 
@@ -316,8 +328,9 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
 		wait_event(q->mq_freeze_wq,
 			   (!q->mq_freeze_depth &&
 			    blk_pm_resume_queue(pm, q)) ||
-			   blk_queue_dying(q));
-		if (blk_queue_dying(q))
+			   blk_queue_dying(q) ||
+			   test_bit(GD_DEAD, &disk->state));
+		if (blk_queue_dying(q) || test_bit(GD_DEAD, &disk->state))
 			return -ENODEV;
 	}
 
diff --git a/block/genhd.c b/block/genhd.c
index 1c05dd4c6980..c213a0cf8268 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -583,12 +583,6 @@ static void blk_report_disk_dead(struct gendisk *disk, bool surprise)
 
 static void __blk_mark_disk_dead(struct gendisk *disk)
 {
-	/*
-	 * Fail any new I/O.
-	 */
-	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);
 
@@ -649,6 +643,12 @@ void del_gendisk(struct gendisk *disk)
 
 	disk_del_events(disk);
 
+	/*
+	 * Fail any new I/O.
+	 */
+	test_bit(GD_DEAD, &disk->state);
+	blk_queue_disk_dead(disk->queue);
+
 	/*
 	 * Prevent new openers by unlinked the bdev inode.
 	 */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 50c3b959da28..aaaa6fa12328 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -862,6 +862,7 @@ extern int blk_lld_busy(struct request_queue *q);
 extern int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags);
 extern void blk_queue_exit(struct request_queue *q);
 extern void blk_sync_queue(struct request_queue *q);
+void blk_queue_disk_dead(struct request_queue *q);
 
 /* Helper to convert REQ_OP_XXX to its string format XXX */
 extern const char *blk_op_str(enum req_op op);

---

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

My preference would be a simpler short-term fix (cherry-pick to older
kernels are much easier this way).