Re: [PATCH v4 3/5] blktrace: fix debugfs use after free

[Luis Chamberlain <mcgrof@kernel.org>]

On Sat, May 09, 2020 at 05:58:55PM -0700, Bart Van Assche wrote:
> On 2020-05-08 20:10, Luis Chamberlain wrote:
> > Screenshots of what the debugfs for block looks like after running
> > blktrace on a system with sg0  which has a raid controllerand then sg1
> > as the media changer:
> > 
> >  # ls -l /sys/kernel/debug/block
> > total 0
> > drwxr-xr-x  3 root root 0 May  9 02:31 bsg
> > drwxr-xr-x 19 root root 0 May  9 02:31 nvme0n1
> > drwxr-xr-x 19 root root 0 May  9 02:31 nvme1n1
> > lrwxrwxrwx  1 root root 0 May  9 02:31 nvme1n1p1 -> nvme1n1
> > lrwxrwxrwx  1 root root 0 May  9 02:31 nvme1n1p2 -> nvme1n1
> > lrwxrwxrwx  1 root root 0 May  9 02:31 nvme1n1p3 -> nvme1n1
> > lrwxrwxrwx  1 root root 0 May  9 02:31 nvme1n1p5 -> nvme1n1
> > lrwxrwxrwx  1 root root 0 May  9 02:31 nvme1n1p6 -> nvme1n1
> > drwxr-xr-x  2 root root 0 May  9 02:33 sch0
> > lrwxrwxrwx  1 root root 0 May  9 02:33 sg0 -> bsg/2:0:0:0
> > lrwxrwxrwx  1 root root 0 May  9 02:33 sg1 -> sch0
> > drwxr-xr-x  5 root root 0 May  9 02:31 vda
> > lrwxrwxrwx  1 root root 0 May  9 02:31 vda1 -> vda
> 
> So this patch creates one soft link per partition at partition creation
> time instead of letting the blktrace code create one directory per
> partition when tracing starts?

Yes.

> Does this break running blktrace
> simultaneously for a partition and for the entire block device?

blktrace already has this limitation, one blktrace is only allowed per
request_queue, the next patch clarifies this, as we currently just error
out without telling the user what has happened.e user what has
happened.e user what has happened.e user what has happened.

> > +static struct dentry *queue_debugfs_symlink_type(struct request_queue *q,
> > +						 const char *src,
> > +						 const char *dst,
> > +						 enum blk_debugfs_dir_type type)
> > +{
> > +	struct dentry *dentry = ERR_PTR(-EINVAL);
> > +	char *dir_dst;
> > +
> > +	dir_dst = kzalloc(PATH_MAX, GFP_KERNEL);
> > +	if (!dir_dst)
> > +		return dentry;
> > +
> > +	switch (type) {
> > +	case BLK_DBG_DIR_BASE:
> > +		if (dst)
> > +			snprintf(dir_dst, PATH_MAX, "%s", dst);
> > +		else if (!IS_ERR_OR_NULL(q->debugfs_dir))
> > +			snprintf(dir_dst, PATH_MAX, "%s",
> > +				 q->debugfs_dir->d_name.name);
> > +		else
> > +			goto out;
> > +		break;
> > +	case BLK_DBG_DIR_BSG:
> > +		if (dst)
> > +			snprintf(dir_dst, PATH_MAX, "bsg/%s", dst);
> > +		else
> > +			goto out;
> > +		break;
> > +	}
> > +
> > +	/*
> > +	 * The base block debugfs directory is always used for the symlinks,
> > +	 * their target is what changes.
> > +	 */
> > +	dentry = debugfs_create_symlink(src, blk_debugfs_root, dir_dst);
> > +out:
> > +	kfree(dir_dst);
> > +
> > +	return dentry;
> > +}
> 
> Please use kasprintf() instead of k?alloc() followed by snprintf().

Sure thing.

  Luis