Re: [PATCH] xfs: Fix unreferenced object reported by kmemleak in xfs_sysfs_init()

[Darrick J. Wong]

On Mon, Oct 17, 2022 at 11:00:29AM +0800, Li Zetao wrote:
> kmemleak reported a sequence of memory leaks, and one of them indicated we
> failed to free a pointer:
>   comm "mount", pid 19610, jiffies 4297086464 (age 60.635s)
>     hex dump (first 8 bytes):
>       73 64 61 00 81 88 ff ff                          sda.....
>     backtrace:
>       [<00000000d77f3e04>] kstrdup_const+0x46/0x70
>       [<00000000e51fa804>] kobject_set_name_vargs+0x2f/0xb0
>       [<00000000247cd595>] kobject_init_and_add+0xb0/0x120
>       [<00000000f9139aaf>] xfs_mountfs+0x367/0xfc0
>       [<00000000250d3caf>] xfs_fs_fill_super+0xa16/0xdc0
>       [<000000008d873d38>] get_tree_bdev+0x256/0x390
>       [<000000004881f3fa>] vfs_get_tree+0x41/0xf0
>       [<000000008291ab52>] path_mount+0x9b3/0xdd0
>       [<0000000022ba8f2d>] __x64_sys_mount+0x190/0x1d0
> 
> As mentioned in kobject_init_and_add() comment, if this function
> returns an error, kobject_put() must be called to properly clean up
> the memory associated with the object. Apparently, xfs_sysfs_init()

Now there's a caller requirement that I didn't know about.

> does not follow such a requirement. When kobject_init_and_add()
> returns an error, the space of kobj->kobject.name alloced by
> kstrdup_const() is unfree, which will cause the above stack.
> 
> Fix it by adding kobject_put() when kobject_init_and_add returns an
> error.
> 
> Fixes: a31b1d3d89e4 ("xfs: add xfs_mount sysfs kobject")
> Signed-off-by: Li Zetao <lizetao1@xxxxxxxxxx>

Thanks for fixing this,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>

--D

> ---
>  fs/xfs/xfs_sysfs.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_sysfs.h b/fs/xfs/xfs_sysfs.h
> index 43585850f154..513095e353a5 100644
> --- a/fs/xfs/xfs_sysfs.h
> +++ b/fs/xfs/xfs_sysfs.h
> @@ -33,10 +33,15 @@ xfs_sysfs_init(
>  	const char		*name)
>  {
>  	struct kobject		*parent;
> +	int err;
>  
>  	parent = parent_kobj ? &parent_kobj->kobject : NULL;
>  	init_completion(&kobj->complete);
> -	return kobject_init_and_add(&kobj->kobject, ktype, parent, "%s", name);
> +	err = kobject_init_and_add(&kobj->kobject, ktype, parent, "%s", name);
> +	if (err)
> +		kobject_put(&kobj->kobject);
> +
> +	return err;
>  }
>  
>  static inline void
> -- 
> 2.31.1
>