Re: [PATCH] JFS: fix memleak in jfs_mount

[Dave Kleikamp <dave.kleikamp@oracle.com>]

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);
>   
>