[PATCH v4 0/1] xfs: Fail remount with noattr2 on a v5 xfs with v4 enabled kernel.

[PATCH v4 0/1] xfs: Fail remount with noattr2 on a v5 xfs with v4 enabled kernel.

[Nirjhar Roy (IBM)]

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].

[v3] --> v4
 - Added RB[2] of Christoph.

NOTE: The primary reason to send v4 is that this patch got missed in the previous update of linux-xfs kernel release.

[v1] https://lore.kernel.org/all/7c4202348f67788db55c7ec445cbe3f2d587daf2.1744394929.git.nirjhar.roy.lists@gmail.com/
[v2] https://lore.kernel.org/all/cover.1745916682.git.nirjhar.roy.lists@gmail.com/
[v3] https://lore.kernel.org/all/cover.1745937794.git.nirjhar.roy.lists@gmail.com/
[1] https://lore.kernel.org/all/Z65o6nWxT00MaUrW@dread.disaster.area/
[2] https://lore.kernel.org/all/aBRt2SNUxb6WuMO-@infradead.org/

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

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

--
2.43.5

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

[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/Z65o6nWxT00MaUrW@dread.disaster.area/

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_super.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index b2dd0c0bf509..58a0431ab52d 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -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.");
+			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

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

[Carlos Maiolino]

Hi Nirjhar.

The patch looks fine, with a caveat below.

On Wed, May 07, 2025 at 12:59:13PM +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 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@dread.disaster.area/
> 
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_super.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index b2dd0c0bf509..58a0431ab52d 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -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

> +			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
> 
> 

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

[Nirjhar Roy (IBM)]

On 5/12/25 15:06, Carlos Maiolino wrote:
> Hi Nirjhar.
>
> The patch looks fine, with a caveat below.
>
> On Wed, May 07, 2025 at 12:59:13PM +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 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@dread.disaster.area/
>>
>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   fs/xfs/xfs_super.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index b2dd0c0bf509..58a0431ab52d 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -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."

Okay, makes sense. Thank you for pointing it out. I will make the change 
and send a v5.

--NR

>
> Carlos
>
>> +			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
>>
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore