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

[Bart Van Assche <bvanassche@acm.org>]

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.