linux-ext4 - Re: [PATCH v2] xfs: disallow atomic writes on DAX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250722175820.GV2672070@frogsfrogsfrogs>
Date: Tue, 22 Jul 2025 10:58:20 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: John Garry <john.g.garry@...cle.com>
Cc: hch@....de, cem@...nel.org, linux-xfs@...r.kernel.org,
	linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v2] xfs: disallow atomic writes on DAX

[cc linux-ext4 because ext4_file_write_iter has the same problem]

On Tue, Jul 22, 2025 at 03:00:16PM +0000, John Garry wrote:
> Atomic writes are not currently supported for DAX, but two problems exist:
> - we may go down DAX write path for IOCB_ATOMIC, which does not handle
>   IOCB_ATOMIC properly
> - we report non-zero atomic write limits in statx (for DAX inodes)
> 
> We may want atomic writes support on DAX in future, but just disallow for
> now.
> 
> For this, ensure when IOCB_ATOMIC is set that we check the write size
> versus the atomic write min and max before branching off to the DAX write
> path. This is not strictly required for DAX, as we should not get this far
> in the write path as FMODE_CAN_ATOMIC_WRITE should not be set.
> 
> In addition, due to reflink being supported for DAX, we automatically get
> CoW-based atomic writes support being advertised. Remedy this by
> disallowing atomic writes for a DAX inode for both sw and hw modes.

You might want to add a separate patch to insert:

	if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC))
		return -EIO;

into dax_iomap_rw to make it clear that DAX doesn't support ATOMIC
writes.

> Reported-by: Darrick J. Wong <djwong@...nel.org>
> Fixes: 9dffc58f2384 ("xfs: update atomic write limits")
> Signed-off-by: John Garry <john.g.garry@...cle.com>

Otherwise seems reasonable to me...
Reviewed-by: "Darrick J. Wong" <djwong@...nel.org>

--D

> ---
> Difference to v1:
> - allow use max atomic mount option and always dax together (Darrick)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index ed69a65f56d7..979abcb25bc7 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1099,9 +1099,6 @@ xfs_file_write_iter(
>  	if (xfs_is_shutdown(ip->i_mount))
>  		return -EIO;
>  
> -	if (IS_DAX(inode))
> -		return xfs_file_dax_write(iocb, from);
> -
>  	if (iocb->ki_flags & IOCB_ATOMIC) {
>  		if (ocount < xfs_get_atomic_write_min(ip))
>  			return -EINVAL;
> @@ -1114,6 +1111,9 @@ xfs_file_write_iter(
>  			return ret;
>  	}
>  
> +	if (IS_DAX(inode))
> +		return xfs_file_dax_write(iocb, from);
> +
>  	if (iocb->ki_flags & IOCB_DIRECT) {
>  		/*
>  		 * Allow a directio write to fall back to a buffered
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 07fbdcc4cbf5..bd6d33557194 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -358,9 +358,20 @@ static inline bool xfs_inode_has_bigrtalloc(const struct xfs_inode *ip)
>  
>  static inline bool xfs_inode_can_hw_atomic_write(const struct xfs_inode *ip)
>  {
> +	if (IS_DAX(VFS_IC(ip)))
> +		return false;
> +
>  	return xfs_inode_buftarg(ip)->bt_awu_max > 0;
>  }
>  
> +static inline bool xfs_inode_can_sw_atomic_write(const struct xfs_inode *ip)
> +{
> +	if (IS_DAX(VFS_IC(ip)))
> +		return false;
> +
> +	return xfs_can_sw_atomic_write(ip->i_mount);
> +}
> +
>  /*
>   * In-core inode flags.
>   */
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 149b5460fbfd..603effabe1ee 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -616,7 +616,8 @@ xfs_get_atomic_write_min(
>  	 * write of exactly one single fsblock if the bdev will make that
>  	 * guarantee for us.
>  	 */
> -	if (xfs_inode_can_hw_atomic_write(ip) || xfs_can_sw_atomic_write(mp))
> +	if (xfs_inode_can_hw_atomic_write(ip) ||
> +	    xfs_inode_can_sw_atomic_write(ip))
>  		return mp->m_sb.sb_blocksize;
>  
>  	return 0;
> @@ -633,7 +634,7 @@ xfs_get_atomic_write_max(
>  	 * write of exactly one single fsblock if the bdev will make that
>  	 * guarantee for us.
>  	 */
> -	if (!xfs_can_sw_atomic_write(mp)) {
> +	if (!xfs_inode_can_sw_atomic_write(ip)) {
>  		if (xfs_inode_can_hw_atomic_write(ip))
>  			return mp->m_sb.sb_blocksize;
>  		return 0;
> -- 
> 2.43.5
>