Re: [PATCH v18 08/15] xfs: don't remove written extents past EOF on fsverity inodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] xfs: written verity extents unmapped in xfs_free_eofblocks() on bigrtalloc
- [Medium] xfs: fsverity_active() check in xfs_can_free_eofblocks() fails if !VERITY
- [Medium] xfs: premature XFS_IEOFBLOCKS clear in xfs_can_free_eofblocks()
--

commit 2307cc3d64e773ab52b2cf76f7c9b88f364d2957
Author: Andrey Albershteyn <aalbersh@kernel.org>

xfs: don't remove written extents past EOF on fsverity inodes

This commit updates xfs_can_free_eofblocks() to skip active fsverity inodes
and inodes undergoing Merkle tree construction, and modifies
xfs_free_eofblocks() to punch only delalloc and unwritten extents past EOF on
fsverity inodes while preserving written metadata extents.

> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 865aec3ae5ab3..32cec523fd52d 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -31,6 +31,7 @@
>  #include "xfs_rtbitmap.h"
>  #include "xfs_rtgroup.h"
>  #include "xfs_zone_alloc.h"
> +#include <linux/fsverity.h>
>  
>  /* Kernel only BMAP related definitions and functions */
>  
> @@ -553,6 +554,15 @@ xfs_can_free_eofblocks(
>  	if (last_fsb <= end_fsb)
>  		return false;
>  
> +	/*
> +	 * Don't clean fsverity inodes as they already have been cleaned up and
> +	 * are read-only. Skip inodes which have merkle tree being built, the
> +	 * merkle tree is written beyond EOF
> +	 */
> +	if (fsverity_active(VFS_IC(ip)) ||
> +	    xfs_iflags_test(ip, XFS_VERITY_CONSTRUCTION))
> +		return false;

[Severity: Medium]
As reported by Sashiko on v16 [1], in xfs_can_free_eofblocks() in
fs/xfs/xfs_bmap_util.c, does checking fsverity_active(VFS_IC(ip)) instead of
ip->i_diflags2 & XFS_DIFLAG2_VERITY cause sealed fsverity inodes to go
through unnecessary inactivation and tr_itruncate transactions on every VFS
cache eviction when CONFIG_FS_VERITY=n?

The follow-up commit ("xfs: enable ro-compat fs-verity flag") unconditionally
includes XFS_SB_FEAT_RO_COMPAT_VERITY in XFS_SB_FEAT_RO_COMPAT_ALL, allowing
read-write mounts of fsverity filesystems even on kernels built with
CONFIG_FS_VERITY=n. When CONFIG_FS_VERITY=n, fsverity_active() in
include/linux/fsverity.h is a stub returning false:

    static inline bool fsverity_active(const struct inode *inode)
    {
        return false;
    }

Whereas xfs_free_eofblocks() checks:

    bool has_verity = ip->i_diflags2 & XFS_DIFLAG2_VERITY;

When a sealed extent-format fsverity inode (with written Merkle tree and
descriptor extents past EOF) is looked up and later evicted on a
CONFIG_FS_VERITY=n kernel:

    xfs_inode_mark_reclaimable()
      -> xfs_inode_needs_inactive()
        -> xfs_can_free_eofblocks()

xfs_can_free_eofblocks() sees fsverity_active() == false, finds the post-EOF
Merkle tree extents via xfs_iext_lookup_extent(), and returns true. This
queues the inode for background inactivation, where xfs_inactive() ->
xfs_free_eofblocks() sees has_verity == true, allocates a tr_itruncate
transaction, walks all post-EOF extents with XFS_BMAPI_UNWRITTEN without
freeing the written Merkle tree extents, logs XFS_ILOG_CORE in
xfs_itruncate_extents_flags(), and commits a dirty transaction on every
eviction.

Should xfs_can_free_eofblocks() check ip->i_diflags2 & XFS_DIFLAG2_VERITY (or
IS_VERITY(VFS_IC(ip))) to match xfs_free_eofblocks()?

[Severity: Medium]
Also in xfs_can_free_eofblocks() in fs/xfs/xfs_bmap_util.c, can returning
false when XFS_VERITY_CONSTRUCTION is set cause xfs_inode_free_eofblocks() to
prematurely clear the XFS_IEOFBLOCKS tag and leak post-EOF speculative
preallocations if Merkle tree construction later fails?

During FS_IOC_ENABLE_VERITY, enable_verity() in fs/verity/enable.c acquires
inode_lock(inode) (XFS_IOLOCK_EXCL), calls ->begin_enable_verity() which sets
XFS_VERITY_CONSTRUCTION, and then drops inode_lock(inode) while calling
build_merkle_tree(). While build_merkle_tree() writes past EOF without
holding XFS_IOLOCK_EXCL:

    xfs_fsverity_write_merkle()
      -> iomap_fsverity_write()
        -> xfs_buffered_write_iomap_begin()
          -> xfs_bmapi_reserve_delalloc()
            -> xfs_inode_set_eofblocks_tag()

If background xfs_blockgc_worker() or a synchronous -ENOSPC/-EDQUOT scan runs
xfs_inode_free_eofblocks() in fs/xfs/xfs_icache.c while
XFS_VERITY_CONSTRUCTION is set, xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)
succeeds and xfs_can_free_eofblocks(ip) returns false:

    if (xfs_can_free_eofblocks(ip))
        return xfs_free_eofblocks(ip);

    /* inode could be preallocated */
    trace_xfs_inode_free_eofblocks_invalid(ip);
    xfs_inode_clear_eofblocks_tag(ip);
    return 0;

This removes the inode from the per-AG XFS_ICI_BLOCKGC_TAG radix tree while
post-EOF delalloc and speculative preallocation blocks are still attached. If
build_merkle_tree() subsequently fails and enable_verity() invokes the
rollback path ->end_enable_verity(filp, NULL, 0), xfs_fsverity_end_enable()
jumps to out with error == 0 and clears XFS_VERITY_CONSTRUCTION without
calling xfs_free_eofblocks() or xfs_fsverity_reset_inode(). Because the file
descriptor for FS_IOC_ENABLE_VERITY is opened O_RDONLY, xfs_release() does
not free post-EOF blocks on close either, leaving the post-EOF blocks
untracked by blockgc until inode eviction.

>  
>  	/*
>  	 * Check if there is an post-EOF extent to free.  If there are any
> @@ -579,6 +589,9 @@ xfs_free_eofblocks(
>  	struct xfs_trans	*tp;
>  	struct xfs_mount	*mp = ip->i_mount;
>  	int			error;
> +	int			bmapi_flags = XFS_BMAPI_NODISCARD;
> +	bool			has_verity =
> +			ip->i_diflags2 & XFS_DIFLAG2_VERITY;
>  
>  	/* Attach the dquots to the inode up front. */
>  	error = xfs_qm_dqattach(ip);
> @@ -593,15 +606,20 @@ xfs_free_eofblocks(
>  	 *
>  	 * Note that this means we also leave speculative preallocations in
>  	 * place for preallocated files.
> +	 *
> +	 * Clean up delalloc reservations for fsverity too as those won't be
> +	 * used
>  	 */
> -	if (ip->i_diflags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)) {
> +	if (ip->i_diflags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND) ||
> +			has_verity) {
>  		if (ip->i_delayed_blks) {
>  			xfs_bmap_punch_delalloc_range(ip, XFS_DATA_FORK,
>  				round_up(XFS_ISIZE(ip), mp->m_sb.sb_blocksize),
>  				LLONG_MAX, NULL);
>  		}
>  		xfs_inode_clear_eofblocks_tag(ip);
> -		return 0;
> +		if (!has_verity)
> +			return 0;
>  	}
>  
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> @@ -613,6 +631,13 @@ xfs_free_eofblocks(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> +	/*
> +	 * While fs-verity writes metadata after EOF, it can leave unwritten
> +	 * preallocations.  Clear all that out.
> +	 */
> +	if (has_verity)
> +		bmapi_flags |= XFS_BMAPI_UNWRITTEN;
> +
>  	/*
>  	 * Do not update the on-disk file size.  If we update the on-disk file
>  	 * size and then the system crashes before the contents of the file are
> @@ -620,7 +645,7 @@ xfs_free_eofblocks(
>  	 * bug).
>  	 */
>  	error = xfs_itruncate_extents_flags(&tp, ip, XFS_DATA_FORK,
> -				XFS_ISIZE(ip), XFS_BMAPI_NODISCARD);
> +				XFS_ISIZE(ip), bmapi_flags);

[Severity: High]
In xfs_free_eofblocks() in fs/xfs/xfs_bmap_util.c, if an inode has both
XFS_DIFLAG2_VERITY and bigrtalloc (xfs_inode_has_bigrtalloc(ip)), can calling
xfs_itruncate_extents_flags() with XFS_BMAPI_UNWRITTEN and a 0-block
transaction reservation convert written post-EOF fsverity metadata blocks to
unwritten and unmap them (or trigger an -ENOSPC filesystem shutdown)?

Although xfs_fsverity_begin_enable() in the follow-up patch ("xfs: add
fs-verity support") checks xfs_inode_has_bigrtalloc(ip), enable_verity() in
fs/verity/enable.c drops inode_lock(inode) immediately after
->begin_enable_verity() returns. While inode_lock(inode) is dropped on a file
with no allocated data blocks yet (where xfs_inode_has_filedata(ip) is false,
such as an empty or hole-only file before Merkle tree blocks are allocated), a
concurrent FS_IOC_FSSETXATTR / file_setattr() call reaches:

    xfs_fileattr_set()
      -> xfs_ioctl_setattr_xflags()

which does not check XFS_VERITY_CONSTRUCTION or XFS_DIFLAG2_VERITY and allows
setting XFS_DIFLAG_REALTIME on the inode. In addition, xfs_dinode_verify() in
fs/xfs/libxfs/xfs_inode_buf.c does not reject on-disk inodes with both
XFS_DIFLAG2_VERITY and bigrtalloc.

When xfs_fsverity_end_enable() writes the fsverity descriptor on a bigrtalloc
inode (sb_rextsize > 1), xfs_buffered_write_iomap_begin() delegates via
xfs_get_extsz_hint() to xfs_direct_write_iomap_begin(), preallocating an
sb_rextsize-aligned unwritten realtime extent and converting only the
written descriptor block(s) to XFS_EXT_NORM, leaving an XFS_EXT_UNWRITTEN
tail in the same realtime extent.

When xfs_free_eofblocks() then calls xfs_itruncate_extents_flags() with
XFS_BMAPI_UNWRITTEN and tp->t_blk_res == 0, __xfs_bunmapi() in
fs/xfs/libxfs/xfs_bmap.c hits the unwritten tail of that realtime extent
(where del.br_state == XFS_EXT_UNWRITTEN, aligned at the end of the realtime
extent, but with mod = xfs_rtb_to_rtxoff(mp, del.br_startblock) != 0 and
del.br_startoff > start):

    } else if (del.br_state == XFS_EXT_UNWRITTEN) {
        struct xfs_bmbt_irec prev;
        xfs_fileoff_t unwrite_start;

        /*
         * This one is already unwritten.
         * It must have a written left neighbor.
         * Unwrite the killed part of that one and
         * try again.
         */
        if (!xfs_iext_prev_extent(ifp, &icur, &prev))
            ASSERT(0);
        ASSERT(prev.br_state == XFS_EXT_NORM);

__xfs_bunmapi() converts the written left neighbor prev (containing the
written fsverity metadata) to XFS_EXT_UNWRITTEN via
xfs_bmap_add_extent_unwritten_real() without any block reservation for a bmap
btree split, and then unmaps and frees the entire realtime extent on the next
iteration.

[1] https://lore.kernel.org/r/20260918111539.1003439-15-aalbersh@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102144147.2393369-1-aalbersh@kernel.org?part=8