linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* bdi: fix use-after-free for dev_name(bdi->dev)
@ 2020-04-16  7:15 Christoph Hellwig
  2020-04-16  7:15 ` [PATCH 1/8] bdi: move bdi_dev_name out of line Christoph Hellwig
                   ` (8 more replies)
  0 siblings, 9 replies; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16  7:15 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

Hi all,

the first three patches are my take on the proposal from Yufen Yu
to fix the use after free of the device name of the bdi device.

The rest is vaguely related cleanups.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 1/8] bdi: move bdi_dev_name out of line
  2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
@ 2020-04-16  7:15 ` Christoph Hellwig
  2020-04-16  7:52   ` Greg KH
  2020-04-16 12:32   ` Jan Kara
  2020-04-16  7:15 ` [PATCH 2/8] bdi: use bdi_dev_name() to get device name Christoph Hellwig
                   ` (7 subsequent siblings)
  8 siblings, 2 replies; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16  7:15 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

bdi_dev_name is not a fast path function, move it out of line.  This
prepares for using it from modular callers without having to export
an implementation detail like bdi_unknown_name.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/backing-dev.h |  9 +--------
 mm/backing-dev.c            | 10 +++++++++-
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index f88197c1ffc2..c9ad5c3b7b4b 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -505,13 +505,6 @@ static inline int bdi_rw_congested(struct backing_dev_info *bdi)
 				  (1 << WB_async_congested));
 }
 
-extern const char *bdi_unknown_name;
-
-static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
-{
-	if (!bdi || !bdi->dev)
-		return bdi_unknown_name;
-	return dev_name(bdi->dev);
-}
+const char *bdi_dev_name(struct backing_dev_info *bdi);
 
 #endif	/* _LINUX_BACKING_DEV_H */
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c81b4f3a7268..c2c44c89ee5d 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -21,7 +21,7 @@ struct backing_dev_info noop_backing_dev_info = {
 EXPORT_SYMBOL_GPL(noop_backing_dev_info);
 
 static struct class *bdi_class;
-const char *bdi_unknown_name = "(unknown)";
+static const char *bdi_unknown_name = "(unknown)";
 
 /*
  * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
@@ -1043,6 +1043,14 @@ void bdi_put(struct backing_dev_info *bdi)
 }
 EXPORT_SYMBOL(bdi_put);
 
+const char *bdi_dev_name(struct backing_dev_info *bdi)
+{
+	if (!bdi || !bdi->dev)
+		return bdi_unknown_name;
+	return dev_name(bdi->dev);
+}
+EXPORT_SYMBOL_GPL(bdi_dev_name);
+
 static wait_queue_head_t congestion_wqh[2] = {
 		__WAIT_QUEUE_HEAD_INITIALIZER(congestion_wqh[0]),
 		__WAIT_QUEUE_HEAD_INITIALIZER(congestion_wqh[1])
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 2/8] bdi: use bdi_dev_name() to get device name
  2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
  2020-04-16  7:15 ` [PATCH 1/8] bdi: move bdi_dev_name out of line Christoph Hellwig
@ 2020-04-16  7:15 ` Christoph Hellwig
  2020-04-16  7:52   ` Greg KH
  2020-04-16  7:15 ` [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info Christoph Hellwig
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16  7:15 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

From: Yufen Yu <yuyufen@huawei.com>

Use the common interface bdi_dev_name() to get device name.

Signed-off-by: Yufen Yu <yuyufen@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bfq-iosched.c        | 5 +++--
 block/blk-cgroup.c         | 2 +-
 fs/ceph/debugfs.c          | 2 +-
 include/trace/events/wbt.h | 8 ++++----
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 78ba57efd16b..4d4fe44a9eea 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4976,8 +4976,9 @@ bfq_set_next_ioprio_data(struct bfq_queue *bfqq, struct bfq_io_cq *bic)
 	ioprio_class = IOPRIO_PRIO_CLASS(bic->ioprio);
 	switch (ioprio_class) {
 	default:
-		dev_err(bfqq->bfqd->queue->backing_dev_info->dev,
-			"bfq: bad prio class %d\n", ioprio_class);
+		pr_err("bdi %s: bfq: bad prio class %d\n",
+				bdi_dev_name(bfqq->bfqd->queue->backing_dev_info),
+				ioprio_class);
 		/* fall through */
 	case IOPRIO_CLASS_NONE:
 		/*
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index c5dc833212e1..930212c1a512 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -496,7 +496,7 @@ const char *blkg_dev_name(struct blkcg_gq *blkg)
 {
 	/* some drivers (floppy) instantiate a queue w/o disk registered */
 	if (blkg->q->backing_dev_info->dev)
-		return dev_name(blkg->q->backing_dev_info->dev);
+		return bdi_dev_name(blkg->q->backing_dev_info);
 	return NULL;
 }
 
diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 481ac97b4d25..dcaed75de9e6 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -271,7 +271,7 @@ void ceph_fs_debugfs_init(struct ceph_fs_client *fsc)
 				    &congestion_kb_fops);
 
 	snprintf(name, sizeof(name), "../../bdi/%s",
-		 dev_name(fsc->sb->s_bdi->dev));
+		 bdi_dev_name(fsc->sb->s_bdi));
 	fsc->debugfs_bdi =
 		debugfs_create_symlink("bdi",
 				       fsc->client->debugfs_dir,
diff --git a/include/trace/events/wbt.h b/include/trace/events/wbt.h
index 37342a13c9cb..9996420d7ec4 100644
--- a/include/trace/events/wbt.h
+++ b/include/trace/events/wbt.h
@@ -33,7 +33,7 @@ TRACE_EVENT(wbt_stat,
 	),
 
 	TP_fast_assign(
-		strlcpy(__entry->name, dev_name(bdi->dev),
+		strlcpy(__entry->name, bdi_dev_name(bdi),
 			ARRAY_SIZE(__entry->name));
 		__entry->rmean		= stat[0].mean;
 		__entry->rmin		= stat[0].min;
@@ -68,7 +68,7 @@ TRACE_EVENT(wbt_lat,
 	),
 
 	TP_fast_assign(
-		strlcpy(__entry->name, dev_name(bdi->dev),
+		strlcpy(__entry->name, bdi_dev_name(bdi),
 			ARRAY_SIZE(__entry->name));
 		__entry->lat = div_u64(lat, 1000);
 	),
@@ -105,7 +105,7 @@ TRACE_EVENT(wbt_step,
 	),
 
 	TP_fast_assign(
-		strlcpy(__entry->name, dev_name(bdi->dev),
+		strlcpy(__entry->name, bdi_dev_name(bdi),
 			ARRAY_SIZE(__entry->name));
 		__entry->msg	= msg;
 		__entry->step	= step;
@@ -141,7 +141,7 @@ TRACE_EVENT(wbt_timer,
 	),
 
 	TP_fast_assign(
-		strlcpy(__entry->name, dev_name(bdi->dev),
+		strlcpy(__entry->name, bdi_dev_name(bdi),
 			ARRAY_SIZE(__entry->name));
 		__entry->status		= status;
 		__entry->step		= step;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info
  2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
  2020-04-16  7:15 ` [PATCH 1/8] bdi: move bdi_dev_name out of line Christoph Hellwig
  2020-04-16  7:15 ` [PATCH 2/8] bdi: use bdi_dev_name() to get device name Christoph Hellwig
@ 2020-04-16  7:15 ` Christoph Hellwig
  2020-04-16  7:52   ` Greg KH
  2020-04-16  8:34   ` Yufen Yu
  2020-04-16  7:15 ` [PATCH 4/8] driver core: remove device_create_vargs Christoph Hellwig
                   ` (5 subsequent siblings)
  8 siblings, 2 replies; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16  7:15 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

Cache a copy of the name for the life time of the backing_dev_info
structure so that we can reference it even after unregistering.

Fixes: 68f23b89067f ("memcg: fix a crash in wb_workfn when a device disappears")
Reported-by: Yufen Yu <yuyufen@huawei.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/backing-dev-defs.h |  1 +
 mm/backing-dev.c                 | 13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index 4fc87dee005a..249590bcccf7 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -220,6 +220,7 @@ struct backing_dev_info {
 	wait_queue_head_t wb_waitq;
 
 	struct device *dev;
+	const char *dev_name;
 	struct device *owner;
 
 	struct timer_list laptop_mode_wb_timer;
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c2c44c89ee5d..4f6c05df72f9 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -938,9 +938,15 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
 	if (bdi->dev)	/* The driver needs to use separate queues per device */
 		return 0;
 
-	dev = device_create_vargs(bdi_class, NULL, MKDEV(0, 0), bdi, fmt, args);
-	if (IS_ERR(dev))
+	bdi->dev_name = kvasprintf(GFP_KERNEL, fmt, args);
+	if (!bdi->dev_name)
+		return -ENOMEM;
+
+	dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
+	if (IS_ERR(dev)) {
+		kfree(bdi->dev_name);
 		return PTR_ERR(dev);
+	}
 
 	cgwb_bdi_register(bdi);
 	bdi->dev = dev;
@@ -1034,6 +1040,7 @@ static void release_bdi(struct kref *ref)
 	WARN_ON_ONCE(bdi->dev);
 	wb_exit(&bdi->wb);
 	cgwb_bdi_exit(bdi);
+	kfree(bdi->dev_name);
 	kfree(bdi);
 }
 
@@ -1047,7 +1054,7 @@ const char *bdi_dev_name(struct backing_dev_info *bdi)
 {
 	if (!bdi || !bdi->dev)
 		return bdi_unknown_name;
-	return dev_name(bdi->dev);
+	return bdi->dev_name;
 }
 EXPORT_SYMBOL_GPL(bdi_dev_name);
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 4/8] driver core: remove device_create_vargs
  2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
                   ` (2 preceding siblings ...)
  2020-04-16  7:15 ` [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info Christoph Hellwig
@ 2020-04-16  7:15 ` Christoph Hellwig
  2020-04-16  7:52   ` Greg KH
  2020-04-16  7:15 ` [PATCH 5/8] bdi: unexport bdi_register_va Christoph Hellwig
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16  7:15 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

All external users of device_create_vargs are gone, so remove it and
open code it in the only caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/base/core.c    | 37 ++-----------------------------------
 include/linux/device.h |  4 ----
 2 files changed, 2 insertions(+), 39 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 139cdf7e7327..fb8ae248e5aa 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3188,40 +3188,6 @@ device_create_groups_vargs(struct class *class, struct device *parent,
 	return ERR_PTR(retval);
 }
 
-/**
- * device_create_vargs - creates a device and registers it with sysfs
- * @class: pointer to the struct class that this device should be registered to
- * @parent: pointer to the parent struct device of this new device, if any
- * @devt: the dev_t for the char device to be added
- * @drvdata: the data to be added to the device for callbacks
- * @fmt: string for the device's name
- * @args: va_list for the device's name
- *
- * This function can be used by char device classes.  A struct device
- * will be created in sysfs, registered to the specified class.
- *
- * A "dev" file will be created, showing the dev_t for the device, if
- * the dev_t is not 0,0.
- * If a pointer to a parent struct device is passed in, the newly created
- * struct device will be a child of that device in sysfs.
- * The pointer to the struct device will be returned from the call.
- * Any further sysfs files that might be required can be created using this
- * pointer.
- *
- * Returns &struct device pointer on success, or ERR_PTR() on error.
- *
- * Note: the struct class passed to this function must have previously
- * been created with a call to class_create().
- */
-struct device *device_create_vargs(struct class *class, struct device *parent,
-				   dev_t devt, void *drvdata, const char *fmt,
-				   va_list args)
-{
-	return device_create_groups_vargs(class, parent, devt, drvdata, NULL,
-					  fmt, args);
-}
-EXPORT_SYMBOL_GPL(device_create_vargs);
-
 /**
  * device_create - creates a device and registers it with sysfs
  * @class: pointer to the struct class that this device should be registered to
@@ -3253,7 +3219,8 @@ struct device *device_create(struct class *class, struct device *parent,
 	struct device *dev;
 
 	va_start(vargs, fmt);
-	dev = device_create_vargs(class, parent, devt, drvdata, fmt, vargs);
+	dev = device_create_groups_vargs(class, parent, devt, drvdata, NULL,
+					  fmt, vargs);
 	va_end(vargs);
 	return dev;
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index ac8e37cd716a..15460a5ac024 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -884,10 +884,6 @@ extern bool device_is_bound(struct device *dev);
 /*
  * Easy functions for dynamically creating devices on the fly
  */
-extern __printf(5, 0)
-struct device *device_create_vargs(struct class *cls, struct device *parent,
-				   dev_t devt, void *drvdata,
-				   const char *fmt, va_list vargs);
 extern __printf(5, 6)
 struct device *device_create(struct class *cls, struct device *parent,
 			     dev_t devt, void *drvdata,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 5/8] bdi: unexport bdi_register_va
  2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
                   ` (3 preceding siblings ...)
  2020-04-16  7:15 ` [PATCH 4/8] driver core: remove device_create_vargs Christoph Hellwig
@ 2020-04-16  7:15 ` Christoph Hellwig
  2020-04-16  7:53   ` Greg KH
  2020-04-16 12:03   ` Jan Kara
  2020-04-16  7:15 ` [PATCH 6/8] bdi: remove bdi_register_owner Christoph Hellwig
                   ` (3 subsequent siblings)
  8 siblings, 2 replies; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16  7:15 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

bdi_register_va is only used by super.c, which can't be modular.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/backing-dev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 4f6c05df72f9..a7eb91146c9c 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -969,7 +969,6 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
 	trace_writeback_bdi_register(bdi);
 	return 0;
 }
-EXPORT_SYMBOL(bdi_register_va);
 
 int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...)
 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 6/8] bdi: remove bdi_register_owner
  2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
                   ` (4 preceding siblings ...)
  2020-04-16  7:15 ` [PATCH 5/8] bdi: unexport bdi_register_va Christoph Hellwig
