Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Fri, 8 Dec 2023 15:01:25 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     <dinghao.liu@zju.edu.cn>, Ira Weiny <ira.weiny@intel.com>
CC:     Dave Jiang <dave.jiang@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Dan Williams <dan.j.williams@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: <6573a0451c4b7_1e7d27294ea@iweiny-mobl.notmuch>
References: <20231207034332.24107-1-dinghao.liu@zju.edu.cn>
 <23a91617-4562-4399-a8c6-df2f3f28c7a9@intel.com>
 <65722f2a94d68_1c7b6229452@iweiny-mobl.notmuch>
 <7205fcd5.258f3.18c48233162.Coremail.dinghao.liu@zju.edu.cn>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <7205fcd5.258f3.18c48233162.Coremail.dinghao.liu@zju.edu.cn>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?KP0h3jVYvk8yfFqqjArtpdBCkSjRPBWbSzkDW4UQ+SgtCf1pUNeC56/+/v4q?=
 =?us-ascii?Q?vxDl4aS4XwpMZsjHvijppL6fgvmbFTromU3/mOUL8QGYjVFJwDcJ2ZHmFjvE?=
 =?us-ascii?Q?yA0z4hRfC5fmsof5Cz2VUvA9Myo6cPKr9MLIOTSbemaLYzKSIrYn4EaGWUfk?=
 =?us-ascii?Q?lxh+7MmPYBzUNzt5Us94OW4hOqntmThq/wfCw9TB2li70JkGsyA2LWQ35N5j?=
 =?us-ascii?Q?YScjUaM9sS7wmr7B2n6hCbQk4iVZrBu+z10j4egOTEXDan3zjjR3/A0jHniO?=
 =?us-ascii?Q?uhx1cqXmh56d2Y3GzETi0tKaNPcUJl/xJnurorZLezsnA9PL/mAELMkbuAG9?=
 =?us-ascii?Q?UmaobFAQHCDWCfioq7qbgWXN4fkJaPvxhVJkwQZFRo9BSlfOd57RcM6JB2gv?=
 =?us-ascii?Q?Sy/tXvQA2qEB8oUNuB+gl2aIzbj8OMddADfGSZP8UfKYbDDd72EWq6WgEGCx?=
 =?us-ascii?Q?lbVD/zAdFqe3yrD9+BVLPG2BMBI5pwMIhUrII9igdUWpvfbyge/zfTdZ5Ala?=
 =?us-ascii?Q?4H9ow+yS7ZUZmujoluzA7Zy+OZBV4ZDRoKxjMguGGvvf1Z/e135K0UbGhhz6?=
 =?us-ascii?Q?r4TdZ6T8cgv62ACIEImVnKy/Ikztq7q1r5NfNFyV6QYVYQUIfWPuZ0fqi2Hw?=
 =?us-ascii?Q?5DBz1GQei1rUgKgtN1LgZ5KCkuIn6T1W8xklWo8EYugi3kvsx6kDKmNGpGMt?=
 =?us-ascii?Q?3AUMe6iIBj0idYT1T/rUm2fC+/Z0bqpwl1QaQzu0GpXut2C/Xd2yoHdRIRod?=
 =?us-ascii?Q?6eQqmnH+y89VS7gRsbJ6TJ78i5qZWw4882DQ57+V9KymzUrEvviPtclsf3rb?=
 =?us-ascii?Q?JRYE7vBEH2MgN77r2JBwsRnpapy8QbH10MccAXxH3jupeF7S4mr+9nIjD2gT?=
 =?us-ascii?Q?bmAbHhqod7Jpbq+4kFtqjzsj3/iKImOMszAMt4muyKH0qIui+CL7kQDVDCS9?=
 =?us-ascii?Q?hSxoMWGpJn2OnsoytH1zvQUyCGTkKFkfXgKlcfzXSBH6xGlHvlMXLea9IopM?=
 =?us-ascii?Q?MLMQ2qabHPgMVF/1dOYBEnc9I230CgjyOgxWqOzybOJUihloToUZwTWUixWz?=
 =?us-ascii?Q?Z4LC16ZRYlFnRKiLNcDaK37RlMUOmdU42Yg3IYwqwZzXcQS2lYYkuhX1BTPm?=
 =?us-ascii?Q?4CZ10tXlpQ0AQAGj9b8La1aQ2bQakCHWWMxox4u95qbft/D2nZlcPaG3RewS?=
 =?us-ascii?Q?HkjjHFG+9Vpa6bSoTj4803w8bvUrpBcP0E7LLu75y6CwqR8aqnGQ5HhOw5/h?=
 =?us-ascii?Q?TTCewKPLFevBGxEahD1kaFL6SzKUAyxFKVFOnl5o0zStaU2UFFfyZSFx4Dge?=
 =?us-ascii?Q?B/Zuo0yzSG3BI6d6xXoIMqbT8O3LzPFQ4Ob8eSW1pO0fwluQtqNCmEVzLpdl?=
 =?us-ascii?Q?d09D53IYlJJImCtV98nE19QHswbBmV9w3rdOeVb38DAgUer0bHmLSRNm/oAw?=
 =?us-ascii?Q?o86rdQWV3VM6FSH2CFZWMyiWn8EW7WePQXhj03wpFTNG7QuMHMHrlaOYO3ow?=
 =?us-ascii?Q?DSEysDeIvDUMWxFFu5MxAFgE2MViRYFUG40ESRG0oXE26Tock3shRhL+N5o+?=
 =?us-ascii?Q?HjXPr36S7rrsgpXARwoak4/jkAWSnoGWMMIUkwkm?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2dc53a5a-0654-46b1-66a0-08dbf8419be9
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2023 23:01:27.7894
 (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: dgOBmxG1VuIO/t43/cy+iw63Joqpt0JeZDY1KgeesoStBLWE7ujJPhLpNdKr8ObYzCYXX9DtRQS/AidARyjkHQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4659
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 fry.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 (fry.vger.email [0.0.0.0]); Fri, 08 Dec 2023 15:01:42 -0800 (PST)

dinghao.liu@ wrote:
> > Dave Jiang wrote:

[snip]

> > 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)
> > 
> 
> Thanks for pointing out this issue! I rechecked discover_arenas() and found
> that btt_rtt_init() may also trigger a memleak for the same reason as
> btt_freelist_init(). Also, I checked another call trace:
> 
>     btt_init() -> btt_meta_init() -> btt_maplocks_init()
> 
> I think there is a memleak if btt_maplocks_init() succeeds but an error
> happens in btt_init() after btt_meta_init() (e.g., when btt_blk_init()
> returns an error). Therefore, we may need to fix three functions.

Yea I think we need to trace this code better.  This is why devm_ is nice for
memory allocated for the life of the device.

> 
> > 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?
> > 
> 
> Sure. Currently I plan to send 2 patches as follows:
> 1. Using devm_kcalloc() to replace kcalloc() in btt_freelist_init(), 
>    btt_rtt_init(), and btt_maplocks_init(), and removing the corresponding
>    kfree in free_arenas(). I checked some uses of devm_kcalloc() and it
>    seems that we need not to call devm_kfree(). The memory is automatically
>    freed on driver detach, right?

On device put yes.  So if these allocations are scoped to the life of the
device there would be no reason to call devm_kfree() on them at all.  I was not
sure if they got reallocated at some point or not.

> 2. Using the scoped based management for struct btt_sb *super (not a bug,
>    but it could improve the code).

Good!

> 
> I'm not quite sure whether my understanding or bug fixing plan is correct.
> If there are any issues, please correct me, thanks!

The above sounds right.
Ira