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

[Dave Chinner <david@fromorbit.com>]

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).

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.

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.

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

-Dave.
-- 
Dave Chinner
david@fromorbit.com