Received-SPF: pass (google.com: domain of linux-kernel+bounces-215530-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 14 Jun 2024 17:38:20 -0500
From: Ira Weiny <ira.weiny@intel.com>
CC: "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
	"vishal.l.verma@intel.com" <vishal.l.verma@intel.com>, "dave.jiang@intel.com"
	<dave.jiang@intel.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] nvdimm: Fix devs leaks in scan_labels()
Message-ID: <666cc65c76781_262728294ae@iweiny-mobl.notmuch>
References: <20240604031658.951493-1-lizhijian@fujitsu.com>
 <6661e897cfd4c_16ff40294f@iweiny-mobl.notmuch>
 <5a5c91a4-9b0c-47d7-a9e9-43d15a28c0d4@fujitsu.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <5a5c91a4-9b0c-47d7-a9e9-43d15a28c0d4@fujitsu.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?VJbx71Hm/znPsFEScIDMniQx4hQHzFl4cDerpYKmv2V7ykRzV0z7pznSE1SZ?=
 =?us-ascii?Q?0rmICPkUmSMj0Q2JQNO+Pte/+W/8MVoW5F4hJ031+R+AvVvly8+oj5+KCRyN?=
 =?us-ascii?Q?tG/law02feegbGSLi12GIGHiiobenVkDjPDh2fk6NUzL2SJEJVRm8hD4b6l9?=
 =?us-ascii?Q?OgP9b5otHAc/Smwen33MTCIZNkoalZf6Pm52Ad/X7adrIwpas0KT9vIbWKRF?=
 =?us-ascii?Q?UP4I362KfGW/vJ8H7pAIuOxrFrAW1E8W1ndaq87Dt4ocqdWbLLm5V6CHhg7t?=
 =?us-ascii?Q?NpvBHnBSQOcxpRA9mFQ3G2lSc5bOWEv3gJy/l2m5Pvm4u+pAnkN9CvOweIau?=
 =?us-ascii?Q?WF7sg7YJ99J0BpY9oZ8J/lfSUKl9RIQb4mvRiT/PV+w7AXqyEdlfV3S74Jvl?=
 =?us-ascii?Q?SyqJWmeSew9HkZvn6CIVuRL4Z5KJX4SWVgvMQskvoycpaj3USHsB9bHxcKDJ?=
 =?us-ascii?Q?bE0zJ72peGu3Yoyvsd4Krv1Gq2rzRMOlnGKM3Z9/luO+Yey8JH39ooTrUCJK?=
 =?us-ascii?Q?ZFRQ0q7/n6BM+Ixwn5ras7pPo0E1R413g8CRbtDPQJ1HNxHmcH3hwImIa8bU?=
 =?us-ascii?Q?P8Jik8sEKds+wV5GQxyVS+MAX/uvcJagebPQMDX9XGvnja+TBpSdH5DJwRnC?=
 =?us-ascii?Q?6PJJDRGWwLNWyhiXMAuwi2MpBa5w+BcFHe+HR6YmtnL6LiihYXgbffpLwk9x?=
 =?us-ascii?Q?Xok8orzWM6+BkMuIO704kRothUl2TFEStv0qoYxgCnFmtVe4HNWE0wwDIgQc?=
 =?us-ascii?Q?42zOmfKyXYJhq12Otoqih0rEqoQB1h0BfGdO4PLpYkPCepeELsB6d+VPhoir?=
 =?us-ascii?Q?FTIEPL6bwbyKFke8t5K5+yiCNlsx59BLn8VIZBywGeowQBVjlGL78/ODhze/?=
 =?us-ascii?Q?gv8eZ9uiOQSh6AsSwXs7xJlW7X7bVu9zcH/GbK0x5LcKJKDPZzsPLLxpxua5?=
 =?us-ascii?Q?j2oft7m4JwekvIYICwsiOkfKh58aSkrM0frHPeDmvmLzyOwn3YfUp42KzlZ7?=
 =?us-ascii?Q?ucdN9gTuoqGrRanL+xD2aBUyvHCU7m7LLeSCtv8vHFnIebPEYwcPi+Qxbfv+?=
 =?us-ascii?Q?F4KP4KCsJi8UZh2zo2qhpoSg0qilfdD17u1pvB9Bl8dF9okoQeQEYMoktzTD?=
 =?us-ascii?Q?7pW4/Nqx7hAvTVfrKNcyy8nfZl/L+US1jbUJfGEtt5Zbqb7jdC32lpHoBOqZ?=
 =?us-ascii?Q?plpT1ClMhuEMB3QUkEzmJHIDIa905JWF2bRZeiWF/8Rnwz6Z2KBQ3mnITChW?=
 =?us-ascii?Q?/ItJ/VB4Kfu6Uc7s+V5hzQrn55j61PysuDNgUGiNvWQ1ZYKao8C96R14HlZ6?=
 =?us-ascii?Q?tWT/zt7AF9pSeDaSKQiW/TwYULbSb8HnjvkuGF9GMLOlJQyp3h2MBVrhcRrZ?=
 =?us-ascii?Q?qu8IPGXHI8iTtnNTYuPcvPSSUIP7PobBlFoRQjZTVDOBLeQsPEc2kJ1MYDLh?=
 =?us-ascii?Q?Eo4Zzp5Mib59ge+OLVrgzJbPrkO69ePcARaZBlC6lpDowiOznUm577CFm9Q1?=
 =?us-ascii?Q?qhRHrkizSyQhTsIAi1y5WVFkpg6PChfiL5Ur4Xlwok681VXZQCdTFXKjaUh5?=
 =?us-ascii?Q?aAHIapmdjiL0Az5EQCaZ9PP9ZfIvsB/C+ZYHj7Wa?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 410517cb-f86b-44e6-0411-08dc8cc2b414
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 22:38:25.5082
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: NT/YNvMc8L+lIXQNx15smDyYrJ0SFdVvXplrrmhZFIOlhl7CseCXg5+JYujlZkltWtf8uFAXUfwpTUIs44LVLQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB7006
X-OriginatorOrg: intel.com

