Re: [PATCH v18 15/15] xfs: enable ro-compat fs-verity flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] xfs: skipped cleanup and early S_VERITY in xfs_fsverity_end_enable()
- [High] xfs: missing blocksize > PAGE_SIZE check in xfs_fsverity_begin_enable()
- [Medium] xfs: unhandled verity errors and uncleared sickness in xchk_inode()
- [Medium] xfs: unnecessary eofblocks transactions on verity inodes when CONFIG_FS_VERITY=n
--

commit 2c04fa631aa020ee4bcc0f60d29a09521669013e
Author: Andrey Albershteyn <aalbersh@kernel.org>

xfs: enable ro-compat fs-verity flag

This commit finalizes fs-verity integration in XFS by adding
XFS_SB_FEAT_RO_COMPAT_VERITY to XFS_SB_FEAT_RO_COMPAT_ALL so the kernel
allows read-write mounts of filesystems with the fs-verity ro-compat feature
enabled.

> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index e1f50ad18f0b5..6594fbe4f7f6a 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -378,8 +378,9 @@ xfs_sb_has_compat_feature(
>  #define XFS_SB_FEAT_RO_COMPAT_ALL \
>  		(XFS_SB_FEAT_RO_COMPAT_FINOBT | \
>  		 XFS_SB_FEAT_RO_COMPAT_RMAPBT | \
> -		 XFS_SB_FEAT_RO_COMPAT_REFLINK| \
> -		 XFS_SB_FEAT_RO_COMPAT_INOBTCNT)
> +		 XFS_SB_FEAT_RO_COMPAT_REFLINK | \
> +		 XFS_SB_FEAT_RO_COMPAT_INOBTCNT | \
> +		 XFS_SB_FEAT_RO_COMPAT_VERITY)

[Severity: High]
Now that XFS_SB_FEAT_RO_COMPAT_VERITY is enabled for read-write mounts,
making FS_IOC_ENABLE_VERITY reachable through xfs_has_verity(mp), does
xfs_fsverity_end_enable() skip cleaning up failed Merkle tree metadata when
desc is NULL, and set S_VERITY before rollback is no longer possible?

In fs/verity/enable.c:enable_verity(), if build_merkle_tree(),
fsverity_create_info(), or fsverity_set_info() fails after
vops->begin_enable_verity() succeeded, enable_verity() jumps to rollback and
calls vops->end_enable_verity(filp, NULL, 0, params.tree_size).

In fs/xfs/xfs_fsverity.c:xfs_fsverity_end_enable():

	int			error = 0;
	...
	/* fs-verity failed, just cleanup */
	if (desc == NULL)
		goto out;
	...
	inode_set_flags(inode, S_VERITY, S_VERITY);

	/*
	 * Proactively remove unwritten extents left by COW preallocations and
	 * write preallocation in the merkle tree holes and past descriptor,
	 * and any delayed preallocations
	 */
	error = xfs_free_eofblocks(ip);
out:
	if (error) {
		int	error2;

		error2 = xfs_fsverity_reset_inode(ip);
		...
	}

	xfs_iflags_clear(ip, XFS_VERITY_CONSTRUCTION);
	return error;

When desc == NULL, error remains 0 at out, so xfs_fsverity_reset_inode(ip)
and xfs_fsverity_delete_metadata(ip) are skipped while
XFS_VERITY_CONSTRUCTION is cleared. Any post-EOF dirty folios and delalloc
extents written by xfs_fsverity_write_merkle() remain attached to the inode;
when writeback runs without XFS_VERITY_CONSTRUCTION set,
xfs_iomap_writepages() does not set IOMAP_F_FSVERITY and
iomap_writeback_handle_eof() skips the post-EOF folios after clearing their
dirty bit, stranding unwritten delalloc blocks.

