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

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

[Christoph Hellwig]

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.

Changes since v1:
 - use a static dev_name buffer inside struct backing_dev_info

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

[Christoph Hellwig]

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: Jan Kara <jack@suse.cz>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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

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

[Bart Van Assche]

On 2020-04-16 09:54, 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.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

[Christoph Hellwig]

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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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

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

[Bart Van Assche]

On 2020-04-16 09:54, 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

[Christoph Hellwig]

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                 | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index 4fc87dee005a..2849bdbb3acb 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;
+	char dev_name[64];
 	struct device *owner;
 
 	struct timer_list laptop_mode_wb_timer;
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c2c44c89ee5d..efc5b83acd2d 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -938,7 +938,8 @@ 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);
+	vsnprintf(bdi->dev_name, sizeof(bdi->dev_name), fmt, args);
+	dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
 	if (IS_ERR(dev))
 		return PTR_ERR(dev);
 
@@ -1047,7 +1048,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

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

[Jan Kara]

On Thu 16-04-20 18:54:48, 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>

...

> @@ -938,7 +938,8 @@ 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);
> +	vsnprintf(bdi->dev_name, sizeof(bdi->dev_name), fmt, args);
> +	dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
>  	if (IS_ERR(dev))
>  		return PTR_ERR(dev);
>  

