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

[Bart Van Assche]

On 4/19/20 5:04 PM, Luis Chamberlain wrote:
> On Sun, Apr 19, 2020 at 02:55:42PM -0700, Bart Van Assche wrote:
> > On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > > +int __must_check blk_queue_debugfs_register(struct request_queue *q)
> > > +{
> > > +	struct dentry *dir = NULL;
> > > +
> > > +	/* This can happen if we have a bug in the lower layers */
> >
> > What does "this" refer to? Which layers does "lower layers" refer to? Most
> > software developers consider a module that calls directly into another
> > module as a higher layer (callbacks through function pointers do not count;
> > see also https://en.wikipedia.org/wiki/Modular_programming). According to
> > that definition block drivers are a software layer immediately above the
> > block layer core.
> >
> > How about changing that comment into the following to make it unambiguous
> > (if this is what you meant)?
> >
> > 	/*
> > 	 * Check whether the debugfs directory already exists. This can
> > 	 * only happen as the result of a bug in a block driver.
> > 	 */
>
> But I didn't mean on a block driver. I meant the block core. In our
> case, the async request_queue removal is an example. There is nothing
> that block drivers could have done to help much with that.
>
> I could just change "lower layers" to "block layer" ?

That sounds good to me.

> > Independent of what the purpose of the above code is, can that code be
> > rewritten such that it does not depend on the details of how names are
> > assigned to disks and partitions? Would disk_get_part() be useful here?
>
> I did try, but couldn't figure out a way. I'll keep looking but likewise
> let me know if you find a way.

How about making blk_trace_setup() pass the result of the following 
expression as an additional argument to blk_trace_setup():

	bdev != bdev->bd_contains

I think that is a widely used approach to verify after a block device 
has been opened whether or not 'bdev' refers to a partition (bdev != 
bdev->bd_contains means that 'bdev' represents a partition).

Thanks,

Bart.