Zhijian Li (Fujitsu) wrote:
> 
> 
> On 07/06/2024 00:49, Ira Weiny wrote:
> > Li Zhijian wrote:
> >> Don't allocate devs again when it's valid pointer which has pionted to
> >> the memory allocated above with size (count + 2 * sizeof(dev)).
> >>
> >> 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
> >>
> >> Fixes: 1b40e09a1232 ("libnvdimm: blk labels and namespace instantiation")
> >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> >> ---
> >>   drivers/nvdimm/namespace_devs.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> >> index d6d558f94d6b..56b016dbe307 100644
> >> --- a/drivers/nvdimm/namespace_devs.c
> >> +++ b/drivers/nvdimm/namespace_devs.c
> >> @@ -1994,7 +1994,9 @@ 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);
> >> +		/* devs probably has been allocated */
> > 
> > I don't think this is where the bug is.  The loop above is processing the
> > known labels and should exit with a count > 0 if devs is not NULL.
> > 
> >  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.
> 
> Per the piece of the code, return EAGAIN and ENODEV could cause this issue in theory.
> 
> 1980                 dev = create_namespace_pmem(nd_region, nd_mapping, nd_label);
> 1981                 if (IS_ERR(dev)) {
> 1982                         switch (PTR_ERR(dev)) {
> 1983                         case -EAGAIN:
> 1984                                 /* skip invalid labels */
> 1985                                 continue;
> 1986                         case -ENODEV:
> 1987                                 /* fallthrough to seed creation */
> 1988                                 break;
> 1989                         default:
> 1990                                 goto err;
> 1991                         }
> 1992                 } else
> 1993                         devs[count++] = dev;
> 
> 
> > 
> > Can you trace the error you are seeing a bit more to see if this is the
> > case?
> 
> 
> I just tried, but I cannot reproduce this leaking again.
> When it happened(100% reproduce at that time), I probably had a corrupted LSA(I see empty
> output with command 'ndctl list'). It seemed the QEMU emulated Nvdimm device was broken
> for some reasons.

I agree that it was probably a corrupted LSA.  But that is where we need to fix
the bug.

The ENODEV will no longer be returned from create_namespace_pmem() AFAICS.
Which is why I pointed to the EAGAIN case.  This could also be another
cleanup as shown in[1].

But to clean this up completely one must account for the case that some labels
may be ok with a final label being found corrupted.  So the allocation of the
array should only occur when at least 1 valid label is found.

So combining these ideas I think the fix is as show in [2].  Could this case be
added as a test?  And then the patch checked out as a fix?

Ira


[1] because select_pmem_id() is always called with a valid pmem_id the ENODEV
case can never happen.  So From what I can see removing that error case thusly
is ok.

diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index d6d558f94d6b..7069e7267a7d 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] Fix, compile tested only.

diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index d6d558f94d6b..6401ebee3db2 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;
@@ -1961,12 +1955,6 @@ 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;
 
                dev = create_namespace_pmem(nd_region, nd_mapping, nd_label);
                if (IS_ERR(dev)) {
@@ -1974,15 +1962,18 @@ 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;
                        }
-               } else
-                       devs[count++] = dev;
+               }
 
+               __devs = kcalloc(count + 2, sizeof(dev), GFP_KERNEL);
+               if (!__devs)
+                       goto err;
+               memcpy(__devs, devs, sizeof(dev) * count);
+               kfree(devs);
+               devs = __devs;
+               devs[count++] = dev;
        }
 
        dev_dbg(&nd_region->dev, "discovered %d namespace%s\n", count,