* [PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer @ 2019-01-30 9:29 Geliang Tang 2019-02-06 8:37 ` Coly Li 0 siblings, 1 reply; 4+ messages in thread From: Geliang Tang @ 2019-01-30 9:29 UTC (permalink / raw) To: Coly Li, Kent Overstreet; +Cc: Geliang Tang, linux-bcache, linux-kernel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer 2019-01-30 9:29 [PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer Geliang Tang @ 2019-02-06 8:37 ` Coly Li 2019-02-26 10:01 ` Geliang Tang 0 siblings, 1 reply; 4+ messages in thread From: Coly Li @ 2019-02-06 8:37 UTC (permalink / raw) To: Geliang Tang; +Cc: Kent Overstreet, linux-bcache, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer 2019-02-06 8:37 ` Coly Li @ 2019-02-26 10:01 ` Geliang Tang 2019-02-26 11:03 ` Coly Li 0 siblings, 1 reply; 4+ messages in thread From: Geliang Tang @ 2019-02-26 10:01 UTC (permalink / raw) To: Coly Li, Kent Overstreet; +Cc: linux-bcache, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer 2019-02-26 10:01 ` Geliang Tang @ 2019-02-26 11:03 ` Coly Li 0 siblings, 0 replies; 4+ messages in thread From: Coly Li @ 2019-02-26 11:03 UTC (permalink / raw) To: Geliang Tang; +Cc: Kent Overstreet, linux-bcache, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-26 11:03 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-01-30 9:29 [PATCH] bcache: use kmemdup_nul for CACHED_LABEL buffer Geliang Tang 2019-02-06 8:37 ` Coly Li 2019-02-26 10:01 ` Geliang Tang 2019-02-26 11:03 ` Coly Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox