[PATCH v5 0/3] fix blktrace debugfs entries leakage

[PATCH v5 0/3] fix blktrace debugfs entries leakage

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Changes in v5:
 - blk_trace_shutdown() can't be used for module, add a new patch to use
 inline function for blk_trace_remove() to fix build warning from v3.
 - add review tag for patch 2,3 that is the same from v3.

Changes in v4:
 - blk_trace_remove() will trigger build warning if blktrace config is
 not enabled, use blk_trace_shutdown() instead.

Changes in v3:
 - add a new patch to handle /dev/sg

Changes in v2:
 - cleanup bltkrace in disk_release() instead of blk_free_queue()

Yu Kuai (3):
  blktrace: use inline function for blk_trace_remove() while blktrace is
    disabled
  scsi: sg: fix blktrace debugfs entries leakage
  block: fix blktrace debugfs entries leakage

 block/genhd.c                | 5 ++++-
 drivers/scsi/sg.c            | 9 +++++++++
 include/linux/blktrace_api.h | 6 +++++-
 3 files changed, 18 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH v5 1/3] blktrace: use inline function for blk_trace_remove() while blktrace is disabled

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

If config is disabled, call blk_trace_remove() directly will trigger
build warning, hence use inline function instead, prepare to fix
blktrace debugfs entries leakage.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 include/linux/blktrace_api.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index cfbda114348c..122c62e561fc 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -85,10 +85,14 @@ extern int blk_trace_remove(struct request_queue *q);
 # define blk_add_driver_data(rq, data, len)		do {} while (0)
 # define blk_trace_setup(q, name, dev, bdev, arg)	(-ENOTTY)
 # define blk_trace_startstop(q, start)			(-ENOTTY)
-# define blk_trace_remove(q)				(-ENOTTY)
 # define blk_add_trace_msg(q, fmt, ...)			do { } while (0)
 # define blk_add_cgroup_trace_msg(q, cg, fmt, ...)	do { } while (0)
 # define blk_trace_note_message_enabled(q)		(false)
+
+static inline int blk_trace_remove(struct request_queue *q)
+{
+	return -ENOTTY;
+}
 #endif /* CONFIG_BLK_DEV_IO_TRACE */
 
 #ifdef CONFIG_COMPAT
-- 
2.39.2

Re: [PATCH v5 1/3] blktrace: use inline function for blk_trace_remove() while blktrace is disabled

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v5 2/3] scsi: sg: fix blktrace debugfs entries leakage

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

sg_ioctl() support to enable blktrace, which will create debugfs entries
"/sys/kernel/debug/block/sgx/", however, there is no guarantee that user
will remove these entries through ioctl, and deleting sg device doesn't
cleanup these blktrace entries.

This problem can be fixed by cleanup blktrace while releasing
request_queue, however, it's not a good idea to do this special handling
in common layer just for sg device.

Fix this problem by shutdown bltkrace in sg_device_destroy(), where the
device is deleted and all the users close the device, also grab a
scsi_device reference from sg_add_device() to prevent scsi_device to be
freed before sg_device_destroy();

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sg.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 037f8c98a6d3..0adfbd77437f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1496,6 +1496,10 @@ sg_add_device(struct device *cl_dev)
 	int error;
 	unsigned long iflags;
 
+	error = scsi_device_get(scsidp);
+	if (error)
+		return error;
+
 	error = -ENOMEM;
 	cdev = cdev_alloc();
 	if (!cdev) {
@@ -1553,6 +1557,7 @@ sg_add_device(struct device *cl_dev)
 out:
 	if (cdev)
 		cdev_del(cdev);
+	scsi_device_put(scsidp);
 	return error;
 }
 
@@ -1560,6 +1565,7 @@ static void
 sg_device_destroy(struct kref *kref)
 {
 	struct sg_device *sdp = container_of(kref, struct sg_device, d_ref);
+	struct request_queue *q = sdp->device->request_queue;
 	unsigned long flags;
 
 	/* CAUTION!  Note that the device can still be found via idr_find()
@@ -1567,6 +1573,9 @@ sg_device_destroy(struct kref *kref)
 	 * any other cleanup.
 	 */
 
+	blk_trace_remove(q);
+	scsi_device_put(sdp->device);
+
 	write_lock_irqsave(&sg_index_lock, flags);
 	idr_remove(&sg_index_idr, sdp->index);
 	write_unlock_irqrestore(&sg_index_lock, flags);
-- 
2.39.2

Re: [PATCH v5 2/3] scsi: sg: fix blktrace debugfs entries leakage

[Martin K. Petersen]

> Fix this problem by shutdown bltkrace in sg_device_destroy(), where
> the device is deleted and all the users close the device, also grab a
> scsi_device reference from sg_add_device() to prevent scsi_device to
> be freed before sg_device_destroy();

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v5 3/3] block: fix blktrace debugfs entries leakage

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Commit 99d055b4fd4b ("block: remove per-disk debugfs files in
blk_unregister_queue") moves blk_trace_shutdown() from
blk_release_queue() to blk_unregister_queue(), this is safe if blktrace
is created through sysfs, however, there is a regression in corner
case.

blktrace can still be enabled after del_gendisk() through ioctl if
the disk is opened before del_gendisk(), and if blktrace is not shutdown
through ioctl before closing the disk, debugfs entries will be leaked.

Fix this problem by shutdown blktrace in disk_release(), this is safe
because blk_trace_remove() is reentrant.

Fixes: 99d055b4fd4b ("block: remove per-disk debugfs files in blk_unregister_queue")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/genhd.c b/block/genhd.c
index 4e5fd6aaa883..5689f5b5187c 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -25,8 +25,9 @@
 #include <linux/pm_runtime.h>
 #include <linux/badblocks.h>
 #include <linux/part_stat.h>
-#include "blk-throttle.h"
+#include <linux/blktrace_api.h>
 
+#include "blk-throttle.h"
 #include "blk.h"
 #include "blk-mq-sched.h"
 #include "blk-rq-qos.h"
@@ -1148,6 +1149,8 @@ static void disk_release(struct device *dev)
 	might_sleep();
 	WARN_ON_ONCE(disk_live(disk));
 
+	blk_trace_remove(disk->queue);
+
 	/*
 	 * To undo the all initialization from blk_mq_init_allocated_queue in
 	 * case of a probe failure where add_disk is never called we have to
-- 
2.39.2

Re: [PATCH v5 0/3] fix blktrace debugfs entries leakage

[Jens Axboe]

On Sat, 10 Jun 2023 10:20:00 +0800, Yu Kuai wrote:
> Changes in v5:
>  - blk_trace_shutdown() can't be used for module, add a new patch to use
>  inline function for blk_trace_remove() to fix build warning from v3.
>  - add review tag for patch 2,3 that is the same from v3.
> 
> Changes in v4:
>  - blk_trace_remove() will trigger build warning if blktrace config is
>  not enabled, use blk_trace_shutdown() instead.
> 
> [...]

Applied, thanks!

[1/3] blktrace: use inline function for blk_trace_remove() while blktrace is disabled
      commit: cbe7cff4a76bc749dd70264ca5cf924e2adf9296
[2/3] scsi: sg: fix blktrace debugfs entries leakage
      commit: db59133e927916d8a25ee1fd8264f2808040909d
[3/3] block: fix blktrace debugfs entries leakage
      commit: dd7de3704af9989b780693d51eaea49a665bd9c2

Best regards,
-- 
Jens Axboe