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

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Sep 18, 2026 at 11:43:07AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Yes, it should.

> 
> [ ... ]
> > +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
>