From: Eric Biggers <[email protected]>
'not_mnt OPTIONS' seems to have been intended to test that the
filesystem cannot be mounted at all with the given OPTIONS, meaning that
the mount fails as opposed to the options being ignored. However, this
doesn't actually work, as shown by the fact that the test case 'not_mnt
test_dummy_encryption=v3' is passing in the !CONFIG_FS_ENCRYPTION case.
Actually ext4 ignores this mount option when !CONFIG_FS_ENCRYPTION.
(The ext4 behavior might be changed, but that is besides the point.)
The problem is that the do_mnt() helper function is being misused in a
context where a mount failure is expected, and it does some additional
remount tests that don't make sense in that context. So if the mount
unexpectedly succeeds, then one of these later tests can still "fail",
causing the unexpected success to be shadowed by a later failure, which
causes the overall test case to pass since it expects a failure.
Fix this by reworking not_mnt() and not_remount_noumount() to use
simple_mount() in cases where they are expecting a failure. Also fix
up some of the naming and calling conventions to be less confusing.
Finally, make sure to test that remounting fails too, not just mounting.
Signed-off-by: Eric Biggers <[email protected]>
---
tests/ext4/053 | 148 ++++++++++++++++++++++++++-----------------------
1 file changed, 78 insertions(+), 70 deletions(-)
diff --git a/tests/ext4/053 b/tests/ext4/053
index e1e79592..bf6e3f6b 100755
--- a/tests/ext4/053
+++ b/tests/ext4/053
@@ -225,6 +225,9 @@ do_mnt() {
return $ret
}
+# Test that mounting or remounting with the specified mount option(s) fails
+# (meaning that the mount or remount fails, as opposed to the mount option(s)
+# just being ignored).
not_mnt() {
# We don't need -t not not_ variant
if [ "$1" == "-t" ]; then
@@ -232,13 +235,21 @@ not_mnt() {
fi
print_log "SHOULD FAIL mounting $fstype \"$1\" "
- do_mnt $@
- if [ $? -eq 0 ]; then
+ if simple_mount -o $1 $SCRATCH_DEV $SCRATCH_MNT; then
+ print_log "(mount unexpectedly succeeded)"
fail
- else
- ok
+ $UMOUNT_PROG $SCRATCH_MNT
+ return
fi
- $UMOUNT_PROG $SCRATCH_MNT 2> /dev/null
+ ok
+
+ if ! simple_mount $SCRATCH_DEV $SCRATCH_MNT; then
+ print_log "(normal mount unexpectedly failed)"
+ fail
+ return
+ fi
+ not_remount $1
+ $UMOUNT_PROG $SCRATCH_MNT
}
mnt_only() {
@@ -321,43 +332,40 @@ remount() {
$UMOUNT_PROG $SCRATCH_MNT 2> /dev/null
}
-# $1 - options to mount with, or -r argument
-# $2 - options to remount with
-not_remount_noumount() {
- remount_only=0
- # If -r is specified we're going to do remount only
- if [ "$1" == "-r" ]; then
- remount_only=1
- # Dont need shift since first argument would
- # have been consumed by mount anyway
- fi
-
- if [ $remount_only -eq 0 ]; then
- print_log "mounting $fstype \"$1\" "
- do_mnt $1
- [ $? -ne 0 ] && fail && return
- fi
- print_log "SHOULD FAIL remounting $fstype \"$2\" "
- do_mnt remount,$2 $3
- if [ $? -eq 0 ]; then
+# Test that the filesystem cannot be remounted with option(s) $1 (meaning that
+# the remount fails, as opposed to the mount option(s) just being ignored). The
+# filesystem must already be mounted, and it is not unmounted afterwards.
+not_remount() {
+ print_log "SHOULD FAIL remounting $fstype \"$1\" "
+ # Try specifying both dev and mnt.
+ if simple_mount -o remount,$1 $SCRATCH_DEV $SCRATCH_MNT; then
+ print_log "(remount unexpectedly succeeded)"
fail
- else
- ok
+ return
fi
+ ok
- # Now just specify mnt
- print_log "SHOULD FAIL remounting $fstype (MNT ONLY) \"$2\" "
- do_mnt -n remount,$2 $3
- if [ $? -eq 0 ]; then
+ # Try specifying mnt only.
+ print_log "SHOULD FAIL remounting $fstype (MNT ONLY) \"$1\" "
+ if simple_mount -o remount,$1 $SCRATCH_MNT; then
+ print_log "(remount unexpectedly succeeded)"
fail
- else
- ok
+ return
fi
+ ok
}
-not_remount() {
- not_remount_noumount $*
- $UMOUNT_PROG $SCRATCH_MNT 2> /dev/null
+# Mount the filesystem with option(s) $1, then test that it cannot be remounted
+# with option(s) $2 (meaning that the remount fails, as opposed to the mount
+# option(s) just being ignored).
+mnt_then_not_remount() {
+ print_log "mounting $fstype \"$1\" "
+ if ! do_mnt $1; then
+ fail
+ return
+ fi
+ not_remount $2
+ $UMOUNT_PROG $SCRATCH_MNT
}
@@ -514,7 +522,7 @@ for fstype in ext2 ext3 ext4; do
mnt test_dummy_encryption ^test_dummy_encryption
mnt test_dummy_encryption=v1 ^test_dummy_encryption=v1
mnt test_dummy_encryption=v2 ^test_dummy_encryption=v2
- not_mnt test_dummy_encryption=v3
+ mnt test_dummy_encryption=v3 ^test_dummy_encryption=v3
not_mnt test_dummy_encryption=
fi
@@ -567,14 +575,14 @@ for fstype in ext2 ext3 ext4; do
mnt dax=never
mnt dax=inode
- not_remount lazytime dax
- not_remount dax=always dax=never
+ mnt_then_not_remount lazytime dax
+ mnt_then_not_remount dax=always dax=never
if [[ $fstype != "ext2" ]]; then
- not_remount data=journal dax
- not_remount data=journal dax=always
- not_remount data=journal dax=never
- not_remount data=journal dax=inode
+ mnt_then_not_remount data=journal dax
+ mnt_then_not_remount data=journal dax=always
+ mnt_then_not_remount data=journal dax=never
+ mnt_then_not_remount data=journal dax=inode
fi
fi
@@ -584,20 +592,20 @@ for fstype in ext2 ext3 ext4; do
remount usrquota usrjquota=q.u,jqfmt=vfsv0
remount grpquota grpjquota=q.g,jqfmt=vfsv0
- not_remount usrquota grpjquota=q.g,jqfmt=vfsv0
- not_remount grpquota usrjquota=q.u,jqfmt=vfsv0
+ mnt_then_not_remount usrquota grpjquota=q.g,jqfmt=vfsv0
+ mnt_then_not_remount grpquota usrjquota=q.u,jqfmt=vfsv0
remount quota usrjquota=q.u,jqfmt=vfsv0
- not_remount quota grpjquota=q.g,jqfmt=vfsv0
+ mnt_then_not_remount quota grpjquota=q.g,jqfmt=vfsv0
remount usrjquota=q.u,jqfmt=vfsv0 grpjquota=q.g
- not_remount usrjquota=q.u,jqfmt=vfsv0 usrjquota=q.ua
- not_remount grpjquota=q.g,jqfmt=vfsv0 grpjquota=q.ga
+ mnt_then_not_remount usrjquota=q.u,jqfmt=vfsv0 usrjquota=q.ua
+ mnt_then_not_remount grpjquota=q.g,jqfmt=vfsv0 grpjquota=q.ga
remount usrjquota=q.u,jqfmt=vfsv0 usrquota usrjquota=q.u,jqfmt=vfsv0
remount grpjquota=q.g,jqfmt=vfsv0 grpquota grpjquota=q.g,jqfmt=vfsv0
- not_remount usrjquota=q.u,jqfmt=vfsv0 grpquota
- not_remount grpjquota=q.g,jqfmt=vfsv0 usrquota
+ mnt_then_not_remount usrjquota=q.u,jqfmt=vfsv0 grpquota
+ mnt_then_not_remount grpjquota=q.g,jqfmt=vfsv0 usrquota
remount grpjquota=q.g,jqfmt=vfsv0 grpjquota= ^grpjquota=
remount usrjquota=q.u,jqfmt=vfsv0 usrjquota= ^usrjquota=
@@ -608,12 +616,12 @@ for fstype in ext2 ext3 ext4; do
if [[ $fstype != "ext2" ]]; then
remount noload data=journal norecovery
- not_remount data=journal data=ordered
- not_remount data=journal data=writeback
- not_remount data=ordered data=journal
- not_remount data=ordered data=writeback
- not_remount data=writeback data=journal
- not_remount data=writeback data=ordered
+ mnt_then_not_remount data=journal data=ordered
+ mnt_then_not_remount data=journal data=writeback
+ mnt_then_not_remount data=ordered data=journal
+ mnt_then_not_remount data=ordered data=writeback
+ mnt_then_not_remount data=writeback data=journal
+ mnt_then_not_remount data=writeback data=ordered
fi
do_mkfs -O journal_dev $LOOP_LOGDEV ${LOGSIZE}k
@@ -621,9 +629,9 @@ for fstype in ext2 ext3 ext4; do
mnt defaults
mnt journal_path=$LOOP_LOGDEV ignored
mnt journal_dev=$LOGDEV_DEVNUM ignored
- not_mnt journal_path=${LOOP_LOGDEV}_nonexistent ignored
- not_mnt journal_dev=123456 ignored
- not_mnt journal_dev=999999999999999 ignored
+ not_mnt journal_path=${LOOP_LOGDEV}_nonexistent
+ not_mnt journal_dev=123456
+ not_mnt journal_dev=999999999999999
do_mkfs -E quotatype=prjquota $SCRATCH_DEV ${SIZE}k
mnt prjquota
@@ -636,11 +644,11 @@ for fstype in ext2 ext3 ext4; do
quotacheck -vugm $SCRATCH_MNT >> $seqres.full 2>&1
quotaon -vug $SCRATCH_MNT >> $seqres.full 2>&1
- not_remount_noumount -r grpjquota=
- not_remount_noumount -r usrjquota=aaquota.user
- not_remount_noumount -r grpjquota=aaquota.group
- not_remount_noumount -r jqfmt=vfsv1
- not_remount_noumount -r noquota
+ not_remount grpjquota=
+ not_remount usrjquota=aaquota.user
+ not_remount grpjquota=aaquota.group
+ not_remount jqfmt=vfsv1
+ not_remount noquota
mnt_only remount,usrquota,grpquota ^usrquota,^grpquota
$UMOUNT_PROG $SCRATCH_MNT > /dev/null 2>&1
@@ -654,10 +662,10 @@ for fstype in ext2 ext3 ext4; do
quotacheck -vugm $SCRATCH_MNT >> $seqres.full 2>&1
quotaon -vug $SCRATCH_MNT >> $seqres.full 2>&1
- not_remount_noumount -r noquota
- not_remount_noumount -r usrjquota=aquota.user
- not_remount_noumount -r grpjquota=aquota.group
- not_remount_noumount -r jqfmt=vfsv1
+ not_remount noquota
+ not_remount usrjquota=aquota.user
+ not_remount grpjquota=aquota.group
+ not_remount jqfmt=vfsv1
mnt_only remount,grpjquota= grpquota,^grpjquota
mnt_only remount,usrjquota= usrquota,^usrjquota
mnt_only remount,usrquota,grpquota usrquota,grpquota
@@ -674,9 +682,9 @@ for fstype in ext2 ext3 ext4; do
mnt prjquota
mnt usrquota
mnt grpquota
- not_remount defaults usrjquota=aquota.user
- not_remount defaults grpjquota=aquota.user
- not_remount defaults jqfmt=vfsv1
+ mnt_then_not_remount defaults usrjquota=aquota.user
+ mnt_then_not_remount defaults grpjquota=aquota.user
+ mnt_then_not_remount defaults jqfmt=vfsv1
remount defaults grpjquota=,usrjquota= ignored
done #for fstype in ext2 ext3 ext4; do
base-commit: fbc6486be09c93a68d3863ebf7e3ed851fc4721c
--
2.36.0
On Sat, Apr 30, 2022 at 12:21:30PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[email protected]>
>
> 'not_mnt OPTIONS' seems to have been intended to test that the
> filesystem cannot be mounted at all with the given OPTIONS, meaning that
> the mount fails as opposed to the options being ignored. However, this
> doesn't actually work, as shown by the fact that the test case 'not_mnt
> test_dummy_encryption=v3' is passing in the !CONFIG_FS_ENCRYPTION case.
> Actually ext4 ignores this mount option when !CONFIG_FS_ENCRYPTION.
> (The ext4 behavior might be changed, but that is besides the point.)
>
> The problem is that the do_mnt() helper function is being misused in a
> context where a mount failure is expected, and it does some additional
> remount tests that don't make sense in that context. So if the mount
> unexpectedly succeeds, then one of these later tests can still "fail",
> causing the unexpected success to be shadowed by a later failure, which
> causes the overall test case to pass since it expects a failure.
>
> Fix this by reworking not_mnt() and not_remount_noumount() to use
> simple_mount() in cases where they are expecting a failure. Also fix
> up some of the naming and calling conventions to be less confusing.
> Finally, make sure to test that remounting fails too, not just mounting.
>
> Signed-off-by: Eric Biggers <[email protected]>
> ---
> tests/ext4/053 | 148 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 78 insertions(+), 70 deletions(-)
Lukas, any thoughts on this patch? You're the author of this test.
- Eric
On Mon, May 09, 2022 at 04:42:03PM -0700, Eric Biggers wrote:
> On Sat, Apr 30, 2022 at 12:21:30PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <[email protected]>
> >
> > 'not_mnt OPTIONS' seems to have been intended to test that the
> > filesystem cannot be mounted at all with the given OPTIONS, meaning that
> > the mount fails as opposed to the options being ignored. However, this
> > doesn't actually work, as shown by the fact that the test case 'not_mnt
> > test_dummy_encryption=v3' is passing in the !CONFIG_FS_ENCRYPTION case.
> > Actually ext4 ignores this mount option when !CONFIG_FS_ENCRYPTION.
> > (The ext4 behavior might be changed, but that is besides the point.)
> >
> > The problem is that the do_mnt() helper function is being misused in a
> > context where a mount failure is expected, and it does some additional
> > remount tests that don't make sense in that context. So if the mount
> > unexpectedly succeeds, then one of these later tests can still "fail",
> > causing the unexpected success to be shadowed by a later failure, which
> > causes the overall test case to pass since it expects a failure.
> >
> > Fix this by reworking not_mnt() and not_remount_noumount() to use
> > simple_mount() in cases where they are expecting a failure. Also fix
> > up some of the naming and calling conventions to be less confusing.
> > Finally, make sure to test that remounting fails too, not just mounting.
> >
> > Signed-off-by: Eric Biggers <[email protected]>
> > ---
> > tests/ext4/053 | 148 ++++++++++++++++++++++++++-----------------------
> > 1 file changed, 78 insertions(+), 70 deletions(-)
>
> Lukas, any thoughts on this patch? You're the author of this test.
>
> - Eric
Haven't tested it myself but the change looks fine, thanks.
You can add
Reviewed-by: Lukas Czerner <[email protected]>
On Tue, May 10, 2022 at 11:43:08AM +0200, Lukas Czerner wrote:
> On Mon, May 09, 2022 at 04:42:03PM -0700, Eric Biggers wrote:
> > On Sat, Apr 30, 2022 at 12:21:30PM -0700, Eric Biggers wrote:
> > > From: Eric Biggers <[email protected]>
> > >
> > > 'not_mnt OPTIONS' seems to have been intended to test that the
> > > filesystem cannot be mounted at all with the given OPTIONS, meaning that
> > > the mount fails as opposed to the options being ignored. However, this
> > > doesn't actually work, as shown by the fact that the test case 'not_mnt
> > > test_dummy_encryption=v3' is passing in the !CONFIG_FS_ENCRYPTION case.
> > > Actually ext4 ignores this mount option when !CONFIG_FS_ENCRYPTION.
> > > (The ext4 behavior might be changed, but that is besides the point.)
> > >
> > > The problem is that the do_mnt() helper function is being misused in a
> > > context where a mount failure is expected, and it does some additional
> > > remount tests that don't make sense in that context. So if the mount
> > > unexpectedly succeeds, then one of these later tests can still "fail",
> > > causing the unexpected success to be shadowed by a later failure, which
> > > causes the overall test case to pass since it expects a failure.
> > >
> > > Fix this by reworking not_mnt() and not_remount_noumount() to use
> > > simple_mount() in cases where they are expecting a failure. Also fix
> > > up some of the naming and calling conventions to be less confusing.
> > > Finally, make sure to test that remounting fails too, not just mounting.
> > >
> > > Signed-off-by: Eric Biggers <[email protected]>
> > > ---
> > > tests/ext4/053 | 148 ++++++++++++++++++++++++++-----------------------
> > > 1 file changed, 78 insertions(+), 70 deletions(-)
> >
> > Lukas, any thoughts on this patch? You're the author of this test.
> >
> > - Eric
>
> Haven't tested it myself but the change looks fine, thanks.
Thanks for you help to review this patch. There's an new failure[1] after we
merged this patch:
"SHOULD FAIL remounting ext2 "commit=7" (remount unexpectedly succeeded) FAILED"
As this test generally passed, so before I give "Oops" to others, I hope to
check with you that if this's an expected failure we need to fix in kernel
or in this case itself?
Thanks,
Zorro
>
> You can add
> Reviewed-by: Lukas Czerner <[email protected]>
>
On Tue, May 10, 2022 at 11:43:59PM +0800, Zorro Lang wrote:
> On Tue, May 10, 2022 at 11:43:08AM +0200, Lukas Czerner wrote:
> > On Mon, May 09, 2022 at 04:42:03PM -0700, Eric Biggers wrote:
> > > On Sat, Apr 30, 2022 at 12:21:30PM -0700, Eric Biggers wrote:
> > > > From: Eric Biggers <[email protected]>
> > > >
> > > > 'not_mnt OPTIONS' seems to have been intended to test that the
> > > > filesystem cannot be mounted at all with the given OPTIONS, meaning that
> > > > the mount fails as opposed to the options being ignored. However, this
> > > > doesn't actually work, as shown by the fact that the test case 'not_mnt
> > > > test_dummy_encryption=v3' is passing in the !CONFIG_FS_ENCRYPTION case.
> > > > Actually ext4 ignores this mount option when !CONFIG_FS_ENCRYPTION.
> > > > (The ext4 behavior might be changed, but that is besides the point.)
> > > >
> > > > The problem is that the do_mnt() helper function is being misused in a
> > > > context where a mount failure is expected, and it does some additional
> > > > remount tests that don't make sense in that context. So if the mount
> > > > unexpectedly succeeds, then one of these later tests can still "fail",
> > > > causing the unexpected success to be shadowed by a later failure, which
> > > > causes the overall test case to pass since it expects a failure.
> > > >
> > > > Fix this by reworking not_mnt() and not_remount_noumount() to use
> > > > simple_mount() in cases where they are expecting a failure. Also fix
> > > > up some of the naming and calling conventions to be less confusing.
> > > > Finally, make sure to test that remounting fails too, not just mounting.
> > > >
> > > > Signed-off-by: Eric Biggers <[email protected]>
> > > > ---
> > > > tests/ext4/053 | 148 ++++++++++++++++++++++++++-----------------------
> > > > 1 file changed, 78 insertions(+), 70 deletions(-)
> > >
> > > Lukas, any thoughts on this patch? You're the author of this test.
> > >
> > > - Eric
> >
> > Haven't tested it myself but the change looks fine, thanks.
>
> Thanks for you help to review this patch. There's an new failure[1] after we
> merged this patch:
> "SHOULD FAIL remounting ext2 "commit=7" (remount unexpectedly succeeded) FAILED"
>
> As this test generally passed, so before I give "Oops" to others, I hope to
> check with you that if this's an expected failure we need to fix in kernel
> or in this case itself?
>
This appears to be a kernel bug, so to fix it I've sent the patch
"ext4: reject the 'commit' option on ext2 filesystems"
(https://lore.kernel.org/r/[email protected]).
I didn't notice this earlier because it's not reproducible with
CONFIG_EXT2_FS=y. But it is reproducible with CONFIG_EXT2_FS=n and
CONFIG_EXT4_USE_FOR_EXT2=y.
- Eric
On Tue, May 10, 2022 at 11:36:18AM -0700, Eric Biggers wrote:
> On Tue, May 10, 2022 at 11:43:59PM +0800, Zorro Lang wrote:
> > On Tue, May 10, 2022 at 11:43:08AM +0200, Lukas Czerner wrote:
> > > On Mon, May 09, 2022 at 04:42:03PM -0700, Eric Biggers wrote:
> > > > On Sat, Apr 30, 2022 at 12:21:30PM -0700, Eric Biggers wrote:
> > > > > From: Eric Biggers <[email protected]>
> > > > >
> > > > > 'not_mnt OPTIONS' seems to have been intended to test that the
> > > > > filesystem cannot be mounted at all with the given OPTIONS, meaning that
> > > > > the mount fails as opposed to the options being ignored. However, this
> > > > > doesn't actually work, as shown by the fact that the test case 'not_mnt
> > > > > test_dummy_encryption=v3' is passing in the !CONFIG_FS_ENCRYPTION case.
> > > > > Actually ext4 ignores this mount option when !CONFIG_FS_ENCRYPTION.
> > > > > (The ext4 behavior might be changed, but that is besides the point.)
> > > > >
> > > > > The problem is that the do_mnt() helper function is being misused in a
> > > > > context where a mount failure is expected, and it does some additional
> > > > > remount tests that don't make sense in that context. So if the mount
> > > > > unexpectedly succeeds, then one of these later tests can still "fail",
> > > > > causing the unexpected success to be shadowed by a later failure, which
> > > > > causes the overall test case to pass since it expects a failure.
> > > > >
> > > > > Fix this by reworking not_mnt() and not_remount_noumount() to use
> > > > > simple_mount() in cases where they are expecting a failure. Also fix
> > > > > up some of the naming and calling conventions to be less confusing.
> > > > > Finally, make sure to test that remounting fails too, not just mounting.
> > > > >
> > > > > Signed-off-by: Eric Biggers <[email protected]>
> > > > > ---
> > > > > tests/ext4/053 | 148 ++++++++++++++++++++++++++-----------------------
> > > > > 1 file changed, 78 insertions(+), 70 deletions(-)
> > > >
> > > > Lukas, any thoughts on this patch? You're the author of this test.
> > > >
> > > > - Eric
> > >
> > > Haven't tested it myself but the change looks fine, thanks.
> >
> > Thanks for you help to review this patch. There's an new failure[1] after we
> > merged this patch:
> > "SHOULD FAIL remounting ext2 "commit=7" (remount unexpectedly succeeded) FAILED"
> >
> > As this test generally passed, so before I give "Oops" to others, I hope to
> > check with you that if this's an expected failure we need to fix in kernel
> > or in this case itself?
> >
>
> This appears to be a kernel bug, so to fix it I've sent the patch
> "ext4: reject the 'commit' option on ext2 filesystems"
> (https://lore.kernel.org/r/[email protected]).
Thanks, great to know that.
>
> I didn't notice this earlier because it's not reproducible with
> CONFIG_EXT2_FS=y. But it is reproducible with CONFIG_EXT2_FS=n and
> CONFIG_EXT4_USE_FOR_EXT2=y.
Yes, I tested with this config. I'll remind this failure in next fstests
announcement, Thanks a lot!
Thanks,
Zorro
>
> - Eric
>