Skip to contentSkip to conversation

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

PATCH Superseded Nirjhar Roy (IBM) 2 participants 4 messages Apr 29, 2025 XFS filesystem

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Nirjhar Roy (IBM)

author
#0 lore raw

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

This patch fixes an issue where remount with noattr2 doesn't fail explicitly on v5 xfs with CONFIG_XFS_SUPPORT_V4=y. Details are there in the commit message of the patch.

Related discussion in [1].

[v1] --> v2
- Added another final verification with xfs_finish_flags() in xfs_fs_reconfigure() during remount.

[v1] https://lore.kernel.org/all/7c4202348f67788db55c7ec445cbe3f2d587daf2.1744394929.git.nirjhar.roy.lists@gmail.com/
[1] https://lore.kernel.org/all/[email protected]/

Nirjhar Roy (IBM) (1):
xfs: Fail remount with noattr2 on a v5 with v4 enabled

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

--
2.43.5

author
#1 lore raw

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

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

Signed-off-by Nirjhar Roy (IBM) <[email protected]>
---
 fs/xfs/xfs_super.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
fs/xfs/xfs_super.c +25 -0
index b2dd0c0bf509..1fd45567ae00 100644
@@ -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.");
+			return -EINVAL;
+		}
+		else {
+			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 +2142,15 @@ 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);
Signature · 1 line
2.43.5
#2 lore raw

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

Quoted 64 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 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]>
---
 fs/xfs/xfs_super.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
fs/xfs/xfs_super.c +0 -0
index b2dd0c0bf509..1fd45567ae00 100644

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

Quoted 7 lines
fs/xfs/xfs_super.c +4 -0
@@ -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.");

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

Quoted 3 lines
fs/xfs/xfs_super.c +3 -0
+			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.

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

Quoted 5 lines
fs/xfs/xfs_super.c +5 -0
+	/* 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.

author
#3 lore raw

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

Quoted 94 lines · Christoph Hellwig
Quoted 64 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 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]>
---
  fs/xfs/xfs_super.c | 25 +++++++++++++++++++++++++
  1 file changed, 25 insertions(+)
fs/xfs/xfs_super.c +0 -0
index b2dd0c0bf509..1fd45567ae00 100644

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

Quoted 7 lines
fs/xfs/xfs_super.c +4 -0
@@ -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.");

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

Quoted 3 lines
fs/xfs/xfs_super.c +3 -0
+			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.

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

Quoted 5 lines
fs/xfs/xfs_super.c +5 -0
+	/* 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

Quoted 3 lines
	/*

at the beginning of a comment on a separate line.

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

--NR

Quoted 1 line
Signature · 3 lines
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore