Re: [PATCH v16 15/21] xfs: add fs-verity support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] xfs: AB-BA lock inversion in xfs_fsverity_delete_metadata()
- [High] xfs: verity metadata destroyed after commit on xfs_free_eofblocks failure
- [Low] xfs: duplicate xfs_fsverity_delete_metadata call on cleanup failure
--

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

xfs: add fs-verity support

This commit integrates fs-verity with XFS, storing the Merkle tree and 
descriptor in the data fork past EOF. It sets up the enablement process
and handles read and write operations for the verity metadata.

> --- a/fs/xfs/xfs_fsverity.c
> +++ b/fs/xfs/xfs_fsverity.c
[ ... ]
> +static int
> +xfs_fsverity_delete_metadata(
> +	struct xfs_inode	*ip)
> +{
> +	struct xfs_trans	*tp;
> +	struct xfs_mount	*mp = ip->i_mount;
> +	int			error;
> +
> +	xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> +	if (error) {
> +		xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
> +		return error;
> +	}
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, 0);
> +
> +	truncate_inode_pages(VFS_I(ip)->i_mapping, XFS_ISIZE(ip));

[Severity: High]
Does this sequence create a lock inversion between the page/folio locks and
the XFS_ILOCK?

XFS lock ordering rules require folio locks to be acquired before the log space
reservation (xfs_trans_alloc) and the XFS_ILOCK. Because truncate_inode_pages
sequentially locks folios, calling it while holding XFS_ILOCK_EXCL could cause
an AB-BA deadlock if a background thread (like writeback) has locked a folio
and is concurrently trying to acquire the XFS_ILOCK for block mapping.

Could truncate_inode_pages be moved outside the transaction and before
acquiring XFS_ILOCK_EXCL?

[ ... ]
> +static int
> +xfs_fsverity_end_enable(
> +	struct file		*file,
> +	const void		*desc,
> +	size_t			desc_size,
> +	u64			merkle_tree_size)
> +{
> +	struct inode		*inode = file_inode(file);
> +	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_trans	*tp;
> +	int			error = 0;
> +	loff_t			range_start = xfs_fsverity_metadata_offset(ip);
> +
> +	xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL);
> +
> +	/* fs-verity failed, just cleanup */
> +	if (desc == NULL) {
> +		error = xfs_fsverity_delete_metadata(ip);
> +		goto out;
> +	}
[ ... ]
> +	/*
> +	 * Ensure that we've persisted the verity information before we enable
> +	 * it on the inode and tell the caller we have sealed the inode.
> +	 */
> +	ip->i_diflags2 |= XFS_DIFLAG2_VERITY;
> +
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +	xfs_trans_set_sync(tp);
> +
> +	error = xfs_trans_commit(tp);
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	if (error)
> +		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_delete_metadata(ip);

[Severity: Low]
Is it intentional to call xfs_fsverity_delete_metadata a second time if the
initial early cleanup fails?

If desc is NULL at the start of the function, xfs_fsverity_delete_metadata is
called. If that call returns an error, it jumps to the out label. Because
error is preserved, this block executes and calls xfs_fsverity_delete_metadata
again with the exact same failed state.

[Severity: High]
Can a failure in xfs_free_eofblocks cause the metadata of a fully committed
verity inode to be destroyed?

If the verity enablement is successful, the transaction is committed via
xfs_trans_commit and the inode is permanently marked with XFS_DIFLAG2_VERITY.
If the subsequent post-commit optimization call to xfs_free_eofblocks fails
(for example, due to memory constraints or space issues), it returns an error,
which leads directly to this unconditional cleanup path.

This deletes the Merkle tree and descriptor extents. Because the on-disk flag
was already committed, the file remains verity-enabled but loses its metadata.
This would permanently fail verity validation and lock the user out of their
file.

Should this cleanup be skipped if the transaction has already been committed?

> +		if (error2)
> +			xfs_alert(ip->i_mount,
> +"ino 0x%llx failed to clean up new fsverity metadata, err %d",
> +					I_INO(ip), error2);
> +	}
> +
> +	xfs_iflags_clear(ip, XFS_VERITY_CONSTRUCTION);
> +	return error;
> +}

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