In addition, on the normal enable path, inode_set_flags(inode, S_VERITY,
S_VERITY) is called before xfs_free_eofblocks(ip). Because enable_verity()
has already inserted vi into fsverity_info_hash, setting S_VERITY immediately
exposes vi to concurrent readers via fsverity_get_info(inode). If
xfs_free_eofblocks(ip) fails and triggers xfs_fsverity_reset_inode(ip),
either S_VERITY remains set if xfs_fsverity_delete_metadata(ip) also fails
(triggering WARN_ON_ONCE(IS_VERITY(inode)) in enable_verity()), or S_VERITY
is cleared and enable_verity() calls fsverity_remove_info(vi) while
concurrent readers or read ioends may already be using vi.

Should desc == NULL call xfs_fsverity_reset_inode(ip), and should
inode_set_flags(inode, S_VERITY, S_VERITY) only be called after
xfs_free_eofblocks(ip) succeeds?

[Severity: High]
Can enabling and reading an fs-verity file on an XFS filesystem with
sb_blocksize > PAGE_SIZE trigger VM_BUG_ON_PGFLAGS(PageCompound(page))?

When mp->m_sb.sb_blocksize > PAGE_SIZE, xfs_ialloc_setup_geometry() sets
igeo->min_folio_order = mp->m_sb.sb_blocklog - PAGE_SHIFT, and
xfs_inode_alloc() / xfs_reinit_inode() configures the mapping with
mapping_set_folio_min_order(inode->i_mapping, M_IGEO(mp)->min_folio_order).

In fs/xfs/xfs_fsverity.c:xfs_fsverity_begin_enable():

	if (xfs_inode_has_bigrtalloc(ip))
		return -EINVAL;

	if (inode->i_size > XFS_FSVERITY_LARGEST_FILE)
		return -EFBIG;

There is no check rejecting sb_blocksize > PAGE_SIZE (or
mapping_min_folio_order(inode->i_mapping) > 0), and
fsverity_init_merkle_tree_params() allows a 4K Merkle tree block size as long
as log_blocksize <= PAGE_SHIFT and log_blocksize <= inode->i_blkbits.

When reading the file, verify_data_block() calls:

xfs_fsverity_read_merkle()
  -> generic_read_merkle_tree_page()
     -> read_mapping_folio()
     -> folio_file_page()

Because mapping_min_folio_order(inode->i_mapping) > 0, read_mapping_folio()
allocates a compound folio and folio_file_page() returns a page with
PageCompound(hpage) == true. verify_data_block() then calls
SetPageChecked(hpage), which is defined with PF_NO_COMPOUND and triggers
VM_BUG_ON_PGFLAGS(1 && PageCompound(page), page).

Should xfs_fsverity_begin_enable() (or xfs_has_verity() / mount-time checks)
reject filesystems with blocksize > PAGE_SIZE until fs-verity supports large
folios for Merkle tree caching?

[Severity: Medium]
In online scrub and repair, can corrupt fsverity descriptors cause
xchk_inode_setup_verity() to abort the scrub ioctl with -EINVAL or -EFBIG,
and does repairing or scrubbing a verity inode leave XFS_SICK_INO_FSVERITY
permanently set?

In fs/xfs/scrub/common.c:xchk_inode_setup_verity():

	error = fsverity_ensure_verity_info(VFS_I(sc->ip));
	switch (error) {
	case 0:
		/* fsverity is active */
		break;
	case -ENODATA:
	case -EMSGSIZE:
	case -EFSCORRUPTED:
	case -ERANGE:
	case -EBADMSG:
		/*
		 * The nonzero errno codes above are the error codes that can
		 * be returned from fsverity on metadata validation errors.
		 */
		return 0;
	default:
		/* runtime errors */
		return error;
	}

