[PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y

[PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y

[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 # This should fail but it doesn't

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 is 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>
---
 fs/xfs/xfs_super.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index b2dd0c0bf509..fd72c8fcd3a7 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2114,6 +2114,23 @@ 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;
-- 
2.43.5

Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y

[Christoph Hellwig]

On Fri, Apr 11, 2025 at 11:44:52PM +0530, Nirjhar Roy (IBM) wrote:
> mkfs.xfs -f /dev/loop0
> mount /dev/loop0 /mnt/scratch
> mount -o remount,noattr2 /dev/loop0 /mnt/scratch # This should fail but it doesn't

Please reflow your commit log to not exceed the standard 73 characters

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

But that also means the mount time check is wrong as well.

> +	/* 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;
> +		}

So this check should probably go into xfs_fs_validate_params, and
also have a more useful warning like:

	if (xfs_has_crc(mp) && xfs_has_noattr2(new_mp)) {
		xfs_warn(mp,
"noattr2 cannot be specified for v5 file systems.");
                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;
> +	}

Some of the indentation here looks broken.  Please always use one
tab per indentation level, places the closing brace before the else,
and don't use else after a return statement.

Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y

[Nirjhar Roy (IBM)]

On 4/14/25 11:18, Christoph Hellwig wrote:
> On Fri, Apr 11, 2025 at 11:44:52PM +0530, Nirjhar Roy (IBM) wrote:
>> mkfs.xfs -f /dev/loop0
>> mount /dev/loop0 /mnt/scratch
>> mount -o remount,noattr2 /dev/loop0 /mnt/scratch # This should fail but it doesn't
> Please reflow your commit log to not exceed the standard 73 characters
Noted. I will update this in the next revision.
>
>> 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.
> But that also means the mount time check is wrong as well.

So during mount, xfs_fs_fill_super() calls the following functions are 
called in sequence :

xfs_fs_validate_params()

<...>

xfs_readsb()

xfs_finish_flags().

If I am trying to "mount -o noattr2 /dev/loop0 /mnt1/test", then the 
invalid condition(noattr2 on v5) is not caught in 
xfs_fs_validate_params() because the superblock isn't read yet and 
"struct xfs_mount    *mp" is still not aware of whether the underlying 
filesystem is v5 or v4 (i.e, whether crc=0 or crc=1). So, even if the 
underlying filesystem is v5, xfs_has_attr2() will return false, because 
the m_features isn't populated yet. However, once xfs_readsb() is done, 
m_features is populated (mp->m_features |= 
xfs_sb_version_to_features(sbp); called at the end of xfs_readsb()). 
After that, when xfs_finish_flags() is called, the invalid mount option 
(i.e, noattr2 with crc=1) is caught, and the mount fails correctly. So, 
m_features is partially populated while xfs_fs_validate_params() is 
getting executed, I am not sure if that is done intentionally. IMO, we 
should have read the superblock, made sure that the m_features is fully 
populated within xfs_fs_validate_params() with the existing 
configurations of the underlying disk/fs and the ones supplied the by 
mount program - this can avoid such false negatives. Can you please let 
me know if my understanding is correct?

>
>> +	/* 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;
>> +		}
> So this check should probably go into xfs_fs_validate_params, and
> also have a more useful warning like:
>
> 	if (xfs_has_crc(mp) && xfs_has_noattr2(new_mp)) {
> 		xfs_warn(mp,
> "noattr2 cannot be specified for v5 file systems.");
>                  return -EINVAL;
> 	}
xfs_fs_validate_params() takes only one parameter. Are you suggesting to 
add another optional (NULLable) parameter "new_mp" and add the above 
check there? In that case, all other remount related checks in 
xfs_fs_reconfigure() qualify to be moved to xfs_fs_validate_params(), 
right? Is my understanding correct?
>
>
>> +		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;
>> +	}
> Some of the indentation here looks broken.  Please always use one
> tab per indentation level, places the closing brace before the else,
> and don't use else after a return statement.

Okay, I will fix this in the next revision. Thank you for pointing this 
out.

--NR

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y

[Christoph Hellwig]

On Tue, Apr 15, 2025 at 12:48:39PM +0530, Nirjhar Roy (IBM) wrote:
> condition(noattr2 on v5) is not caught in xfs_fs_validate_params() because
> the superblock isn't read yet and "struct xfs_mount    *mp" is still not
> aware of whether the underlying filesystem is v5 or v4 (i.e, whether crc=0
> or crc=1). So, even if the underlying filesystem is v5, xfs_has_attr2() will
> return false, because the m_features isn't populated yet.

Yes.

> However, once
> xfs_readsb() is done, m_features is populated (mp->m_features |=
> xfs_sb_version_to_features(sbp); called at the end of xfs_readsb()). After
> that, when xfs_finish_flags() is called, the invalid mount option (i.e,
> noattr2 with crc=1) is caught, and the mount fails correctly. So, m_features
> is partially populated while xfs_fs_validate_params() is getting executed, I
> am not sure if that is done intentionally.

As you pointed out above it can't be fully populated becaue we don't
have all the information.  And that can't be fixed because some of the
options are needed to even get to reading the superblock.

So we do need a second pass of verification for everything that depends
on informationtion from the superblock.  The fact that m_features
mixes user options and on-disk feature bits is unfortunately not very
helpful for a clear structure here.

Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y

[Nirjhar Roy (IBM)]

On 4/16/25 11:39, Christoph Hellwig wrote:
> On Tue, Apr 15, 2025 at 12:48:39PM +0530, Nirjhar Roy (IBM) wrote:
>> condition(noattr2 on v5) is not caught in xfs_fs_validate_params() because
>> the superblock isn't read yet and "struct xfs_mount    *mp" is still not
>> aware of whether the underlying filesystem is v5 or v4 (i.e, whether crc=0
>> or crc=1). So, even if the underlying filesystem is v5, xfs_has_attr2() will
>> return false, because the m_features isn't populated yet.
> Yes.
>
>> However, once
>> xfs_readsb() is done, m_features is populated (mp->m_features |=
>> xfs_sb_version_to_features(sbp); called at the end of xfs_readsb()). After
>> that, when xfs_finish_flags() is called, the invalid mount option (i.e,
>> noattr2 with crc=1) is caught, and the mount fails correctly. So, m_features
>> is partially populated while xfs_fs_validate_params() is getting executed, I
>> am not sure if that is done intentionally.
> As you pointed out above it can't be fully populated becaue we don't
> have all the information.  And that can't be fixed because some of the
> options are needed to even get to reading the superblock.
>
> So we do need a second pass of verification for everything that depends

Yes, we need a second pass and I think that is already being done by 
xfs_finish_flags() in xfs_fs_fill_super(). However, in 
xfs_fs_reconfigure(), we still need a check to make a transition from /* 
attr2 -> noattr2 */ and /* noattr2 -> attr2 */ (only if it is permitted 
to) and update mp->m_features accordingly, just like it is being done 
for inode32 <-> inode64, right? Also, in your previous reply[1], you did 
suggest moving the crc+noattr2 check to xfs_fs_validate_params() - Are 
you suggesting to add another optional (NULLable) parameter "new_mp" 
to xfs_fs_validate_params() and then moving the check to 
xfs_fs_validate_params()?

[1] https://lore.kernel.org/all/Z_yhpwBQz7Xs4WLI@infradead.org/

--NR

> on informationtion from the superblock.  The fact that m_features
> mixes user options and on-disk feature bits is unfortunately not very
> helpful for a clear structure here.
>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y

[Nirjhar Roy (IBM)]

On 4/16/25 13:05, Nirjhar Roy (IBM) wrote:
>
> On 4/16/25 11:39, Christoph Hellwig wrote:
>> On Tue, Apr 15, 2025 at 12:48:39PM +0530, Nirjhar Roy (IBM) wrote:
>>> condition(noattr2 on v5) is not caught in xfs_fs_validate_params() 
>>> because
>>> the superblock isn't read yet and "struct xfs_mount    *mp" is still 
>>> not
>>> aware of whether the underlying filesystem is v5 or v4 (i.e, whether 
>>> crc=0
>>> or crc=1). So, even if the underlying filesystem is v5, 
>>> xfs_has_attr2() will
>>> return false, because the m_features isn't populated yet.
>> Yes.
>>
>>> However, once
>>> xfs_readsb() is done, m_features is populated (mp->m_features |=
>>> xfs_sb_version_to_features(sbp); called at the end of xfs_readsb()). 
>>> After
>>> that, when xfs_finish_flags() is called, the invalid mount option (i.e,
>>> noattr2 with crc=1) is caught, and the mount fails correctly. So, 
>>> m_features
>>> is partially populated while xfs_fs_validate_params() is getting 
>>> executed, I
>>> am not sure if that is done intentionally.
>> As you pointed out above it can't be fully populated becaue we don't
>> have all the information.  And that can't be fixed because some of the
>> options are needed to even get to reading the superblock.
>>
>> So we do need a second pass of verification for everything that depends
>
> Yes, we need a second pass and I think that is already being done by 
> xfs_finish_flags() in xfs_fs_fill_super(). However, in 
> xfs_fs_reconfigure(), we still need a check to make a transition from 
> /* attr2 -> noattr2 */ and /* noattr2 -> attr2 */ (only if it is 
> permitted to) and update mp->m_features accordingly, just like it is 
> being done for inode32 <-> inode64, right? Also, in your previous 
> reply[1], you did suggest moving the crc+noattr2 check to 
> xfs_fs_validate_params() - Are you suggesting to add another optional 
> (NULLable) parameter "new_mp" to xfs_fs_validate_params() and then 
> moving the check to xfs_fs_validate_params()?
>
> [1] https://lore.kernel.org/all/Z_yhpwBQz7Xs4WLI@infradead.org/
>
> --NR
>
Hi Christoph,

Any further feedback on the above and the overall patch? Can you please 
suggest the changes you want me to do for the patch?

--NR

>> on informationtion from the superblock. The fact that m_features
>> mixes user options and on-disk feature bits is unfortunately not very
>> helpful for a clear structure here.
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y

[Christoph Hellwig]

Hi Nirjhar,

sorry for the delay, I dropped the ball while on vacation.

On Wed, Apr 16, 2025 at 01:05:46PM +0530, Nirjhar Roy (IBM) wrote:
> 
> Yes, we need a second pass and I think that is already being done by
> xfs_finish_flags() in xfs_fs_fill_super(). However, in xfs_fs_reconfigure(),
> we still need a check to make a transition from /* attr2 -> noattr2 */ and
> /* noattr2 -> attr2 */ (only if it is permitted to) and update
> mp->m_features accordingly, just like it is being done for inode32 <->
> inode64, right?

Yes.

> Also, in your previous reply[1], you did suggest moving the
> crc+noattr2 check to xfs_fs_validate_params() - Are you suggesting to add
> another optional (NULLable) parameter "new_mp" to xfs_fs_validate_params()
> and then moving the check to xfs_fs_validate_params()?

No, let's skip that.

But we really should share the code for the validation.  So while a lot
of the checks in xfs_finish_flags are uselss in remount, it might still
be a good idea to just use that for the option validation so that we
don't miss checks in remount.

Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y

[Nirjhar Roy (IBM)]

On 4/25/25 18:35, Christoph Hellwig wrote:
> Hi Nirjhar,
>
> sorry for the delay, I dropped the ball while on vacation.
>
> On Wed, Apr 16, 2025 at 01:05:46PM +0530, Nirjhar Roy (IBM) wrote:
>> Yes, we need a second pass and I think that is already being done by
>> xfs_finish_flags() in xfs_fs_fill_super(). However, in xfs_fs_reconfigure(),
>> we still need a check to make a transition from /* attr2 -> noattr2 */ and
>> /* noattr2 -> attr2 */ (only if it is permitted to) and update
>> mp->m_features accordingly, just like it is being done for inode32 <->
>> inode64, right?
> Yes.
Okay.
>
>> Also, in your previous reply[1], you did suggest moving the
>> crc+noattr2 check to xfs_fs_validate_params() - Are you suggesting to add
>> another optional (NULLable) parameter "new_mp" to xfs_fs_validate_params()
>> and then moving the check to xfs_fs_validate_params()?
> No, let's skip that.
>
> But we really should share the code for the validation.  So while a lot
> of the checks in xfs_finish_flags are uselss in remount, it might still
> be a good idea to just use that for the option validation so that we
> don't miss checks in remount.

Okay. I will make the changes in v2. Thank you for your suggestions.

--NR

>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore