Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] JFS: fix memleak in jfs_mount
To:     Dongliang Mu <mudongliangabcd@gmail.com>
Cc:     jfs-discussion@lists.sourceforge.net, linux-kernel@vger.kernel.org
References: <20210818102612.864127-1-mudongliangabcd@gmail.com>
From:   Dave Kleikamp <dave.kleikamp@oracle.com>
Message-ID: <fc1dae76-eac1-e6f4-2ba6-f49e15ad0b46@oracle.com>
Date:   Fri, 3 Sep 2021 16:51:18 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
In-Reply-To: <20210818102612.864127-1-mudongliangabcd@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Z0dnUTg5VjF6c1BvNkcvWkFWaU5NQUpaS28vUHl6SUMxN0xTWEl4WFFZZmhl?=
 =?utf-8?B?eXhsNEpHZGRpL2N6dGJSSHZ4NzYxN09IakdoSzNGcS9VMGY0TGN0Z1RHSVlJ?=
 =?utf-8?B?akVBdGZVWk9RQzNpYmlMMWw5TzUwRXlFL3Iydld2b2ZjZ2I3V0VFT3YzTUQx?=
 =?utf-8?B?eEp5TkcwNno1TVlpOGRLTm8xWmZVR2VvZGJJU0hDN3lVQ1hCWTRLWGlDVS8r?=
 =?utf-8?B?NEtQSFNwSnI4TDdiQU14clBHeVBvZWdIZDlPL0hsMWdhaVRpTVExMmdXSUlH?=
 =?utf-8?B?T3Q5U0hvOWl4N1F1R3dMYURlSCsvL1JaS0drVDREU29PRlF2RGUySUtLSGZ4?=
 =?utf-8?B?RTZoS0FmYVp4clVVcWZxcjd2dU1HYzh1RVZSWmFtb0ptMndYNkJuSmx5elhE?=
 =?utf-8?B?Y1pQdWVPZDU3U004RDRIdU5PaGFOQlR3OEdpSTJ0NGFYVWdzaEN2VkdIcGJX?=
 =?utf-8?B?anN5cmlhL3JLU1dvWnc0TjBSYTlXdHowUkxZWVh0S3lCM2QySkg2UnArN0hM?=
 =?utf-8?B?S3oySjZRZ1JIbElmK09lUEd5c0Q1ajN5dkRucmRodFU4MjcrZzNJWjNzcEVw?=
 =?utf-8?B?QmJBRmlMbUhBS3RJYi9mVUxXdDJpOFBKejVpaDExRUdVUlFkZnpsOWFOWU1a?=
 =?utf-8?B?Qm5lNWk3WWxzcTRzOGdYRkVtR1ZORUFmcnA1eE9ZNE5US1JQb21ZcnlxN0pH?=
 =?utf-8?B?ZkxjOHlYZzA4aXFLRzIxUHdCUjMzam4waVp1QTVyTEhmV3FqKzlRRTlUQVRn?=
 =?utf-8?B?TFMwdjBEUVJ1blhTdGdvNDlUblM2aDNZVThlKzZ2WkNlamNWbFA0L3dEOXZI?=
 =?utf-8?B?Z21aQzlTV25LOE9ZN1JocFE4WVlOMm9ZVXRyUWlqSjRZMXV3aTdkWkxib3NG?=
 =?utf-8?B?WTNwd2gvMVJkaFdLaVlxbFBmMWFuWGNaSUo4ay94bTBiRW5LZHN6RFpORERP?=
 =?utf-8?B?MG12UnlxVGFlcldYbC81eFFZdXAyQUlFaTlEL3RTcktZMS9xbDhDWTRhZW9j?=
 =?utf-8?B?bTAxaWV5U2pGU2RaZnBSRktnVTZBS0RoaUF0dU9aK2t0SkxXK1ZXazlkRFp0?=
 =?utf-8?B?aDZaMUNLQWxXZFRNQ2hNYWp6ckJlaXI5UUg0ZFVoQ2RRTk9SYTIzcFdVdGdm?=
 =?utf-8?B?OWxaRjdyZFEwekRaeDFkVDhLZktZbkNBZHNZdmlsQVR6T3I2TjM2VXR5OXM2?=
 =?utf-8?B?WGE3cDhDU3Eyb3JjUkprVTBtQTZid2R4elE4OG9DUXo2aytpaVhtTHJXb3gw?=
 =?utf-8?B?cWVOUDFUZTRzYm1QMjNnanZ6T0I4cForZTdXU1hZaTk3bFgzUGZqR0xlM1NH?=
 =?utf-8?B?UWJvUVM5ODZ4dVMvRjlHU0VxZkZFSTUvWVh1TkhoSHV4NEtXbWRaVVVxWXd2?=
 =?utf-8?B?WFZIVHpmTU9EU0tzemZqNVViM01TcVdXYzEwV0lGcUhMMktOaTlwOGJqbU8x?=
 =?utf-8?B?NXZaT1ZTSmtkakZDN1RjVldBQ2RNYUNjQk1yWmIxOVFqTjdFU3c4WU1iTzFa?=
 =?utf-8?B?ZVpPUzRjY25DT2Faano3M1dpc1h1RThSc0R6NEMxYUt0ckg0OElYZC9BeGhO?=
 =?utf-8?B?aDhqQ2xzelpEdXllTkxaQ1N3TVQzcjg3QjRzS0VZb3E2ZjlnbzF2ZC9kWnNW?=
 =?utf-8?B?MVN1VEJtMDQyQ2FzR3dmOTNXU0MrUExVYkN0QTRvZUtxczNKWjJkc002MDNZ?=
 =?utf-8?B?KzNPNmp1UTVnUXh3V0trK2hEc0J2ZkRMaWJQd3Y1MHdOV05wdjBzdUprRGg4?=
 =?utf-8?Q?dxg3fSqJdsRWXBtBAeV8/ZchLhYLl6EdyIAALc+?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e0c8d097-66d2-4158-702b-08d96f24f74c
