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
