xfs: Fail remount with noattr2 on a v5 xfs with v4 enabled kernel.

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 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/[email protected]/

Signed-off-by Nirjhar Roy (IBM) <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_super.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

fs/xfs/xfs_super.c +0 -0

index b2dd0c0bf509..58a0431ab52d 100644

fs/xfs/xfs_super.c +21 -1

+			return -EINVAL;
+		}
+		mp->m_features &= ~XFS_FEAT_ATTR2;
+		mp->m_features |= XFS_FEAT_NOATTR2;
+	} else if (xfs_has_attr2(new_mp)) {
+		/* noattr2 -> attr2 */
+		mp->m_features &= ~XFS_FEAT_NOATTR2;
+		mp->m_features |= XFS_FEAT_ATTR2;
+	}
+
 	/* inode32 -> inode64 */
 	if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) {
 		mp->m_features &= ~XFS_FEAT_SMALL_INUMS;

@@ -2126,6 +2141,17 @@ xfs_fs_reconfigure(
 		mp->m_maxagi = xfs_set_inode_alloc(mp, mp->m_sb.sb_agcount);
 	}
 
+	/*
+	 * 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.
+	 */
+	error = xfs_finish_flags(mp);
+	if (error)
+		return error;
+
 	/* ro -> rw */
 	if (xfs_is_readonly(mp) && !(flags & SB_RDONLY)) {
 		error = xfs_remount_rw(mp);
--

2.43.5

Hi Nirjhar.

The patch looks fine, with a caveat below.

Quoted 65 lines · Nirjhar Roy (IBM)

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 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/[email protected]/

Signed-off-by Nirjhar Roy (IBM) <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
  fs/xfs/xfs_super.c | 26 ++++++++++++++++++++++++++
  1 file changed, 26 insertions(+)

fs/xfs/xfs_super.c +0 -0

index b2dd0c0bf509..58a0431ab52d 100644

fs/xfs/xfs_super.c xfs_fs_reconfigure( · line 2121

Quoted 8 lines

fs/xfs/xfs_super.c +5 -0

@@ -2114,6 +2114,21 @@ 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.");

This message doesn't seem to make sense to me. Your code checks the attr2 option is not being changed on a V5 FS, but your error message states both mount options are bing specified at the command line, confusing the user.

V5 format always use attr2, and can't use noattr2. So, this message is misleading.

IMO this should be something like:

"attr2 is always enabled for for a V5 filesystem and can't be changed."

Carlos

Quoted 35 lines

fs/xfs/xfs_super.c +21 -1

+			return -EINVAL;
+		}
+		mp->m_features &= ~XFS_FEAT_ATTR2;
+		mp->m_features |= XFS_FEAT_NOATTR2;
+	} else if (xfs_has_attr2(new_mp)) {
+		/* noattr2 -> attr2 */
+		mp->m_features &= ~XFS_FEAT_NOATTR2;
+		mp->m_features |= XFS_FEAT_ATTR2;
+	}
+
  	/* inode32 -> inode64 */
  	if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) {
  		mp->m_features &= ~XFS_FEAT_SMALL_INUMS;

@@ -2126,6 +2141,17 @@ xfs_fs_reconfigure(
  		mp->m_maxagi = xfs_set_inode_alloc(mp, mp->m_sb.sb_agcount);
  	}
 
+	/*
+	 * 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.
+	 */
+	error = xfs_finish_flags(mp);
+	if (error)
+		return error;
+
  	/* ro -> rw */
  	if (xfs_is_readonly(mp) && !(flags & SB_RDONLY)) {
  		error = xfs_remount_rw(mp);
--

2.43.5