Re: [PATCH v2 03/10] blktrace: fix debugfs use after free

[Christoph Hellwig <hch@infradead.org>]

On Wed, Apr 22, 2020 at 07:48:02AM +0000, Luis Chamberlain wrote:
> > I don't see why we need this check.  If it is valueable enough we
> > should have a debugfs_create_dir_exclusive or so that retunrns an error
> > for an exsting directory, instead of reimplementing it in the caller in
> > a racy way.  But I'm not really sure we need it to start with.
> 
> In short races, and even with synchronous request_queue removal I'm
> seeing the race is still possible, but that's due to some other races
> I'm going to chase down now.
> 
> The easier solution really is to just have a debugfs dir created for
> each partition if debugfs is enabled, this way the directory will
> always be there, and the lookups are gone.

That sounds like the best plan to me.

> 
> > > +
> > > +	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
> > > +					    blk_debugfs_root);
> > > +	if (!q->debugfs_dir)
> > > +		return -ENOMEM;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void blk_queue_debugfs_unregister(struct request_queue *q)
> > > +{
> > > +	debugfs_remove_recursive(q->debugfs_dir);
> > > +	q->debugfs_dir = NULL;
> > > +}
> > 
> > Which to me suggests we can just fold these two into the callers,
> > with an IS_ENABLED for the creation case given that we check for errors
> > and the stub will always return an error.
> 
> Sorry not sure I follow this.

Don't both with the two above functions and just open code them in
the callers.  IFF you still want to check for errors after the
discussion with Greg, wrap the call in a

	if (IS_ENABLED(CONFIG_DEBUG_FS))

to ensure that you don't fail queue creation in the !DEBUG_FS
case.