Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <23a91617-4562-4399-a8c6-df2f3f28c7a9@intel.com>
Date:   Thu, 7 Dec 2023 08:43:42 -0700
User-Agent: Betterbird (Linux)
Subject: Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init
To:     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>
References: <20231207034332.24107-1-dinghao.liu@zju.edu.cn>
Content-Language: en-US
From:   Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <20231207034332.24107-1-dinghao.liu@zju.edu.cn>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UWFHK2RRVG9RblZwd1VXelI3bFU1QTY0SEpIb3JLc056MVhlVmtKVzc3cXVy?=
 =?utf-8?B?OEdzdFNzMTZQMGFvSnJMSUZFNyt1ZVBlNzJwa2p5OU4rNi9qNXpjRTVBS0Ru?=
 =?utf-8?B?bWlnTmthQXVFekVLSFJZUk9Od1lYQkZoSzFqbkJjQUJNM0JhMXlOT3RWMitp?=
 =?utf-8?B?cytPNzc2QU9Jb0F5dllCU1poS0N1ckxCTllxdGRKeUladmJ1V0w3dHQ5OUVy?=
 =?utf-8?B?RmdyZUtveUlKODFoaThVZGpHenJ1Z2FHUzhhMkh6cmVpTXpCY3MwbExrc2VK?=
 =?utf-8?B?K1FiUE44VWVTdkFaQXVCeFkyOUhNUVBWd3B2YW50eStydzZuUWhOV3o2cEsw?=
 =?utf-8?B?dUxUUDRSNlBwejZvVzRPaGZaS1VxWWI2K1gyS1J5bTUvVy9TQ1U4VGFqWDJs?=
 =?utf-8?B?V1oyQzFzamN0TzNzOGVkVjJjZXVMcVVkcTZBNG1VSnpCVWtpaldJV0EwdmV6?=
 =?utf-8?B?bGEzaEJ3Qkhld2xhUVV2WjFodWFGYUw1VTVYQmE3U1VYdTkzZ3dCM3h6QlNG?=
 =?utf-8?B?aTAyOFBCOXN1eEFJeG5Jek1GTXowM0MrS09HNVRIbnJKek15YXA4dGVXN1pm?=
 =?utf-8?B?ZEF0bEFaTzJBaGgxaTA4Um1QT3pQMEt1WUk1NVVya0djUFdDNkh6Q2Jqbmh4?=
 =?utf-8?B?cHhTRi9QQzgvNk1UV0dZSy9hNXdGODVEbVEyczdkS3lkQndmNmN4aHF0Zmxo?=
 =?utf-8?B?NkYvdjV4TWJveE5qb3c0RHQ2TzFMVFdJM2dOWVFXa2tYQXJ2VWlJSStvTXFM?=
 =?utf-8?B?S016WElIOTlRdW1FNVBPc1pTRjRENnU4ZnkzY3J5YjlLaXJGd3dXYkNwWEpt?=
 =?utf-8?B?eTc3WU5lR2pobHRxVHBiM1UvOHVBVE1kYmJvLzBuMWNvY0s0NXRPd2pkTVR6?=
 =?utf-8?B?Y25jeHpLSE13R1ZZSThaa3MyUzRFWW9POUVkaEJRRVhHTUl6RS90Qlp1dm50?=
 =?utf-8?B?Q2ZFL3JPOCs3c0w0QWxiNEtwb1VnNWVSSjZCTEthVXpEdCsvRnFrUFBsdXJM?=
 =?utf-8?B?TkNZVkJ0K3FrVmFZeExPVWE5WmZRMEt6NDNhRUJYVGw3S040K05YVjFDc0RN?=
 =?utf-8?B?MDV2NlVZa2M4cWtGd1U4RWFFdkZDZk5LRkF3M3FkWjkvT3hzdzAzcHlKdytL?=
 =?utf-8?B?Skp0dncvK2ZDeXY0dzhhMFl0NEVIc29GVjAzTzkrR3dqMTVRQkcyb3pmMkhJ?=
 =?utf-8?B?bjZHcEJTU2hpR2wwdXA2RnlaSVJYS0g1OWVZUmtoenFVSXhPeDZsUi9MaVJW?=
 =?utf-8?B?YjNrclVhZEhza1ZiNkFjUXcxWDh4bGEzTGxndFRwVVM2OExSN21UUmFjalg2?=
 =?utf-8?B?eTJkR1dwVzhYY05oY3hrUEJidFRhVW0rL1lRR1pJS3B0N2Irc1l4UzBOaU5l?=
 =?utf-8?B?eG56dTJ1YjRJeU5abkNhblBBaTQ5bXFzcy8vNS9ZUVNlMU01YkxieVc4R2I2?=
 =?utf-8?B?eE1ybURoRDdCZnpUdFdpNnlpT0gwT1JwcENHN2s2cDlNcHpFSzVJWTA1M3dy?=
 =?utf-8?B?dHBSc0IveE9ZMFR2ODk4ZGZMa2lqTTRPa0F3RVNkZEExZnFKbmp2SjdsUVZQ?=
 =?utf-8?B?OUhQUTlHUGR0aGZiMnF6ckUxY1EzL2VDWVVMSUgwZmdRZExNbHhTRWoxRVNS?=
 =?utf-8?B?Z2RqWmQxSTh4anNPVUZZS1JKemVHUVZvUkdJSFR3RmFSNXhXU2xEbjlBdys5?=
 =?utf-8?B?MlZ4eFdwSGRhcVRNUlR5SWU1VmhTcDZoOHJOWnR2OGZ2T3dQNGZXenBaWFU0?=
 =?utf-8?B?d1FOTVBrTzZ4U2xicVhTcWpMejNEeWJ5TWtnaENKUWdjUG5BYWY3OGcxOThJ?=
 =?utf-8?B?WTNxTFNNbUUrZVNrdVZ5d1A1VVVidWtJbEVOZzc5NmZYZEt6NEYzazQyUy9G?=
 =?utf-8?B?blFxcmpkOGNNbXA4SHJiZ2k3RHZKdVVWb2h0VUJxQlQvQzhYd0RlRThCNU5F?=
 =?utf-8?B?OXU2VGNKMjR6QmRsazNTNU0wQUFXUlQzZ3BNRUlLdGQzWjhlV1lHWG81Q2dH?=
 =?utf-8?B?dmRnS0tPWEh2dlJxUEN0Z1FJNCtMTHgwd0FuK0lYd290S2UzR21VRzhYWCth?=
 =?utf-8?B?blNiTGlRZ2pWMGhvWUZPbnpHek1qNGp4WGo5bnVlN3Z3MUlONFBjRUgzZFhj?=
 =?utf-8?Q?RiH9q/bHwrYRWeZy6X4gNpW1f?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 35b1ca92-895c-4ded-adc3-08dbf73b4b83
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Dec 2023 15:43:44.7317
 (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: Yea1k/i8yVBgsUQ/gnFu9nz1ILW0Z6P9tuidq19zg44ysPTqfyuA6zJzpg1vlgJpLdNJFG3gQDbRHkZ4aM49Fw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5473
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 morse.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 (morse.vger.email [0.0.0.0]); Thu, 07 Dec 2023 07:43:58 -0800 (PST)


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/


> ---
>  drivers/nvdimm/btt.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index d5593b0dc700..d8c4ba8bfdda 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -544,8 +544,10 @@ static int btt_freelist_init(struct arena_info *arena)
>  
>  	for (i = 0; i < arena->nfree; i++) {
>  		new = btt_log_read(arena, i, &log_new, LOG_NEW_ENT);
> -		if (new < 0)
> -			return new;
> +		if (new < 0) {
> +			ret = new;
> +			goto out_free;
> +		}
>  
>  		/* old and new map entries with any flags stripped out */
>  		log_oldmap = ent_lba(le32_to_cpu(log_new.old_map));
> @@ -577,7 +579,7 @@ static int btt_freelist_init(struct arena_info *arena)
>  		ret = btt_map_read(arena, le32_to_cpu(log_new.lba), &map_entry,
>  				NULL, NULL, 0);
>  		if (ret)
> -			return ret;
> +			goto out_free;
>  
>  		/*
>  		 * The map_entry from btt_read_map is stripped of any flag bits,
> @@ -594,11 +596,16 @@ static int btt_freelist_init(struct arena_info *arena)
>  			ret = btt_map_write(arena, le32_to_cpu(log_new.lba),
>  					le32_to_cpu(log_new.new_map), 0, 0, 0);
>  			if (ret)
> -				return ret;
> +				goto out_free;
>  		}
>  	}
>  
>  	return 0;
> +
> +out_free:
> +	kfree(arena->freelist);
> +	arena->freelist = NULL;
> +	return ret;
>  }
>  
>  static bool ent_is_padding(struct log_entry *ent)