2023-07-25 18:22:26

by Ritesh Harjani

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] ext4: fix BUG in ext4_mb_new_inode_pa() due to overflow

Baokun Li <[email protected]> writes:

> When we calculate the end position of ext4_free_extent, this position may
> be exactly where ext4_lblk_t (i.e. uint) overflows. For example, if
> ac_g_ex.fe_logical is 4294965248 and ac_orig_goal_len is 2048, then the
> computed end is 0x100000000, which is 0. If ac->ac_o_ex.fe_logical is not
> the first case of adjusting the best extent, that is, new_bex_end > 0, the
> following BUG_ON will be triggered:
>
> =========================================================
> kernel BUG at fs/ext4/mballoc.c:5116!
> invalid opcode: 0000 [#1] PREEMPT SMP PTI
> CPU: 3 PID: 673 Comm: xfs_io Tainted: G E 6.5.0-rc1+ #279
> RIP: 0010:ext4_mb_new_inode_pa+0xc5/0x430
> Call Trace:
> <TASK>
> ext4_mb_use_best_found+0x203/0x2f0
> ext4_mb_try_best_found+0x163/0x240
> ext4_mb_regular_allocator+0x158/0x1550
> ext4_mb_new_blocks+0x86a/0xe10
> ext4_ext_map_blocks+0xb0c/0x13a0
> ext4_map_blocks+0x2cd/0x8f0
> ext4_iomap_begin+0x27b/0x400
> iomap_iter+0x222/0x3d0
> __iomap_dio_rw+0x243/0xcb0
> iomap_dio_rw+0x16/0x80
> =========================================================
>
> A simple reproducer demonstrating the problem:
>
> mkfs.ext4 -F /dev/sda -b 4096 100M
> mount /dev/sda /tmp/test
> fallocate -l1M /tmp/test/tmp
> fallocate -l10M /tmp/test/file
> fallocate -i -o 1M -l16777203M /tmp/test/file
> fsstress -d /tmp/test -l 0 -n 100000 -p 8 &
> sleep 10 && killall -9 fsstress
> rm -f /tmp/test/tmp
> xfs_io -c "open -ad /tmp/test/file" -c "pwrite -S 0xff 0 8192"
>
> We simply refactor the logic for adjusting the best extent by adding
> a temporary ext4_free_extent ex and use extent_logical_end() to avoid
> overflow, which also simplifies the code.
>
> Cc: [email protected] # 6.4
> Fixes: 93cdf49f6eca ("ext4: Fix best extent lstart adjustment logic in ext4_mb_new_inode_pa()")
> Signed-off-by: Baokun Li <[email protected]>
> ---
> fs/ext4/mballoc.c | 31 ++++++++++++++-----------------
> 1 file changed, 14 insertions(+), 17 deletions(-)

Looks good to me. Feel free to add:

Reviewed-by: Ritesh Harjani (IBM) <[email protected]>