Re: [PATCH v7 6/8] blktrace: fix debugfs use after free

[Luis Chamberlain <mcgrof@kernel.org>]

On Sat, Jun 20, 2020 at 10:31:51AM -0700, Bart Van Assche wrote:
> On 2020-06-19 13:47, Luis Chamberlain wrote:
> > This goes tested with:
>        ^^^^
>        got?
> 
> > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > index 7ff2ea5cd05e..e6e2d25fdbd6 100644
> > --- a/kernel/trace/blktrace.c
> > +++ b/kernel/trace/blktrace.c
> > @@ -524,10 +524,18 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
> >  	if (!bt->msg_data)
> >  		goto err;
> >  
> > -	ret = -ENOENT;
> > -
> > -	dir = debugfs_lookup(buts->name, blk_debugfs_root);
> > -	if (!dir)
> > +#ifdef CONFIG_BLK_DEBUG_FS
> > +	/*
> > +	 * When tracing whole make_request drivers (multiqueue) block devices,
> > +	 * reuse the existing debugfs directory created by the block layer on
> > +	 * init. For request-based block devices, all partitions block devices,
>                                                   ^^^^^^^^^^^^^^^^^^^^^
> It seems like a word is missing from the comment? Or did you perhaps
> want to refer to "all partition block devices"?

Yes, the later.

> > +	 * and scsi-generic block devices we create a temporary new debugfs
> > +	 * directory that will be removed once the trace ends.
> > +	 */
> > +	if (queue_is_mq(q) && bdev && bdev == bdev->bd_contains)
> > +		dir = q->debugfs_dir;
> > +	else
> > +#endif
> >  		bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> 
> Can it happen that two different threads each try to set up block
> tracing and hence that debugfs_create_dir() fails because a directory
> with name buts->name already exists?

Great question, the answer is no. The reason is that we first use the
mutex and then we check for q->blk_trace. If you hold the lock *and*
you have checked for q->blk_trace and its NULL, you are sure you should
not have a duplicate.

Its why the commit log mentioned:

  The new clarifications on relying on the q->blk_trace_mutex *and* also
  checking for q->blk_trace *prior* to processing a blktrace ensures the
  debugfs directories are only created if no possible directory name
  clashes are possible.

These clarifications were prompted through discussions with Jan Kara
on the patches he posted which you CC'd me on. I agreed with his
patch *but* I suggested it would hold true only if check for the
q->blk_trace first, and this is why my patch titled "blktrace: break
out of blktrace setup on concurrent calls" got merged prior to Jan
Kara's "blktrace: Avoid sparse warnings when assigning q->blk_trace".

> >  	bt->dev = dev;
> > @@ -565,8 +573,6 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
> >  
> >  	ret = 0;
> >  err:
> > -	if (dir && !bt->dir)
> > -		dput(dir);
> >  	if (ret)
> >  		blk_trace_free(bt);
> >  	return ret;
> 
> Shouldn't bt->dir be removed in this error path for make_request drivers?

If there is an error, bt->dir will be removed still, as I never modified
the removal of bt->dir in this patch.

  Luis