@ 2020-04-16  7:15 ` Christoph Hellwig
  2020-04-16  7:53   ` Greg KH
  2020-04-16 12:05   ` Jan Kara
  2020-04-16  7:15 ` [PATCH 7/8] bdi: simplify bdi_alloc Christoph Hellwig
                   ` (2 subsequent siblings)
  8 siblings, 2 replies; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16  7:15 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

Split out a new bdi_set_owner helper to set the owner, and move the policy
for creating the bdi name back into genhd.c, where it belongs.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c               |  8 +++++---
 include/linux/backing-dev.h |  2 +-
 mm/backing-dev.c            | 12 ++----------
 3 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 06b642b23a07..7d10cfc38c70 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -840,13 +840,15 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 		disk->flags |= GENHD_FL_SUPPRESS_PARTITION_INFO;
 		disk->flags |= GENHD_FL_NO_PART_SCAN;
 	} else {
+		struct backing_dev_info *bdi = disk->queue->backing_dev_info;
+		struct device *dev = disk_to_dev(disk);
 		int ret;
 
 		/* Register BDI before referencing it from bdev */
-		disk_to_dev(disk)->devt = devt;
-		ret = bdi_register_owner(disk->queue->backing_dev_info,
-						disk_to_dev(disk));
+		dev->devt = devt;
+		ret = bdi_register(bdi, "%u:%u", MAJOR(devt), MINOR(devt));
 		WARN_ON(ret);
+		bdi_set_owner(bdi, dev);
 		blk_register_region(disk_devt(disk), disk->minors, NULL,
 				    exact_match, exact_lock, disk);
 	}
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index c9ad5c3b7b4b..4098ed6ba6b4 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -33,7 +33,7 @@ int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...);
 __printf(2, 0)
 int bdi_register_va(struct backing_dev_info *bdi, const char *fmt,
 		    va_list args);
-int bdi_register_owner(struct backing_dev_info *bdi, struct device *owner);
+void bdi_set_owner(struct backing_dev_info *bdi, struct device *owner);
 void bdi_unregister(struct backing_dev_info *bdi);
 
 struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id);
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index a7eb91146c9c..1ba9a7b30933 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -982,20 +982,12 @@ int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...)
 }
 EXPORT_SYMBOL(bdi_register);
 
-int bdi_register_owner(struct backing_dev_info *bdi, struct device *owner)
+void bdi_set_owner(struct backing_dev_info *bdi, struct device *owner)
 {
-	int rc;
-
-	rc = bdi_register(bdi, "%u:%u", MAJOR(owner->devt), MINOR(owner->devt));
-	if (rc)
-		return rc;
-	/* Leaking owner reference... */
-	WARN_ON(bdi->owner);
+	WARN_ON_ONCE(bdi->owner);
 	bdi->owner = owner;
 	get_device(owner);
-	return 0;
 }
-EXPORT_SYMBOL(bdi_register_owner);
 
 /*
  * Remove bdi from bdi_list, and ensure that it is no longer visible
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 7/8] bdi: simplify bdi_alloc
  2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
                   ` (5 preceding siblings ...)
  2020-04-16  7:15 ` [PATCH 6/8] bdi: remove bdi_register_owner Christoph Hellwig
@ 2020-04-16  7:15 ` Christoph Hellwig
  2020-04-16  7:54   ` Greg KH
  2020-04-16 12:06   ` Jan Kara
  2020-04-16  7:15 ` [PATCH 8/8] bdi: remove the name field in struct backing_dev_info Christoph Hellwig
  2020-04-16 15:29 ` bdi: fix use-after-free for dev_name(bdi->dev) Jens Axboe
  8 siblings, 2 replies; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16  7:15 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

Merge the _node vs normal version and drop the superflous gfp_t argument.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c            | 2 +-
 drivers/mtd/mtdcore.c       | 2 +-
 fs/super.c                  | 2 +-
 include/linux/backing-dev.h | 6 +-----
 mm/backing-dev.c            | 7 +++----
 5 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 7e4a1da0715e..ab87f2833ab2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -484,7 +484,7 @@ struct request_queue *__blk_alloc_queue(int node_id)
 	if (ret)
 		goto fail_id;
 
-	q->backing_dev_info = bdi_alloc_node(GFP_KERNEL, node_id);
+	q->backing_dev_info = bdi_alloc(node_id);
 	if (!q->backing_dev_info)
 		goto fail_split;
 
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 2916674208b3..39ec563d9a14 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -2036,7 +2036,7 @@ static struct backing_dev_info * __init mtd_bdi_init(char *name)
 	struct backing_dev_info *bdi;
 	int ret;
 
-	bdi = bdi_alloc(GFP_KERNEL);
+	bdi = bdi_alloc(NUMA_NO_NODE);
 	if (!bdi)
 		return ERR_PTR(-ENOMEM);
 
diff --git a/fs/super.c b/fs/super.c
index cd352530eca9..dd28fcd706ff 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1598,7 +1598,7 @@ int super_setup_bdi_name(struct super_block *sb, char *fmt, ...)
 	int err;
 	va_list args;
 
-	bdi = bdi_alloc(GFP_KERNEL);
+	bdi = bdi_alloc(NUMA_NO_NODE);
 	if (!bdi)
 		return -ENOMEM;
 
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 4098ed6ba6b4..6b3504bf7a42 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -36,11 +36,7 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt,
 void bdi_set_owner(struct backing_dev_info *bdi, struct device *owner);
 void bdi_unregister(struct backing_dev_info *bdi);
 
-struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id);
-static inline struct backing_dev_info *bdi_alloc(gfp_t gfp_mask)
-{
-	return bdi_alloc_node(gfp_mask, NUMA_NO_NODE);
-}
+struct backing_dev_info *bdi_alloc(int node_id);
 
 void wb_start_background_writeback(struct bdi_writeback *wb);
 void wb_workfn(struct work_struct *work);
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 1ba9a7b30933..119a41650833 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -865,12 +865,11 @@ static int bdi_init(struct backing_dev_info *bdi)
 	return ret;
 }
 
-struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id)
+struct backing_dev_info *bdi_alloc(int node_id)
 {
 	struct backing_dev_info *bdi;
 
-	bdi = kmalloc_node(sizeof(struct backing_dev_info),
-			   gfp_mask | __GFP_ZERO, node_id);
+	bdi = kzalloc_node(sizeof(*bdi), GFP_KERNEL, node_id);
 	if (!bdi)
 		return NULL;
 
@@ -880,7 +879,7 @@ struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id)
 	}
 	return bdi;
 }
-EXPORT_SYMBOL(bdi_alloc_node);
+EXPORT_SYMBOL(bdi_alloc);
 
 static struct rb_node **bdi_lookup_rb_node(u64 id, struct rb_node **parentp)
 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 8/8] bdi: remove the name field in struct backing_dev_info
  2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
                   ` (6 preceding siblings ...)
  2020-04-16  7:15 ` [PATCH 7/8] bdi: simplify bdi_alloc Christoph Hellwig
@ 2020-04-16  7:15 ` Christoph Hellwig
  2020-04-16  7:54   ` Greg KH
  2020-04-16 12:23   ` Jan Kara
  2020-04-16 15:29 ` bdi: fix use-after-free for dev_name(bdi->dev) Jens Axboe
  8 siblings, 2 replies; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16  7:15 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

The name is only printed for a not registered bdi in writeback.  Use the
device name there as is more useful anyway for the unlike case that the
warning triggers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c                 | 1 -
 drivers/mtd/mtdcore.c            | 1 -
 fs/fs-writeback.c                | 2 +-
 fs/super.c                       | 2 --
 include/linux/backing-dev-defs.h | 2 --
 mm/backing-dev.c                 | 1 -
 6 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index ab87f2833ab2..f37068c611bf 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -494,7 +494,6 @@ struct request_queue *__blk_alloc_queue(int node_id)
 
 	q->backing_dev_info->ra_pages = VM_READAHEAD_PAGES;
 	q->backing_dev_info->capabilities = BDI_CAP_CGROUP_WRITEBACK;
-	q->backing_dev_info->name = "block";
 	q->node = node_id;
 
 	timer_setup(&q->backing_dev_info->laptop_mode_wb_timer,
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 39ec563d9a14..fcb018ce17c3 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -2040,7 +2040,6 @@ static struct backing_dev_info * __init mtd_bdi_init(char *name)
 	if (!bdi)
 		return ERR_PTR(-ENOMEM);
 
-	bdi->name = name;
 	/*
 	 * We put '-0' suffix to the name to get the same name format as we
 	 * used to get. Since this is called only once, we get a unique name. 
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 76ac9c7d32ec..d85323607b49 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -2320,7 +2320,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 
 			WARN(bdi_cap_writeback_dirty(wb->bdi) &&
 			     !test_bit(WB_registered, &wb->state),
-			     "bdi-%s not registered\n", wb->bdi->name);
+			     "bdi-%s not registered\n", bdi_dev_name(wb->bdi));
 
 			inode->dirtied_when = jiffies;
 			if (dirtytime)
diff --git a/fs/super.c b/fs/super.c
index dd28fcd706ff..4991f441988e 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1602,8 +1602,6 @@ int super_setup_bdi_name(struct super_block *sb, char *fmt, ...)
 	if (!bdi)
 		return -ENOMEM;
 
-	bdi->name = sb->s_type->name;
-
 	va_start(args, fmt);
 	err = bdi_register_va(bdi, fmt, args);
 	va_end(args);
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index 249590bcccf7..0a55170b2142 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -194,8 +194,6 @@ struct backing_dev_info {
 	congested_fn *congested_fn; /* Function pointer if device is md/dm */
 	void *congested_data;	/* Pointer to aux data for congested func */
 
-	const char *name;
-
 	struct kref refcnt;	/* Reference counter for the structure */
 	unsigned int capabilities; /* Device capabilities */
 	unsigned int min_ratio;
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 119a41650833..dc7215dfd56b 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -15,7 +15,6 @@
 #include <trace/events/writeback.h>
 
 struct backing_dev_info noop_backing_dev_info = {
-	.name		= "noop",
 	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK,
 };
 EXPORT_SYMBOL_GPL(noop_backing_dev_info);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH 4/8] driver core: remove device_create_vargs
  2020-04-16  7:15 ` [PATCH 4/8] driver core: remove device_create_vargs Christoph Hellwig
@ 2020-04-16  7:52   ` Greg KH
  0 siblings, 0 replies; 31+ messages in thread
