| Message ID | de61a54dcf5f7240d971150cb51faf0038d3d835.1739363803.git.nirjhar.roy.lists@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add mount and remount related tests | expand |
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. -Dave.
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? 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 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? --NR > > -Dave.
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.
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.
On Mon, Feb 17, 2025 at 10:18:48AM +0530, Nirjhar Roy (IBM) wrote: > 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: > > 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, Not at this point in time, because xfs/189 is supposed to exercise attr2/noattr2 mount/remount behaviour and take into account all the weirdness of the historic mount behaviour. Obviously, it is not detecting that this noattr2 remount behaviour was broken, so that test needs fixing/additions. Indeed, it's probably important to understand why xfs/189 isn't detecting this failure before going any further, right? IMO, it is better to fix existing tests that exercise the behaviour in question than it is to add a new test that covers just what the old test missed. -Dave.
On 2/18/25 03:59, Dave Chinner wrote: > On Mon, Feb 17, 2025 at 10:18:48AM +0530, Nirjhar Roy (IBM) wrote: >> 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: >>> 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, > Not at this point in time, because xfs/189 is supposed to exercise > attr2/noattr2 mount/remount behaviour and take into account all the > weirdness of the historic mount behaviour. > > Obviously, it is not detecting that this noattr2 remount behaviour > was broken, so that test needs fixing/additions. Indeed, it's > probably important to understand why xfs/189 isn't detecting this > failure before going any further, right? Yes. Let me look into what xfs/189 does and why it isn't detecting the noattr2 remount broken behavior. Thank you for the pointer. About "Patch 1/3: xfs/539: Skip noattr2 remount option on v5 file systems" --> I wrote the patch because xfs/539 has started failing in one of fstests CI runs because RHEL 10 has started disabling xfs v4 support i.e, CONFIG_XFS_SUPPORT_V4=n. Do you think modifying patch 1/3(xfs/539) in such a way that the test ignores the remount failures with noattr2 and continues the test is an appropriate idea (since the test xfs/539 only intends to check the dmesg warnings)? --NR > > IMO, it is better to fix existing tests that exercise the behaviour > in question than it is to add a new test that covers just what the > old test missed. > > -Dave.
diff --git a/tests/xfs/634 b/tests/xfs/634 new file mode 100755 index 00000000..dc153047 --- /dev/null +++ b/tests/xfs/634 @@ -0,0 +1,35 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2025 Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>. All Rights Reserved. +# +# FS QA Test 634 +# +# This test checks that mounting and remounting a v5 xfs filesystem with +# noattr2 fails. Currently, this test will pass with CONFIG_XFS_SUPPORT_V4=n but +# with CONFIG_XFS_SUPPORT_V4=y, this will fail i.e, mount -o remount,noattr2 +# command succeeds incorrectly. +# +. ./common/preamble +. ./common/filter + +_begin_fstest auto quick mount +_require_scratch +# Import common functions. + +_fixed_by_kernel_commit xxxx \ + "" +_require_scratch_xfs_v5 +_scratch_mkfs -m crc=1 > $seqres.full 2>&1 || + _notrun "need an xfs v5 filesystem" +_scratch_mount "-o noattr2" |& grep -iq "fail" && \ + echo "mount failed successfully" +_scratch_mount +! _scratch_remount noattr2 >> $seqres.full 2>&1 && \ + echo "remount failed successfully" +_scratch_unmount + +# success, all done +status=0 +exit +status=0 +exit diff --git a/tests/xfs/634.out b/tests/xfs/634.out new file mode 100644 index 00000000..8a98c05c --- /dev/null +++ b/tests/xfs/634.out @@ -0,0 +1,3 @@ +QA output created by 634 +mount failed successfully +remount failed successfully
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. Ideally the above mount command should always fail with a v5 xfs filesystem irrespective of whether CONFIG_XFS_SUPPORT_V4 is set or not. Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com> --- tests/xfs/634 | 35 +++++++++++++++++++++++++++++++++++ tests/xfs/634.out | 3 +++ 2 files changed, 38 insertions(+) create mode 100755 tests/xfs/634 create mode 100644 tests/xfs/634.out