Received-SPF: pass (google.com: domain of linux-kernel+bounces-1326-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 15 Dec 2023 08:45:10 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: Ira Weiny <ira.weiny@intel.com>, Dinghao Liu <dinghao.liu@zju.edu.cn>
CC: Dan Williams <dan.j.williams@intel.com>, Vishal Verma
	<vishal.l.verma@intel.com>, Dave Jiang <dave.jiang@intel.com>, Ira Weiny
	<ira.weiny@intel.com>, <nvdimm@lists.linux.dev>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] [v2] nvdimm-btt: fix several memleaks
Message-ID: <657c82966e358_2947c22941a@iweiny-mobl.notmuch>
References: <20231210085817.30161-1-dinghao.liu@zju.edu.cn>
 <657b9cb088175_27db80294d2@iweiny-mobl.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <657b9cb088175_27db80294d2@iweiny-mobl.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?SML7w0bPldxGWql/etWBXATkK4hl5BCVueD/4i4XVkhrLl2JPbUtogBPcM5y?=
 =?us-ascii?Q?0/Q6ttaVRBDp6u9TQ4mvsIXtnkAMN+VyVO5r1spgTKoHjIjnXRpa2oRoy4EC?=
 =?us-ascii?Q?NjcxfzssDKQIOP5q5jY966lco9BuyRshBP+lT6WrhiI4azJyeJqvqAqGPJ1h?=
 =?us-ascii?Q?07dtqPl5QdQ5sf15Oc1fvc3uSLTW85UeFA/o7GTKb35TjaRV803ESWrhSCLv?=
 =?us-ascii?Q?YlbmGO+GLGMSukg/mPfRrI1gBkcIafQ9V/0l8BYwNnidW0USdHvLbq4Ead+t?=
 =?us-ascii?Q?xgqy3EYhlcJxlq/M1MDQFJH15YoeSaJ5sncV65YTjD8cFGAS1GldGnrUlCjZ?=
 =?us-ascii?Q?ZKOAh1S8S1Xl4XEAxLupgwovSGIfsXir8zW4J1tsNDnQtJc3M5ch1tBWCk9x?=
 =?us-ascii?Q?35g7pl/dEoQ/3Gtu2Z6X9mMi7nsezHERQpPNbLF23Hu4VpAxB2hH8onoJmUB?=
 =?us-ascii?Q?A/h3JYvQubnOhsJPj3D5unoyQv7DiZi/yI14EZtBWA1xGQLkZZyIb4lJT0LR?=
 =?us-ascii?Q?MiSVoMMs8TAVqEhHt+ZJEYp9sQ2rfVjjvbTmwuxhn4DxeYM5MppjlZJsBhxK?=
 =?us-ascii?Q?zaEmFpRJnFzN+vfUYXdAEyUQHqdmKCMb0I0RUNnUuaylZG9IvYF0ChujOFjE?=
 =?us-ascii?Q?RJVwsOUSt54TeabyzQQfwV976CT1UiBqoj2qVKhKTG5vZnHSl5dJ8aBA/B/z?=
 =?us-ascii?Q?KkW1mEtqkYtNDCOCNzJchdshw1g0hinGAFggCNEStxaBFfOzKfctg/OyyI7g?=
 =?us-ascii?Q?iT0xz73baejVcWbmsZh0cXOfyJjuvs5Vdi19ra9ScbWPe+5vI6hhgtIJcVi2?=
 =?us-ascii?Q?X+r2cmw0yPKQClMV9Nwu0eqJR2BFO1qtHyvbaU0aK+uNkDACLlABWkJwT4aN?=
 =?us-ascii?Q?pqaMKpDbbfeEUq0P9GCuyPh0Pvyhxot9P0gXo2s8k/tSZeOCAK9RNV2m+wmK?=
 =?us-ascii?Q?8WJp2XViXUW6Kygum7XeRwHwjbYYdkdUz0F55MHJMD1gGhtNRJRxLGr+ByXw?=
 =?us-ascii?Q?zCrATh+5XM7C5G0dAqWT8ERwmuZ3RoAoAuewBZJkdY9J8fh9bU0ck/glce9M?=
 =?us-ascii?Q?mhf7/14ALpLQenKMNoFB+b3+ng6B6U5oiaT7w+re15KxzWaYCkMvTRJJXKDT?=
 =?us-ascii?Q?6zJqIKmREBJ9V0BtxC7ewSmwLkTQeKxBASp/JWA2emsgK4LKHv9SPqp8nbw2?=
 =?us-ascii?Q?Tg8IeAF3U/w9jk2qreIFh66itPMiiBW/u9kuzBCMxtflKNlHq7lrfnHMVh7L?=
 =?us-ascii?Q?i3u5a3Y7Aj3xADP5BeE7VBFXF+9Fvzisf1ElFxhuSj01ZPWeiynzSvv3y637?=
 =?us-ascii?Q?7UsjWLPcRbbQ4p0m6fTYb97gsgH6mpp3MmKA+Ad3/bpU3Bv/tsyONTbSC+LP?=
 =?us-ascii?Q?nLMIjg2ZeM9mJUb4QZRiBxhIguPuaVimjVmB33HpY49oFuPQ7F8o5fJmDNMV?=
 =?us-ascii?Q?IZYiFnVkOfXSrrQic6oV+J7k/deqnIUlsie2wbKpTEIdiD0fs95OpkE4KpF7?=
 =?us-ascii?Q?MryWq+AqW4x1AbUuxvFCSO7khLOSj9zYlSgBu3soySU3zqiy0co0rB2PhM5E?=
 =?us-ascii?Q?u366faFPSNu28ZyxXONjyuq/1wZU7EEIjF4olJGF?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 171c14cb-70cf-4a26-3019-08dbfd8d355d
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Dec 2023 16:45:13.2223
 (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: FJpHZPW5LSwwb4x8P2MnyOTpXq+4ZTWAyzEkZR9fmZ73AZVGiQjxh8WIjGygPMQO9tZtUP37TMeK/RE47NO6Ag==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6136
X-OriginatorOrg: intel.com

Ira Weiny wrote:
> Dinghao Liu wrote:

[snip]

> >  
> > -static int btt_maplocks_init(struct arena_info *arena)
> > +static int btt_maplocks_init(struct device *dev, struct arena_info *arena)
> >  {
> >  	u32 i;
> >  
> > -	arena->map_locks = kcalloc(arena->nfree, sizeof(struct aligned_lock),
> > +	arena->map_locks = devm_kcalloc(dev, arena->nfree, sizeof(struct aligned_lock),
> >  				GFP_KERNEL);
> >  	if (!arena->map_locks)
> >  		return -ENOMEM;
> > @@ -805,9 +805,6 @@ static void free_arenas(struct btt *btt)
> >  
> >  	list_for_each_entry_safe(arena, next, &btt->arena_list, list) {
> >  		list_del(&arena->list);
> > -		kfree(arena->rtt);
> > -		kfree(arena->map_locks);
> > -		kfree(arena->freelist);
> 
> This does not quite work.
> 
> free_arenas() is used in the error paths of create_arenas() and
> discover_arenas().  In those cases devm_kfree() is probably a better way
> to clean up this.
> 
> However...
> 
> >  		debugfs_remove_recursive(arena->debugfs_dir);
> >  		kfree(arena);
> 
> Why can't arena be allocated with devm_*?
> 
> We need to change this up a bit more to handle the error path vs regular
> device shutdown free (automatic devm frees).

We might want to look at using no_free_ptr() in this code.  See this
patch[1] for an example of how to inhibit the cleanup and pass the pointer
on when the function succeeds.

[1] https://lore.kernel.org/all/170261791914.1714654.6447680285357545638.stgit@dwillia2-xfh.jf.intel.com/

Ira