X-MS-Exchange-CrossTenant-AuthSource: SA2PR10MB4665.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Sep 2021 21:51:20.9537
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AfkDC0E6/m5z6bVqTiwl8Ckv6YU1cvhk12MbKNrWO9N7eUeyxdLYGREG9bitqLSBzwAk0DWcpJcus2i6Gs7wdruYnHkbUzpY4Pw2Ow0DjZM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR10MB4793
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10096 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0
 mlxlogscore=999 mlxscore=0 suspectscore=0 spamscore=0 phishscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2108310000 definitions=main-2109030128
X-Proofpoint-ORIG-GUID: D8P4Wt3L-Emq0kl_4d_o54pYJZ2hKHfk
X-Proofpoint-GUID: D8P4Wt3L-Emq0kl_4d_o54pYJZ2hKHfk
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Thanks for waiting. I have just a couple comments, but this looks good. 
I appreciate the clean up.

On 8/18/21 5:25 AM, Dongliang Mu wrote:
> In jfs_mount, when diMount(ipaimap2) fails, it goes to errout35. However,
> the following code does not free ipaimap2 allocated by diReadSpecial.
> 
> Fix this by refactoring the error handling code of jfs_mount. To be
> specific, modify the lable name and free ipaimap2 when the above error
> ocurrs.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>   fs/jfs/jfs_mount.c | 53 +++++++++++++++++++---------------------------
>   1 file changed, 22 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/jfs/jfs_mount.c b/fs/jfs/jfs_mount.c
> index 5d7d7170c03c..638a4ecc4069 100644
> --- a/fs/jfs/jfs_mount.c
> +++ b/fs/jfs/jfs_mount.c
> @@ -81,14 +81,14 @@ int jfs_mount(struct super_block *sb)
>   	 * (initialize mount inode from the superblock)
>   	 */
>   	if ((rc = chkSuper(sb))) {
> -		goto errout20;
> +		return rc;

This may have been intentional, but it isn't mentioned in the patch header.

If chkSuper fails, we will no longer call
	jfs_err("Mount JFS Failure: %d", rc);

I don't necessarily see this as a bad thing. In many cases, chkSuper 
prints a more helpful message. In the case where it silently fails, it's 
not even recognizing the superblock as a supported version of JFS and 
this message isn't particularly helpful. In fact this jfs_err() 
statement might be best deleted in its entirety.

>   	}
>   
>   	ipaimap = diReadSpecial(sb, AGGREGATE_I, 0);
>   	if (ipaimap == NULL) {
>   		jfs_err("jfs_mount: Failed to read AGGREGATE_I");
>   		rc = -EIO;
> -		goto errout20;
> +		goto out;
>   	}
>   	sbi->ipaimap = ipaimap;
>   
> @@ -99,7 +99,7 @@ int jfs_mount(struct super_block *sb)
>   	 */
>   	if ((rc = diMount(ipaimap))) {
>   		jfs_err("jfs_mount: diMount(ipaimap) failed w/rc = %d", rc);
> -		goto errout21;
> +		goto err_ipaimap;
>   	}
>   
>   	/*
> @@ -108,7 +108,7 @@ int jfs_mount(struct super_block *sb)
>   	ipbmap = diReadSpecial(sb, BMAP_I, 0);
>   	if (ipbmap == NULL) {
>   		rc = -EIO;
> -		goto errout22;
> +		goto err_umount_ipaimap;
>   	}
>   
>   	jfs_info("jfs_mount: ipbmap:0x%p", ipbmap);
> @@ -120,7 +120,7 @@ int jfs_mount(struct super_block *sb)
>   	 */
>   	if ((rc = dbMount(ipbmap))) {
>   		jfs_err("jfs_mount: dbMount failed w/rc = %d", rc);
> -		goto errout22;
> +		goto err_ipbmap;
>   	}
>   
>   	/*
> @@ -139,7 +139,7 @@ int jfs_mount(struct super_block *sb)
>   		if (!ipaimap2) {
>   			jfs_err("jfs_mount: Failed to read AGGREGATE_I");
>   			rc = -EIO;
> -			goto errout35;
> +			goto err_umount_ipbmap;
>   		}
>   		sbi->ipaimap2 = ipaimap2;
>   
> @@ -151,7 +151,7 @@ int jfs_mount(struct super_block *sb)
>   		if ((rc = diMount(ipaimap2))) {
>   			jfs_err("jfs_mount: diMount(ipaimap2) failed, rc = %d",
>   				rc);
> -			goto errout35;
> +			goto err_ipaimap2;
>   		}
>   	} else
>   		/* Secondary aggregate inode table is not valid */
> @@ -168,7 +168,7 @@ int jfs_mount(struct super_block *sb)
>   		jfs_err("jfs_mount: Failed to read FILESYSTEM_I");
>   		/* open fileset secondary inode allocation map */
>   		rc = -EIO;
> -		goto errout40;
> +		goto err_umount_ipaimap2;
>   	}
>   	jfs_info("jfs_mount: ipimap:0x%p", ipimap);
>   
> @@ -178,41 +178,32 @@ int jfs_mount(struct super_block *sb)
>   	/* initialize fileset inode allocation map */
>   	if ((rc = diMount(ipimap))) {
>   		jfs_err("jfs_mount: diMount failed w/rc = %d", rc);
> -		goto errout41;
> +		goto err_ipimap;
>   	}
>   
> -	goto out;
> +	return rc;
>   
>   	/*
>   	 *	unwind on error
>   	 */
> -      errout41:		/* close fileset inode allocation map inode */
> +err_ipimap:
> +	/* close fileset inode allocation map inode */
>   	diFreeSpecial(ipimap);
> -
> -      errout40:		/* fileset closed */
> -
> +err_umount_ipaimap2:
>   	/* close secondary aggregate inode allocation map */
> -	if (ipaimap2) {
> -		diUnmount(ipaimap2, 1);
> -		diFreeSpecial(ipaimap2);
> -	}
> -
> -      errout35:
> -
> -	/* close aggregate block allocation map */
> +	if (ipaimap2) diUnmount(ipaimap2, 1);

Coding style: this should be split between two lines:
	if (ipaimap2)
		diUnmount(ipaimap2, 1);

> +err_ipaimap2:
> +	/* close aggregate inodes */
> +	if (ipaimap2) diFreeSpecial(ipaimap2);

Same here.

> +err_umount_ipbmap:	/* close aggregate block allocation map */
>   	dbUnmount(ipbmap, 1);
> +err_ipbmap:		/* close aggregate inodes */
>   	diFreeSpecial(ipbmap);
> -
> -      errout22:		/* close aggregate inode allocation map */
> -
> +err_umount_ipaimap:	/* close aggregate inode allocation map */
>   	diUnmount(ipaimap, 1);
> -
> -      errout21:		/* close aggregate inodes */
> +err_ipaimap:		/* close aggregate inodes */
>   	diFreeSpecial(ipaimap);
> -      errout20:		/* aggregate closed */
> -
> -      out:
> -
> +out:
>   	if (rc)
>   		jfs_err("Mount JFS Failure: %d", rc);
>   
>