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

From: Luis Chamberlain
Date: Mon May 11 2020 - 09:44:11 EST


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