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