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: <757190c8-f7e4-404b-88cd-772e0b62dea5@gmail.com>
Date: Wed, 16 Apr 2025 13:05:46 +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 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