linux-ext4 - Re: [PATCH] ext4: don't set SB_RDONLY after filesystem errors

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]

Message-ID: <20240930113434.hhkro4bofhvapwm7@quack3>
Date: Mon, 30 Sep 2024 13:34:34 +0200
From: Jan Kara <jack@...e.cz>
To: Jan Stancek <jstancek@...hat.com>
Cc: Jan Kara <jack@...e.cz>, ltp@...ts.linux.it, Ted Tso <tytso@....edu>,
	linux-ext4@...r.kernel.org, Christian Brauner <brauner@...nel.org>,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] ext4: don't set SB_RDONLY after filesystem errors

On Mon 30-09-24 12:15:11, Jan Stancek wrote:
> On Mon, Aug 05, 2024 at 10:12:41PM +0200, Jan Kara wrote:
> > When the filesystem is mounted with errors=remount-ro, we were setting
> > SB_RDONLY flag to stop all filesystem modifications. We knew this misses
> > proper locking (sb->s_umount) and does not go through proper filesystem
> > remount procedure but it has been the way this worked since early ext2
> > days and it was good enough for catastrophic situation damage
> > mitigation. Recently, syzbot has found a way (see link) to trigger
> > warnings in filesystem freezing because the code got confused by
> > SB_RDONLY changing under its hands. Since these days we set
> > EXT4_FLAGS_SHUTDOWN on the superblock which is enough to stop all
> > filesystem modifications, modifying SB_RDONLY shouldn't be needed. So
> > stop doing that.
> > 
> > Link: https://lore.kernel.org/all/000000000000b90a8e061e21d12f@google.com
> > Reported-by: Christian Brauner <brauner@...nel.org>
> > Signed-off-by: Jan Kara <jack@...e.cz>
> > ---
> > fs/ext4/super.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > Note that this patch introduces fstests failure with generic/459 test because
> > it assumes that either freezing succeeds or 'ro' is among mount options. But
> > we fail the freeze with EFSCORRUPTED. This needs fixing in the test but at this
> > point I'm not sure how exactly.
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index e72145c4ae5a..93c016b186c0 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -735,11 +735,12 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
> > 
> > 	ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
> > 	/*
> > -	 * Make sure updated value of ->s_mount_flags will be visible before
> > -	 * ->s_flags update
> > +	 * EXT4_FLAGS_SHUTDOWN was set which stops all filesystem
> > +	 * modifications. We don't set SB_RDONLY because that requires
> > +	 * sb->s_umount semaphore and setting it without proper remount
> > +	 * procedure is confusing code such as freeze_super() leading to
> > +	 * deadlocks and other problems.
> > 	 */
> > -	smp_wmb();
> > -	sb->s_flags |= SB_RDONLY;
> 
> Hi,
> 
> shouldn't the SB_RDONLY still be set (in __ext4_remount()) for the case
> when user triggers the abort with mount(.., "abort")? Because now we seem
> to always hit the condition that returns EROFS to user-space.

Thanks for report! I agree returning EROFS from the mount although
'aborting' succeeded is confusing and is mostly an unintended side effect
that after aborting the fs further changes to mount state are forbidden but
the testcase additionally wants to remount the fs read-only.

I don't think forcibly setting SB_RDONLY in this case is good because it
would still bypass all VFS checks for read-only remount and thus possibly
causing undesirable surprises (similar to those d3476f3dad4a ("ext4: don't
set SB_RDONLY after filesystem errors") tried to fix). At this point I'm
undecided whether __ext4_remount() should just return success after
aborting the filesystem, ignoring all other mount options, or whether we
should follow more the old behavior where we still reflect other mount
options (mostly resulting in cleanup of lazy itable initialization thread,
mmpd thread and similar).  I'm more leaning towards option 1) but I could
be convinced otherwise. Ted, what do you think?

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR