答复: [PATCH] exfat: use kvmalloc_array/kvfree instead of kmalloc_array/kfree

[gaoming <gaoming20@hihonor.com>]

exfat_get_dentry_set could be called after the u-disk have been inserted, 
through exfat_find, __exfat_write_inode functions.
This could happen at any time, which scenario can not guarantee the 
continuity of physical memory.
This bugfix will enhance the robustness of exfat.

Thanks.
-----邮件原件-----
发件人: Namjae Jeon <linkinjeon@kernel.org> 
发送时间: 2023年7月7日 7:10
收件人: gaoming <gaoming20@hihonor.com>
抄送: Sungjong Seo <sj1557.seo@samsung.com>; open list:EXFAT FILE SYSTEM <linux-fsdevel@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; fengbaopeng <fengbaopeng@hihonor.com>; gaoxu <gaoxu2@hihonor.com>; wangfei 00014658 <wangfei66@hihonor.com>; shenchen 00013118 <harry.shen@hihonor.com>
主题: Re: [PATCH] exfat: use kvmalloc_array/kvfree instead of kmalloc_array/kfree

2023-07-05 18:15 GMT+09:00, gaoming <gaoming20@hihonor.com>:
> The call stack shown below is a scenario in the Linux 4.19 kernel.
> Allocating memory failed where exfat fs use kmalloc_array due to 
> system memory fragmentation, while the u-disk was inserted without 
> recognition.
> Devices such as u-disk using the exfat file system are pluggable and 
> may be insert into the system at any time.
> However, long-term running systems cannot guarantee the continuity of 
> physical memory. Therefore, it's necessary to address this issue.
>
> Binder:2632_6: page allocation failure: order:4, 
> mode:0x6040c0(GFP_KERNEL|__GFP_COMP), nodemask=(null) Call trace:
> [242178.097582]  dump_backtrace+0x0/0x4 [242178.097589]  
> dump_stack+0xf4/0x134 [242178.097598]  warn_alloc+0xd8/0x144 
> [242178.097603]  __alloc_pages_nodemask+0x1364/0x1384
> [242178.097608]  kmalloc_order+0x2c/0x510 [242178.097612]  
> kmalloc_order_trace+0x40/0x16c [242178.097618]  __kmalloc+0x360/0x408 
> [242178.097624]  load_alloc_bitmap+0x160/0x284 [242178.097628]  
> exfat_fill_super+0xa3c/0xe7c [242178.097635]  mount_bdev+0x2e8/0x3a0 
> [242178.097638]  exfat_fs_mount+0x40/0x50 [242178.097643]  
> mount_fs+0x138/0x2e8 [242178.097649]  vfs_kern_mount+0x90/0x270 
> [242178.097655]  do_mount+0x798/0x173c [242178.097659]  
> ksys_mount+0x114/0x1ac [242178.097665]  __arm64_sys_mount+0x24/0x34 
> [242178.097671]  el0_svc_common+0xb8/0x1b8 [242178.097676]  
> el0_svc_handler+0x74/0x90 [242178.097681]  el0_svc+0x8/0x340
>
> By analyzing the exfat code,we found that continuous physical memory 
> is not required here,so kvmalloc_array is used can solve this problem.
>
> Signed-off-by: gaoming <gaoming20@hihonor.com>
> ---
>  fs/exfat/balloc.c | 4 ++--
>  fs/exfat/dir.c    | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index 
> 9f42f25fab92..a183558cb7a0 100644
> --- a/fs/exfat/balloc.c
> +++ b/fs/exfat/balloc.c
> @@ -69,7 +69,7 @@ static int exfat_allocate_bitmap(struct super_block *sb,
>  	}
>  	sbi->map_sectors = ((need_map_size - 1) >>
>  			(sb->s_blocksize_bits)) + 1;
> -	sbi->vol_amap = kmalloc_array(sbi->map_sectors,
> +	sbi->vol_amap = kvmalloc_array(sbi->map_sectors,
>  				sizeof(struct buffer_head *), GFP_KERNEL);
>  	if (!sbi->vol_amap)
>  		return -ENOMEM;
> @@ -84,7 +84,7 @@ static int exfat_allocate_bitmap(struct super_block *sb,
>  			while (j < i)
>  				brelse(sbi->vol_amap[j++]);
>
> -			kfree(sbi->vol_amap);
> +			kvfree(sbi->vol_amap);
>  			sbi->vol_amap = NULL;
>  			return -EIO;
>  		}
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 
> 957574180a5e..5cbb78d0a2a2 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -649,7 +649,7 @@ int exfat_put_dentry_set(struct 
> exfat_entry_set_cache *es, int sync)
>  			brelse(es->bh[i]);
>
>  	if (IS_DYNAMIC_ES(es))
> -		kfree(es->bh);
> +		kvfree(es->bh);
>
>  	return err;
>  }
> @@ -888,7 +888,7 @@ int exfat_get_dentry_set(struct 
> exfat_entry_set_cache *es,
>
>  	num_bh = EXFAT_B_TO_BLK_ROUND_UP(off + num_entries * DENTRY_SIZE, sb);
>  	if (num_bh > ARRAY_SIZE(es->__bh)) {
> -		es->bh = kmalloc_array(num_bh, sizeof(*es->bh), GFP_KERNEL);
> +		es->bh = kvmalloc_array(num_bh, sizeof(*es->bh), GFP_KERNEL);
Could you please elaborate why you change this to kvmalloc_array also?

Thanks.
>  		if (!es->bh) {
>  			brelse(bh);
>  			return -ENOMEM;
> --
> 2.17.1
>
>