Message-ID: <d3a67b6e-c5df-4175-ba75-72aac63a7986@fujitsu.com>
Date: Mon, 19 Aug 2024 03:45:02 +0000
From: "Zhijian Li (Fujitsu)" <lizhijian@...itsu.com>
To: Dan Williams <dan.j.williams@...el.com>, "nvdimm@...ts.linux.dev"
	<nvdimm@...ts.linux.dev>
CC: "vishal.l.verma@...el.com" <vishal.l.verma@...el.com>,
	"dave.jiang@...el.com" <dave.jiang@...el.com>, "ira.weiny@...el.com"
	<ira.weiny@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] nvdimm: Fix devs leaks in scan_labels()

(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@...el.com>

Powered by blists - more mailing lists