linux-ext4 - Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6d1f2cf1-2097-492e-a36e-43a3fd865c4a@gmail.com>
Date: Fri, 25 Apr 2025 11:22:52 +0530
From: "Nirjhar Roy (IBM)" <nirjhar.roy.lists@...il.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: linux-xfs@...r.kernel.org, linux-ext4@...r.kernel.org,
 fstests@...r.kernel.org, ritesh.list@...il.com, ojaswin@...ux.ibm.com,
 djwong@...nel.org, zlang@...nel.org, david@...morbit.com
Subject: Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with
 CONFIG_XFS_SUPPORT_V4=y


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