[PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer

[PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer

[Geliang Tang]

This patch uses kmemdup_nul to create a NUL-terminated string from
dc->sb.label. This is better than open coding it.

With this, we can move env[2] initialization into env[] array to make
code more elegant.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 drivers/md/bcache/super.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 4dee119c3664..84ab241c8516 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -906,21 +906,18 @@ static int cached_dev_status_update(void *arg)
 void bch_cached_dev_run(struct cached_dev *dc)
 {
 	struct bcache_device *d = &dc->disk;
-	char buf[SB_LABEL_SIZE + 1];
+	char *buf = kmemdup_nul(dc->sb.label, SB_LABEL_SIZE, GFP_KERNEL);
 	char *env[] = {
 		"DRIVER=bcache",
 		kasprintf(GFP_KERNEL, "CACHED_UUID=%pU", dc->sb.uuid),
-		NULL,
+		kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf ? : ""),
 		NULL,
 	};
 
-	memcpy(buf, dc->sb.label, SB_LABEL_SIZE);
-	buf[SB_LABEL_SIZE] = '\0';
-	env[2] = kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf);
-
 	if (atomic_xchg(&dc->running, 1)) {
 		kfree(env[1]);
 		kfree(env[2]);
+		kfree(buf);
 		return;
 	}
 
@@ -944,6 +941,7 @@ void bch_cached_dev_run(struct cached_dev *dc)
 	kobject_uevent_env(&disk_to_dev(d->disk)->kobj, KOBJ_CHANGE, env);
 	kfree(env[1]);
 	kfree(env[2]);
+	kfree(buf);
 
 	if (sysfs_create_link(&d->kobj, &disk_to_dev(d->disk)->kobj, "dev") ||
 	    sysfs_create_link(&disk_to_dev(d->disk)->kobj, &d->kobj, "bcache"))
-- 
2.17.1

Re: [PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer

[Coly Li]

On 2019/1/30 5:29 下午, Geliang Tang wrote:
> This patch uses kmemdup_nul to create a NUL-terminated string from
> dc->sb.label. This is better than open coding it.
> 
> With this, we can move env[2] initialization into env[] array to make
> code more elegant.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>

Hi Geliang,

In general I am OK with your idea. But I feel there might be some
regression with your change. I comment your patch in line, correct me if
I am wrong.


> ---
>  drivers/md/bcache/super.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 4dee119c3664..84ab241c8516 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -906,21 +906,18 @@ static int cached_dev_status_update(void *arg)
>  void bch_cached_dev_run(struct cached_dev *dc)
>  {
>  	struct bcache_device *d = &dc->disk;
> -	char buf[SB_LABEL_SIZE + 1];
> +	char *buf = kmemdup_nul(dc->sb.label, SB_LABEL_SIZE, GFP_KERNEL);

If kdumdup_null() is failed, buf will be NULL.

>  	char *env[] = {
>  		"DRIVER=bcache",
>  		kasprintf(GFP_KERNEL, "CACHED_UUID=%pU", dc->sb.uuid),
> -		NULL,
> +		kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf ? : ""),

If buf is NULL, env[2] here is pointed to "" which is allocated in
read-only data segment, and not a dynamic memory.

>  		NULL,
>  	};
>  
> -	memcpy(buf, dc->sb.label, SB_LABEL_SIZE);
> -	buf[SB_LABEL_SIZE] = '\0';
> -	env[2] = kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf);
> -
>  	if (atomic_xchg(&dc->running, 1)) {
>  		kfree(env[1]);
>  		kfree(env[2]);

Then kfree() here will try to release a read-only memory segment. I
guess this is problematic.

> +		kfree(buf);
>  		return;
>  	}
>  
> @@ -944,6 +941,7 @@ void bch_cached_dev_run(struct cached_dev *dc)
>  	kobject_uevent_env(&disk_to_dev(d->disk)->kobj, KOBJ_CHANGE, env);
>  	kfree(env[1]);
>  	kfree(env[2]);

Same problem might happen here for env[2].

> +	kfree(buf);
>  
>  	if (sysfs_create_link(&d->kobj, &disk_to_dev(d->disk)->kobj, "dev") ||
>  	    sysfs_create_link(&disk_to_dev(d->disk)->kobj, &d->kobj, "bcache"))
> 


-- 

Coly Li

Re: [PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer

[Geliang Tang]

On Wed, Feb 06, 2019 at 04:37:36PM +0800, Coly Li wrote:
> On 2019/1/30 5:29 下午, Geliang Tang wrote:
> > This patch uses kmemdup_nul to create a NUL-terminated string from
> > dc->sb.label. This is better than open coding it.
> > 
> > With this, we can move env[2] initialization into env[] array to make
> > code more elegant.
> > 
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> 
> Hi Geliang,
> 
> In general I am OK with your idea. But I feel there might be some
> regression with your change. I comment your patch in line, correct me if
> I am wrong.
> 
> 
> > ---
> >  drivers/md/bcache/super.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index 4dee119c3664..84ab241c8516 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -906,21 +906,18 @@ static int cached_dev_status_update(void *arg)
> >  void bch_cached_dev_run(struct cached_dev *dc)
> >  {
> >  	struct bcache_device *d = &dc->disk;
> > -	char buf[SB_LABEL_SIZE + 1];
> > +	char *buf = kmemdup_nul(dc->sb.label, SB_LABEL_SIZE, GFP_KERNEL);
> 
> If kdumdup_null() is failed, buf will be NULL.
> 
> >  	char *env[] = {
> >  		"DRIVER=bcache",
> >  		kasprintf(GFP_KERNEL, "CACHED_UUID=%pU", dc->sb.uuid),
> > -		NULL,
> > +		kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf ? : ""),
> 
> If buf is NULL, env[2] here is pointed to "" which is allocated in
> read-only data segment, and not a dynamic memory.

Hi Coly,

Sorry for my late reply.

If buf is NULL, env[2] is kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", "");
In this case, env[2] is also a dynamic memory, a string like this, "CACHED_LABEL=".
So we can use kfree() to free it. There is no problem.

And here is a test case:

$ cat test.c 
#include <linux/init.h>
#include <linux/module.h>
#include <linux/slab.h>

static int __init test_init(void)
{
	char *env = kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", "");
	pr_info("env = [%s]\n", env);
	kfree(env);
	return 0;
}

static void __exit test_exit(void)
{
}

module_init(test_init);
module_exit(test_exit);

MODULE_LICENSE("GPL");

$ sudo insmod test.ko
$ dmesg
[ 3026.072298] env = [CACHED_LABEL=]
$ sudo rmmod test

Thanks.

-Geliang

> 
> >  		NULL,
> >  	};
> >  
> > -	memcpy(buf, dc->sb.label, SB_LABEL_SIZE);
> > -	buf[SB_LABEL_SIZE] = '\0';
> > -	env[2] = kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf);
> > -
> >  	if (atomic_xchg(&dc->running, 1)) {
> >  		kfree(env[1]);
> >  		kfree(env[2]);
> 
> Then kfree() here will try to release a read-only memory segment. I
> guess this is problematic.
> 
> > +		kfree(buf);
> >  		return;
> >  	}
> >  
> > @@ -944,6 +941,7 @@ void bch_cached_dev_run(struct cached_dev *dc)
> >  	kobject_uevent_env(&disk_to_dev(d->disk)->kobj, KOBJ_CHANGE, env);
> >  	kfree(env[1]);
> >  	kfree(env[2]);
> 
> Same problem might happen here for env[2].
> 
> > +	kfree(buf);
> >  
> >  	if (sysfs_create_link(&d->kobj, &disk_to_dev(d->disk)->kobj, "dev") ||
> >  	    sysfs_create_link(&disk_to_dev(d->disk)->kobj, &d->kobj, "bcache"))
> > 
> 
> 
> -- 
> 
> Coly Li

Re: [PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer

[Coly Li]

On 2019/2/26 6:01 下午, Geliang Tang wrote:
> On Wed, Feb 06, 2019 at 04:37:36PM +0800, Coly Li wrote:
>> On 2019/1/30 5:29 下午, Geliang Tang wrote:
>>> This patch uses kmemdup_nul to create a NUL-terminated string from
>>> dc->sb.label. This is better than open coding it.
>>>
>>> With this, we can move env[2] initialization into env[] array to make
>>> code more elegant.
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>
>> Hi Geliang,
>>
>> In general I am OK with your idea. But I feel there might be some
>> regression with your change. I comment your patch in line, correct me if
>> I am wrong.
>>
>>
>>> ---
>>>  drivers/md/bcache/super.c | 10 ++++------
>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>>> index 4dee119c3664..84ab241c8516 100644
>>> --- a/drivers/md/bcache/super.c
>>> +++ b/drivers/md/bcache/super.c
>>> @@ -906,21 +906,18 @@ static int cached_dev_status_update(void *arg)
>>>  void bch_cached_dev_run(struct cached_dev *dc)
>>>  {
>>>  	struct bcache_device *d = &dc->disk;
>>> -	char buf[SB_LABEL_SIZE + 1];
>>> +	char *buf = kmemdup_nul(dc->sb.label, SB_LABEL_SIZE, GFP_KERNEL);
>>
>> If kdumdup_null() is failed, buf will be NULL.
>>
>>>  	char *env[] = {
>>>  		"DRIVER=bcache",
>>>  		kasprintf(GFP_KERNEL, "CACHED_UUID=%pU", dc->sb.uuid),
>>> -		NULL,
>>> +		kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf ? : ""),
>>
>> If buf is NULL, env[2] here is pointed to "" which is allocated in
>> read-only data segment, and not a dynamic memory.
> 
> Hi Coly,
> 
> Sorry for my late reply.
> 
> If buf is NULL, env[2] is kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", "");

Oh, I overlooked the location of last bracket.


> In this case, env[2] is also a dynamic memory, a string like this, "CACHED_LABEL=".
> So we can use kfree() to free it. There is no problem.
> 

Sure, it is OK to me. I will add it into my for-test directory.

Thanks.

Coly Li

> And here is a test case:
> 
> $ cat test.c 
> #include <linux/init.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> 
> static int __init test_init(void)
> {
> 	char *env = kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", "");
> 	pr_info("env = [%s]\n", env);
> 	kfree(env);
> 	return 0;
> }
> 
> static void __exit test_exit(void)
> {
> }
> 
> module_init(test_init);
> module_exit(test_exit);
> 
> MODULE_LICENSE("GPL");
> 
> $ sudo insmod test.ko
> $ dmesg
> [ 3026.072298] env = [CACHED_LABEL=]
> $ sudo rmmod test
> 
> Thanks.
> 
> -Geliang
> 
>>
>>>  		NULL,
>>>  	};
>>>  
>>> -	memcpy(buf, dc->sb.label, SB_LABEL_SIZE);
>>> -	buf[SB_LABEL_SIZE] = '\0';
>>> -	env[2] = kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf);
>>> -
>>>  	if (atomic_xchg(&dc->running, 1)) {
>>>  		kfree(env[1]);
>>>  		kfree(env[2]);
>>
>> Then kfree() here will try to release a read-only memory segment. I
>> guess this is problematic.
>>
>>> +		kfree(buf);
>>>  		return;
>>>  	}
>>>  
>>> @@ -944,6 +941,7 @@ void bch_cached_dev_run(struct cached_dev *dc)
>>>  	kobject_uevent_env(&disk_to_dev(d->disk)->kobj, KOBJ_CHANGE, env);
>>>  	kfree(env[1]);
>>>  	kfree(env[2]);
>>
>> Same problem might happen here for env[2].
>>
>>> +	kfree(buf);
>>>  
>>>  	if (sysfs_create_link(&d->kobj, &disk_to_dev(d->disk)->kobj, "dev") ||
>>>  	    sysfs_create_link(&disk_to_dev(d->disk)->kobj, &d->kobj, "bcache"))
>>>
>>
>>
>> -- 
>>
>> Coly Li
> 


-- 

Coly Li