Re: [PATCH v2 1/1] xfs: Fail remount with noattr2 on a v5 with v4 enabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 4/29/25 17:48, Christoph Hellwig wrote:
On Tue, Apr 29, 2025 at 02:32:08PM +0530, Nirjhar Roy (IBM) wrote:
Bug: When we compile the kernel with CONFIG_XFS_SUPPORT_V4=y,
remount with "-o remount,noattr2" on a v5 XFS does not
fail explicitly.

Reproduction:
mkfs.xfs -f /dev/loop0
mount /dev/loop0 /mnt/scratch
mount -o remount,noattr2 /dev/loop0 /mnt/scratch

However, with CONFIG_XFS_SUPPORT_V4=n, the remount
correctly fails explicitly. This is because the way the
following 2 functions are defined:

static inline bool xfs_has_attr2 (struct xfs_mount *mp)
{
	return !IS_ENABLED(CONFIG_XFS_SUPPORT_V4) ||
		(mp->m_features & XFS_FEAT_ATTR2);
}
static inline bool xfs_has_noattr2 (const struct xfs_mount *mp)
{
	return mp->m_features & XFS_FEAT_NOATTR2;
}

xfs_has_attr2() returns true when CONFIG_XFS_SUPPORT_V4=n
and hence, the the following if condition in
xfs_fs_validate_params() succeeds and returns -EINVAL:

/*
  * We have not read the superblock at this point, so only the attr2
  * mount option can set the attr2 feature by this stage.
  */

if (xfs_has_attr2(mp) && xfs_has_noattr2(mp)) {
	xfs_warn(mp, "attr2 and noattr2 cannot both be specified.");
	return -EINVAL;
}

With CONFIG_XFS_SUPPORT_V4=y, xfs_has_attr2() always return
false and hence no error is returned.

Fix: Check if the existing mount has crc enabled(i.e, of
type v5 and has attr2 enabled) and the
remount has noattr2, if yes, return -EINVAL.

I have tested xfs/{189,539} in fstests with v4
and v5 XFS with both CONFIG_XFS_SUPPORT_V4=y/n and
they both behave as expected.

This patch also fixes remount from noattr2 -> attr2 (on a v4 xfs).

Related discussion in [1]

[1] https://lore.kernel.org/all/Z65o6nWxT00MaUrW@xxxxxxxxxxxxxxxxxxx/

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@xxxxxxxxx>
---
  fs/xfs/xfs_super.c | 25 +++++++++++++++++++++++++
  1 file changed, 25 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index b2dd0c0bf509..1fd45567ae00 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2114,6 +2114,22 @@ xfs_fs_reconfigure(
  	if (error)
  		return error;
+ /* attr2 -> noattr2 */
+	if (xfs_has_noattr2(new_mp)) {
+		if (xfs_has_crc(mp)) {
+			xfs_warn(mp, "attr2 and noattr2 cannot both be specified.");
Nit: normal xfs style is to move the message to a new line when іt
overflows 80 characters:

		if (xfs_has_crc(mp)) {
			xfs_warn(mp,
	"attr2 and noattr2 cannot both be specified.");

+			return -EINVAL;
+		}
+		else {
No need for an else after a return.  And for cases where there is an
else the kernel coding style keeps it on the same line as the closing
brace.

+	/* Now that mp has been modified according to the remount options, we do a
+	 * final option validation with xfs_finish_flags() just like it is done
+	 * during mount. We cannot use xfs_finish_flags() on new_mp as it contains
+	 * only the user given options.
+	 */
Please keep comments to 80 characters.  Also the kernel coding style
keeps the

	/*

at the beginning of a comment on a separate line.

Sure, I will address the above coding style related issues. Thank you.

--NR


--
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore





[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux