linux-ext4 - Re: [xfstests PATCH] ext4/053: fix the rejected mount option testing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 May 2022 23:43:59 +0800
From:   Zorro Lang <zlang@...hat.com>
To:     Lukas Czerner <lczerner@...hat.com>
Cc:     Eric Biggers <ebiggers@...nel.org>, fstests@...r.kernel.org,
        linux-ext4@...r.kernel.org
Subject: Re: [xfstests PATCH] ext4/053: fix the rejected mount option testing

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 <ebiggers@...gle.com>
> > > 
> > > '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 <ebiggers@...gle.com>
> > > ---
> > >  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 <lczerner@...hat.com>
>