Re: [PATCH v3 17/33] ufs: don't set SB_RDONLY after filesystem errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ufs: cylinder group leak and SB_RDONLY clear in
  ufs_reconfigure()
--

commit eaf412f5390fa6f1c30001040b118660141a9bd6
Author: Christian Brauner <brauner@kernel.org>

ufs: don't set SB_RDONLY after filesystem errors

This commit switches ufs_error() and ufs_panic() from setting SB_RDONLY to
calling sb_set_emergency_ro(), and keeps cylinder group structures allocated
across read-only remounts after an error so inode eviction can still use
them.

> diff --git a/fs/ufs/super.c b/fs/ufs/super.c
> index 3569ac92b065b..d82cf73360a79 100644
> --- a/fs/ufs/super.c
> +++ b/fs/ufs/super.c

[ ... ]

> @@ -1265,14 +1281,20 @@ static int ufs_reconfigure(struct fs_context *fc)
>  	 * fs was mouted as rw, remounting ro
>  	 */
>  	if (fc->sb_flags & SB_RDONLY) {
> -		ufs_put_super_internal(sb);
> -		usb1->fs_time = ufs_get_seconds(sb);
> -		if ((flags & UFS_ST_MASK) == UFS_ST_SUN
> -		  || (flags & UFS_ST_MASK) == UFS_ST_SUNOS
> -		  || (flags & UFS_ST_MASK) == UFS_ST_SUNx86) 
> -			ufs_set_fs_state(sb, usb1, usb3,
> -				UFS_FSOK - fs32_to_cpu(sb, usb1->fs_time));
> -		ubh_mark_buffer_dirty (USPI_UBH(uspi));
> +		/*
> +		 * After an error leave the superblock alone and keep the
> +		 * cylinder groups, evict still needs them.
> +		 */
> +		if (!sb_emergency_ro(sb)) {
> +			ufs_put_super_internal(sb);
> +			usb1->fs_time = ufs_get_seconds(sb);
> +			if ((flags & UFS_ST_MASK) == UFS_ST_SUN
> +			  || (flags & UFS_ST_MASK) == UFS_ST_SUNOS
> +			  || (flags & UFS_ST_MASK) == UFS_ST_SUNx86)
> +				ufs_set_fs_state(sb, usb1, usb3,
> +					UFS_FSOK - fs32_to_cpu(sb, usb1->fs_time));
> +			ubh_mark_buffer_dirty (USPI_UBH(uspi));
> +		}
>  		sb->s_flags |= SB_RDONLY;
>  	} else {
>  	/*

[Severity: Medium]
Can reconfiguring via fspick() after an error and a read-only remount
leak the retained cylinder group structures and clear SB_RDONLY?

When a read-write mount hits an error and is then remounted read-only,
this branch skips ufs_put_super_internal() so sbi->s_csp, sbi->s_ucg, and
sbi->s_ucpi stay allocated while SB_RDONLY is set on sb->s_flags.

If userspace then reconfigures the mount via fspick() and fsconfig()
without specifying ro or rw (for example, changing onerror=), fc->sb_flags
and fc->sb_flags_mask are both 0.  reconfigure_super() only rejects
sb_emergency_ro(sb) when SB_RDONLY is in fc->sb_flags_mask, and
ufs_reconfigure() checks (bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb)
without consulting fc->sb_flags_mask.  Execution then falls into the else
branch, where ufs_read_cylinder_structures() overwrites the existing
sbi->s_csp, sbi->s_ucg, and sbi->s_ucpi pointers (leaking memory and
buffer_heads) and clears SB_RDONLY from sb->s_flags.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-work-fs-sb_rdonly-v3-0-0be92bb73709@kernel.org?part=17