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

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

[Jan Kara]

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;
 }
 
 static void update_super_work(struct work_struct *work)
-- 
2.35.3

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

[Christian Brauner]

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>

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

[Jan Kara]

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Theodore Ts'o]

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,
-- 
Theodore Ts'o <tytso@mit.edu>

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

[Jan Stancek]

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

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

[Jan Kara]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Amir Goldstein]

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");
 }

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

[Jan Stancek]

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");
>  }
>

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

[Amir Goldstein]

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.

Re: [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

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

[Gabriel Krisman Bertazi]

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");
>  }

-- 
Gabriel Krisman Bertazi

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

[Theodore Ts'o]

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