In fs/verity/open.c, fsverity_ensure_verity_info() calls
fsverity_get_descriptor() -> validate_fsverity_descriptor(), which returns
-EINVAL when a descriptor field is invalid (bad version, non-zero
__reserved, invalid salt_size, data_size != inode->i_size, or sig_size
overflow), and fsverity_create_info() -> fsverity_init_merkle_tree_params(),
which returns -EINVAL for an invalid hash_algorithm or log_blocksize and
-EFBIG when the Merkle tree parameters exceed limits. Because -EINVAL and
-EFBIG hit the default branch in xchk_inode_setup_verity(),
xchk_prepare_iscrub() and xrep_dinode_core() abort the scrub or repair ioctl
instead of allowing xchk_inode() to flag corruption and xrep_inode_verity()
to clear XFS_DIFLAG2_VERITY.

Furthermore, xfs_fsverity_get_descriptor() marks corrupt inodes with
XFS_SICK_INO_FSVERITY (which is included in XFS_SICK_INO_PRIMARY), but
type_to_health_flag[XFS_SCRUB_TYPE_INODE] in fs/xfs/scrub/health.c only sets
XFS_SICK_INO_CORE, and neither xchk_inode() nor xrep_inode_verity() adds
XFS_SICK_INO_FSVERITY to sc->healthy_mask:

fs/xfs/scrub/inode_repair.c:xrep_inode_verity() {
	if (xchk_inode_verity_broken(sc->ip)) {
		sc->ip->i_diflags2 &= ~XFS_DIFLAG2_VERITY;
		inode_set_flags(inode, 0, S_VERITY);

		xfs_trans_log_inode(sc->tp, sc->ip, XFS_ILOG_CORE);
	}
}

As a result, even after xrep_inode_verity() repairs the inode,
XFS_SICK_INO_FSVERITY remains set on ip->i_sick and causes
xfs_inactive_health() to taint the AG with XFS_SICK_AG_INODES when the inode
is evicted.

[Severity: Medium]
When a kernel built with CONFIG_FS_VERITY=n mounts an XFS filesystem with
XFS_SB_FEAT_RO_COMPAT_VERITY read-write (which adding
XFS_SB_FEAT_RO_COMPAT_VERITY to XFS_SB_FEAT_RO_COMPAT_ALL now permits), will
evicting a verity inode from the icache repeatedly run unnecessary
tr_itruncate transactions?

In fs/xfs/xfs_bmap_util.c:xfs_can_free_eofblocks():

	if (fsverity_active(VFS_IC(ip)) ||
	    xfs_iflags_test(ip, XFS_VERITY_CONSTRUCTION))
		return false;

And in fs/xfs/xfs_iops.c:xfs_report_dioalign():

	if (fsverity_active(VFS_I(ip)))
		return;

When CONFIG_FS_VERITY=n, fsverity_active() is a stub that always returns
false, even though xfs_diflags_to_iflags() still sets S_VERITY on the VFS
inode when XFS_DIFLAG2_VERITY is present.

When a verity inode is looked up (for example via stat(), where
xfs_iformat_data_fork() populates ip->i_df with if_needextents == 0 for
XFS_DINODE_FMT_EXTENTS inodes) and later evicted from the VFS icache:

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

Because fsverity_active() returns false, xfs_can_free_eofblocks() looks up
extents past EOF, finds the post-EOF Merkle tree and descriptor extents, and
returns true. Inactivation then runs xfs_inactive() -> xfs_free_eofblocks()
-> xfs_itruncate_extents_flags(..., XFS_BMAPI_UNWRITTEN), which allocates a
tr_itruncate transaction, walks all post-EOF extents while skipping the
written verity metadata extents, and unconditionally dirties and commits the
inode core via xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE) on every eviction.
Similarly, xfs_report_dioalign() reports non-zero dio_mem_align and
dio_offset_align for verity files when CONFIG_FS_VERITY=n.

Should these checks test IS_VERITY(VFS_IC(ip)) (or ip->i_diflags2 &
XFS_DIFLAG2_VERITY) instead of fsverity_active()?

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