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@xxxxxxxxxx > > Reported-by: Christian Brauner <brauner@xxxxxxxxxx> > > Signed-off-by: Jan Kara <jack@xxxxxxx> > > --- > > 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@xxxxxxxx> SUSE Labs, CR
