Re: [PATCH v18 07/15] xfs: add XFS_BMAPI_UNWRITTEN to unmap unwritten extents in __xfs_bunmapi()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] xfs: __xfs_bunmapi() with XFS_BMAPI_UNWRITTEN unmaps written rt extents
--

commit 823f5c04cb0ac218e733c0788aa8567888bdc3de
Author: Andrey Albershteyn <aalbersh@kernel.org>

xfs: add XFS_BMAPI_UNWRITTEN to unmap unwritten extents in __xfs_bunmapi()

This commit adds the XFS_BMAPI_UNWRITTEN flag to __xfs_bunmapi() so callers
can unmap only unwritten extents while leaving written extents in place.

> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index a35a44cf8c635..706f2ead42f58 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5279,6 +5279,10 @@ __xfs_bunmapi(
>  		if (del.br_startoff + del.br_blockcount > end + 1)
>  			del.br_blockcount = end + 1 - del.br_startoff;
>  
> +		if ((flags & XFS_BMAPI_UNWRITTEN) &&
> +		    del.br_state != XFS_EXT_UNWRITTEN)
> +			goto skip;
> +
>  		if (!isrt || (flags & XFS_BMAPI_REMAP))
>  			goto delete;

[Severity: High]
As reported by Darrick J. Wong on v16 [1], when __xfs_bunmapi() is called
with XFS_BMAPI_UNWRITTEN on a realtime inode with sb_rextsize > 1, can an
unwritten tail of a partially written realtime extent cause the written left
neighbor to be converted to unwritten and freed?

Specifically, if del is an unwritten extent (del.br_state ==
XFS_EXT_UNWRITTEN) at the end of a realtime allocation unit whose head
extent prev is written (prev.br_state == XFS_EXT_NORM), it passes this check
and enters the realtime alignment logic in __xfs_bunmapi():

		mod = xfs_rtb_to_rtxoff(mp, del.br_startblock);
		if (mod) {
			...
			} else if (del.br_startoff == start &&
				   (del.br_state == XFS_EXT_UNWRITTEN ||
				    tp->t_blk_res == 0)) {
				...
				continue;
			} else if (del.br_state == XFS_EXT_UNWRITTEN) {
				...
				if (!xfs_iext_prev_extent(ifp, &icur, &prev))
					ASSERT(0);
				ASSERT(prev.br_state == XFS_EXT_NORM);
				...
				prev.br_state = XFS_EXT_UNWRITTEN;
				error = xfs_bmap_add_extent_unwritten_real(tp,
						ip, whichfork, &icur, &cur,
						&prev, &logflags);
				if (error)
					goto error0;
				goto nodelete;
			}

When del.br_startoff > start and del.br_blockcount <= off, this branch
assumes the caller is also unmapping the preceding range. It converts the
written left neighbor prev to XFS_EXT_UNWRITTEN (merging it with del) and
jumps to nodelete. On the next loop iteration, the newly merged extent is
both unwritten and aligned to the realtime extent boundary, so
__xfs_bunmapi() jumps to delete and frees the written blocks as well.

Although xfs_fsverity_begin_enable() in the subsequent fs-verity patch
checks xfs_inode_has_bigrtalloc(ip), enable_verity() drops inode_lock()
after begin_enable_verity() while building the Merkle tree. On an empty or
sparse file where xfs_inode_has_filedata(ip) is false, a concurrent
FS_IOC_FSSETXATTR ioctl via xfs_ioctl_setattr_xflags() (which does not check
XFS_VERITY_CONSTRUCTION) can set XFS_DIFLAG_REALTIME before
xfs_fsverity_end_enable() writes the verity descriptor and calls
xfs_free_eofblocks(). Additionally, xfs_dinode_verify() does not reject
on-disk inodes that have both XFS_DIFLAG2_VERITY and XFS_DIFLAG_REALTIME
when sb_rextsize > 1.

Should __xfs_bunmapi() skip partial unwritten realtime extents when
XFS_BMAPI_UNWRITTEN is set instead of converting the written left neighbor?

[1] https://lore.kernel.org/r/20260922044911.GA6239@frogsfrogsfrogs

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