Re: [PATCH blktests] block/002: delay debugfs directory check

[Ming Lei <ming.lei@redhat.com>]

On Wed, Apr 20, 2022 at 12:42:14PM +0000, Shinichiro Kawasaki wrote:
> On Apr 20, 2022 / 17:34, Ming Lei wrote:
> > On Wed, Apr 20, 2022 at 01:59:11PM +0900, Shin'ichiro Kawasaki wrote:
> > > The test case block/002 checks that device removal during blktrace run
> > > does not leak debugfs directory. The Linux kernel commit 0a9a25ca7843
> > > ("block: let blkcg_gq grab request queue's refcnt") triggered failure of
> > > the test case. The commit delayed queue release and debugfs directory
> > > removal then the test case checks directory existence too early. To
> > > avoid this false-positive failure, delay the directory existence check.
> > > 
> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > ---
> > >  tests/block/002 | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tests/block/002 b/tests/block/002
> > > index 9b183e7..8061c91 100755
> > > --- a/tests/block/002
> > > +++ b/tests/block/002
> > > @@ -29,6 +29,7 @@ test() {
> > >  		echo "debugfs directory deleted with blktrace active"
> > >  	fi
> > >  	{ kill $!; wait; } >/dev/null 2>/dev/null
> > > +	sleep 0.5
> > >  	if [[ -d /sys/kernel/debug/block/${SCSI_DEBUG_DEVICES[0]} ]]; then
> > >  		echo "debugfs directory leaked"
> > >  	fi
> > 
> > Hello,
> > 
> > Jens has merged Yu Kuai's fix[1], so I think it won't be triggered now.
> > 
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=block-5.18&id=a87c29e1a85e64b28445bb1e80505230bf2e3b4b
> 
> Hi Ming, I applied the patch above on top of v5.18-rc3 and ran block/002.
> Unfortunately, it failed with a new symptom with KASAN use-after-free [2]. I
> ran block/002 with linux-block/block-5.18 branch tip with git hash a87c29e1a85e
> and got the same KASAN uaf. Reverting the patch from the linux-block/block-5.18
> branch, the KASAN uaf disappears (Still block/002 fails). Regarding block/002,
> it looks the patch made the failure symptom worse.

Hi Shinichiro,

Looks Yu Kuai's patch has other problem, can you drop that patch and
apply & test the attached patch?

Jens, looks the patch of "blk-mq: fix possible creation failure for 'debugfs_dir'"
isn't ready to go, can you drop it first from block-5.18?

diff --git a/block/blk-core.c b/block/blk-core.c
index f305cb66c72a..c41c415849d9 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -438,6 +438,7 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
 {
 	struct request_queue *q;
 	int ret;
+	char q_name[32];
 
 	q = kmem_cache_alloc_node(blk_get_queue_kmem_cache(alloc_srcu),
 			GFP_KERNEL | __GFP_ZERO, node_id);
@@ -495,6 +496,9 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu)
 	blk_set_default_limits(&q->limits);
 	q->nr_requests = BLKDEV_DEFAULT_RQ;
 
+	snprintf(q_name, 32, "%d", q->id);
+	q->debugfs_dir = debugfs_create_dir(q_name, blk_debugfs_root);
+
 	return q;
 
 fail_stats:
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 88bd41d4cb59..651ec10a5a87 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -837,8 +837,8 @@ int blk_register_queue(struct gendisk *disk)
 	}
 
 	mutex_lock(&q->debugfs_mutex);
-	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
-					    blk_debugfs_root);
+	q->debugfs_dir = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
+			blk_debugfs_root, kobject_name(q->kobj.parent));
 	mutex_unlock(&q->debugfs_mutex);
 
 	if (queue_is_mq(q)) {
@@ -913,6 +913,7 @@ int blk_register_queue(struct gendisk *disk)
 void blk_unregister_queue(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
+	char q_name[32];
 
 	if (WARN_ON(!q))
 		return;
@@ -951,5 +952,11 @@ void blk_unregister_queue(struct gendisk *disk)
 
 	mutex_unlock(&q->sysfs_dir_lock);
 
+	mutex_lock(&q->debugfs_mutex);
+	snprintf(q_name, 32, "%d", q->id);
+	q->debugfs_dir = debugfs_rename(blk_debugfs_root, q->debugfs_dir,
+			blk_debugfs_root, q_name);
+	mutex_unlock(&q->debugfs_mutex);
+
 	kobject_put(&disk_to_dev(disk)->kobj);
 }

Thanks,
Ming