The changes in the patch "[PATCH V13 05/16] xfs: Check for extent overflow
when removing dir entries" are not correct. I noticed this only after sending
the patchset. The correct change should be,
error = xfs_iext_count_may_overflow(ip, whichfork, 1);
if (error) {
ASSERT(S_ISDIR(VFS_I(ip)->i_mode) &&
whichfork == XFS_DATA_FORK);
error = -ENOSPC;
goto done;
}
Hence please ignore this patchset. I will send the fixed version soon. Sorry
about the noise.
On 10 Jan 2021 at 08:59, Chandan Babu R 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-v13.
>
> 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.
>
> Changelog:
> V12 -> V13:
> 1. xfs_rename():
> - Add comment explaining why we do not check for extent count
> overflow for the source directory entry of a rename operation.
> - Fix grammatical nit in a comment.
> 2. xfs_bmap_del_extent_real():
> Replace explicit checks for inode's mode and fork with an
> assert() call since extent count overflow check here is
> applicable only to directory entry remove/rename operation.
>
> V11 -> V12:
> 1. Rebase patches on top of Linux v5.11-rc1.
> 2. Revert back to using using a pseudo max inode extent count of 10.
> Hence the patches
> - [PATCH V12 05/14] xfs: Check for extent overflow when adding/removing xattrs
> - [PATCH V12 10/14] xfs: Introduce error injection to reduce maximum
> have been reverted back (including retaining of corresponding RVB
> tags) to how it was under V10 of the patchset.
>
> V11 of the patchset had increased the max pseudo extent count to
> 35 to allow for "directory entry remove" operation to always
> succeed. However the corresponding logic was incorrect. Please
> refer to "[PATCH V12 04/14] xfs: Check for extent overflow when
> adding/removing dir entries" to find logic and explaination of
> the newer logic.
>
> "[PATCH V12 04/14] xfs: Check for extent overflow when
> adding/removing dir entries" is the only patch yet to be reviewed.
>
> V10 -> V11:
> 1. For directory/xattr insert operations we now reserve sufficient
> number of "extent count" so as to guarantee a future
> directory/xattr remove operation.
> 2. The pseudo max extent count value has been increased to 35.
>
> V9 -> V10:
> 1. Pull back changes which cause xfs_bmap_compute_alignments() to
> return "stripe alignment" into 12th patch i.e. "xfs: Compute bmap
> extent alignments in a separate function".
>
> V8 -> V9:
> 1. Enabling XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT error tag will
> always allocate single block sized free extents (if
> available).
> 2. xfs_bmap_compute_alignments() now returns stripe alignment as its
> return value.
> 3. Dropped Allison's RVB tag for "xfs: Compute bmap extent
> alignments in a separate function" and "xfs: Introduce error
> injection to allocate only minlen size extents for files".
>
> V7 -> V8:
> 1. Rename local variable in xfs_alloc_fix_freelist() from "i" to "stat".
>
> V6 -> V7:
> 1. Create new function xfs_bmap_exact_minlen_extent_alloc() (enabled
> only when CONFIG_XFS_DEBUG is set to y) which issues allocation
> requests for minlen sized extents only. In order to achieve this,
> common code from xfs_bmap_btalloc() have been refactored into new
> functions.
> 2. All major functions implementing logic associated with
> XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT error tag are compiled only
> when CONFIG_XFS_DEBUG is set to y.
> 3. Remove XFS_IEXT_REFLINK_REMAP_CNT macro and replace it with an
> integer which holds the number of new extents to be
> added to the data fork.
>
> V5 -> V6:
> 1. Rebased the patchset on xfs-linux/for-next branch.
> 2. Drop "xfs: Set tp->t_firstblock only once during a transaction's
> lifetime" patch from the patchset.
> 3. Add a comment to xfs_bmap_btalloc() describing why it was chosen
> to start "free space extent search" from AG 0 when
> XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT is enabled and when the
> transaction is allocating its first extent.
> 4. Fix review comments associated with coding style.
>
> V4 -> V5:
> 1. Introduce new error tag XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT to
> let user space programs to be able to guarantee that free space
> requests for files are satisfied by allocating minlen sized
> extents.
> 2. Change xfs_bmap_btalloc() and xfs_alloc_vextent() to allocate
> minlen sized extents when XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT is
> enabled.
> 3. Introduce a new patch that causes tp->t_firstblock to be assigned
> to a value only when its previous value is NULLFSBLOCK.
> 4. Replace the previously introduced MAXERRTAGEXTNUM (maximum inode
> fork extent count) with the hardcoded value of 10.
> 5. xfs_bui_item_recover(): Use XFS_IEXT_ADD_NOSPLIT_CNT when mapping
> an extent.
> 6. xfs_swap_extent_rmap(): Use xfs_bmap_is_real_extent() instead of
> xfs_bmap_is_update_needed() to assess if the extent really needs
> to be swapped.
>
> V3 -> V4:
> 1. Introduce new patch which lets userspace programs to test "extent
> count overflow detection" by injecting an error tag. The new
> error tag reduces the maximum allowed extent count to 10.
> 2. Injecting the newly defined error tag prevents
> xfs_bmap_add_extent_hole_real() from merging a new extent with
> its neighbours to allow writing deterministic tests for testing
> extent count overflow for Directories, Xattr and growing realtime
> devices. This is required because the new extent being allocated
> can be contiguous with its neighbours (w.r.t both file and disk
> offsets).
> 3. Injecting the newly defined error tag forces block sized extents
> to be allocated for summary/bitmap files when growing a realtime
> device. This is required because xfs_growfs_rt_alloc() allocates
> as large an extent as possible for summary/bitmap files and hence
> it would be impossible to write deterministic tests.
> 4. Rename XFS_IEXT_REMOVE_CNT to XFS_IEXT_PUNCH_HOLE_CNT to reflect
> the actual meaning of the fs operation.
> 5. Fold XFS_IEXT_INSERT_HOLE_CNT code into that associated with
> XFS_IEXT_PUNCH_HOLE_CNT since both perform the same job.
> 6. xfs_swap_extent_rmap(): Check for extent overflow should be made
> on the source file only if the donor file extent has a valid
> on-disk mapping and vice versa.
>
> V2 -> V3:
> 1. Move the definition of xfs_iext_count_may_overflow() from
> libxfs/xfs_trans_resv.c to libxfs/xfs_inode_fork.c. Also, I tried
> to make xfs_iext_count_may_overflow() an inline function by
> placing the definition in libxfs/xfs_inode_fork.h. However this
> required that the definition of 'struct xfs_inode' be available,
> since xfs_iext_count_may_overflow() uses a 'struct xfs_inode *'
> type variable.
> 2. Handle XFS_COW_FORK within xfs_iext_count_may_overflow() by
> returning a success value.
> 3. Rename XFS_IEXT_ADD_CNT to XFS_IEXT_ADD_NOSPLIT_CNT. Thanks to
> Darrick for the suggesting the new name.
> 4. Expand comments to make use of 80 columns.
>
> V1 -> V2:
> 1. Rename helper function from xfs_trans_resv_ext_cnt() to
> xfs_iext_count_may_overflow().
> 2. Define and use macros to represent fs operations and the
> corresponding increase in extent count.
> 3. Split the patches based on the fs operation being performed.
>
> Chandan Babu R (16):
> xfs: Add helper for checking per-inode extent count overflow
> xfs: Check for extent overflow when trivally adding a new extent
> xfs: Check for extent overflow when punching a hole
> xfs: Check for extent overflow when adding dir entries
> xfs: Check for extent overflow when removing dir entries
> xfs: Check for extent overflow when renaming dir entries
> xfs: Check for extent overflow when adding/removing xattrs
> xfs: Check for extent overflow when writing to unwritten extent
> xfs: Check for extent overflow when moving extent from cow to data
> fork
> xfs: Check for extent overflow when remapping an extent
> xfs: Check for extent overflow when swapping extents
> xfs: Introduce error injection to reduce maximum inode fork extent
> count
> xfs: Remove duplicate assert statement in xfs_bmap_btalloc()
> xfs: Compute bmap extent alignments in a separate function
> xfs: Process allocated extent in a separate function
> xfs: Introduce error injection to allocate only minlen size extents
> for files
>
> fs/xfs/libxfs/xfs_alloc.c | 50 ++++++
> fs/xfs/libxfs/xfs_alloc.h | 3 +
> fs/xfs/libxfs/xfs_attr.c | 13 ++
> fs/xfs/libxfs/xfs_bmap.c | 284 ++++++++++++++++++++++++---------
> fs/xfs/libxfs/xfs_errortag.h | 6 +-
> fs/xfs/libxfs/xfs_inode_fork.c | 27 ++++
> fs/xfs/libxfs/xfs_inode_fork.h | 63 ++++++++
> fs/xfs/xfs_bmap_item.c | 10 ++
> fs/xfs/xfs_bmap_util.c | 31 ++++
> fs/xfs/xfs_dquot.c | 8 +-
> fs/xfs/xfs_error.c | 6 +
> fs/xfs/xfs_inode.c | 54 ++++++-
> fs/xfs/xfs_iomap.c | 10 ++
> fs/xfs/xfs_reflink.c | 16 ++
> fs/xfs/xfs_rtalloc.c | 5 +
> fs/xfs/xfs_symlink.c | 5 +
> 16 files changed, 512 insertions(+), 79 deletions(-)
--
chandan
