[PATCH v3 1/2] nvdimm: Fix devs leaks in scan_labels()

[PATCH v3 1/2] nvdimm: Fix devs leaks in scan_labels()

[Li Zhijian]

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.

Allocate the minimum resources unconditionally and release them when
unneeded to avoid the memory leak.

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")
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---
V3:
  update commit log and allocate the minimum(2 *dev) unconditionally. # Dan

V2:
  update description and comment
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 drivers/nvdimm/namespace_devs.c | 34 ++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index d6d558f94d6b..35d9f3cc2efa 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)) {
@@ -1993,11 +1999,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,11 +2037,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);
-	}
+	for (i = 0; devs[i]; i++)
+		namespace_pmem_release(devs[i]);
+	kfree(devs);
+
 	return NULL;
 }
 
-- 
2.29.2

[PATCH v3 2/2] nvdimm: Remove dead code for ENODEV checking in scan_labels()

[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 35d9f3cc2efa..55cfbf1e0a95 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;
@@ -1980,9 +1974,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

Re: [PATCH v3 1/2] nvdimm: Fix devs leaks in scan_labels()

[Ira Weiny]

Li Zhijian wrote:
> 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.
> 
> Allocate the minimum resources unconditionally and release them when
> unneeded to avoid the memory leak.
> 
> 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")
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>


> ---
> V3:
>   update commit log and allocate the minimum(2 *dev) unconditionally. # Dan
> 
> V2:
>   update description and comment
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  drivers/nvdimm/namespace_devs.c | 34 ++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index d6d558f94d6b..35d9f3cc2efa 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)) {
> @@ -1993,11 +1999,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,11 +2037,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);
> -	}
> +	for (i = 0; devs[i]; i++)
> +		namespace_pmem_release(devs[i]);
> +	kfree(devs);
> +
>  	return NULL;
>  }
>  
> -- 
> 2.29.2
> 

Re: [PATCH v3 2/2] nvdimm: Remove dead code for ENODEV checking in scan_labels()

[Ira Weiny]

Li Zhijian wrote:
> 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>

Reviewed-by: Ira Weiny <ira.weiny@intel.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 35d9f3cc2efa..55cfbf1e0a95 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;
> @@ -1980,9 +1974,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
>