On Mon, May 23, 2022 at 10:06 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Mon, May 23, 2022 at 7:17 PM Chandan Babu R <chandan.babu@xxxxxxxxxx> wrote: > > > > 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@xxxxxxxxx> 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@xxxxxxxxx/). > > >> > > >> 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). > > > > Thanks for this information! > > I understand that the "fixes" are not needed, but the moto of the stable > tree maintainers is that taking harmless patches is preferred over non > clean backports and without those patches, the rest of the series does > not apply cleanly. > > So the question is: does it hurt to take those patches to the stable tree? All right, I've found the revert partial patch in for-next: 83a21c18441f xfs: Directory's data fork extent counter can never overflow I can backport this patch to stable after it hits mainline (since this is not an urgent fix I would wait for v.5.19.0) with the obvious omission of the XFS_MAX_EXTCNT_*_FORK_LARGE constants. But even then, unless we have a clear revert in mainline, it is better to have the history in stable as it was in mainline. Furthermore, stable, even more than mainline, should always prefer safety over performance optimization, the sending the 3 patches already in mainline to stable without the partial revert is better than sending no patches at all and better then delaying the process. Thanks, Amir.
