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