Received-SPF: pass (google.com: domain of linux-kernel+bounces-204729-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 6 Jun 2024 11:49:27 -0500
From: Ira Weiny <ira.weiny@intel.com>
To: Li Zhijian <lizhijian@fujitsu.com>, <nvdimm@lists.linux.dev>
CC: <dan.j.williams@intel.com>, <vishal.l.verma@intel.com>,
	<dave.jiang@intel.com>, <ira.weiny@intel.com>,
	<linux-kernel@vger.kernel.org>, Li Zhijian <lizhijian@fujitsu.com>
Subject: Re: [PATCH] nvdimm: Fix devs leaks in scan_labels()
Message-ID: <6661e897cfd4c_16ff40294f@iweiny-mobl.notmuch>
References: <20240604031658.951493-1-lizhijian@fujitsu.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240604031658.951493-1-lizhijian@fujitsu.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?86/q4c7guSnuRSIFYn5WA+fNYkUzLYmEOP05102MndRI7DEos9sCvGaE1KNr?=
 =?us-ascii?Q?w52KfewCyXfcYSQs12c9sCPbh3glKfDlem+q6m49DR48tin2o0oLmafVNmas?=
 =?us-ascii?Q?odivTohI6nALIlwu6s49xVDZ0tuL9LBiEt+9vGUUju8VQkgXeeaNOtxCixH/?=
 =?us-ascii?Q?psUpd+N/T6g0eUnZNaSElfVkOOH1mnCZNlKSlpkfPwbfDmanMa2FklSbJ+tB?=
 =?us-ascii?Q?MLPr9KB2K4faOz526PzVmUbwUCscP4pGgkReQIdRQm/Ga8mtxIZe8rg/rLJF?=
 =?us-ascii?Q?eREJeP6FV6gsXIZuxF82xhu6UTSDkJmX4jTdD+ruejvF0y9DngcFXyuyoI9x?=
 =?us-ascii?Q?++e6es4oCbiEicrgSidelyNnktsRZbEz6ZH/z7iDQTn24KSpDKbESBU+3cey?=
 =?us-ascii?Q?RGqamHZuwyqlAwFo2CbTC5Rh+UMfNpuUpPnBC3syzWvQCvAQJpKcnu0zWfkr?=
 =?us-ascii?Q?lF8YnnN6jiDWKydNtK9XnxhMQrfn3JvgAtDX9gceIAmYJ73IyleLKWf/kQj2?=
 =?us-ascii?Q?z2OgQj71lOUTGG5GR1p6AWAu7t5GRGl0hT3v11RmCYoa62woQEIOQ6UEjo1w?=
 =?us-ascii?Q?5hCgUIymbD0Dgg4Xi4zEz8zZOMRVWbVUg6wSvubgUH3tQMHZHfLtZ0ay3vh6?=
 =?us-ascii?Q?fp0ZHeKRNXVCpsshVVsd8Gr2aXkELCnf+35VsFro9q2YEVb2ajj/XZtWLcyr?=
 =?us-ascii?Q?d0TMVhhlErX7WH1tsCUzQu7i47h+D3CcB9DFw4UV+1MlZmfoxrkfPDeZNa+X?=
 =?us-ascii?Q?SJZmGdcOwrl+/lEJm4+f8CpgP99BZR6FDdr+vr4IE6RD31AgJDx1GbDJ5/Wk?=
 =?us-ascii?Q?YKWQnxXHc4Ws/jnO0XgV8Pt8QPZBO5tqM+zjxrGxO1QTpPnLDch+HL7ezB5i?=
 =?us-ascii?Q?qpxyUxrGFCYxAXzZKkhFXpbC2Y91V7GQG1arcWjv32loBUgT1mkThEqxMeJA?=
 =?us-ascii?Q?Owp4VWVb5d1epsnAVK+TNBzUO45GrRhnalQVMRbL/nV5th2OnSli3f4mFsYH?=
 =?us-ascii?Q?Dd8rQhhMGsGpHHxRrk1ZsK/dVKoKpVfGE8Yy2LmJCzfcpJUfEtr16baF+k3T?=
 =?us-ascii?Q?kWV/8kCyp9T4e6UwdQjj8hsgunfX+dEpn3xuQ5lVb2N98XlZwJRXkqxvIy2E?=
 =?us-ascii?Q?A2e2PXc1yqUAiRXDN1TDRixo6hLJF5WdYb2ZItQQV0YC/HHCGzjXC+bvHc8d?=
 =?us-ascii?Q?tplHnihs5II+Hl0FKpKdVP8jjq6jKI6RNBkpwbO8zj2IgXifs07BGX/3mng1?=
 =?us-ascii?Q?/SJOPLE7tYOC6sS7fRwwx/KfDTqiyJXv5YpO0QL6mtTtIZLuwWr+EwQDrGn/?=
 =?us-ascii?Q?mVcnzDgWbH7scwnfbYVGxKnDthSrppVqkJJk2AY26EZyVdew9PlaM/G10lgo?=
 =?us-ascii?Q?K5DY/LCQ6hte7HWIwNI38dxgjzypoW7m2Ew58GRhwe8RBT7VQRJq3uHtILk+?=
 =?us-ascii?Q?szPt4vGoX0amNZHGMAcZqEi83ZkLp0ZDM3E67sR+u5y5hTcv1FQIsq6ikzmx?=
 =?us-ascii?Q?9yULNjbbv9gm/E64pCnK8SFzMVLxnP2ifEja/mKWyY+N7/z23B9+lb7+arZM?=
 =?us-ascii?Q?j++/0PlJzulZh+vHPyr9QkUDHlWIv3/5rVm0ESkp?=
X-MS-Exchange-CrossTenant-Network-Message-Id: df047e8b-01b6-4f95-256c-08dc8648a371
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2024 16:49:31.9467
 (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: uPTYitgTprMt9rJj7Ttvxt4IeCS4QNPl0ERVySMaUiDsdZxU94wfIewpb6EfgF8jnNmxlGPeLT4P1PbeLHywpQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6205
X-OriginatorOrg: intel.com

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.

Can you trace the error you are seeing a bit more to see if this is the
case?  

Thanks,
Ira

> +		if (!devs)
> +			devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
>  		if (!devs)
>  			goto err;
>  
> -- 
> 2.29.2
>