Re: [PATCH] block: fix "Directory XXXXX with parent 'block' already present!"

["yukuai (C)" <yukuai3@xxxxxxxxxx>]

在 2022/04/22 1:28, Hannes Reinecke 写道:
> On 4/21/22 10:34, Ming Lei wrote:
> > q->debugfs_dir is used by blk-mq debugfs and blktrace. The dentry is
> > created when adding disk, and removed when releasing request queue.
> >
> > There is small window between releasing disk and releasing request
> > queue, and during the period, one disk with same name may be created
> > and added, so debugfs_create_dir() may complain with "Directory XXXXX
> > with parent 'block' already present!"
> >
> > Fixes the issue by moving debugfs_create_dir() into blk_alloc_queue(),
> > and the dir name is named with q->id from beginning, and switched to
> > disk name when adding disk, and finally changed to q->id in 
> > disk_release().
> >
> > Reported-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Cc: yukuai (C) <yukuai3@xxxxxxxxxx>
> > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> >   block/blk-core.c  | 4 ++++
> >   block/blk-sysfs.c | 4 ++--
> >   block/genhd.c     | 8 ++++++++
> >   3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index f305cb66c72a..245ec664753d 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[16];
> >       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;
> > +    sprintf(q_name, "%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..1f986c20a07b 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)) {
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 36532b931841..08895f9f7087 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -25,6 +25,7 @@
> >   #include <linux/pm_runtime.h>
> >   #include <linux/badblocks.h>
> >   #include <linux/part_stat.h>
> > +#include <linux/debugfs.h>
> >   #include "blk-throttle.h"
> >   #include "blk.h"
> > @@ -1160,6 +1161,7 @@ static void disk_release_mq(struct request_queue 
> > *q)
> >   static void disk_release(struct device *dev)
> >   {
> >       struct gendisk *disk = dev_to_disk(dev);
> > +    char q_name[16];
> >       might_sleep();
> >       WARN_ON_ONCE(disk_live(disk));
> > @@ -1173,6 +1175,12 @@ static void disk_release(struct device *dev)
> >       kfree(disk->random);
> >       xa_destroy(&disk->part_tbl);
> > +    mutex_lock(&disk->queue->debugfs_mutex);
> > +    sprintf(q_name, "%d", disk->queue->id);
> > +    disk->queue->debugfs_dir = debugfs_rename(blk_debugfs_root,
> > +            disk->queue->debugfs_dir, blk_debugfs_root, q_name);
> > +    mutex_unlock(&disk->queue->debugfs_mutex);
> > +
> >       disk->queue->disk = NULL;
> >       blk_put_queue(disk->queue);
>
> I don't think this is the right approach.
>  From my POV the underlying reason is an imbalance between 
> debugfs_create_dir() (which happens in blk_register_queue()) and
> debugfs_remove_dir() (which happens in blk_release_queue())
>
> So there is a small race window between blk_unregister_queue() and 
> blk_release_queue(), during which the queue might be re-registered and 
> then traipses over the (still-existant) queue.
>
> So we should rather move the call to debugfs_remove_dir() into 
> blk_unregister_queue() to have them both symmetric.
>
> Basically the patch '[PATCH RESEND] blk-mq: fix possible creation 
> failure for 'debugfs_dir'' from yukuai ...
Hi,

I forgot to move 'q->rqos_debugfs_dir' which causes a UAF in
block/002, and Ming was worried that:

blktrace still may work for passthrough req trace after disk is
deleted.

I can shutdown blktrace in blk_unregister_queue(), however I was
worried that concurrent blk_trace_setup() might reenable it.

I can add some delay and check if this is possible, if so, we might
need some other guarantee that blktrace will not running after
blk_unregister_queue().

Thanks,
Kuai
> Cheers,
>
> Hannes