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

From: Bart Van Assche <bvanassche@acm.org>
To: Luis Chamberlain <mcgrof@kernel.org>,
	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
Cc: 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: Sun, 19 Apr 2020 14:55:42 -0700	[thread overview]
Message-ID: <91c82e6a-24ce-0b7d-e6e4-e8aa89f3fb79@acm.org> (raw)
In-Reply-To: <20200419194529.4872-4-mcgrof@kernel.org>

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

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

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

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?

Thanks,

Bart.