* [PATCH v2 2/2] nvdimm: Remove dead code for ENODEV checking in scan_labels()
2024-07-19 1:58 [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels() Li Zhijian
@ 2024-07-19 1:58 ` Li Zhijian
2024-07-26 13:37 ` [PATCH v2 1/2] nvdimm: Fix devs leaks " Ira Weiny
2024-08-09 22:38 ` Dan Williams
2 siblings, 0 replies; 8+ messages in thread
From: Li Zhijian @ 2024-07-19 1:58 UTC (permalink / raw)
To: nvdimm
Cc: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny,
linux-kernel, Li Zhijian
The only way create_namespace_pmem() returns an ENODEV code is if
select_pmem_id(nd_region, &uuid) returns ENODEV when its 2nd parameter
is a null pointer. However, this is impossible because &uuid is always
valid.
Furthermore, create_namespace_pmem() is the only user of
select_pmem_id(), it's safe to remove the 'return -ENODEV' branch.
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V2:
new patch.
It's found when I'm Reviewing/tracing the return values of create_namespace_pmem()
---
drivers/nvdimm/namespace_devs.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 28c9afc01dca..21020cd16117 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1612,9 +1612,6 @@ static int select_pmem_id(struct nd_region *nd_region, const uuid_t *pmem_id)
{
int i;
- if (!pmem_id)
- return -ENODEV;
-
for (i = 0; i < nd_region->ndr_mappings; i++) {
struct nd_mapping *nd_mapping = &nd_region->mapping[i];
struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
@@ -1790,9 +1787,6 @@ static struct device *create_namespace_pmem(struct nd_region *nd_region,
case -EINVAL:
dev_dbg(&nd_region->dev, "invalid label(s)\n");
break;
- case -ENODEV:
- dev_dbg(&nd_region->dev, "label not found\n");
- break;
default:
dev_dbg(&nd_region->dev, "unexpected err: %d\n", rc);
break;
@@ -1974,9 +1968,6 @@ static struct device **scan_labels(struct nd_region *nd_region)
case -EAGAIN:
/* skip invalid labels */
continue;
- case -ENODEV:
- /* fallthrough to seed creation */
- break;
default:
goto err;
}
--
2.29.2
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels()
2024-07-19 1:58 [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels() Li Zhijian
2024-07-19 1:58 ` [PATCH v2 2/2] nvdimm: Remove dead code for ENODEV checking " Li Zhijian
@ 2024-07-26 13:37 ` Ira Weiny
2024-07-29 2:32 ` Zhijian Li (Fujitsu)
2024-08-09 22:38 ` Dan Williams
2 siblings, 1 reply; 8+ messages in thread
From: Ira Weiny @ 2024-07-26 13:37 UTC (permalink / raw)
To: Li Zhijian, nvdimm
Cc: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny,
linux-kernel, Li Zhijian
Li Zhijian wrote:
> The leakage would happend when create_namespace_pmem() meets an invalid
> label which gets failure in validating isetcookie.
>
> Try to resuse the devs that may have already been allocated with size
> (2 * sizeof(dev)) previously.
>
> A kmemleak reports:
> unreferenced object 0xffff88800dda1980 (size 16):
> comm "kworker/u10:5", pid 69, jiffies 4294671781
> hex dump (first 16 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace (crc 0):
> [<00000000c5dea560>] __kmalloc+0x32c/0x470
> [<000000009ed43c83>] nd_region_register_namespaces+0x6fb/0x1120 [libnvdimm]
> [<000000000e07a65c>] nd_region_probe+0xfe/0x210 [libnvdimm]
> [<000000007b79ce5f>] nvdimm_bus_probe+0x7a/0x1e0 [libnvdimm]
> [<00000000a5f3da2e>] really_probe+0xc6/0x390
> [<00000000129e2a69>] __driver_probe_device+0x78/0x150
> [<000000002dfed28b>] driver_probe_device+0x1e/0x90
> [<00000000e7048de2>] __device_attach_driver+0x85/0x110
> [<0000000032dca295>] bus_for_each_drv+0x85/0xe0
> [<00000000391c5a7d>] __device_attach+0xbe/0x1e0
> [<0000000026dabec0>] bus_probe_device+0x94/0xb0
> [<00000000c590d936>] device_add+0x656/0x870
> [<000000003d69bfaa>] nd_async_device_register+0xe/0x50 [libnvdimm]
> [<000000003f4c52a4>] async_run_entry_fn+0x2e/0x110
> [<00000000e201f4b0>] process_one_work+0x1ee/0x600
> [<000000006d90d5a9>] worker_thread+0x183/0x350
>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Fixes: 1b40e09a1232 ("libnvdimm: blk labels and namespace instantiation")
What is the bigger effect the user will see?
Does this cause a long term user effect? For example, if a users system
has a bad label I think this is going to be a pretty minor memory leak
which just hangs around until reboot, correct?
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>
> Cc: Ira Weiny <ira.weiny@intel.com>
> > From what I can tell create_namespace_pmem() must be returning EAGAIN
> > which leaves devs allocated but fails to increment count. Thus there are
> > no valid labels but devs was not free'ed.
>
> > Can you trace the error you are seeing a bit more to see if this is the
> > case?
> Hi Ira, Sorry for the late reply. I have reproduced it these days.
> Yeah, the LSA is containing a label in which the isetcookie is invalid.
NP, sometimes it takes a while to really debug something.
>
> V2:
> update description and comment
> ---
> drivers/nvdimm/namespace_devs.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index d6d558f94d6b..28c9afc01dca 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1994,7 +1994,13 @@ static struct device **scan_labels(struct nd_region *nd_region)
> /* Publish a zero-sized namespace for userspace to configure. */
> nd_mapping_free_labels(nd_mapping);
>
> - devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
> + /*
> + * Try to use the devs that may have already been allocated
> + * above first. This would happend when create_namespace_pmem()
> + * meets an invalid label.
> + */
> + if (!devs)
> + devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
I'm still tempted to try and fix the count but I think this will work.
Let me know about the severity of the issue.
Ira
> if (!devs)
> goto err;
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels()
2024-07-26 13:37 ` [PATCH v2 1/2] nvdimm: Fix devs leaks " Ira Weiny
@ 2024-07-29 2:32 ` Zhijian Li (Fujitsu)
0 siblings, 0 replies; 8+ messages in thread
From: Zhijian Li (Fujitsu) @ 2024-07-29 2:32 UTC (permalink / raw)
To: Ira Weiny, nvdimm
Cc: dan.j.williams, vishal.l.verma, dave.jiang, linux-kernel
On 26/07/2024 21:37, Ira Weiny wrote:
> Li Zhijian wrote:
>> The leakage would happend when create_namespace_pmem() meets an invalid
>> label which gets failure in validating isetcookie.
>>
>> Try to resuse the devs that may have already been allocated with size
>> (2 * sizeof(dev)) previously.
>>
>> A kmemleak reports:
>> unreferenced object 0xffff88800dda1980 (size 16):
>> comm "kworker/u10:5", pid 69, jiffies 4294671781
>> hex dump (first 16 bytes):
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> backtrace (crc 0):
>> [<00000000c5dea560>] __kmalloc+0x32c/0x470
>> [<000000009ed43c83>] nd_region_register_namespaces+0x6fb/0x1120 [libnvdimm]
>> [<000000000e07a65c>] nd_region_probe+0xfe/0x210 [libnvdimm]
>> [<000000007b79ce5f>] nvdimm_bus_probe+0x7a/0x1e0 [libnvdimm]
>> [<00000000a5f3da2e>] really_probe+0xc6/0x390
>> [<00000000129e2a69>] __driver_probe_device+0x78/0x150
>> [<000000002dfed28b>] driver_probe_device+0x1e/0x90
>> [<00000000e7048de2>] __device_attach_driver+0x85/0x110
>> [<0000000032dca295>] bus_for_each_drv+0x85/0xe0
>> [<00000000391c5a7d>] __device_attach+0xbe/0x1e0
>> [<0000000026dabec0>] bus_probe_device+0x94/0xb0
>> [<00000000c590d936>] device_add+0x656/0x870
>> [<000000003d69bfaa>] nd_async_device_register+0xe/0x50 [libnvdimm]
>> [<000000003f4c52a4>] async_run_entry_fn+0x2e/0x110
>> [<00000000e201f4b0>] process_one_work+0x1ee/0x600
>> [<000000006d90d5a9>] worker_thread+0x183/0x350
>>
>> Cc: Dave Jiang <dave.jiang@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Fixes: 1b40e09a1232 ("libnvdimm: blk labels and namespace instantiation")
>
> What is the bigger effect the user will see?
*Users* cannot use this pmem until they zero-label the device.
In my understanding, once the leakage occurs, there is no way to reclaim the memory until reboot
>
> Does this cause a long term user effect? For example, if a users system
> has a bad label I think this is going to be a pretty minor memory leak
> which just hangs around until reboot, correct?
>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>>> From what I can tell create_namespace_pmem() must be returning EAGAIN
>>> which leaves devs allocated but fails to increment count. Thus there are
>>> no valid labels but devs was not free'ed.
>>
>>> Can you trace the error you are seeing a bit more to see if this is the
>>> case?
>> Hi Ira, Sorry for the late reply. I have reproduced it these days.
>> Yeah, the LSA is containing a label in which the isetcookie is invalid.
>
> NP, sometimes it takes a while to really debug something.
>
>>
>> V2:
>> update description and comment
>> ---
>> drivers/nvdimm/namespace_devs.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
>> index d6d558f94d6b..28c9afc01dca 100644
>> --- a/drivers/nvdimm/namespace_devs.c
>> +++ b/drivers/nvdimm/namespace_devs.c
>> @@ -1994,7 +1994,13 @@ static struct device **scan_labels(struct nd_region *nd_region)
>> /* Publish a zero-sized namespace for userspace to configure. */
>> nd_mapping_free_labels(nd_mapping);
>>
>> - devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
>> + /*
>> + * Try to use the devs that may have already been allocated
>> + * above first. This would happend when create_namespace_pmem()
>> + * meets an invalid label.
>> + */
>> + if (!devs)
>> + devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
>
> I'm still tempted to try and fix the count but I think this will work.
I cannot get your *fix the count* ?
Does "fix the count*" means to free the devs in the case of error cases:
$ git diff
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 28c9afc01dca..3fae00a05ad7 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1970,6 +1970,10 @@ static struct device **scan_labels(struct nd_region *nd_region)
dev = create_namespace_pmem(nd_region, nd_mapping, nd_label);
if (IS_ERR(dev)) {
+ if (!count) {
+ kfree(devs);
+ devs = NULL;
+ }
> Let me know about the severity of the issue.
>
> Ira
>
>> if (!devs)
>> goto err;
>>
>> --
>> 2.29.2
>>
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels()
2024-07-19 1:58 [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels() Li Zhijian
2024-07-19 1:58 ` [PATCH v2 2/2] nvdimm: Remove dead code for ENODEV checking " Li Zhijian
2024-07-26 13:37 ` [PATCH v2 1/2] nvdimm: Fix devs leaks " Ira Weiny
@ 2024-08-09 22:38 ` Dan Williams
2024-08-09 22:55 ` Dan Williams
2024-08-19 3:45 ` Zhijian Li (Fujitsu)
2 siblings, 2 replies; 8+ messages in thread
From: Dan Williams @ 2024-08-09 22:38 UTC (permalink / raw)
To: Li Zhijian, nvdimm
Cc: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny,
linux-kernel, Li Zhijian
I notice this patch is not upstream yet. Let's try to get it over the
goal line.
Li Zhijian wrote:
> The leakage would happend when create_namespace_pmem() meets an invalid
> label which gets failure in validating isetcookie.
I would rewrite this as:
"scan_labels() leaks memory when label scanning fails and it falls back
to just creating a default "seed" namespace for userspace to configure.
Root can force the kernel to leak memory."
...then a distribution developer knows the urgency to backport this fix.
> Try to resuse the devs that may have already been allocated with size
> (2 * sizeof(dev)) previously.
Rather than conditionally reallocating I think it would be better to
unconditionally allocate the minimum, something like:
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index d6d558f94d6b..1c38c93bee21 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1937,12 +1937,16 @@ static int cmp_dpa(const void *a, const void *b)
static struct device **scan_labels(struct nd_region *nd_region)
{
int i, count = 0;
- struct device *dev, **devs = NULL;
+ struct device *dev, **devs;
struct nd_label_ent *label_ent, *e;
struct nd_mapping *nd_mapping = &nd_region->mapping[0];
struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
resource_size_t map_end = nd_mapping->start + nd_mapping->size - 1;
+ devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
+ if (!devs)
+ return NULL;
+
/* "safe" because create_namespace_pmem() might list_move() label_ent */
list_for_each_entry_safe(label_ent, e, &nd_mapping->labels, list) {
struct nd_namespace_label *nd_label = label_ent->label;
@@ -1961,12 +1965,14 @@ static struct device **scan_labels(struct nd_region *nd_region)
goto err;
if (i < count)
continue;
- __devs = kcalloc(count + 2, sizeof(dev), GFP_KERNEL);
- if (!__devs)
- goto err;
- memcpy(__devs, devs, sizeof(dev) * count);
- kfree(devs);
- devs = __devs;
+ if (count) {
+ __devs = kcalloc(count + 2, sizeof(dev), GFP_KERNEL);
+ if (!__devs)
+ goto err;
+ memcpy(__devs, devs, sizeof(dev) * count);
+ kfree(devs);
+ devs = __devs;
+ }
dev = create_namespace_pmem(nd_region, nd_mapping, nd_label);
if (IS_ERR(dev)) {
@@ -1994,10 +2000,6 @@ static struct device **scan_labels(struct nd_region *nd_region)
/* Publish a zero-sized namespace for userspace to configure. */
nd_mapping_free_labels(nd_mapping);
- devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
- if (!devs)
- goto err;
-
nspm = kzalloc(sizeof(*nspm), GFP_KERNEL);
if (!nspm)
goto err;
@@ -2036,12 +2038,10 @@ static struct device **scan_labels(struct nd_region *nd_region)
return devs;
err:
- if (devs) {
- for (i = 0; devs[i]; i++)
- namespace_pmem_release(devs[i]);
- kfree(devs);
- }
- return NULL;
+ for (i = 0; devs[i]; i++)
+ namespace_pmem_release(devs[i]);
+ kfree(devs);
+ return NULL;
}
static struct device **create_namespaces(struct nd_region *nd_region)
> A kmemleak reports:
> unreferenced object 0xffff88800dda1980 (size 16):
> comm "kworker/u10:5", pid 69, jiffies 4294671781
> hex dump (first 16 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace (crc 0):
> [<00000000c5dea560>] __kmalloc+0x32c/0x470
> [<000000009ed43c83>] nd_region_register_namespaces+0x6fb/0x1120 [libnvdimm]
> [<000000000e07a65c>] nd_region_probe+0xfe/0x210 [libnvdimm]
> [<000000007b79ce5f>] nvdimm_bus_probe+0x7a/0x1e0 [libnvdimm]
> [<00000000a5f3da2e>] really_probe+0xc6/0x390
> [<00000000129e2a69>] __driver_probe_device+0x78/0x150
> [<000000002dfed28b>] driver_probe_device+0x1e/0x90
> [<00000000e7048de2>] __device_attach_driver+0x85/0x110
> [<0000000032dca295>] bus_for_each_drv+0x85/0xe0
> [<00000000391c5a7d>] __device_attach+0xbe/0x1e0
> [<0000000026dabec0>] bus_probe_device+0x94/0xb0
> [<00000000c590d936>] device_add+0x656/0x870
> [<000000003d69bfaa>] nd_async_device_register+0xe/0x50 [libnvdimm]
> [<000000003f4c52a4>] async_run_entry_fn+0x2e/0x110
> [<00000000e201f4b0>] process_one_work+0x1ee/0x600
> [<000000006d90d5a9>] worker_thread+0x183/0x350
Thanks for including this.
With the above changes you can add:
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels()
2024-08-09 22:38 ` Dan Williams
@ 2024-08-09 22:55 ` Dan Williams
2024-08-19 6:04 ` Zhijian Li (Fujitsu)
2024-08-19 3:45 ` Zhijian Li (Fujitsu)
1 sibling, 1 reply; 8+ messages in thread
From: Dan Williams @ 2024-08-09 22:55 UTC (permalink / raw)
To: Dan Williams, Li Zhijian, nvdimm
Cc: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny,
linux-kernel, Li Zhijian
Dan Williams wrote:
[..]
> @@ -2036,12 +2038,10 @@ static struct device **scan_labels(struct nd_region *nd_region)
...of course you would also need something like:
if (!count) {
kfree(devs);
return NULL;
}
...here, I'll leave that to you to fix up and test.
> return devs;
>
> err:
> - if (devs) {
> - for (i = 0; devs[i]; i++)
> - namespace_pmem_release(devs[i]);
> - kfree(devs);
> - }
> - return NULL;
> + for (i = 0; devs[i]; i++)
> + namespace_pmem_release(devs[i]);
> + kfree(devs);
> + return NULL;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels()
2024-08-09 22:55 ` Dan Williams
@ 2024-08-19 6:04 ` Zhijian Li (Fujitsu)
0 siblings, 0 replies; 8+ messages in thread
From: Zhijian Li (Fujitsu) @ 2024-08-19 6:04 UTC (permalink / raw)
To: Dan Williams, nvdimm; +Cc: vishal.l.verma, dave.jiang, ira.weiny, linux-kernel
On 10/08/2024 06:55, Dan Williams wrote:
> Dan Williams wrote:
> [..]
>> @@ -2036,12 +2038,10 @@ static struct device **scan_labels(struct nd_region *nd_region)
>
> ...of course you would also need something like:
>
> if (!count) {
> kfree(devs);
> return NULL;
> }
It seems we don't need this cleanup, in the count=0 case, we would reach `err` to free devs.
Thanks
Zhijian
>
> ...here, I'll leave that to you to fix up and test.
>
>> return devs;
>>
>> err:
>> - if (devs) {
>> - for (i = 0; devs[i]; i++)
>> - namespace_pmem_release(devs[i]);
>> - kfree(devs);
>> - }
>> - return NULL;
>> + for (i = 0; devs[i]; i++)
>> + namespace_pmem_release(devs[i]);
>> + kfree(devs);
>> + return NULL;
>> }
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels()
2024-08-09 22:38 ` Dan Williams
2024-08-09 22:55 ` Dan Williams
@ 2024-08-19 3:45 ` Zhijian Li (Fujitsu)
1 sibling, 0 replies; 8+ messages in thread
From: Zhijian Li (Fujitsu) @ 2024-08-19 3:45 UTC (permalink / raw)
To: Dan Williams, nvdimm; +Cc: vishal.l.verma, dave.jiang, ira.weiny, linux-kernel
(Just back from the summer holiday)
Sorry for the late reply.
On 10/08/2024 06:38, Dan Williams wrote:
> I notice this patch is not upstream yet. Let's try to get it over the
> goal line.
>
> Li Zhijian wrote:
>> The leakage would happen when create_namespace_pmem() meets an invalid
>> label which gets failure in validating isetcookie.
>
> I would rewrite this as:
>
> "scan_labels() leaks memory when label scanning fails and it falls back
> to just creating a default "seed" namespace for userspace to configure.
> Root can force the kernel to leak memory."
It sounds good to me.
>
> ...then a distribution developer knows the urgency to backport this fix.
>
>> Try to resuse the devs that may have already been allocated with size
>> (2 * sizeof(dev)) previously.
>
> Rather than conditionally reallocating I think it would be better to
> unconditionally allocate the minimum, something like:
Okay, I will update it in V3.
>
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index d6d558f94d6b..1c38c93bee21 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1937,12 +1937,16 @@ static int cmp_dpa(const void *a, const void *b)
> static struct device **scan_labels(struct nd_region *nd_region)
> {
> int i, count = 0;
> - struct device *dev, **devs = NULL;
> + struct device *dev, **devs;
> struct nd_label_ent *label_ent, *e;
> struct nd_mapping *nd_mapping = &nd_region->mapping[0];
> struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> resource_size_t map_end = nd_mapping->start + nd_mapping->size - 1;
>
> + devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
> + if (!devs)
> + return NULL;
> +
> /* "safe" because create_namespace_pmem() might list_move() label_ent */
> list_for_each_entry_safe(label_ent, e, &nd_mapping->labels, list) {
> struct nd_namespace_label *nd_label = label_ent->label;
> @@ -1961,12 +1965,14 @@ static struct device **scan_labels(struct nd_region *nd_region)
> goto err;
> if (i < count)
> continue;
> - __devs = kcalloc(count + 2, sizeof(dev), GFP_KERNEL);
> - if (!__devs)
> - goto err;
> - memcpy(__devs, devs, sizeof(dev) * count);
> - kfree(devs);
> - devs = __devs;
> + if (count) {
> + __devs = kcalloc(count + 2, sizeof(dev), GFP_KERNEL);
> + if (!__devs)
> + goto err;
> + memcpy(__devs, devs, sizeof(dev) * count);
> + kfree(devs);
> + devs = __devs;
> + }
>
> dev = create_namespace_pmem(nd_region, nd_mapping, nd_label);
> if (IS_ERR(dev)) {
> @@ -1994,10 +2000,6 @@ static struct device **scan_labels(struct nd_region *nd_region)
> /* Publish a zero-sized namespace for userspace to configure. */
> nd_mapping_free_labels(nd_mapping);
>
> - devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
> - if (!devs)
> - goto err;
> -
> nspm = kzalloc(sizeof(*nspm), GFP_KERNEL);
> if (!nspm)
> goto err;
> @@ -2036,12 +2038,10 @@ static struct device **scan_labels(struct nd_region *nd_region)
> return devs;
>
> err:
> - if (devs) {
> - for (i = 0; devs[i]; i++)
> - namespace_pmem_release(devs[i]);
> - kfree(devs);
> - }
> - return NULL;
> + for (i = 0; devs[i]; i++)
> + namespace_pmem_release(devs[i]);
> + kfree(devs);
> + return NULL;
> }
>
> static struct device **create_namespaces(struct nd_region *nd_region)
>
>
>> A kmemleak reports:
>> unreferenced object 0xffff88800dda1980 (size 16):
>> comm "kworker/u10:5", pid 69, jiffies 4294671781
>> hex dump (first 16 bytes):
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> backtrace (crc 0):
>> [<00000000c5dea560>] __kmalloc+0x32c/0x470
>> [<000000009ed43c83>] nd_region_register_namespaces+0x6fb/0x1120 [libnvdimm]
>> [<000000000e07a65c>] nd_region_probe+0xfe/0x210 [libnvdimm]
>> [<000000007b79ce5f>] nvdimm_bus_probe+0x7a/0x1e0 [libnvdimm]
>> [<00000000a5f3da2e>] really_probe+0xc6/0x390
>> [<00000000129e2a69>] __driver_probe_device+0x78/0x150
>> [<000000002dfed28b>] driver_probe_device+0x1e/0x90
>> [<00000000e7048de2>] __device_attach_driver+0x85/0x110
>> [<0000000032dca295>] bus_for_each_drv+0x85/0xe0
>> [<00000000391c5a7d>] __device_attach+0xbe/0x1e0
>> [<0000000026dabec0>] bus_probe_device+0x94/0xb0
>> [<00000000c590d936>] device_add+0x656/0x870
>> [<000000003d69bfaa>] nd_async_device_register+0xe/0x50 [libnvdimm]
>> [<000000003f4c52a4>] async_run_entry_fn+0x2e/0x110
>> [<00000000e201f4b0>] process_one_work+0x1ee/0x600
>> [<000000006d90d5a9>] worker_thread+0x183/0x350
>
> Thanks for including this.
>
> With the above changes you can add:
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
^ permalink raw reply [flat|nested] 8+ messages in thread