[LTP] [PATCH] ext4: don't set SB_RDONLY after filesystem errors

[Jan Kara]

On Fri 04-10-24 15:28:12, Amir Goldstein wrote:
> On Fri, Oct 4, 2024 at 2:50 PM Jan Stancek <jstancek@redhat.com> wrote:
> >
> > On Fri, Oct 4, 2024 at 2:32 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Mon, Sep 30, 2024 at 1:34 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > 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@kernel.org>
> > > > > > Signed-off-by: Jan Kara <jack@suse.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.
> > >
> > > Regardless of what is right or wrong to do in ext4, I don't think that the test
> > > really cares about remount read-only.
> > > I don't see anything in the test that requires it. Gabriel?
> > > If I remove MS_RDONLY from the test it works just fine.
> > >
> > > Any objection for LTP maintainers to apply this simple test fix?
> >
> > Does that change work for you on older kernels? On 6.11 I get EROFS:
> >
> > fanotify22.c:59: TINFO: Mounting /dev/loop0 to
> > /tmp/LTP_fangb5wuO/test_mnt fstyp=ext4 flags=20
> > fanotify22.c:59: TBROK: mount(/dev/loop0, test_mnt, ext4, 32,
> > 0x4211ed) failed: EROFS (30)
> >
> 
> Yeh me too, but if you change s/SAFE_MOUNT/mount
> the test works just fine on 6.11 and 6.12-rc1 with or without MS_RDONLY.
> The point of trigger_fs_abort() is to trigger the FS_ERROR event and it
> does not matter whether remount succeeds or not for that matter at all.

Well, the handling of 'abort' option is suboptimal. It gets acted on in the
middle of the remount process so some mount options get applied before it (and can
fail with error which would make 'abort' ignored), some get applied after
it which can fail because the fs is already shutdown.

> So you can either ignore the return value of mount() or assert that it
> can either succeed or get EROFS for catching unexpected errors.

Either that or I'm currently testing ext4 fix which will make 'abort'
handling more consistent.

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