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

From: Amir Goldstein <amir73il@gmail.com>
To: Chandan Babu R <chandan.babu@oracle.com>
Cc: Chandan Babu R <chandanrlinux@gmail.com>,
	linux-xfs <linux-xfs@vger.kernel.org>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	"Darrick J. Wong" <djwong@kernel.org>,
	Christoph Hellwig <hch@lst.de>,
	Allison Henderson <allison.henderson@oracle.com>,
	"Luis R. Rodriguez" <mcgrof@kernel.org>,
	Theodore Tso <tytso@mit.edu>
Subject: Re: [PATCH V14 00/16] Bail out if transaction can cause extent count to overflow
Date: Mon, 23 May 2022 22:06:47 +0300	[thread overview]
Message-ID: <CAOQ4uxj2Q2mEvrBmBBV7w=NN5SXsLgk=8ph_iAa0jFCV9QP8NA@mail.gmail.com> (raw)
In-Reply-To: <878rqs2pg9.fsf@debian-BULLSEYE-live-builder-AMD64>

On Mon, May 23, 2022 at 7:17 PM Chandan Babu R <chandan.babu@oracle.com> 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@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).
>

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?

> 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
>

Well the test does not fail so it doesn't hurt either. Right?
In my test env, we will occasionally pull latest fstests and then
the unneeded test will be removed.

Does that sound right?

Thanks,
Amir.