From: Greg KH @ 2020-04-16  7:52 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 09:15:15AM +0200, Christoph Hellwig wrote:
> All external users of device_create_vargs are gone, so remove it and
> open code it in the only caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yes!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info
  2020-04-16  7:15 ` [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info Christoph Hellwig
@ 2020-04-16  7:52   ` Greg KH
  2020-04-16  8:34   ` Yufen Yu
  1 sibling, 0 replies; 31+ messages in thread
From: Greg KH @ 2020-04-16  7:52 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 09:15:14AM +0200, Christoph Hellwig wrote:
> Cache a copy of the name for the life time of the backing_dev_info
> structure so that we can reference it even after unregistering.
> 
> Fixes: 68f23b89067f ("memcg: fix a crash in wb_workfn when a device disappears")
> Reported-by: Yufen Yu <yuyufen@huawei.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/8] bdi: use bdi_dev_name() to get device name
  2020-04-16  7:15 ` [PATCH 2/8] bdi: use bdi_dev_name() to get device name Christoph Hellwig
@ 2020-04-16  7:52   ` Greg KH
  0 siblings, 0 replies; 31+ messages in thread
From: Greg KH @ 2020-04-16  7:52 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 09:15:13AM +0200, Christoph Hellwig wrote:
> From: Yufen Yu <yuyufen@huawei.com>
> 
> Use the common interface bdi_dev_name() to get device name.
> 
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/8] bdi: move bdi_dev_name out of line
  2020-04-16  7:15 ` [PATCH 1/8] bdi: move bdi_dev_name out of line Christoph Hellwig
@ 2020-04-16  7:52   ` Greg KH
  2020-04-16 12:32   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Greg KH @ 2020-04-16  7:52 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 09:15:12AM +0200, Christoph Hellwig wrote:
> bdi_dev_name is not a fast path function, move it out of line.  This
> prepares for using it from modular callers without having to export
> an implementation detail like bdi_unknown_name.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 5/8] bdi: unexport bdi_register_va
  2020-04-16  7:15 ` [PATCH 5/8] bdi: unexport bdi_register_va Christoph Hellwig
@ 2020-04-16  7:53   ` Greg KH
  2020-04-16 12:03   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Greg KH @ 2020-04-16  7:53 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 09:15:16AM +0200, Christoph Hellwig wrote:
> bdi_register_va is only used by super.c, which can't be modular.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 6/8] bdi: remove bdi_register_owner
  2020-04-16  7:15 ` [PATCH 6/8] bdi: remove bdi_register_owner Christoph Hellwig
@ 2020-04-16  7:53   ` Greg KH
  2020-04-16 12:05   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Greg KH @ 2020-04-16  7:53 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 09:15:17AM +0200, Christoph Hellwig wrote:
> Split out a new bdi_set_owner helper to set the owner, and move the policy
> for creating the bdi name back into genhd.c, where it belongs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 7/8] bdi: simplify bdi_alloc
  2020-04-16  7:15 ` [PATCH 7/8] bdi: simplify bdi_alloc Christoph Hellwig
@ 2020-04-16  7:54   ` Greg KH
  2020-04-16 12:06   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Greg KH @ 2020-04-16  7:54 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 09:15:18AM +0200, Christoph Hellwig wrote:
> Merge the _node vs normal version and drop the superflous gfp_t argument.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 8/8] bdi: remove the name field in struct backing_dev_info
  2020-04-16  7:15 ` [PATCH 8/8] bdi: remove the name field in struct backing_dev_info Christoph Hellwig
@ 2020-04-16  7:54   ` Greg KH
  2020-04-16 12:23   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Greg KH @ 2020-04-16  7:54 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 09:15:19AM +0200, Christoph Hellwig wrote:
> The name is only printed for a not registered bdi in writeback.  Use the
> device name there as is more useful anyway for the unlike case that the
> warning triggers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info
  2020-04-16  7:15 ` [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info Christoph Hellwig
  2020-04-16  7:52   ` Greg KH
@ 2020-04-16  8:34   ` Yufen Yu
  2020-04-16 12:02     ` Jan Kara
  1 sibling, 1 reply; 31+ messages in thread
From: Yufen Yu @ 2020-04-16  8:34 UTC (permalink / raw)
  To: Christoph Hellwig, axboe
  Cc: tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

Hi,

On 2020/4/16 15:15, Christoph Hellwig wrote:
> Cache a copy of the name for the life time of the backing_dev_info
> structure so that we can reference it even after unregistering.
> 
> Fixes: 68f23b89067f ("memcg: fix a crash in wb_workfn when a device disappears")
> Reported-by: Yufen Yu <yuyufen@huawei.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   include/linux/backing-dev-defs.h |  1 +
>   mm/backing-dev.c                 | 13 ++++++++++---
>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
> index 4fc87dee005a..249590bcccf7 100644
> --- a/include/linux/backing-dev-defs.h
> +++ b/include/linux/backing-dev-defs.h
> @@ -220,6 +220,7 @@ struct backing_dev_info {
>   	wait_queue_head_t wb_waitq;
>   
>   	struct device *dev;
> +	const char *dev_name;
>   	struct device *owner;
>   
>   	struct timer_list laptop_mode_wb_timer;
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index c2c44c89ee5d..4f6c05df72f9 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -938,9 +938,15 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
>   	if (bdi->dev)	/* The driver needs to use separate queues per device */
>   		return 0;
>   
> -	dev = device_create_vargs(bdi_class, NULL, MKDEV(0, 0), bdi, fmt, args);
> -	if (IS_ERR(dev))
> +	bdi->dev_name = kvasprintf(GFP_KERNEL, fmt, args);
> +	if (!bdi->dev_name)
> +		return -ENOMEM;
> +
> +	dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
> +	if (IS_ERR(dev)) {
> +		kfree(bdi->dev_name);
>   		return PTR_ERR(dev);
> +	}
>   
>   	cgwb_bdi_register(bdi);
>   	bdi->dev = dev;
> @@ -1034,6 +1040,7 @@ static void release_bdi(struct kref *ref)
>   	WARN_ON_ONCE(bdi->dev);
>   	wb_exit(&bdi->wb);
>   	cgwb_bdi_exit(bdi);
> +	kfree(bdi->dev_name);
>   	kfree(bdi);
>   }


When driver try to to re-register bdi but without release_bdi(), the old dev_name
will be cover directly by the newer in bdi_register_va(). So, I am not sure whether
it can cause memory leak for bdi->dev_name.

Thanks,
Yufen

>   
> @@ -1047,7 +1054,7 @@ const char *bdi_dev_name(struct backing_dev_info *bdi)
>   {
>   	if (!bdi || !bdi->dev)
>   		return bdi_unknown_name;
> -	return dev_name(bdi->dev);
> +	return bdi->dev_name;
>   }
>   EXPORT_SYMBOL_GPL(bdi_dev_name);





^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info
  2020-04-16  8:34   ` Yufen Yu
@ 2020-04-16 12:02     ` Jan Kara
  2020-04-16 12:19       ` Christoph Hellwig
  0 siblings, 1 reply; 31+ messages in thread
From: Jan Kara @ 2020-04-16 12:02 UTC (permalink / raw)
  To: Yufen Yu
  Cc: Christoph Hellwig, axboe, tj, jack, bvanassche, tytso, gregkh,
	linux-block, linux-kernel

On Thu 16-04-20 16:34:13, Yufen Yu wrote:
> Hi,
> 
> On 2020/4/16 15:15, Christoph Hellwig wrote:
> > Cache a copy of the name for the life time of the backing_dev_info
> > structure so that we can reference it even after unregistering.
> > 
> > Fixes: 68f23b89067f ("memcg: fix a crash in wb_workfn when a device disappears")
> > Reported-by: Yufen Yu <yuyufen@huawei.com>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >   include/linux/backing-dev-defs.h |  1 +
> >   mm/backing-dev.c                 | 13 ++++++++++---
> >   2 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
> > index 4fc87dee005a..249590bcccf7 100644
> > --- a/include/linux/backing-dev-defs.h
> > +++ b/include/linux/backing-dev-defs.h
> > @@ -220,6 +220,7 @@ struct backing_dev_info {
> >   	wait_queue_head_t wb_waitq;
> >   	struct device *dev;
> > +	const char *dev_name;
> >   	struct device *owner;
> >   	struct timer_list laptop_mode_wb_timer;
> > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > index c2c44c89ee5d..4f6c05df72f9 100644
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -938,9 +938,15 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
> >   	if (bdi->dev)	/* The driver needs to use separate queues per device */
> >   		return 0;
> > -	dev = device_create_vargs(bdi_class, NULL, MKDEV(0, 0), bdi, fmt, args);
> > -	if (IS_ERR(dev))
> > +	bdi->dev_name = kvasprintf(GFP_KERNEL, fmt, args);
> > +	if (!bdi->dev_name)
> > +		return -ENOMEM;
> > +
> > +	dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
> > +	if (IS_ERR(dev)) {
> > +		kfree(bdi->dev_name);
> >   		return PTR_ERR(dev);
> > +	}
> >   	cgwb_bdi_register(bdi);
> >   	bdi->dev = dev;
> > @@ -1034,6 +1040,7 @@ static void release_bdi(struct kref *ref)
> >   	WARN_ON_ONCE(bdi->dev);
> >   	wb_exit(&bdi->wb);
> >   	cgwb_bdi_exit(bdi);
> > +	kfree(bdi->dev_name);
> >   	kfree(bdi);
> >   }
> 
> 
> When driver try to to re-register bdi but without release_bdi(), the old
> dev_name will be cover directly by the newer in bdi_register_va(). So, I
> am not sure whether it can cause memory leak for bdi->dev_name.

Yes, that can indeed happen. E.g. I remember that drivers/scsi/sd.c calls
device_add_disk() + del_gendisk() repeatedly for one request_queue and that
would result in leaking the name (and possibly cause use-after-free
issues). I think dev_name has to be just a static array inside
backing_dev_info which gets overwritten on reregistration. The question is
how big should be this array... Some grepping shows that 40 bytes should be
enough for everybody except fs/vboxsf/super.c which puts 'fc->source' into
the name which can be presumably rather large. Anyway, I'd make it 40 and
just truncate it case in case it does not fit. bdi_dev_name() is used for
informational purposes anyway...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 5/8] bdi: unexport bdi_register_va
  2020-04-16  7:15 ` [PATCH 5/8] bdi: unexport bdi_register_va Christoph Hellwig
  2020-04-16  7:53   ` Greg KH
@ 2020-04-16 12:03   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Jan Kara @ 2020-04-16 12:03 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block,
	linux-kernel

On Thu 16-04-20 09:15:16, Christoph Hellwig wrote:
> bdi_register_va is only used by super.c, which can't be modular.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/backing-dev.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 4f6c05df72f9..a7eb91146c9c 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -969,7 +969,6 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
>  	trace_writeback_bdi_register(bdi);
>  	return 0;
>  }
> -EXPORT_SYMBOL(bdi_register_va);
>  
>  int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...)
>  {
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 6/8] bdi: remove bdi_register_owner
  2020-04-16  7:15 ` [PATCH 6/8] bdi: remove bdi_register_owner Christoph Hellwig
  2020-04-16  7:53   ` Greg KH
@ 2020-04-16 12:05   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Jan Kara @ 2020-04-16 12:05 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block,
	linux-kernel

