Re: [PATCH v3] ext4: Regression test of ext4_lblk_t overflow

[Baokun Li <libaokun1@huawei.com>]

On 2023/11/24 1:03, Darrick J. Wong wrote:
> On Thu, Nov 23, 2023 at 09:46:37PM +0800, Baokun Li wrote:
>> On 2023/11/23 0:32, Darrick J. Wong wrote:
>>> On Wed, Nov 22, 2023 at 07:53:14PM +0800, Baokun Li wrote:
>>>> Append writes to a file approaching 16T and observe if a kernel crash is
>>>> caused by ext4_lblk_t overflow triggering BUG_ON at ext4_mb_new_inode_pa().
>>>> This is a regression test for commit bc056e7163ac ("ext4: fix BUG in
>>>> ext4_mb_new_inode_pa() due to overflow")
>>>>
>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>> ---
>>>> V1->V2:
>>>> 	Changes to make the use case more generic, not just for testing
>>>> 	ext4.(ext4 and xfs have been tested)
>>>> V2->V3:
>>>> 	Clean up the code and remove hardcoding.
>>>>
>>>>    tests/generic/737     | 53 +++++++++++++++++++++++++++++++++++++++++++
>>>>    tests/generic/737.out |  2 ++
>>>>    2 files changed, 55 insertions(+)
>>>>    create mode 100755 tests/generic/737
>>>>    create mode 100644 tests/generic/737.out
>>>>
>>>> diff --git a/tests/generic/737 b/tests/generic/737
>>>> new file mode 100755
>>>> index 00000000..29d428ad
>>>> --- /dev/null
>>>> +++ b/tests/generic/737
>>>> @@ -0,0 +1,53 @@
>>>> +#! /bin/bash
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +# Copyright (c) 2023 HUAWEI.  All Rights Reserved.
>>>> +#
>>>> +# FS QA Test No. 737
>>>> +#
>>>> +# Append writes to a file approaching 16T and observe if a kernel crash is
>>>> +# caused by ext4_lblk_t overflow triggering BUG_ON at ext4_mb_new_inode_pa().
>>>> +# This is a regression test for commit
>>>> +# bc056e7163ac ("ext4: fix BUG in ext4_mb_new_inode_pa() due to overflow")
>>>> +#
>>>> +. ./common/preamble
>>>> +. ./common/populate
>>>> +_begin_fstest auto quick insert prealloc
>>>> +
>>>> +# real QA test starts here
>>>> +[[ "$FSTYP" =~ ext* ]] && _fixed_by_kernel_commit bc056e7163ac \
>>>> +	"ext4: fix BUG in ext4_mb_new_inode_pa() due to overflow"
>>>> +
>>>> +_require_odirect
>>>> +_require_xfs_io_command "falloc"
>>>> +_require_xfs_io_command "finsert"
>>>> +
>>>> +dev_size=$((100 * 1024 * 1024))
>>>> +_scratch_mkfs_sized $dev_size >>$seqres.full 2>&1 || _fail "mkfs failed"
>>>> +
>>>> +_scratch_mount
>>>> +blksz="$(_get_block_size ${SCRATCH_MNT})"
>>> _get_file_block_size, not _get_block_size.  The first one retrieves the
>>> file allocation unit (e.g. ext4 bigalloc cluster size / xfs rt extent
>>> size) whereas the second merely returns the base fs block size.
>>>
>>> That is an important distinction when you're messing with fallocate. :)
>> _get_file_block_size is implemented as follows：
>>
>> _get_file_block_size()
>> {
>>          if [ -z $1 ] || [ ! -d $1 ]; then
>>                  echo "Missing mount point argument for _get_file_block_size"
>>                  exit 1
>> fi
>>
>>          case "$FSTYP" in
>> "ocfs2")
>>                  stat -c '%o' $1
>> ;;
>> "xfs")
>>                  _xfs_get_file_block_size $1
>> ;;
>> *)
>>                  _get_block_size $1
>> ;;
>> esac
>> }
>>
>> The return values of ocfs2 and xfs may be different, but they are the same
>> for ext4. And the logical blocks recorded in ext4 are in blocks, not
>> clusters.
>> I'll replace _get_block_size with _get_file_block_size if
>> _get_file_block_size
>> should be used in xfs.
> Oh silly me, I forgot that the logical block mappings in ext4 remain in
> units of fs blocks, not bigalloc clusters.  So this doesn't make much of
> a difference.
>
>>>> +# Reserve 1M space
>>>> +$XFS_IO_PROG -f -c "falloc 0 1M" "${SCRATCH_MNT}/tmp" >> $seqres.full
>>>> +
>>>> +# Create a file (~16T) with logical block numbers close to overflow
>>>> +$XFS_IO_PROG -f -c "falloc 0 10M" "${SCRATCH_MNT}/file" >> $seqres.full
>>>> +insert_size=$((blksz * 4096 - 10 - 3))
>>> What if blksz == 64k ?  This won't compute a file position slightly
>>> below 16T.  I think the comment is wrong since you're trying to overflow
>>> the u32 ext4_lblk_t, correct?
>> Yes, the comment here is wrong. The actual intention here is to construct a
>> file with logical blocks close to 0x100000000.
>>> I think what you really want is something more like...
>>>
>>> # Shift the last 9M of the file preallocations to a position just short
>>> # of overflowing ext4_lblk_t.
>>> max_pos=$(( 0xffffffff * file_blksz ))
>>> finsert_len=$(( max_pos - ((10 + 3) << 20) ))
>>> $XFS_IO_PROG -f -c "finsert 1M ${finsert_len}" "${SCRATCH_MNT}/file" >> $seqres.full
>> Exactly!
>>> Not sure why you shift 9M of data to 13M below what I think is the
>>> upper range of ext4_lblk_t; I would have thought that would be
>>> (max_pos - 9MB) but I'm assuming you know the reproduction circumstances
>>> better than me...
>>>
>>> --D
>> At 4k block size, when appending writes to a file close to 16T, the block
>> allocation
>> request will be enlarged to 8M, and the current file size + block allocation
>> request
>> size will not exceed 16T.
>>
>> Therefore, the above is just using finsert to construct a file with maximum
>> logical
>> block number close to 0x100000000, the corresponding size at 4k can be in
>> the
>> range of (16T-8M, 16T), the insertion location does not have any special
>> meaning.
>>
>> 3M is not a special value, theoretically it can be in the range of (1M
>> (reserved tmp), 8M].
>> But ext4 reserves 2% of the blocks for metadata, which in this case is 2M,
>> so the
>> interval in which the problem can be triggered becomes (2M, 8M].
> Does the test trigger the bug on other blocksizes like 1k or 64k?
>
> Oh, there's a v4, will go look at that.
>
> --D
In 1k block size, the block allocation request will be enlarged to 2M,
the reserved 2% corresponds to 2M, so we can allocate this 2M extent
instead of the 1M extent of tmp. BUG_ON will only trigger when the
actual allocated request size is smaller than the above 2M. Therefore,
if the ext4 file size is greater than or equal to 100M, the 1k block size
cannot trigger the problem. In this case, we change the mkfs size to
80M, 3M to 2M, after testing, the problem can be triggered in
1k, 4k, 64k block size.

Please ignore the v4 sent yesterday, I will send a new v5 version soon.

Furthermore, while testing the 64k block, I found another bug, and I
will also send a patch to the ext4 mailing list as soon as possible.
>>>> +$XFS_IO_PROG -f -c "finsert 1M ${insert_size}M" "${SCRATCH_MNT}/file" >> $seqres.full
>>>> +
>>>> +# Filling up the free space ensures that the pre-allocated space is the reserved space.
>>>> +nr_free=$(stat -f -c '%f' ${SCRATCH_MNT})
>>>> +_fill_fs $((nr_free * blksz)) ${SCRATCH_MNT}/fill $blksz 0 >> $seqres.full 2>&1
>>>> +sync
>>>> +
>>>> +# Remove reserved space to gain free space for allocation
>>>> +rm -f ${SCRATCH_MNT}/tmp
>>>> +
>>>> +# Trying to allocate two blocks triggers BUG_ON.
>>>> +$XFS_IO_PROG -c "open -ad ${SCRATCH_MNT}/file" -c "pwrite -S 0xff 0 $((2 * blksz))" >> $seqres.full
>>>> +
>>>> +echo "Silence is golden"
>>>> +
>>>> +# success, all done
>>>> +status=0
>>>> +exit
>>>> diff --git a/tests/generic/737.out b/tests/generic/737.out
>>>> new file mode 100644
>>>> index 00000000..67b83d78
>>>> --- /dev/null
>>>> +++ b/tests/generic/737.out
>>>> @@ -0,0 +1,2 @@
>>>> +QA output created by 737
>>>> +Silence is golden
>>>> -- 
>>>> 2.31.1
>>>>
>>>>
>>
Thanks!
-- 
With Best Regards,
Baokun Li
.