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

["Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com>]

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