| Message ID | 20240805201241.27286-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: don't set SB_RDONLY after filesystem errors | expand |
On Mon, Aug 05, 2024 at 10:12:41PM GMT, 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> > --- Thank you! Reviewed-by: Christian Brauner <brauner@kernel.org>
On Mon 05-08-24 22:12:41, 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. OK, I have noticed that recent versions of fstests have the check already improved and so generic/459 passes with these changes. Honza > > 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; > } > > static void update_super_work(struct work_struct *work) > -- > 2.35.3 >
On Mon, 05 Aug 2024 22:12:41 +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. > > [...] Applied, thanks! [1/1] ext4: don't set SB_RDONLY after filesystem errors commit: d3476f3dad4ad68ae5f6b008ea6591d1520da5d8 Best regards,
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. I'm seeing LTP's fanotify22 failing for a about week (roughly since commit de5cb0dcb74c) on: fanotify22.c:59: TINFO: Mounting /dev/loop0 to /tmp/LTP_fanqgL299/test_mnt fstyp=ext4 flags=21 fanotify22.c:59: TBROK: mount(/dev/loop0, test_mnt, ext4, 33, 0x4211ed) failed: EROFS (30) static void trigger_fs_abort(void) { SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, MS_REMOUNT|MS_RDONLY, "abort"); } Thanks, Jan
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. 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
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? Thanks, Amir. --- a/testcases/kernel/syscalls/fanotify/fanotify22.c +++ b/testcases/kernel/syscalls/fanotify/fanotify22.c @@ -57,7 +57,7 @@ static struct fanotify_fid_t bad_link_fid; static void trigger_fs_abort(void) { SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, - MS_REMOUNT|MS_RDONLY, "abort"); + MS_REMOUNT, "abort"); }
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) > > Thanks, > Amir. > > --- a/testcases/kernel/syscalls/fanotify/fanotify22.c > +++ b/testcases/kernel/syscalls/fanotify/fanotify22.c > @@ -57,7 +57,7 @@ static struct fanotify_fid_t bad_link_fid; > static void trigger_fs_abort(void) > { > SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, > - MS_REMOUNT|MS_RDONLY, "abort"); > + MS_REMOUNT, "abort"); > } >
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. So you can either ignore the return value of mount() or assert that it can either succeed or get EROFS for catching unexpected errors. Thanks, Amir.
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
Amir Goldstein <amir73il@gmail.com> writes: > 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. If I recall correctly, no, there is no need for the MS_RDONLY. We only care about getting the event to test FS_ERROR. Thanks, > > Any objection for LTP maintainers to apply this simple test fix? > > Thanks, > Amir. > > --- a/testcases/kernel/syscalls/fanotify/fanotify22.c > +++ b/testcases/kernel/syscalls/fanotify/fanotify22.c > @@ -57,7 +57,7 @@ static struct fanotify_fid_t bad_link_fid; > static void trigger_fs_abort(void) > { > SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, > - MS_REMOUNT|MS_RDONLY, "abort"); > + MS_REMOUNT, "abort"); > }
On Fri, Oct 04, 2024 at 03:33:56PM -0400, Gabriel Krisman Bertazi wrote: > > 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. > > If I recall correctly, no, there is no need for the MS_RDONLY. We only > care about getting the event to test FS_ERROR. Looking at ltp/testcases/kernel/syscalls/fanotify/fanotify22.c this appears to be true. I'll note though that this comment in fanotify22.c is a bit misleading: /* Unmount and mount the filesystem to get it out of the error state */ SAFE_UMOUNT(MOUNT_PATH); SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, 0, NULL); Once ext4_error() gets called, the file system will be marked as containing errors. This can be seen when you mount the file system, and when you run fsck -p: root@kvm-xfstests:~# mount /dev/vdc /vdc [ 1893.569015] EXT4-fs (vdc): mounted filesystem 9bf1a2df-fc1c-4b46-a8e8-c9e3e9bc7a26 r/w with ordered data mode. Quota mode: none. root@kvm-xfstests:~# echo "testing 123" > /sys/fs/ext4/vdc/trigger_fs_error [ 1907.268249] EXT4-fs error (device vdc): trigger_test_error:129: comm bash: testing 123 root@kvm-xfstests:~# umount /vdc [ 1919.722934] EXT4-fs (vdc): unmounting filesystem 9bf1a2df-fc1c-4b46-a8e8-c9e3e9bc7a26. root@kvm-xfstests:~# mount /dev/vdc /vdc [ 1923.270852] EXT4-fs (vdc): warning: mounting fs with errors, running e2fsck is recommended [ 1923.297998] EXT4-fs (vdc): mounted filesystem 9bf1a2df-fc1c-4b46-a8e8-c9e3e9bc7a26 r/w with ordered data mode. Quota mode: none. root@kvm-xfstests:~# umount /vdc [ 1925.889086] EXT4-fs (vdc): unmounting filesystem 9bf1a2df-fc1c-4b46-a8e8-c9e3e9bc7a26. root@kvm-xfstests:~# fsck -p /dev/vdc fsck from util-linux 2.38.1 /dev/vdc contains a file system with errors, check forced. /dev/vdc: 12/327680 files (0.0% non-contiguous), 42398/1310720 blocks Unmounting and remounting the file system is not going to clear file system's error state. You have to clear the EXT2_ERROR_FS state, and if you want to prevent fsck.ext4 -p from running, you'll need to set the EXT2_VALID_FS bit, via 'debugfs -w -R "ssv state 1" /dev/vdc' or you could just run 'fsck.ext4 -p /dev/vdc'. Also note that way I generally recommend that people simulate triggering a file system error is via a command like this: echo "test error description" > /sys/fs/ext4/vdc/trigger_fs_error To be honest, I had completely forgotten that the abort mount option exists at all, and if I didn't know that ltp was using it, I'd be inclined to deprecate and remove it.... - Ted
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; } static void update_super_work(struct work_struct *work)
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.