Re: [PATCH] shmem: fix unicode_map leak with repeated casefold= option

[Gabriel Krisman Bertazi]

Mohammed EL Kadiri <med08elkadiri@xxxxxxxxx> writes:

> Each casefold= option makes shmem_parse_opt_casefold() allocate a
> unicode_map with utf8_load() and store it in ctx->encoding.  When the
> option is given more than once in the same mount, the later call
> overwrites ctx->encoding and the earlier map is never freed.
>
> Reproducer:
>
>   mount -t tmpfs -o casefold,casefold tmpfs /t; umount /t
>
> Repeating this 50000 times grows kmalloc-32 by about 50000 objects,
> and kmemleak reports them as allocated from:
>
>   utf8_load+0x21/0x110
>   shmem_parse_opt_casefold.isra.0+0x65/0x100
>   shmem_parse_one+0x368/0x510
>
> Free the old map before storing the new one, so only the last
> casefold= is kept.  On a normal mount with a single casefold=,
> ctx->encoding is still NULL at that point and utf8_unload(NULL)
> does nothing, so nothing changes there.
>
> With this patch the same loop leaves kmalloc-32 flat and kmemleak
> reports nothing.
>
> Fixes: 58e55efd6c72 ("tmpfs: Add casefold lookup support")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Mohammed EL Kadiri <med08elkadiri@xxxxxxxxx>
> ---
> This is a separate leak from the remount one fixed in
> https://lore.kernel.org/all/ba38f80f629fd093c77f3a49fdab7b71ee7bbc5f.1790687276.git.med08elkadiri@xxxxxxxxx/
> which Gabriel has reviewed; I found it while testing that patch.
> The two apply in either order.  Tested on mm-unstable, alone and
> together: kmalloc-32 stays flat for duplicate casefold=, single
> casefold= and remount, and kmemleak is clean.
>
>  mm/shmem.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 07b2855dfb7b..262f1bcb6144 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -4731,6 +4731,7 @@ static int shmem_parse_opt_casefold(struct fs_context *fc, struct fs_parameter *
>  	pr_info("tmpfs: Using encoding : utf8-%u.%u.%u\n",
>  		unicode_major(version), unicode_minor(version), unicode_rev(version));
>  
> +	utf8_unload(ctx->encoding);
>  	ctx->encoding = encoding;

IMO, nack, this is not the right fix.  There is no point in accepting
multiple casefold parameters and we shouldn't allow it.  If
ctx->encoding is already set, we should -EINVAL and fail the mount.


>  
>  	return 0;
>
> base-commit: 90ddfbd1963659ca4a5d3d7f20717a4222682a8e
> -- 
> 2.53.0
>

-- 
Gabriel Krisman Bertazi