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

From: Luis Chamberlain <mcgrof@kernel.org>
To: Bart Van Assche <bvanassche@acm.org>
Cc: axboe@kernel.dk, viro@zeniv.linux.org.uk,
	gregkh@linuxfoundation.org, rostedt@goodmis.org,
	mingo@redhat.com, jack@suse.cz, ming.lei@redhat.com,
	nstange@suse.de, akpm@linux-foundation.org, mhocko@suse.com,
	yukuai3@huawei.com, linux-block@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, Omar Sandoval <osandov@fb.com>,
	Hannes Reinecke <hare@suse.com>, Michal Hocko <mhocko@kernel.org>,
	syzbot+603294af2d01acfdd6da@syzkaller.appspotmail.com
Subject: Re: [PATCH v2 03/10] blktrace: fix debugfs use after free
Date: Mon, 20 Apr 2020 00:04:36 +0000	[thread overview]
Message-ID: <20200420000436.GI11244@42.do-not-panic.com> (raw)
In-Reply-To: <91c82e6a-24ce-0b7d-e6e4-e8aa89f3fb79@acm.org>

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