[PATCH v2 04/34] ext2: don't set SB_RDONLY after filesystem errors

[Jan Kara]

On Thu 29-10-26 17:06:55, Christian Brauner wrote:
> With errors=remount-ro ext2_error() sets SB_RDONLY from whatever context
> hit the error and without holding s_umount. That races with everyone
> else changing sb->s_flags and it breaks freezing. If it happens while
> the filesystem is frozen thaw_super() sees a read-only superblock and
> leaves the s_writers rwsems locked so every writer hangs. A lookup in a
> corrupted directory is enough. Karl proposed teaching thaw_super() about
> it but Jan preferred fixing the filesystems (cf. [1]) like ext4 and f2fs
> already did.
> 
> So call sb_set_emergency_ro() instead and let the VFS refuse
> modifications. ext2 still refuses allocations, ext2_write_inode(),
> ext2_sync_fs() and ext2_xattr_set() itself. The superblock isn't written
> after the error is recorded and unlinked inodes are left to e2fsck.
> ext2_handle_dirsync() treats EROFS as success in that state because the
> entry was already changed.
> 
> A remount to read-write fails until umount and writes through file
> descriptors opened before the error fail with EROFS.
> 
> [1]: https://lore.kernel.org/r/hyhuckyohsabx2suvq3z7uy3pbgmihva6wgzxr3op7pzcrlu33@p7zzlgp2ehjf
> 
> Reported-by: Karl Mehltretter <kmehltretter at gmail.com>
> Link: https://lore.kernel.org/r/20260903060855.4610-1-kmehltretter@gmail.com
> Signed-off-by: Christian Brauner (Amutable) <brauner at kernel.org>

Nothing critical but couple of points for discussion:

> @@ -1287,6 +1287,10 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
>  	struct ext2_sb_info *sbi = EXT2_SB(sb);
>  	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
>  
> +	/* Nothing may be written after an error made us read-only. */
> +	if (sb_emergency_ro(sb))
> +		return 0;
> +

So shouldn't we just modify the sb_rdonly() check in sync_filesystem() to
use also sb_emergency_ro() and don't bother with modifying individual
filesystems?

> @@ -1307,6 +1311,9 @@ static int ext2_freeze(struct super_block *sb)
>  {
>  	struct ext2_sb_info *sbi = EXT2_SB(sb);
>  
> +	if (sb_emergency_ro(sb))
> +		return 0;
> +

Also if we have the check in sync_filesystem(), then freeze_super() is
generally going to fail so this check could trigger only if
sb_emergency_ro() was set after sync_filesystem() ran and before we called
->freeze_fs(). Since sb_emergency_ro() checking is pretty much guaranteed
to be racy (it is just a best effort framework to limit damage), we
probably don't have to bother with checking in ->freeze_fs() handlers.

> @@ -1349,7 +1356,9 @@ static int ext2_reconfigure(struct fs_context *fc)
>  	int flags = fc->sb_flags;
>  	int err;
>  
> -	sync_filesystem(sb);
> +	/* Nothing may be written after an error made us read-only. */
> +	if (!sb_emergency_ro(sb))
> +		sync_filesystem(sb);

If there's a check in sync_filesystem(), this change can go away.

> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 8f608930a48c..12dfffff189c 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -429,6 +429,8 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
>  	ea_idebug(inode, "name=%d.%s, value=%p, value_len=%ld",
>  		  name_index, name, value, (long)value_len);
>  
> +	if (sb_emergency_ro(sb))
> +		return -EROFS;

Is this really needed? All the xattr operations go through mnt_want_write()
/ mnt_want_write_file() so this should be caught in VFS...

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