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

[PATCH v5 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].

[v4] --> v5
 - Modified the error message in xfs_fs_reconfigure().

[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/
[v4] https://lore.kernel.org/all/cover.1746600966.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 v5 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..606a95ac816f 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 is always enabled for a V5 filesystem - can't be changed.");
+			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 v5 1/1] xfs: Fail remount with noattr2 on a v5 with v4 enabled

[Carlos Maiolino]

On Mon, May 12, 2025 at 03:27:14PM +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..606a95ac816f 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 is always enabled for a V5 filesystem - can't be changed.");
> +			return -EINVAL;

This looks good to me now:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

I still wish hch's opinion here though before merging it. Giving his was the
first RwB, I want to make sure he still keeps his RwB with the above change.


FWIW, for a next patch, there is no need to copy ext4 list for a code change
that is totally unrelated to ext4. This just generates unnecessary extra
traffic.


> +		}
> +		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 v5 1/1] xfs: Fail remount with noattr2 on a v5 with v4 enabled

[Nirjhar Roy (IBM)]

On 5/12/25 16:40, Carlos Maiolino wrote:
> On Mon, May 12, 2025 at 03:27:14PM +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..606a95ac816f 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 is always enabled for a V5 filesystem - can't be changed.");
>> +			return -EINVAL;
> This looks good to me now:
>
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
>
> I still wish hch's opinion here though before merging it. Giving his was the
> first RwB, I want to make sure he still keeps his RwB with the above change.
Sure.
>
>
> FWIW, for a next patch, there is no need to copy ext4 list for a code change
> that is totally unrelated to ext4. This just generates unnecessary extra
> traffic.

Okay. I will keep this in mind.

--NR

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

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

[Christoph Hellwig]

This still looks good.

One nit:

> +	/*
> +	 * 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.

This could use slightly better formatting:

	/*
	 * 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.
	 */

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

[Nirjhar Roy (IBM)]

On 5/12/25 21:06, Christoph Hellwig wrote:
> This still looks good.
Thanks.
>
> One nit:
>
>> +	/*
>> +	 * 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.
> This could use slightly better formatting:
>
> 	/*
> 	 * 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.
> 	 */

Okay, I will make the change in the next revision.

--NR

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore