Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

[Luis Chamberlain]

On Wed, May 27, 2020 at 03:12:02AM +0000, Luis Chamberlain wrote:
> You forgot to deal with partitions. Putting similar lipstick on the pig,
> this is what I end up with, let me know if this seems agreeable:

So even with the partition stuff in place, this approach still don't
allow multiple uses of blktrace against a scsi-generic device and its
backend real block device, say TYPE_DISK. A simple example is a scsi
drive hooked up used to allow users to do blktrace /dev/sda *and*
blktrace /dev/sg0, but with the proposed change /dev/sg0 no longer
works beacuse the dentry pertains to the '/dev/sda' name, not
'/dev/sg0'.

We can shoehorn in a solution following the style proposed as follows.
We can keep this only slightly cleaner if we don't care about the
extra dentry even if a user disables CONFIG_CHR_DEV_SG. The cost
would just be an extra dentry on the request_queue.

I'll run this through 0-day and then post a new hopefully final series,
but if you don't think this or would prefer something lease please let
me know.

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 86c107de2836..f46bdc7f6509 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -920,6 +920,9 @@ static void blk_release_queue(struct kobject *kobj)
 	blk_trace_shutdown(q);
 
 	debugfs_remove_recursive(q->debugfs_dir);
+#if defined(CONFIG_CHR_DEV_SG) || defined(CONFIG_CHR_DEV_SG_MODULE)
+	debugfs_remove_recursive(q->sg_debugfs_dir);
+#endif
 	if (queue_is_mq(q))
 		blk_mq_debugfs_unregister(q);
 
@@ -939,6 +942,21 @@ struct kobj_type blk_queue_ktype = {
 	.release	= blk_release_queue,
 };
 
+#if defined(CONFIG_CHR_DEV_SG) || defined(CONFIG_CHR_DEV_SG_MODULE)
+/**
+ * blk_sg_debugfs_init - initialize debugs for scsi-generic
+ * @q: the associated queue
+ * @name: name of the scsi-generic device
+ *
+ * To be used by scsi-generic for allowing it to use blktrace.
+ */
+void blk_sg_debugfs_init(struct request_queue *q, const char *name)
+{
+	q->sg_debugfs_dir = debugfs_create_dir(name, blk_debugfs_root);
+}
+EXPORT_SYMBOL_GPL(blk_sg_debugfs_init);
+#endif
+
 /**
  * blk_register_queue - register a block layer queue with sysfs
  * @disk: Disk of which the request queue should be registered with sysfs.
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 20472aaaf630..c87fe1923f3d 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1519,6 +1519,7 @@ static int
 sg_add_device(struct device *cl_dev, struct class_interface *cl_intf)
 {
 	struct scsi_device *scsidp = to_scsi_device(cl_dev->parent);
+	struct request_queue *q = scsidp->request_queue;
 	struct gendisk *disk;
 	Sg_device *sdp = NULL;
 	struct cdev * cdev = NULL;
@@ -1573,6 +1574,7 @@ sg_add_device(struct device *cl_dev, struct class_interface *cl_intf)
 	} else
 		pr_warn("%s: sg_sys Invalid\n", __func__);
 
+	blk_sg_debugfs_init(q, disk->disk_name);
 	sdev_printk(KERN_NOTICE, scsidp, "Attached scsi generic sg%d "
 		    "type %d\n", sdp->index, scsidp->type);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5877b03b8117..be5a40d59f60 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -575,6 +575,9 @@ struct request_queue {
 	struct bio_set		bio_split;
 
 	struct dentry		*debugfs_dir;
+#if defined(CONFIG_CHR_DEV_SG) || defined(CONFIG_CHR_DEV_SG_MODULE)
+	struct dentry		*sg_debugfs_dir;
+#endif
 #ifdef CONFIG_BLK_DEBUG_FS
 	struct dentry		*sched_debugfs_dir;
 	struct dentry		*rqos_debugfs_dir;
@@ -858,6 +861,14 @@ static inline void rq_flush_dcache_pages(struct request *rq)
 
 extern int blk_register_queue(struct gendisk *disk);
 extern void blk_unregister_queue(struct gendisk *disk);
+#if defined(CONFIG_CHR_DEV_SG) || defined(CONFIG_CHR_DEV_SG_MODULE)
+extern void blk_sg_debugfs_init(struct request_queue *q, const char *name);
+#else
+static inline void blk_sg_debugfs_init(struct request_queue *q,
+				       const char *name)
+{
+}
+#endif
 extern blk_qc_t generic_make_request(struct bio *bio);
 extern blk_qc_t direct_make_request(struct bio *bio);
 extern void blk_rq_init(struct request_queue *q, struct request *rq);
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index a55cbfd060f5..5b0310f38e11 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -511,6 +511,11 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	 */
 	if (bdev && bdev != bdev->bd_contains) {
 		dir = bdev->bd_part->debugfs_dir;
+	} else if (q->sg_debugfs_dir &&
+		   strlen(buts->name) == strlen(q->sg_debugfs_dir->d_name.name)
+		   && strcmp(buts->name, q->sg_debugfs_dir->d_name.name) == 0) {
+		/* scsi-generic requires use of its own directory */
+		dir = q->sg_debugfs_dir;
 	} else {
 		/*
 		 * For queues that do not have a gendisk attached to them, that