linux-kernel - Re: [PATCH v7.1 14/14] xfs: allow sysadmins to specify a maximum atomic write limit at mount time

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-prev] [day] [month] [year] [list]

Message-ID: <20250416162649.GJ25675@frogsfrogsfrogs>
Date: Wed, 16 Apr 2025 09:26:49 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: John Garry <john.g.garry@...cle.com>
Cc: brauner@...nel.org, hch@....de, viro@...iv.linux.org.uk, jack@...e.cz,
	cem@...nel.org, linux-fsdevel@...r.kernel.org, dchinner@...hat.com,
	linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	ojaswin@...ux.ibm.com, ritesh.list@...il.com,
	martin.petersen@...cle.com, linux-ext4@...r.kernel.org,
	linux-block@...r.kernel.org, catherine.hoang@...cle.com,
	linux-api@...r.kernel.org
Subject: Re: [PATCH v7.1 14/14] xfs: allow sysadmins to specify a maximum
 atomic write limit at mount time

On Wed, Apr 16, 2025 at 11:08:25AM +0100, John Garry wrote:
> On 15/04/2025 23:36, Darrick J. Wong wrote:
> 
> Thanks for this, but it still seems to be problematic for me.
> 
> In my test, I have agsize=22400, and when I attempt to mount with
> atomic_write_max=8M, it passes when it shouldn't. It should not because
> max_pow_of_two_factor(22400) = 128, and 8MB > 128 FSB.
> 
> How about these addition checks:
> 
> > +
> > +	if (new_max_bytes) {
> > +		xfs_extlen_t	max_write_fsbs =
> > +			rounddown_pow_of_two(XFS_B_TO_FSB(mp, MAX_RW_COUNT));
> > +		xfs_extlen_t	max_group_fsbs =
> > +			max(mp->m_groups[XG_TYPE_AG].blocks,
> > +			    mp->m_groups[XG_TYPE_RTG].blocks);
> > +
> > +		ASSERT(max_write_fsbs <= U32_MAX);
> 
> 		if (!is_power_of_2(new_max_bytes)) {
> 			xfs_warn(mp,
>  "max atomic write size of %llu bytes is not a power-of-2",
> 					new_max_bytes);
> 			return -EINVAL;
> 		}

Long-term I'm not convinced that we really need to have all these power
of two checks because the software fallback can remap just about
anything, but for now I see no harm in doing this because
generic_atomic_write_valid enforces that property on the IO length.

> > +
> > +		if (new_max_bytes % mp->m_sb.sb_blocksize > 0) {
> > +			xfs_warn(mp,
> > + "max atomic write size of %llu bytes not aligned with fsblock",
> > +					new_max_bytes);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (new_max_fsbs > max_write_fsbs) {
> > +			xfs_warn(mp,
> > + "max atomic write size of %lluk cannot be larger than max write size %lluk",
> > +					new_max_bytes >> 10,
> > +					XFS_FSB_TO_B(mp, max_write_fsbs) >> 10);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (new_max_fsbs > max_group_fsbs) {
> > +			xfs_warn(mp,
> > + "max atomic write size of %lluk cannot be larger than allocation group size %lluk",
> > +					new_max_bytes >> 10,
> > +					XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> 
> 	if (new_max_fsbs > max_pow_of_two_factor(max_group_fsbs)) {
> 		xfs_warn(mp,
>  "max atomic write size of %lluk not aligned with allocation group size
> %lluk",
> 				new_max_bytes >> 10,
> 				XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
> 		return -EINVAL;

I think I'd rather clean up these bits:

	if (mp->m_ddev_targp->bt_bdev_awu_min > 0)
		max_agsize = max_pow_of_two_factor(mp->m_sb.sb_agblocks);
	else
		max_agsize = mp->m_ag_max_usable;

and

	if (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_bdev_awu_min > 0)
		max_rgsize = max_pow_of_two_factor(rgs->blocks);
	else
		max_rgsize = rgs->blocks;

into a shared helper for xfs_compute_atomic_write_unit_max so that we
use the exact same logic in both places.  But I agree with the general
direction.

--D

> 	}
> 
> thanks,
> John
>