LinuxLists.cc - bdi: fix use-after-free for dev

[permalink] [raw]

Subject: [PATCH 1/8] bdi: move bdi_dev_name out of line

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 <[email protected]>
---
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

2020-04-16 07:17:45

[permalink] [raw]

Subject: [PATCH 2/8] bdi: use bdi_dev_name() to get device name

From: Yufen Yu <[email protected]>

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

Signed-off-by: Yufen Yu <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Christoph Hellwig <[email protected]>
---
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

2020-04-16 07:18:07

[permalink] [raw]

Subject: [PATCH 6/8] bdi: remove bdi_register_owner

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 <[email protected]>
---
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

2020-04-16 07:18:24

[permalink] [raw]

Subject: [PATCH 8/8] bdi: remove the name field in struct backing_dev_info

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 <[email protected]>
---
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

2020-04-16 07:19:12

[permalink] [raw]

Subject: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info

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 <[email protected]>
Signed-off-by: Christoph Hellwig <[email protected]>
---
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

2020-04-16 07:19:19

[permalink] [raw]

Subject: [PATCH 4/8] driver core: remove device_create_vargs

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 <[email protected]>
---
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

2020-04-16 07:19:57

[permalink] [raw]

Subject: [PATCH 5/8] bdi: unexport bdi_register_va

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

Signed-off-by: Christoph Hellwig <[email protected]>
---
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

2020-04-16 07:20:09

[permalink] [raw]

Subject: [PATCH 7/8] bdi: simplify bdi_alloc

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

Signed-off-by: Christoph Hellwig <[email protected]>
---
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

2020-04-16 07:54:12

[permalink] [raw]

Subject: Re: [PATCH 4/8] driver core: remove device_create_vargs

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 <[email protected]>

Yes!

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2020-04-16 07:54:48

[permalink] [raw]

Subject: Re: [PATCH 2/8] bdi: use bdi_dev_name() to get device name

On Thu, Apr 16, 2020 at 09:15:13AM +0200, Christoph Hellwig wrote:
> From: Yufen Yu <[email protected]>
>
> Use the common interface bdi_dev_name() to get device name.
>
> Signed-off-by: Yufen Yu <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Christoph Hellwig <[email protected]>

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2020-04-16 07:56:00

[permalink] [raw]

Subject: Re: [PATCH 6/8] bdi: remove bdi_register_owner

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 <[email protected]>

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2020-04-16 07:56:29

[permalink] [raw]

Subject: Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info

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 <[email protected]>
> Signed-off-by: Christoph Hellwig <[email protected]>

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2020-04-16 07:57:12

[permalink] [raw]

Subject: Re: [PATCH 1/8] bdi: move bdi_dev_name out of line

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 <[email protected]>

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2020-04-16 07:57:29

[permalink] [raw]

Subject: Re: [PATCH 5/8] bdi: unexport bdi_register_va

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 <[email protected]>

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2020-04-16 07:59:51

[permalink] [raw]

Subject: Re: [PATCH 7/8] bdi: simplify bdi_alloc

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 <[email protected]>

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2020-04-16 08:00:01

[permalink] [raw]

Subject: Re: [PATCH 8/8] bdi: remove the name field in struct backing_dev_info

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 <[email protected]>

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2020-04-16 08:38:47

by Yufen Yu

[permalink] [raw]

Subject: Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info

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 <[email protected]>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> 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);

2020-04-16 12:07:50

[permalink] [raw]

Subject: Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info

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 <[email protected]>
> > Signed-off-by: Christoph Hellwig <[email protected]>
> > ---
> > 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 <[email protected]>
SUSE Labs, CR

2020-04-16 12:08:01

[permalink] [raw]

Subject: Re: [PATCH 6/8] bdi: remove bdi_register_owner

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 <[email protected]>

Fine by me. You can add:

Reviewed-by: Jan Kara <[email protected]>

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 <[email protected]>
SUSE Labs, CR

2020-04-16 12:09:19

[permalink] [raw]

Subject: Re: [PATCH 7/8] bdi: simplify bdi_alloc

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 <[email protected]>

Looks good to me. You can add:

Reviewed-by: Jan Kara <[email protected]>

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 <[email protected]>
SUSE Labs, CR

2020-04-16 12:27:03

[permalink] [raw]

Subject: Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info

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.

2020-04-16 12:28:33

[permalink] [raw]

Subject: Re: [PATCH 8/8] bdi: remove the name field in struct backing_dev_info

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>
SUSE Labs, CR

2020-04-16 12:34:23

[permalink] [raw]

Subject: Re: [PATCH 1/8] bdi: move bdi_dev_name out of line

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 <[email protected]>

Looks good to me. You can add:

Reviewed-by: Jan Kara <[email protected]>

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 <[email protected]>
SUSE Labs, CR

2020-04-16 12:35:37

[permalink] [raw]

Subject: Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info

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 <[email protected]>
SUSE Labs, CR

2020-04-16 15:35:02

[permalink] [raw]

Subject: Re: bdi: fix use-after-free for dev_name(bdi->dev)

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.

2020-04-16 17:44:55

[permalink] [raw]

Subject: Re: [PATCH 5/8] bdi: unexport bdi_register_va

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 <[email protected]>

Looks good to me. You can add:

Reviewed-by: Jan Kara <[email protected]>

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 <[email protected]>
SUSE Labs, CR

2020-04-16 19:07:47