Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Thu, 7 Dec 2023 12:46:34 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dave Jiang <dave.jiang@intel.com>,
        Dinghao Liu <dinghao.liu@zju.edu.cn>
CC:     Vishal Verma <vishal.l.verma@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Ira Weiny <ira.weiny@intel.com>, <nvdimm@lists.linux.dev>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init
Message-ID: <65722f2a94d68_1c7b6229452@iweiny-mobl.notmuch>
References: <20231207034332.24107-1-dinghao.liu@zju.edu.cn>
 <23a91617-4562-4399-a8c6-df2f3f28c7a9@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <23a91617-4562-4399-a8c6-df2f3f28c7a9@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?E/ohY5pOSTE3uwFCjrrQ1bww5KD1P3WRY+dG+UNhQdqo56mapxfq5AcRghdY?=
 =?us-ascii?Q?tsm3d9yKkjmFFq0fAn03LKc0Xn6o+QeyHu8PqhPVt11+FgVBBSaAVeT3FKs6?=
 =?us-ascii?Q?4DaEEsuR5jL8ogdAe0R5op35+f4Arbjf7I6RkRp46clOdKCM6yVSqJewV//U?=
 =?us-ascii?Q?U3goN4ZQWQdd3gYZLex+SyI8KusnIK47IqH9iioDXmTopaAMv18IAG0gSxAP?=
 =?us-ascii?Q?Hj6rLInB+iE8GnsbKBOIcqeoHrOdVkm14lZHR2PFt9X4/86Y0K8PjSx/zfIL?=
 =?us-ascii?Q?fAXgFIYlCvSbalNk9nPdwtm4Net1LTjbbP1mJWtct+bsgbVkv6pktPT4eX7t?=
 =?us-ascii?Q?3jhPQh8k7zbaVVj96Cn8zBz7y1TJdYqBdKH3RVnPOvpjIJWhOhlkPa0s2RUT?=
 =?us-ascii?Q?M242zBnYOAx3lk19zrjn8a2LIBWgOn0sfuZdtVjmBGb/yxwzwkjsF9GVZBfG?=
 =?us-ascii?Q?GsLTn0TcaX1keuc+WC80/WLcORJHtch0uHXheetnBi3ZlxZMB/EN9YUwlgrr?=
 =?us-ascii?Q?eA3G54Q86z1c2m7fV83e/q8nI4Byo6K3nt9Q/DGTy+3fVRyoG5ux+5+vzHJG?=
 =?us-ascii?Q?UmEHadvHQ76w99gpJSludUgqiHNrzY0LsMFPi4cY3O2l+DjQ5fn+3X8b+jvW?=
 =?us-ascii?Q?hT1ZZnAecUOs7lrKZ73Hupp+nNCgQcSOkBN5TlqJVsh7lNZ4BVYAvNvzx9nb?=
 =?us-ascii?Q?V+iu6Iy8ghRwsp9XlYhG5LQ0TBHfXZqv07aXxWbGQqW/cT/oNyMcCOyuHKQz?=
 =?us-ascii?Q?sjZBsdZo1owa32Qxe8bK+1Bdgv9yBepOjcYZaCk5/dF5LeT+RuuLRtutYnE1?=
 =?us-ascii?Q?duegRTibBcy1sO8gJGbsXtD6WnA+D9jh7u3l0RH+CVe7jLusMvs9MqW+kcj0?=
 =?us-ascii?Q?PaFRk+IsY9DViuYeD/W6E4DXJ1iYWFuVYx0I92p0BqrLQtNv+7B31FFYrHOO?=
 =?us-ascii?Q?tfNLx03RhvVeC2S5pbBR8gN5KOeKksXnut0EKrPDzsy7N3ehqUjfZn7A+Mo3?=
 =?us-ascii?Q?acxEr6OOIYDWvqgpr08H+3pJh3l0NLLF2KtLkC4UBEyQ/vSve9rCsrjpRnRu?=
 =?us-ascii?Q?TRaBOumPz2v1S6DZdUGplqxlQX6ohOMAjMH2bdc/8N3xPzdq4YuRcM9RMWSa?=
 =?us-ascii?Q?l0mhDZVub/iJ+bm5Fh0MbQHHz7OFa8j5N8QUmxp6v2trq6bq5x1NySy5BQpU?=
 =?us-ascii?Q?vWaZxg4KuX58Zzd+NSLn7OeN2NwCMIigfxC8KnY0wM7ODVCmddhiOAgFrGm1?=
 =?us-ascii?Q?PlLDTftRQQk9noaJ66FwryalmQEoIx9CQihb1dHV3GV3S+ztwfl02LkybgUz?=
 =?us-ascii?Q?NMNI0Yw4PfKdczVPriLZwD8hXPzKBp9sZx2yZkaa4C62RzPJKEAYIKaUsrIN?=
 =?us-ascii?Q?vA/XnpBgqDECtiJsGL5v0tvFM5kfB0N06HW57Plm3aNW6joLIy4fuHIK1H43?=
 =?us-ascii?Q?A9/V1EuUYtAFc5UvpnUamLPfz0X6qBVm85jWFBnd86bNofT//Lb0kPvFOcSI?=
 =?us-ascii?Q?BZgZIAwt8kzQAGvDj3BqhSfhArCtQ2MItyIG82XO9eVqfPm8Uv59cKj+zp0D?=
 =?us-ascii?Q?B2M+wSRQFKQegOfZ2sbZNqd8nGlN90eLs9bSEtEz?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b044083f-d1cb-498d-8082-08dbf7659b67
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Dec 2023 20:46:37.5979
 (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: +qD4L9US1wOlAIcU10VWoRR9BrL8eRluxb/s7koI+8uQRCcTSpzB1T56gtkO/6b/rfdNxY0lpWg3Y+hX18PgGA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7843
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Thu, 07 Dec 2023 12:46:59 -0800 (PST)

Dave Jiang wrote:
> 

[snip]

First off thanks for the patch.  This code seems to have a few things to
clean up.

> 
> On 12/6/23 20:43, Dinghao Liu wrote:
> > When an error happens in btt_freelist_init(), its caller
> > discover_arenas() will directly free arena, which makes
> > arena->freelist allocated in btt_freelist_init() a leaked
> > memory. Fix this by freeing arena->freelist in all error
> > handling paths of btt_freelist_init().
> > 
> > Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
> > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> 
> How about use the new scope based resource management and we can avoid the goto mess altogether?
> https://lwn.net/Articles/934679/
> 

The freelist is returned as part of arena.  I've not traced both paths of
btt_freelist_init() completely but devm_kcalloc() looks like a better
solution here because this memory needs to live past the function scope.

That said, this patch does not completely fix freelist from leaking in the
following error path.

	discover_arenas()
		btt_freelist_init() -> ok (memory allocated)
		btt_rtt_init() -> fail
			goto out;
			(leak because arena is not yet on btt->arena_list)
		OR
		btt_maplocks_init() -> fail
			goto out;
			(leak because arena is not yet on btt->arena_list)

This error could be fixed by adding to arena_list earlier but devm_*()
also takes care of this without having to worry about that logic.

On normal operation all of this memory can be free'ed with the
corresponding devm_kfree() and/or devm_add_action_*() calls if arenas come
and go.  I'm not sure off the top of my head.

In addition, looking at this code.  discover_arenas() could make use of
the scoped based management for struct btt_sb *super!

Dinghao would you be willing to submit a series of 2 or 3 patches to fix
the above issues?

Thanks!
Ira