Re: [PATCH v1 3/3] xfs: Add a testcase to check remount with noattr2 on a v5 xfs

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

On 2/14/25 03:19, Dave Chinner wrote:
> On Thu, Feb 13, 2025 at 03:30:50PM +0530, Nirjhar Roy (IBM) wrote:
>> On 2/13/25 03:17, Dave Chinner wrote:
>>> On Wed, Feb 12, 2025 at 12:39:58PM +0000, Nirjhar Roy (IBM) wrote:
>>>> This testcase reproduces the following bug:
>>>> Bug:
>>>> mount -o remount,noattr2 <device> <mount_point> succeeds
>>>> unexpectedly on a v5 xfs when CONFIG_XFS_SUPPORT_V4 is set.
>>> AFAICT, this is expected behaviour. Remount intentionally ignores
>>> options that cannot be changed.
>>>
>>>> Ideally the above mount command should always fail with a v5 xfs
>>>> filesystem irrespective of whether CONFIG_XFS_SUPPORT_V4 is set
>>>> or not.
>>> No, we cannot fail remount when invalid options are passed to the
>>> kernel by the mount command for historical reasons. i.e. the mount
>>> command has historically passed invalid options to the kernel on
>>> remount, but expects the kernel to apply just the new options that
>>> they understand and ignore the rest without error.
>>>
>>> i.e. to keep compatibility with older userspace, we cannot fail a
>>> remount because userspace passed an option the kernel does not
>>> understand or cannot change.
>>>
>>> Hence, in this case, XFS emits a deprecation warning for the noattr2
>>> mount option on remount (because it is understood), then ignores
>>> because it it isn't a valid option that remount can change.
>> Thank you, Dave, for the background. This was really helpful. So just to
>> confirm the behavior of mount - remount with noattr2 (or any other invalid
>> option) should always pass irrespective of whether CONFIG_XFS_SUPPORT_V4 is
>> set or not, correct?
> Not necessarily.
>
> It depends on whether the filesystem considers it a known option or
> not. noattr2 is a known option, so if it is invalid to use it as a
> remount option, the remount should always fail.
>
> If the option is -unknown-, then the behaviour of remount is largely
> dependent on filesystem implementation -and- what mount syscall
> interface is being used by userspace.
>
> e.g. a modern mount binary using
> fsconfig(2) allows the kernel to reject unknown options before the
> filesystem is remounted. However, we cannot do that with the
> mount(2) interface because of the historic behaviour of the mount
> binary (see the comment above xfs_fs_reconfigure() about this).
Okay, I will look into the comments above xfs_fs_reconfigure(). Thank 
you for the pointer.
>
> Hence with a modern mount binary using the fsconfig(2) interface,
> the kernel can actually reject bad/unknown mount options without
> breaking anything. i.e. kernel behaviour is dependent on userspace
> implementation...
>
>> This is the behavior that I have observed with CONFIG_XFS_SUPPORT_V4=n on v5
>> xfs:
>>
>> $ mount -o "remount,noattr2" /dev/loop0 /mnt1/test
>> mount: /mnt1/test: mount point not mounted or bad option.
>> $ echo "$?"
>> 32
> This is not useful in itself because of all the above possibilities.
> i.e. What generated that error?
>
> Was if from the mount binary, or the kernel?  What syscall is mount
> using - strace output will tell us if it is fsconfig(2) or mount(2)
> and what is being passed to the kernel.  What does dmesg say - did
> the kernel parse the option and then fail, or something else?
>
> i.e. this is actually really hard to write a kernel and userspace
> version agnostic regression test for.
>
>> With this test, I am also parallelly working on a kernel fix to make the
>> behavior of remount with noattr2 same irrespective of the
>> CONFIG_XFS_SUPPORT_V4's value, and I was under the impression that it should
>> always fail. But, it seems like it should always pass (silently ignoring the
>> invalid mount options) and the failure when CONFIG_XFS_SUPPORT_V4=n is a
>> bug. Is my understanding correct?
> As per above, the behaviour we expose to userspace is actually
> dependent on the syscall interface the mount is using.
>
> That said, I still don't see why CONFIG_XFS_SUPPORT_V4 would change
> how we parse and process noattr2.....
>
> .... Ohhh.
>
> The new xfs_mount being used for reconfiguring the
> superblock on remount doesn't have the superblock feature
> flags initialised. attr2 is defined as:
>
> __XFS_ADD_V4_FEAT(attr2, ATTR2)
>
> Which means if CONFIG_XFS_SUPPORT_V4=n it will always return true.
>
> However, if CONFIG_XFS_SUPPORT_V4=y, then it checks for the ATTR2
> feature flag in the xfs_mount.
>
> Hence when we are validating the noattr2 flag in
> xfs_fs_validate_params(), this check:
>
> 	/*
>           * 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;
>          }
>
> Never triggers on remount when CONFIG_XFS_SUPPORT_V4=y because
> xfs_has_attr2(mp) is always false.  OTOH, when
> CONFIG_XFS_SUPPORT_V4=n, xfs_has_attr2(mp) is always true because of
> the __XFS_ADD_V4_FEAT() macro implementation, and so now it rejects
> the noattr2 mount option because it isn't valid on a v5 filesystem.
Yes, that is correct. This is my analysis too.
>
> Ok, so CONFIG_XFS_SUPPORT_V4=n is the correct behaviour (known mount
> option, invalid configuration being asked for), and it is the
> CONFIG_XFS_SUPPORT_V4=y behaviour that is broken.

Okay, so do you find this testcase (patch 3/3 xfs: Add a testcase to 
check remount with noattr2 on a v5 xfs) useful, and shall I work on the 
corresponding kernel fix for it? I can make the change in "[patch1/3] 
xfs/539: Skip noattr2 remount option on v5 filesystems" to ignore the 
mount failures (since that test is checking for dmesg warnings), what do 
you think? Do you have any other suggestions?

--NR

>
> This likely has been broken since the mount option parsing was
> first changed to use the fscontext interfaces....
>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore