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

[Luis Chamberlain]

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" ?

> > +	dir = debugfs_lookup(kobject_name(q->kobj.parent), blk_debugfs_root);
> > +	if (dir) {
> > +		pr_warn("%s: registering request_queue debugfs directory twice is not allowed\n",
> > +			kobject_name(q->kobj.parent));
> > +		dput(dir);
> > +		return -EALREADY;
> > +	}
> > +
> > +	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
> > +					    blk_debugfs_root);
> > +	if (!q->debugfs_dir)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> 
> kobject_name(q->kobj.parent) is used three times in the above function. How
> about introducing a local variable that holds the result of that expression?

Sure.

> > +static bool blk_trace_target_disk(const char *target, const char *diskname)
> > +{
> > +	if (strlen(target) != strlen(diskname))
> > +		return false;
> > +
> > +	if (!strncmp(target, diskname,
> > +		     min_t(size_t, strlen(target), strlen(diskname))))
> > +		return true;
> > +
> > +	return false;
> > +}
> 
> The above code looks weird to me. When the second if-statement is reached,
> it is guaranteed that 'target' and 'diskname' have the same length. So why
> to calculate the minimum length in the second if-statement of two strings
> that have the same length?

True, no need that that point. Will fix.

> 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.

  Luis