This can have a sideeffect not only bdi->dev_name will be truncated to 64
chars (which generally doesn't matter) but possibly also kobject name will
be truncated in the same way.  Which may have user visible effects. E.g.
for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
other way around - i.e., let device_create_vargs() create the device name
and then copy to bdi->dev_name whatever fits?

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

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

[Christoph Hellwig]

On Fri, Apr 17, 2020 at 10:59:09AM +0200, Jan Kara wrote:
> > -	dev = device_create_vargs(bdi_class, NULL, MKDEV(0, 0), bdi, fmt, args);
> > +	vsnprintf(bdi->dev_name, sizeof(bdi->dev_name), fmt, args);
> > +	dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
> >  	if (IS_ERR(dev))
> >  		return PTR_ERR(dev);
> >  
> 
> This can have a sideeffect not only bdi->dev_name will be truncated to 64
> chars (which generally doesn't matter) but possibly also kobject name will
> be truncated in the same way.  Which may have user visible effects. E.g.
> for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
> other way around - i.e., let device_create_vargs() create the device name
> and then copy to bdi->dev_name whatever fits?

I think having them mismatch is worse, as the kobject name is what
people look for.  Hans, do you know what fc->source typicall contains
and if there is much of a problem if it gets truncated/  Can we switch
to something else that is guranteed to be 64 charaters or less for the
bdi name?

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

[Hans de Goede]

Hi,

<A lot of context has been trimmed here before I got added to the Cc, so
  I'm assuming that we are talking about the vboxsf code here.>

On 4/17/20 3:01 PM, Christoph Hellwig wrote:
> On Fri, Apr 17, 2020 at 10:59:09AM +0200, Jan Kara wrote:
>>> -	dev = device_create_vargs(bdi_class, NULL, MKDEV(0, 0), bdi, fmt, args);
>>> +	vsnprintf(bdi->dev_name, sizeof(bdi->dev_name), fmt, args);
>>> +	dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
>>>   	if (IS_ERR(dev))
>>>   		return PTR_ERR(dev);
>>>   
>>
>> This can have a sideeffect not only bdi->dev_name will be truncated to 64
>> chars (which generally doesn't matter) but possibly also kobject name will
>> be truncated in the same way.  Which may have user visible effects. E.g.
>> for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
>> other way around - i.e., let device_create_vargs() create the device name
>> and then copy to bdi->dev_name whatever fits?
> 
> I think having them mismatch is worse, as the kobject name is what
> people look for.  Hans, do you know what fc->source typicall contains
> and if there is much of a problem if it gets truncated/  Can we switch
> to something else that is guranteed to be 64 charaters or less for the
> bdi name?

It contains the name the user has given to the shared-folder when
exporting it from the host/hypervisor. Typically this will be the
last element of the directory path, e.g. if I export /home/hans/projects/linux
then the default/suggested share name and this the source name to pass to
the host when mounting the shared-folder will be "linux". But the user can
put anything there.

AFAICT for vboxsf the bdi-name can be anything as long as it is unique, hence
the "vboxsf-" prefix to make this unique vs other block-devices and the
".%d" postfix is necessary because the same export can be mounted multiple
times (without using bind mounts), see:
https://github.com/jwrdegoede/vboxsf/issues/3

The presence of the source inside the bdi-name is only for informational
purposes really, so truncating that should be fine, maybe switch to:

"vboxsf%d-%s" as format string and swap the sbi->bdi_id and fc->source
in the args, then if we truncate anything it will be the source (which
as said is only there for informational purposes) and the name will
still be guaranteed to be unique.

Regards,

Hans

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

[Christoph Hellwig]

On Mon, Apr 20, 2020 at 01:41:57PM +0200, Hans de Goede wrote:
> AFAICT for vboxsf the bdi-name can be anything as long as it is unique, hence
> the "vboxsf-" prefix to make this unique vs other block-devices and the
> ".%d" postfix is necessary because the same export can be mounted multiple
> times (without using bind mounts), see:
> https://github.com/jwrdegoede/vboxsf/issues/3

Shouldn't vboxsf switch to get_tree_single instead of get_tree_nodev?
Having two independent dentry trees for a single actual file system
can be pretty dangerous.

>
> The presence of the source inside the bdi-name is only for informational
> purposes really, so truncating that should be fine, maybe switch to:
>
> "vboxsf%d-%s" as format string and swap the sbi->bdi_id and fc->source
> in the args, then if we truncate anything it will be the source (which
> as said is only there for informational purposes) and the name will
> still be guaranteed to be unique.

Can we just switch to vboxsf%d where %d іs a simple monotonically
incrementing count?  That is what various other file systems (e.g. ceph)
do.

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

[Hans de Goede]

Hi,

On 4/20/20 1:58 PM, Christoph Hellwig wrote:
> On Mon, Apr 20, 2020 at 01:41:57PM +0200, Hans de Goede wrote:
>> AFAICT for vboxsf the bdi-name can be anything as long as it is unique, hence
>> the "vboxsf-" prefix to make this unique vs other block-devices and the
>> ".%d" postfix is necessary because the same export can be mounted multiple
>> times (without using bind mounts), see:
>> https://github.com/jwrdegoede/vboxsf/issues/3
> 
> Shouldn't vboxsf switch to get_tree_single instead of get_tree_nodev?
> Having two independent dentry trees for a single actual file system
> can be pretty dangerous.

That is a good point I will look into this.

> 
>>
>> The presence of the source inside the bdi-name is only for informational
>> purposes really, so truncating that should be fine, maybe switch to:
>>
>> "vboxsf%d-%s" as format string and swap the sbi->bdi_id and fc->source
>> in the args, then if we truncate anything it will be the source (which
>> as said is only there for informational purposes) and the name will
>> still be guaranteed to be unique.
> 
> Can we just switch to vboxsf%d where %d іs a simple monotonically
> incrementing count?  That is what various other file systems (e.g. ceph)
> do.

Yes that is fine with me.

Regards,

Hans

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

[Bart Van Assche]

On 2020-04-17 01:59, Jan Kara wrote:
> On Thu 16-04-20 18:54:48, Christoph Hellwig wrote:
>> @@ -938,7 +938,8 @@ 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);
>> +	vsnprintf(bdi->dev_name, sizeof(bdi->dev_name), fmt, args);
>> +	dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
>>  	if (IS_ERR(dev))
>>  		return PTR_ERR(dev);
>>  
> 
> This can have a sideeffect not only bdi->dev_name will be truncated to 64
> chars (which generally doesn't matter) but possibly also kobject name will
> be truncated in the same way.  Which may have user visible effects. E.g.
> for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
> other way around - i.e., let device_create_vargs() create the device name
> and then copy to bdi->dev_name whatever fits?

How about using kvasprintf() instead of vsnprintf()?

Thanks,

Bart.

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

[Christoph Hellwig]

On Sat, Apr 18, 2020 at 08:40:20AM -0700, Bart Van Assche wrote:
> > This can have a sideeffect not only bdi->dev_name will be truncated to 64
> > chars (which generally doesn't matter) but possibly also kobject name will
> > be truncated in the same way.  Which may have user visible effects. E.g.
> > for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
> > other way around - i.e., let device_create_vargs() create the device name
> > and then copy to bdi->dev_name whatever fits?
> 
> How about using kvasprintf() instead of vsnprintf()?

That is what v1 did, see the thread in response to that on why it isn't
a good idea.

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

[Bart Van Assche]

On 4/19/20 12:58 AM, Christoph Hellwig wrote:
> On Sat, Apr 18, 2020 at 08:40:20AM -0700, Bart Van Assche wrote:
>>> This can have a sideeffect not only bdi->dev_name will be truncated to 64
>>> chars (which generally doesn't matter) but possibly also kobject name will
>>> be truncated in the same way.  Which may have user visible effects. E.g.
>>> for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
>>> other way around - i.e., let device_create_vargs() create the device name
>>> and then copy to bdi->dev_name whatever fits?
>>
>> How about using kvasprintf() instead of vsnprintf()?
> 
> That is what v1 did, see the thread in response to that on why it isn't
> a good idea.

Are you perhaps referring to patch "[PATCH 3/8] bdi: add a ->dev_name 
field to struct backing_dev_info" 
(https://lore.kernel.org/linux-block/20200416071519.807660-4-hch@lst.de/) 
and also to the replies to that patch? This is what I found in the 
replies: "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."

Has it been considered to avoid that leak by freeing bdi->dev_name from 
unregister_bdi(), e.g. as follows?

void bdi_unregister(struct backing_dev_info *bdi)
{
+	char *dev_name;

	/* make sure nobody finds us on the bdi_list anymore */
	bdi_remove_from_list(bdi);
	wb_shutdown(&bdi->wb);
	cgwb_bdi_unregister(bdi);

	if (bdi->dev) {
		bdi_debug_unregister(bdi);
		device_unregister(bdi->dev);
		bdi->dev = NULL;
+		dev_name = bdi->dev_name;
+		bdi->dev_name = "(unregistered)";
+		kfree(dev_name);
	}

	if (bdi->owner) {
		put_device(bdi->owner);
		bdi->owner = NULL;
	}
}

Thanks,

Bart.

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

[Christoph Hellwig]

On Sun, Apr 19, 2020 at 08:29:21AM -0700, Bart Van Assche wrote:
> On 4/19/20 12:58 AM, Christoph Hellwig wrote:
>> On Sat, Apr 18, 2020 at 08:40:20AM -0700, Bart Van Assche wrote:
>>>> This can have a sideeffect not only bdi->dev_name will be truncated to 64
>>>> chars (which generally doesn't matter) but possibly also kobject name will
>>>> be truncated in the same way.  Which may have user visible effects. E.g.
>>>> for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
>>>> other way around - i.e., let device_create_vargs() create the device name
>>>> and then copy to bdi->dev_name whatever fits?
>>>
>>> How about using kvasprintf() instead of vsnprintf()?
>>
>> That is what v1 did, see the thread in response to that on why it isn't
>> a good idea.
>
> Are you perhaps referring to patch "[PATCH 3/8] bdi: add a ->dev_name field 
> to struct backing_dev_info" 
> (https://lore.kernel.org/linux-block/20200416071519.807660-4-hch@lst.de/) 
> and also to the replies to that patch? This is what I found in the replies: 
> "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."
>
> Has it been considered to avoid that leak by freeing bdi->dev_name from 
> unregister_bdi(), e.g. as follows?

We'd need some protection against concurrent accesses as unregister_bdi
can race with them.  But with RCU that could be handled, so let me try
that.

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

[Christoph Hellwig]

On Sun, Apr 19, 2020 at 06:06:51PM +0200, Christoph Hellwig wrote:
> > (https://lore.kernel.org/linux-block/20200416071519.807660-4-hch@lst.de/) 
> > and also to the replies to that patch? This is what I found in the replies: 
> > "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."
> >
> > Has it been considered to avoid that leak by freeing bdi->dev_name from 
> > unregister_bdi(), e.g. as follows?
> 
> We'd need some protection against concurrent accesses as unregister_bdi
> can race with them.  But with RCU that could be handled, so let me try
> that.

I looked into it, and while it seems doable I think this goes in the
wrong direction as it pushed the RCU knowledge into the callers.  I'd
rather get something like this series in ASAP, and then for 5.8 or 5.9
move the bdi pointer to the gendisk and stop re-registering it and thus
solve the problems root cause for real.

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

[Jan Kara]

On Mon 20-04-20 09:48:01, Christoph Hellwig wrote:
> On Sun, Apr 19, 2020 at 06:06:51PM +0200, Christoph Hellwig wrote:
> > > (https://lore.kernel.org/linux-block/20200416071519.807660-4-hch@lst.de/) 
> > > and also to the replies to that patch? This is what I found in the replies: 
> > > "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."
> > >
> > > Has it been considered to avoid that leak by freeing bdi->dev_name from 
> > > unregister_bdi(), e.g. as follows?
> > 
> > We'd need some protection against concurrent accesses as unregister_bdi
> > can race with them.  But with RCU that could be handled, so let me try
> > that.
> 
> I looked into it, and while it seems doable I think this goes in the
> wrong direction as it pushed the RCU knowledge into the callers.  I'd
> rather get something like this series in ASAP, and then for 5.8 or 5.9
> move the bdi pointer to the gendisk and stop re-registering it and thus
> solve the problems root cause for real.

Yeah, if it could be done it would be a nice solution. Because
re-registering of BDIs is a long-term source of troubles...

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

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

[Jan Kara]

On Sun 19-04-20 18:06:51, Christoph Hellwig wrote:
> On Sun, Apr 19, 2020 at 08:29:21AM -0700, Bart Van Assche wrote:
> > On 4/19/20 12:58 AM, Christoph Hellwig wrote:
> >> On Sat, Apr 18, 2020 at 08:40:20AM -0700, Bart Van Assche wrote:
> >>>> This can have a sideeffect not only bdi->dev_name will be truncated to 64
> >>>> chars (which generally doesn't matter) but possibly also kobject name will
> >>>> be truncated in the same way.  Which may have user visible effects. E.g.
> >>>> for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
> >>>> other way around - i.e., let device_create_vargs() create the device name
> >>>> and then copy to bdi->dev_name whatever fits?
> >>>
> >>> How about using kvasprintf() instead of vsnprintf()?
> >>
> >> That is what v1 did, see the thread in response to that on why it isn't
> >> a good idea.
> >
> > Are you perhaps referring to patch "[PATCH 3/8] bdi: add a ->dev_name field 
> > to struct backing_dev_info" 
> > (https://lore.kernel.org/linux-block/20200416071519.807660-4-hch@lst.de/) 
> > and also to the replies to that patch? This is what I found in the replies: 
> > "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."
> >
> > Has it been considered to avoid that leak by freeing bdi->dev_name from 
> > unregister_bdi(), e.g. as follows?
> 
> We'd need some protection against concurrent accesses as unregister_bdi
> can race with them.  But with RCU that could be handled, so let me try
> that.

Yeah, that's what Yufen tried in his series some time ago and what I think
you personally didn't like :).

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

[PATCH 4/8] driver core: remove device_create_vargs

[Christoph Hellwig]

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

[PATCH 5/8] bdi: unexport bdi_register_va

[Christoph Hellwig]

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

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 mm/backing-dev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index efc5b83acd2d..eb6b51e49d11 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -964,7 +964,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

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

[Bart Van Assche]

On 2020-04-16 09:54, Christoph Hellwig wrote:
> bdi_register_va is only used by fs/super.c, which can't be modular.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 6/8] bdi: remove bdi_register_owner

[Christoph Hellwig]

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: Jan Kara <jack@suse.cz>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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 eb6b51e49d11..bb993f99d424 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -977,20 +977,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

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

[Bart Van Assche]

On 2020-04-16 09:54, 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.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 7/8] bdi: simplify bdi_alloc

[Christoph Hellwig]

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

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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 bb993f99d424..1f55d5b8269f 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

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

[Bart Van Assche]

On 2020-04-16 09:54, Christoph Hellwig wrote:
> Merge the _node vs normal version and drop the superflous gfp_t argument.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

[Christoph Hellwig]

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: Jan Kara <jack@suse.cz>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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 2849bdbb3acb..011bb8ad0738 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 1f55d5b8269f..d382272bcc31 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

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

[Bart Van Assche]

On 2020-04-16 09:54, 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.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

[Christoph Hellwig]

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.

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

[Christoph Hellwig]

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

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

[Greg KH]

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>

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

[Yufen Yu]

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);

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

[Jan Kara]

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

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

[Christoph Hellwig]

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.

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

[Christoph Hellwig]

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.

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

[Jan Kara]

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