Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
From: Luis Chamberlain <mcgrof@kernel.org>
Date: 2020-06-09 17:29:35
Also in:
linux-fsdevel, linux-mm, lkml
I like this, more below. On Tue, Jun 09, 2020 at 08:06:02AM -0700, Christoph Hellwig wrote:
quoted hunk ↗ jump to hunk
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c index 432fa60e7f8808..44239f603379d5 100644 --- a/kernel/trace/blktrace.c +++ b/kernel/trace/blktrace.c@@ -492,34 +493,23 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, */ strreplace(buts->name, '/', '_'); - /* - * We have to use a partition directory if a partition is being worked - * on. The same request_queue is shared between all partitions. - */ - if (bdev && bdev != bdev->bd_contains) { - dir = bdev->bd_part->debugfs_dir; - } else if (IS_ENABLED(CONFIG_CHR_DEV_SG) && - MAJOR(dev) == SCSI_GENERIC_MAJOR) { + bt = kzalloc(sizeof(*bt), GFP_KERNEL); + if (!bt) + return -ENOMEM; + + if (unlikely(!bdev)) { /* - * scsi-generic exposes the request_queue through the /dev/sg* - * interface but since that uses a different path than whatever - * the respective scsi driver device name may expose and use - * for the request_queue debugfs_dir. We have a dedicated - * dentry for scsi-generic then. + * When tracing something that is not a block device (e.g. the + * /dev/sg nodes), create debugfs directory on demand. This + * directory will be remove when stopping the trace.
Is scsi-generic is the only unwanted ugly child blktrace has to deal with? For some reason I thought drivers/md/md.c was one but it seems like it is not. Do we have an easy way to search for these? I think this would just affect how we express the comment only.
*/ - dir = q->sg_debugfs_dir; + dir = debugfs_create_dir(buts->name, blk_debugfs_root); + bt->dir = dir;
The other chicken and egg problem to consider at least in the comments is that the debugfs directory for these types of devices *have* an exposed path, but the data structure is rather opaque to the device and even blktrace. Fortunately given the recent set of changes around the q->blk_trace and clarifications around its use we have made it clear now that so long as hold the q->blk_trace_mutex *and* check q->blk_trace we *should* not race against two separate creations of debugfs directories, so I think this is safe, so long as these indpendent drivers don't end up re-using the same path for some other things later in the future, and since we have control over what goes under debugfsroot block / I think we should be good. But I think that the concern for race on names may still be worth explaining a bit here. Luis