linux-ext4 - Re: [PATCH] generic/459: don't run on non-journaled ext4 file systems

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] [day] [month] [year] [list]

Message-ID: <20240205163236.GH6188@frogsfrogsfrogs>
Date: Mon, 5 Feb 2024 08:32:36 -0800
From: "Darrick J. Wong" <djwong@...nel.org>
To: Eric Whitney <enwlinux@...il.com>
Cc: Zorro Lang <zlang@...hat.com>, fstests@...r.kernel.org,
	linux-ext4@...r.kernel.org, tytso@....edu
Subject: Re: [PATCH] generic/459: don't run on non-journaled ext4 file systems

On Fri, Feb 02, 2024 at 02:06:15PM -0500, Eric Whitney wrote:
> * Zorro Lang <zlang@...hat.com>:
> > On Wed, Jan 24, 2024 at 02:53:06PM -0500, Eric Whitney wrote:
> > > generic/459 fails when run on an ext4 file system created without a
> > > journal or when its journal has not been loaded at mount time.
> > > 
> > > The test expects that a file system that it has been unable to freeze
> > > will be automatically remounted read only.  However, the default error
> > > handling policy for ext4 is to continue when possible after errors.
> > > 
> > > A workaround was added to the test in the past to force ext4 to
> > > perform a read only remount in order to meet the test's expectations.
> > > The touch command was used to create a new file after a freeze failure.
> > > This forces ext4 to start a new journal transaction, where it discovers
> > > the journal has previously aborted due to lack of space, and triggers
> > > special case error handling that results in a read only remount.
> > > 
> > > The workaround requires a journal.  Since ext4 is behaving as designed,
> > > prevent the test from running if there isn't one.
> > > 
> > > Signed-off-by: Eric Whitney <enwlinux@...il.com>
> > > ---
> > >  tests/generic/459 | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/tests/generic/459 b/tests/generic/459
> > > index c3f0b2b0..63fbbc9b 100755
> > > --- a/tests/generic/459
> > > +++ b/tests/generic/459
> > > @@ -49,6 +49,11 @@ _require_command "$THIN_CHECK_PROG" thin_check
> > >  _require_freeze
> > >  _require_odirect
> > >  
> > > +# non-journaled ext4 won't remount read only after freeze failure
> > > +if [ "$FSTYP" == "ext4" ]; then
> > > +	_require_metadata_journaling
> > 
> > I'm wondering ... won't other fs need this, besides ext4?
> 
> Thanks for looking at this patch.
> 
> I'd been mainly concerned with ext4, but since you mention it, ext2 and ext3
> ought to be added as well, since the failure behavior is similar.
> 
> The check for metadata journaling is limited to just ext4 here because the
> commit history for this test appears to suggest the touch command workaround
> is specific to ext4.  Perhaps the test should unconditionally require metadata
> journaling for all file system types, but given that I don't work with many
> other file systems I was uncomfortable asserting that in this patch.  If
> freezing any file system requires a metadata journal, then it would make
> sense to do that.
> 
> Your thoughts?

ext2 never had journalling, so requiring metadata journalling
effectively _notruns this test.

I don't think there's any harm done if you shortcut
"if [[ $FSTYP == ext* ]];..." to stop this test on ext2 and ext3-nojournal
as well.

That said, the exact behavior of filesystems during freeze is undefined,
so I think the only option is whackamole _notrun-listing. :/

--D

> Eric
> 
> > 
> > > +fi
> > > +
> > >  vgname=vg_$seq
> > >  lvname=lv_$seq
> > >  poolname=pool_$seq
> > > -- 
> > > 2.30.2
> > > 
> > > 
> > 
>