Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Oct 2022 08:16:50 -0700
From:   "Darrick J. Wong" <djwong@kernel.org>
To:     Zeng Heng <zengheng4@huawei.com>
Cc:     bfoster@redhat.com, cmaiolino@redhat.com,
        linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        liwei391@huawei.com
Subject: Re: [PATCH] xfs: fix memory leak in xfs_errortag_init
Message-ID: <Y01x4s9j7TXU/4CW@magnolia>
References: <20221017025155.1810277-1-zengheng4@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221017025155.1810277-1-zengheng4@huawei.com>
Precedence: bulk

On Mon, Oct 17, 2022 at 10:51:55AM +0800, Zeng Heng wrote:
> When `xfs_sysfs_init` returns failed, `mp->m_errortag` needs to free.
> Otherwise kmemleak would report memory leak after mounting xfs image:
> 
> unreferenced object 0xffff888101364900 (size 192):
>   comm "mount", pid 13099, jiffies 4294915218 (age 335.207s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<00000000f08ad25c>] __kmalloc+0x41/0x1b0
>     [<00000000dca9aeb6>] kmem_alloc+0xfd/0x430
>     [<0000000040361882>] xfs_errortag_init+0x20/0x110
>     [<00000000b384a0f6>] xfs_mountfs+0x6ea/0x1a30
>     [<000000003774395d>] xfs_fs_fill_super+0xe10/0x1a80
>     [<000000009cf07b6c>] get_tree_bdev+0x3e7/0x700
>     [<00000000046b5426>] vfs_get_tree+0x8e/0x2e0
>     [<00000000952ec082>] path_mount+0xf8c/0x1990
>     [<00000000beb1f838>] do_mount+0xee/0x110
>     [<000000000e9c41bb>] __x64_sys_mount+0x14b/0x1f0
>     [<00000000f7bb938e>] do_syscall_64+0x3b/0x90
>     [<000000003fcd67a9>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> Fixes: c68401011522 ("xfs: expose errortag knobs via sysfs")
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
>  fs/xfs/xfs_error.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 296faa41d81d..f417320ef9c0 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -234,13 +234,18 @@ int
>  xfs_errortag_init(
>  	struct xfs_mount	*mp)
>  {
> +	int ret;

Nit: xfs naming convention is (usually) 'error' for return values that
are a negative errno or zero; and 'ret' when positive values have
significance.

I don't mind changing that on commit though since this is obviously
correct, so:

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +
>  	mp->m_errortag = kmem_zalloc(sizeof(unsigned int) * XFS_ERRTAG_MAX,
>  			KM_MAYFAIL);
>  	if (!mp->m_errortag)
>  		return -ENOMEM;
>  
> -	return xfs_sysfs_init(&mp->m_errortag_kobj, &xfs_errortag_ktype,
> -			       &mp->m_kobj, "errortag");
> +	ret = xfs_sysfs_init(&mp->m_errortag_kobj, &xfs_errortag_ktype,
> +				&mp->m_kobj, "errortag");
> +	if (ret)
> +		kmem_free(mp->m_errortag);
> +	return ret;
>  }
>  
>  void
> -- 
> 2.25.1
>