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: <0d1d7e6f-d2b9-4c38-9c8e-a25671b6380c@gmail.com>
Date: Tue, 15 Apr 2025 12:48:39 +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/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