On Thu 16-04-20 09:15:17, Christoph Hellwig wrote:
> Split out a new bdi_set_owner helper to set the owner, and move the policy
> for creating the bdi name back into genhd.c, where it belongs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Fine by me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/genhd.c               |  8 +++++---
>  include/linux/backing-dev.h |  2 +-
>  mm/backing-dev.c            | 12 ++----------
>  3 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 06b642b23a07..7d10cfc38c70 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -840,13 +840,15 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
>  		disk->flags |= GENHD_FL_SUPPRESS_PARTITION_INFO;
>  		disk->flags |= GENHD_FL_NO_PART_SCAN;
>  	} else {
> +		struct backing_dev_info *bdi = disk->queue->backing_dev_info;
> +		struct device *dev = disk_to_dev(disk);
>  		int ret;
>  
>  		/* Register BDI before referencing it from bdev */
> -		disk_to_dev(disk)->devt = devt;
> -		ret = bdi_register_owner(disk->queue->backing_dev_info,
> -						disk_to_dev(disk));
> +		dev->devt = devt;
> +		ret = bdi_register(bdi, "%u:%u", MAJOR(devt), MINOR(devt));
>  		WARN_ON(ret);
> +		bdi_set_owner(bdi, dev);
>  		blk_register_region(disk_devt(disk), disk->minors, NULL,
>  				    exact_match, exact_lock, disk);
>  	}
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index c9ad5c3b7b4b..4098ed6ba6b4 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -33,7 +33,7 @@ int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...);
>  __printf(2, 0)
>  int bdi_register_va(struct backing_dev_info *bdi, const char *fmt,
>  		    va_list args);
> -int bdi_register_owner(struct backing_dev_info *bdi, struct device *owner);
> +void bdi_set_owner(struct backing_dev_info *bdi, struct device *owner);
>  void bdi_unregister(struct backing_dev_info *bdi);
>  
>  struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id);
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index a7eb91146c9c..1ba9a7b30933 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -982,20 +982,12 @@ int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...)
>  }
>  EXPORT_SYMBOL(bdi_register);
>  
> -int bdi_register_owner(struct backing_dev_info *bdi, struct device *owner)
> +void bdi_set_owner(struct backing_dev_info *bdi, struct device *owner)
>  {
> -	int rc;
> -
> -	rc = bdi_register(bdi, "%u:%u", MAJOR(owner->devt), MINOR(owner->devt));
> -	if (rc)
> -		return rc;
> -	/* Leaking owner reference... */
> -	WARN_ON(bdi->owner);
> +	WARN_ON_ONCE(bdi->owner);
>  	bdi->owner = owner;
>  	get_device(owner);
> -	return 0;
>  }
> -EXPORT_SYMBOL(bdi_register_owner);
>  
>  /*
>   * Remove bdi from bdi_list, and ensure that it is no longer visible
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 7/8] bdi: simplify bdi_alloc
  2020-04-16  7:15 ` [PATCH 7/8] bdi: simplify bdi_alloc Christoph Hellwig
  2020-04-16  7:54   ` Greg KH
@ 2020-04-16 12:06   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Jan Kara @ 2020-04-16 12:06 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block,
	linux-kernel

On Thu 16-04-20 09:15:18, Christoph Hellwig wrote:
> Merge the _node vs normal version and drop the superflous gfp_t argument.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/blk-core.c            | 2 +-
>  drivers/mtd/mtdcore.c       | 2 +-
>  fs/super.c                  | 2 +-
>  include/linux/backing-dev.h | 6 +-----
>  mm/backing-dev.c            | 7 +++----
>  5 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 7e4a1da0715e..ab87f2833ab2 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -484,7 +484,7 @@ struct request_queue *__blk_alloc_queue(int node_id)
>  	if (ret)
>  		goto fail_id;
>  
> -	q->backing_dev_info = bdi_alloc_node(GFP_KERNEL, node_id);
> +	q->backing_dev_info = bdi_alloc(node_id);
>  	if (!q->backing_dev_info)
>  		goto fail_split;
>  
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 2916674208b3..39ec563d9a14 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -2036,7 +2036,7 @@ static struct backing_dev_info * __init mtd_bdi_init(char *name)
>  	struct backing_dev_info *bdi;
>  	int ret;
>  
> -	bdi = bdi_alloc(GFP_KERNEL);
> +	bdi = bdi_alloc(NUMA_NO_NODE);
>  	if (!bdi)
>  		return ERR_PTR(-ENOMEM);
>  
> diff --git a/fs/super.c b/fs/super.c
> index cd352530eca9..dd28fcd706ff 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1598,7 +1598,7 @@ int super_setup_bdi_name(struct super_block *sb, char *fmt, ...)
>  	int err;
>  	va_list args;
>  
> -	bdi = bdi_alloc(GFP_KERNEL);
> +	bdi = bdi_alloc(NUMA_NO_NODE);
>  	if (!bdi)
>  		return -ENOMEM;
>  
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 4098ed6ba6b4..6b3504bf7a42 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -36,11 +36,7 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt,
>  void bdi_set_owner(struct backing_dev_info *bdi, struct device *owner);
>  void bdi_unregister(struct backing_dev_info *bdi);
>  
> -struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id);
> -static inline struct backing_dev_info *bdi_alloc(gfp_t gfp_mask)
> -{
> -	return bdi_alloc_node(gfp_mask, NUMA_NO_NODE);
> -}
> +struct backing_dev_info *bdi_alloc(int node_id);
>  
>  void wb_start_background_writeback(struct bdi_writeback *wb);
>  void wb_workfn(struct work_struct *work);
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 1ba9a7b30933..119a41650833 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -865,12 +865,11 @@ static int bdi_init(struct backing_dev_info *bdi)
>  	return ret;
>  }
>  
> -struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id)
> +struct backing_dev_info *bdi_alloc(int node_id)
>  {
>  	struct backing_dev_info *bdi;
>  
> -	bdi = kmalloc_node(sizeof(struct backing_dev_info),
> -			   gfp_mask | __GFP_ZERO, node_id);
> +	bdi = kzalloc_node(sizeof(*bdi), GFP_KERNEL, node_id);
>  	if (!bdi)
>  		return NULL;
>  
> @@ -880,7 +879,7 @@ struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id)
>  	}
>  	return bdi;
>  }
> -EXPORT_SYMBOL(bdi_alloc_node);
> +EXPORT_SYMBOL(bdi_alloc);
>  
>  static struct rb_node **bdi_lookup_rb_node(u64 id, struct rb_node **parentp)
>  {
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info
  2020-04-16 12:02     ` Jan Kara
@ 2020-04-16 12:19       ` Christoph Hellwig
  2020-04-16 12:22         ` Christoph Hellwig
  0 siblings, 1 reply; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16 12:19 UTC (permalink / raw)
  To: Jan Kara
  Cc: Yufen Yu, Christoph Hellwig, axboe, tj, bvanassche, tytso,
	gregkh, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 02:02:23PM +0200, Jan Kara wrote:
> Yes, that can indeed happen. E.g. I remember that drivers/scsi/sd.c calls
> device_add_disk() + del_gendisk() repeatedly for one request_queue and that
> would result in leaking the name (and possibly cause use-after-free
> issues).

Sd calls device_add_disk once in ->probe, and del_gendisk once in
sd_remove.  Note that sd_probe allocates a new scsi_disk structure and
a new gendisk everytime, but it does indeed reuse the request_queue
and thus bdi.

> I think dev_name has to be just a static array inside
> backing_dev_info which gets overwritten on reregistration. The question is
> how big should be this array... Some grepping shows that 40 bytes should be
> enough for everybody except fs/vboxsf/super.c which puts 'fc->source' into
> the name which can be presumably rather large. Anyway, I'd make it 40 and
> just truncate it case in case it does not fit. bdi_dev_name() is used for
> informational purposes anyway...

We could just make it a variable sized array at the end of the structure
and size it based on the len.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info
  2020-04-16 12:19       ` Christoph Hellwig
@ 2020-04-16 12:22         ` Christoph Hellwig
  2020-04-16 12:31           ` Jan Kara
  0 siblings, 1 reply; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16 12:22 UTC (permalink / raw)
  To: Jan Kara
  Cc: Yufen Yu, Christoph Hellwig, axboe, tj, bvanassche, tytso,
	gregkh, linux-block, linux-kernel

On Thu, Apr 16, 2020 at 02:19:01PM +0200, Christoph Hellwig wrote:
> On Thu, Apr 16, 2020 at 02:02:23PM +0200, Jan Kara wrote:
> > Yes, that can indeed happen. E.g. I remember that drivers/scsi/sd.c calls
> > device_add_disk() + del_gendisk() repeatedly for one request_queue and that
> > would result in leaking the name (and possibly cause use-after-free
> > issues).
> 
> Sd calls device_add_disk once in ->probe, and del_gendisk once in
> sd_remove.  Note that sd_probe allocates a new scsi_disk structure and
> a new gendisk everytime, but it does indeed reuse the request_queue
> and thus bdi.
> 
> > I think dev_name has to be just a static array inside
> > backing_dev_info which gets overwritten on reregistration. The question is
> > how big should be this array... Some grepping shows that 40 bytes should be
> > enough for everybody except fs/vboxsf/super.c which puts 'fc->source' into
> > the name which can be presumably rather large. Anyway, I'd make it 40 and
> > just truncate it case in case it does not fit. bdi_dev_name() is used for
> > informational purposes anyway...
> 
> We could just make it a variable sized array at the end of the structure
> and size it based on the len.

Which doesn't always work as the size might not always be the same.
But I think the fundamental problem is that we are trying to re-register
previous unregistered bdis.  We really should not have bdi_alloc
separate from bdi_register and solve this properly.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 8/8] bdi: remove the name field in struct backing_dev_info
  2020-04-16  7:15 ` [PATCH 8/8] bdi: remove the name field in struct backing_dev_info Christoph Hellwig
  2020-04-16  7:54   ` Greg KH
@ 2020-04-16 12:23   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Jan Kara @ 2020-04-16 12:23 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block,
	linux-kernel

On Thu 16-04-20 09:15:19, Christoph Hellwig wrote:
> The name is only printed for a not registered bdi in writeback.  Use the
> device name there as is more useful anyway for the unlike case that the
> warning triggers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Ah, cool. So it isn't used in sysfs/debugfs as I was afraid. The patch
looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/blk-core.c                 | 1 -
>  drivers/mtd/mtdcore.c            | 1 -
>  fs/fs-writeback.c                | 2 +-
>  fs/super.c                       | 2 --
>  include/linux/backing-dev-defs.h | 2 --
>  mm/backing-dev.c                 | 1 -
>  6 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index ab87f2833ab2..f37068c611bf 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -494,7 +494,6 @@ struct request_queue *__blk_alloc_queue(int node_id)
>  
>  	q->backing_dev_info->ra_pages = VM_READAHEAD_PAGES;
>  	q->backing_dev_info->capabilities = BDI_CAP_CGROUP_WRITEBACK;
> -	q->backing_dev_info->name = "block";
>  	q->node = node_id;
>  
>  	timer_setup(&q->backing_dev_info->laptop_mode_wb_timer,
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 39ec563d9a14..fcb018ce17c3 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -2040,7 +2040,6 @@ static struct backing_dev_info * __init mtd_bdi_init(char *name)
>  	if (!bdi)
>  		return ERR_PTR(-ENOMEM);
>  
> -	bdi->name = name;
>  	/*
>  	 * We put '-0' suffix to the name to get the same name format as we
>  	 * used to get. Since this is called only once, we get a unique name. 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 76ac9c7d32ec..d85323607b49 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -2320,7 +2320,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
>  
>  			WARN(bdi_cap_writeback_dirty(wb->bdi) &&
>  			     !test_bit(WB_registered, &wb->state),
> -			     "bdi-%s not registered\n", wb->bdi->name);
> +			     "bdi-%s not registered\n", bdi_dev_name(wb->bdi));
>  
>  			inode->dirtied_when = jiffies;
>  			if (dirtytime)
> diff --git a/fs/super.c b/fs/super.c
> index dd28fcd706ff..4991f441988e 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1602,8 +1602,6 @@ int super_setup_bdi_name(struct super_block *sb, char *fmt, ...)
>  	if (!bdi)
>  		return -ENOMEM;
>  
> -	bdi->name = sb->s_type->name;
> -
>  	va_start(args, fmt);
>  	err = bdi_register_va(bdi, fmt, args);
>  	va_end(args);
> diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
> index 249590bcccf7..0a55170b2142 100644
> --- a/include/linux/backing-dev-defs.h
> +++ b/include/linux/backing-dev-defs.h
> @@ -194,8 +194,6 @@ struct backing_dev_info {
>  	congested_fn *congested_fn; /* Function pointer if device is md/dm */
>  	void *congested_data;	/* Pointer to aux data for congested func */
>  
> -	const char *name;
> -
>  	struct kref refcnt;	/* Reference counter for the structure */
>  	unsigned int capabilities; /* Device capabilities */
>  	unsigned int min_ratio;
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 119a41650833..dc7215dfd56b 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -15,7 +15,6 @@
>  #include <trace/events/writeback.h>
>  
>  struct backing_dev_info noop_backing_dev_info = {
> -	.name		= "noop",
>  	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK,
>  };
>  EXPORT_SYMBOL_GPL(noop_backing_dev_info);
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info
  2020-04-16 12:22         ` Christoph Hellwig
@ 2020-04-16 12:31           ` Jan Kara
  0 siblings, 0 replies; 31+ messages in thread
From: Jan Kara @ 2020-04-16 12:31 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jan Kara, Yufen Yu, axboe, tj, bvanassche, tytso, gregkh,
	linux-block, linux-kernel

On Thu 16-04-20 14:22:35, Christoph Hellwig wrote:
> On Thu, Apr 16, 2020 at 02:19:01PM +0200, Christoph Hellwig wrote:
> > On Thu, Apr 16, 2020 at 02:02:23PM +0200, Jan Kara wrote:
> > > Yes, that can indeed happen. E.g. I remember that drivers/scsi/sd.c calls
> > > device_add_disk() + del_gendisk() repeatedly for one request_queue and that
> > > would result in leaking the name (and possibly cause use-after-free
> > > issues).
> > 
> > Sd calls device_add_disk once in ->probe, and del_gendisk once in
> > sd_remove.  Note that sd_probe allocates a new scsi_disk structure and
> > a new gendisk everytime, but it does indeed reuse the request_queue
> > and thus bdi.
> > 
> > > I think dev_name has to be just a static array inside
> > > backing_dev_info which gets overwritten on reregistration. The question is
> > > how big should be this array... Some grepping shows that 40 bytes should be
> > > enough for everybody except fs/vboxsf/super.c which puts 'fc->source' into
> > > the name which can be presumably rather large. Anyway, I'd make it 40 and
> > > just truncate it case in case it does not fit. bdi_dev_name() is used for
> > > informational purposes anyway...
> > 
> > We could just make it a variable sized array at the end of the structure
> > and size it based on the len.
> 
> Which doesn't always work as the size might not always be the same.
> But I think the fundamental problem is that we are trying to re-register
> previous unregistered bdis.  We really should not have bdi_alloc
> separate from bdi_register and solve this properly.

Yes, that would be easier then but it seems like a much larger change
because currently bdi is disassociated from request_queue only in
__blk_release_queue() (blk_exit_queue()). I guess the separate bdi
registration / deregistration is partially a leftover from times when bdi
was still embedded in request_queue but now it's difficult to undo it.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/8] bdi: move bdi_dev_name out of line
  2020-04-16  7:15 ` [PATCH 1/8] bdi: move bdi_dev_name out of line Christoph Hellwig
  2020-04-16  7:52   ` Greg KH
@ 2020-04-16 12:32   ` Jan Kara
  1 sibling, 0 replies; 31+ messages in thread
From: Jan Kara @ 2020-04-16 12:32 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block,
	linux-kernel

On Thu 16-04-20 09:15:12, Christoph Hellwig wrote:
> bdi_dev_name is not a fast path function, move it out of line.  This
> prepares for using it from modular callers without having to export
> an implementation detail like bdi_unknown_name.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/backing-dev.h |  9 +--------
>  mm/backing-dev.c            | 10 +++++++++-
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index f88197c1ffc2..c9ad5c3b7b4b 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -505,13 +505,6 @@ static inline int bdi_rw_congested(struct backing_dev_info *bdi)
>  				  (1 << WB_async_congested));
>  }
>  
> -extern const char *bdi_unknown_name;
> -
> -static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
> -{
> -	if (!bdi || !bdi->dev)
> -		return bdi_unknown_name;
> -	return dev_name(bdi->dev);
> -}
> +const char *bdi_dev_name(struct backing_dev_info *bdi);
>  
>  #endif	/* _LINUX_BACKING_DEV_H */
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index c81b4f3a7268..c2c44c89ee5d 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -21,7 +21,7 @@ struct backing_dev_info noop_backing_dev_info = {
>  EXPORT_SYMBOL_GPL(noop_backing_dev_info);
>  
>  static struct class *bdi_class;
> -const char *bdi_unknown_name = "(unknown)";
> +static const char *bdi_unknown_name = "(unknown)";
>  
>  /*
>   * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
> @@ -1043,6 +1043,14 @@ void bdi_put(struct backing_dev_info *bdi)
>  }
>  EXPORT_SYMBOL(bdi_put);
>  
> +const char *bdi_dev_name(struct backing_dev_info *bdi)
> +{
> +	if (!bdi || !bdi->dev)
> +		return bdi_unknown_name;
> +	return dev_name(bdi->dev);
> +}
> +EXPORT_SYMBOL_GPL(bdi_dev_name);
> +
>  static wait_queue_head_t congestion_wqh[2] = {
>  		__WAIT_QUEUE_HEAD_INITIALIZER(congestion_wqh[0]),
>  		__WAIT_QUEUE_HEAD_INITIALIZER(congestion_wqh[1])
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: bdi: fix use-after-free for dev_name(bdi->dev)
  2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
                   ` (7 preceding siblings ...)
  2020-04-16  7:15 ` [PATCH 8/8] bdi: remove the name field in struct backing_dev_info Christoph Hellwig
@ 2020-04-16 15:29 ` Jens Axboe
  2020-04-16 15:29   ` Christoph Hellwig
  8 siblings, 1 reply; 31+ messages in thread
From: Jens Axboe @ 2020-04-16 15:29 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

On 4/16/20 1:15 AM, Christoph Hellwig wrote:
> Hi all,
> 
> the first three patches are my take on the proposal from Yufen Yu
> to fix the use after free of the device name of the bdi device.
> 
> The rest is vaguely related cleanups.

Applied, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: bdi: fix use-after-free for dev_name(bdi->dev)
  2020-04-16 15:29 ` bdi: fix use-after-free for dev_name(bdi->dev) Jens Axboe
@ 2020-04-16 15:29   ` Christoph Hellwig
  2020-04-16 15:30     ` Jens Axboe
  0 siblings, 1 reply; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16 15:29 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Christoph Hellwig, yuyufen, tj, jack, bvanassche, tytso, gregkh,
	linux-block, linux-kernel

On Thu, Apr 16, 2020 at 09:29:13AM -0600, Jens Axboe wrote:
> On 4/16/20 1:15 AM, Christoph Hellwig wrote:
> > Hi all,
> > 
> > the first three patches are my take on the proposal from Yufen Yu
> > to fix the use after free of the device name of the bdi device.
> > 
> > The rest is vaguely related cleanups.
> 
> Applied, thanks.

Please hold back, we still have a major issues with it.  I will resend
a fixed version tomorrow.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: bdi: fix use-after-free for dev_name(bdi->dev)
  2020-04-16 15:29   ` Christoph Hellwig
@ 2020-04-16 15:30     ` Jens Axboe
  0 siblings, 0 replies; 31+ messages in thread
From: Jens Axboe @ 2020-04-16 15:30 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

On 4/16/20 9:29 AM, Christoph Hellwig wrote:
> On Thu, Apr 16, 2020 at 09:29:13AM -0600, Jens Axboe wrote:
>> On 4/16/20 1:15 AM, Christoph Hellwig wrote:
>>> Hi all,
>>>
>>> the first three patches are my take on the proposal from Yufen Yu
>>> to fix the use after free of the device name of the bdi device.
>>>
>>> The rest is vaguely related cleanups.
>>
>> Applied, thanks.
> 
> Please hold back, we still have a major issues with it.  I will resend
> a fixed version tomorrow.

OK, will do.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 4/8] driver core: remove device_create_vargs
  2020-04-16 16:54 bdi: fix use-after-free for dev_name(bdi->dev) v2 Christoph Hellwig
@ 2020-04-16 16:54 ` Christoph Hellwig
  0 siblings, 0 replies; 31+ messages in thread
From: Christoph Hellwig @ 2020-04-16 16:54 UTC (permalink / raw)
  To: axboe
  Cc: yuyufen, tj, jack, bvanassche, tytso, gregkh, linux-block, linux-kernel

All external users of device_create_vargs are gone, so remove it and
open code it in the only caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/core.c    | 37 ++-----------------------------------
 include/linux/device.h |  4 ----
 2 files changed, 2 insertions(+), 39 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 139cdf7e7327..fb8ae248e5aa 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3188,40 +3188,6 @@ device_create_groups_vargs(struct class *class, struct device *parent,
 	return ERR_PTR(retval);
 }
 
-/**
- * device_create_vargs - creates a device and registers it with sysfs
- * @class: pointer to the struct class that this device should be registered to
- * @parent: pointer to the parent struct device of this new device, if any
- * @devt: the dev_t for the char device to be added
- * @drvdata: the data to be added to the device for callbacks
- * @fmt: string for the device's name
- * @args: va_list for the device's name
- *
- * This function can be used by char device classes.  A struct device
- * will be created in sysfs, registered to the specified class.
- *
- * A "dev" file will be created, showing the dev_t for the device, if
- * the dev_t is not 0,0.
- * If a pointer to a parent struct device is passed in, the newly created
- * struct device will be a child of that device in sysfs.
- * The pointer to the struct device will be returned from the call.
- * Any further sysfs files that might be required can be created using this
- * pointer.
- *
- * Returns &struct device pointer on success, or ERR_PTR() on error.
- *
- * Note: the struct class passed to this function must have previously
- * been created with a call to class_create().
- */
-struct device *device_create_vargs(struct class *class, struct device *parent,
-				   dev_t devt, void *drvdata, const char *fmt,
-				   va_list args)
-{
-	return device_create_groups_vargs(class, parent, devt, drvdata, NULL,
-					  fmt, args);
-}
-EXPORT_SYMBOL_GPL(device_create_vargs);
-
 /**
  * device_create - creates a device and registers it with sysfs
  * @class: pointer to the struct class that this device should be registered to
@@ -3253,7 +3219,8 @@ struct device *device_create(struct class *class, struct device *parent,
 	struct device *dev;
 
 	va_start(vargs, fmt);
-	dev = device_create_vargs(class, parent, devt, drvdata, fmt, vargs);
+	dev = device_create_groups_vargs(class, parent, devt, drvdata, NULL,
+					  fmt, vargs);
 	va_end(vargs);
 	return dev;
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index ac8e37cd716a..15460a5ac024 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -884,10 +884,6 @@ extern bool device_is_bound(struct device *dev);
 /*
  * Easy functions for dynamically creating devices on the fly
  */
-extern __printf(5, 0)
-struct device *device_create_vargs(struct class *cls, struct device *parent,
-				   dev_t devt, void *drvdata,
-				   const char *fmt, va_list vargs);
 extern __printf(5, 6)
 struct device *device_create(struct class *cls, struct device *parent,
 			     dev_t devt, void *drvdata,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2020-04-16 16:55 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-16  7:15 bdi: fix use-after-free for dev_name(bdi->dev) Christoph Hellwig
2020-04-16  7:15 ` [PATCH 1/8] bdi: move bdi_dev_name out of line Christoph Hellwig
2020-04-16  7:52   ` Greg KH
2020-04-16 12:32   ` Jan Kara
2020-04-16  7:15 ` [PATCH 2/8] bdi: use bdi_dev_name() to get device name Christoph Hellwig
2020-04-16  7:52   ` Greg KH
2020-04-16  7:15 ` [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info Christoph Hellwig
2020-04-16  7:52   ` Greg KH
2020-04-16  8:34   ` Yufen Yu
2020-04-16 12:02     ` Jan Kara
2020-04-16 12:19       ` Christoph Hellwig
2020-04-16 12:22         ` Christoph Hellwig
2020-04-16 12:31           ` Jan Kara
2020-04-16  7:15 ` [PATCH 4/8] driver core: remove device_create_vargs Christoph Hellwig
2020-04-16  7:52   ` Greg KH
2020-04-16  7:15 ` [PATCH 5/8] bdi: unexport bdi_register_va Christoph Hellwig
2020-04-16  7:53   ` Greg KH
2020-04-16 12:03   ` Jan Kara
2020-04-16  7:15 ` [PATCH 6/8] bdi: remove bdi_register_owner Christoph Hellwig
2020-04-16  7:53   ` Greg KH
2020-04-16 12:05   ` Jan Kara
2020-04-16  7:15 ` [PATCH 7/8] bdi: simplify bdi_alloc Christoph Hellwig
2020-04-16  7:54   ` Greg KH
2020-04-16 12:06   ` Jan Kara
2020-04-16  7:15 ` [PATCH 8/8] bdi: remove the name field in struct backing_dev_info Christoph Hellwig
2020-04-16  7:54   ` Greg KH
2020-04-16 12:23   ` Jan Kara
2020-04-16 15:29 ` bdi: fix use-after-free for dev_name(bdi->dev) Jens Axboe
2020-04-16 15:29   ` Christoph Hellwig
2020-04-16 15:30     ` Jens Axboe
2020-04-16 16:54 bdi: fix use-after-free for dev_name(bdi->dev) v2 Christoph Hellwig
2020-04-16 16:54 ` [PATCH 4/8] driver core: remove device_create_vargs Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).