Re: [PATCH v7 06/11] xfs: recheck direct write alignment after write checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] xfs: sector-unaligned O_DIRECT writes after write checks
  bounce to buffered I/O
--

commit 9318aacdeda831439c37400a39cd1973c5ae71aa
Author: Chris Mason <mason@kernel.org>

xfs: recheck direct write alignment after write checks

This commit rechecks block alignment in xfs_file_dio_write_aligned() after
xfs_file_write_checks() adjusts the write range for O_APPEND and file size
limits, falling back to the unaligned or buffered path when the final range
is no longer block aligned.

> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 153f53f964b99..8ee88910b487d 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -710,6 +710,17 @@ xfs_file_dio_write_aligned(
>  	if (ret)
>  		goto out_unlock;
>  
> +	/*
> +	 * O_APPEND moves the position and the file size limits shorten the
> +	 * write, so only now is the range final.  If it is not block aligned,
> +	 * this is the wrong path for it.  Return -EAGAIN to
> +	 * xfs_file_dio_write(), which picks the path.
> +	 */
> +	if ((iocb->ki_pos | iov_iter_count(from)) & ip->i_mount->m_blockmask) {
> +		ret = -EAGAIN;
> +		goto out_unlock;
> +	}
> +
>  	/*
>  	 * We don't need to hold the IOLOCK exclusively across the IO, so demote
>  	 * the iolock back to shared if we had to take the exclusive lock in
> @@ -921,6 +932,7 @@ xfs_file_dio_write(
>  	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
>  	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
>  	size_t			count = iov_iter_count(from);
> +	ssize_t			ret;
>  
>  	/* direct I/O must be aligned to device logical sector size */
>  	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
> @@ -937,12 +949,32 @@ xfs_file_dio_write(
>  		return xfs_file_dio_write_atomic(ip, iocb, from);
>  	}
>  
> -	if ((iocb->ki_pos | count) & ip->i_mount->m_blockmask)
> -		return xfs_file_dio_write_unaligned(ip, iocb, from);
> -	if (xfs_is_zoned_inode(ip))
> -		return xfs_file_dio_write_zoned(ip, iocb, from);
> -	return xfs_file_dio_write_aligned(ip, iocb, from,
> -			&xfs_direct_write_iomap_ops, &xfs_dio_write_ops, NULL);
> +	/*
> +	 * O_APPEND and the file size limits can still change the range, in
> +	 * xfs_file_write_checks().  So a range that is block aligned here may
> +	 * not stay so, and the write then comes back with -EAGAIN.  If we are
> +	 * doing nonblocking user I/O, propagate the error.
> +	 */
> +	if (!((iocb->ki_pos | count) & ip->i_mount->m_blockmask)) {
> +		if (xfs_is_zoned_inode(ip))
> +			ret = xfs_file_dio_write_zoned(ip, iocb, from);
> +		else
> +			ret = xfs_file_dio_write_aligned(ip, iocb, from,
> +					&xfs_direct_write_iomap_ops,
> +					&xfs_dio_write_ops, NULL);
> +		if (ret != -EAGAIN || (iocb->ki_flags & IOCB_NOWAIT))
> +			return ret;
> +	}
> +
> +	/*
> +	 * Zoned inodes always write out of place, which does not work for a
> +	 * part of a block.  Fall back to buffered I/O.
> +	 */
> +	if (xfs_is_zoned_inode(ip)) {
> +		trace_xfs_reflink_bounce_dio_write(iocb, from);
> +		return -ENOTBLK;
> +	}
> +	return xfs_file_dio_write_unaligned(ip, iocb, from);
>  }

[Severity: Medium]
Can an O_DIRECT write whose updated range is no longer aligned to
target->bt_logical_sectormask silently fall back to buffered I/O instead of
returning -EINVAL?

When a block-aligned O_DIRECT write enters xfs_file_dio_write_aligned() and
xfs_file_write_checks() moves iocb->ki_pos to a non-sector-aligned EOF for
O_APPEND (or truncates from to a non-sector-aligned RLIMIT_FSIZE), the new
m_blockmask check returns -EAGAIN before iomap_dio_rw() can reject the
unaligned range with -EINVAL.  On the fallback path, neither
xfs_file_dio_write() nor xfs_file_dio_write_unaligned() rechecks
target->bt_logical_sectormask before returning -ENOTBLK for zoned or CoW
inodes, causing xfs_file_write_iter() to complete the sector-unaligned
O_DIRECT write through the page cache.  For non-CoW, non-zoned inodes,
xfs_file_dio_write_unaligned() also proceeds into iomap_dio_rw() and can
allocate unwritten blocks for extending writes before iomap_dio_bio_iter()
fails with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110190417.32650-1-mason@kernel.org?part=6