Re: [PATCH V14 00/16] Bail out if transaction can cause extent count to overflow

[Chandan Babu R <chandan.babu@oracle.com>]

On Mon, May 23, 2022 at 02:15:44 PM +0300, Amir Goldstein wrote:
> On Sun, Jan 10, 2021 at 6:10 PM Chandan Babu R <chandanrlinux@gmail.com> wrote:
>>
>> XFS does not check for possible overflow of per-inode extent counter
>> fields when adding extents to either data or attr fork.
>>
>> For e.g.
>> 1. Insert 5 million xattrs (each having a value size of 255 bytes) and
>>    then delete 50% of them in an alternating manner.
>>
>> 2. On a 4k block sized XFS filesystem instance, the above causes 98511
>>    extents to be created in the attr fork of the inode.
>>
>>    xfsaild/loop0  2008 [003]  1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131
>>
>> 3. The incore inode fork extent counter is a signed 32-bit
>>    quantity. However, the on-disk extent counter is an unsigned 16-bit
>>    quantity and hence cannot hold 98511 extents.
>>
>> 4. The following incorrect value is stored in the xattr extent counter,
>>    # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0
>>    core.naextents = -32561
>>
>> This patchset adds a new helper function
>> (i.e. xfs_iext_count_may_overflow()) to check for overflow of the
>> per-inode data and xattr extent counters and invokes it before
>> starting an fs operation (e.g. creating a new directory entry). With
>> this patchset applied, XFS detects counter overflows and returns with
>> an error rather than causing a silent corruption.
>>
>> The patchset has been tested by executing xfstests with the following
>> mkfs.xfs options,
>> 1. -m crc=0 -b size=1k
>> 2. -m crc=0 -b size=4k
>> 3. -m crc=0 -b size=512
>> 4. -m rmapbt=1,reflink=1 -b size=1k
>> 5. -m rmapbt=1,reflink=1 -b size=4k
>>
>> The patches can also be obtained from
>> https://github.com/chandanr/linux.git at branch xfs-reserve-extent-count-v14.
>>
>> I have two patches that define the newly introduced error injection
>> tags in xfsprogs
>> (https://lore.kernel.org/linux-xfs/20201104114900.172147-1-chandanrlinux@gmail.com/).
>>
>> I have also written tests
>> (https://github.com/chandanr/xfstests/commits/extent-overflow-tests)
>> for verifying the checks introduced in the kernel.
>>
>
> Hi Chandan and XFS folks,
>
> As you may have heard, I am working on producing a series of
> xfs patches for stable v5.10.y.
>
> My patch selection is documented at [1].
> I am in the process of testing the backport patches against the 5.10.y
> baseline using Luis' kdevops [2] fstests runner.
>
> The configurations that we are testing are:
> 1. -m rmbat=0,reflink=1 -b size=4k (default)
> 2. -m crc=0 -b size=4k
> 3. -m crc=0 -b size=512
> 4. -m rmapbt=1,reflink=1 -b size=1k
> 5. -m rmapbt=1,reflink=1 -b size=4k
>
> This patch set is the only largish series that I selected, because:
> - It applies cleanly to 5.10.y
> - I evaluated it as low risk and high value
> - Chandan has written good regression tests
>
> I intend to post the rest of the individual selected patches
> for review in small batches after they pass the tests, but w.r.t this
> patch set -
>
> Does anyone object to including it in the stable kernel
> after it passes the tests?
>

Hi Amir,

The following three commits will have to be skipped from the series,

1. 02092a2f034fdeabab524ae39c2de86ba9ffa15a
   xfs: Check for extent overflow when renaming dir entries

2. 0dbc5cb1a91cc8c44b1c75429f5b9351837114fd
   xfs: Check for extent overflow when removing dir entries

3. f5d92749191402c50e32ac83dd9da3b910f5680f
   xfs: Check for extent overflow when adding dir entries

The maximum size of a directory data fork is ~96GiB. This is much smaller than
what can be accommodated by the existing data fork extent counter (i.e. 2^31
extents).

Also the corresponding test (i.e. xfs/533) has been removed from
fstests. Please refer to
https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=9ae10c882550c48868e7c0baff889bb1a7c7c8e9

-- 
chandan