Re: [PATCH v3 05/33] fs: add a generic emergency read-only state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] vfs: change_mount_ro_state() skips mnt_make_readonly() on
  sb_emergency_ro()
- [Low] quota: do_quotactl() misses sb_emergency_ro() in Q_XQUOTASYNC
  and Q_SYNC
--

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

fs: add a generic emergency read-only state

This commit adds an SB_STATE_EMERGENCY_RO superblock state flag and
updates VFS paths to reject modifications with -EROFS when a filesystem
switches to emergency read-only mode without setting SB_RDONLY.

> diff --git a/fs/namespace.c b/fs/namespace.c
> index 9f6ec35c7b9cd..55fa09818ef90 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -396,7 +396,7 @@ static struct mount *alloc_vfsmnt(const char *name)
>   */
>  bool __mnt_is_readonly(const struct vfsmount *mnt)
>  {
> -	return (mnt->mnt_flags & MNT_READONLY) || sb_rdonly(mnt->mnt_sb);
> +	return (mnt->mnt_flags & MNT_READONLY) || sb_rdonly_any(mnt->mnt_sb);
>  }
>  EXPORT_SYMBOL_GPL(__mnt_is_readonly);

[Severity: Medium]
Does changing __mnt_is_readonly() to check sb_rdonly_any() cause
change_mount_ro_state() to skip mnt_make_readonly() when sb_emergency_ro()
is set?

fs/namespace.c:change_mount_ro_state() {
	bool readonly_request = (mnt_flags & MNT_READONLY);

	if (readonly_request == __mnt_is_readonly(&mnt->mnt))
		return 0;

	if (readonly_request)
		return mnt_make_readonly(mnt);
    ...
}

When a read-write mount has open writable files or active writers and the
filesystem enters SB_STATE_EMERGENCY_RO, a mount(2) call with
MS_REMOUNT | MS_BIND | MS_RDONLY (do_reconfigure_mnt()) sees
__mnt_is_readonly() == true, skips mnt_make_readonly(), and sets
MNT_READONLY via set_mount_attributes() without calling
mnt_hold_writers().  If reconfigure_super() is later called to remount the
superblock read-only, sb_prepare_remount_readonly() skips mounts that
already have MNT_READONLY set and transitions the superblock to SB_RDONLY
while active writers remain on that mount.

[ ... ]

> diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> index 33bacd7075800..b488fb1f9cc44 100644
> --- a/fs/quota/quota.c
> +++ b/fs/quota/quota.c
> @@ -783,6 +783,19 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
>  	if (ret < 0)
>  		return ret;
>  
> +	/* An error made the filesystem read-only. */
> +	if (sb_emergency_ro(sb)) {
> +		switch (cmd) {
> +		case Q_QUOTAON:
> +		case Q_SETINFO:
> +		case Q_SETQUOTA:
> +		case Q_XQUOTAON:
> +		case Q_XQUOTARM:
> +		case Q_XSETQLIM:
> +			return -EROFS;
> +		}
> +	}
> +
>  	switch (cmd) {
>  	case Q_QUOTAON:
>  		return quota_quotaon(sb, type, id, path);

[Severity: Low]
Should Q_XQUOTASYNC and Q_SYNC also be rejected here when
sb_emergency_ro(sb) is set (and quota_sync_one() guarded by
!sb_rdonly_any(sb))?

In do_quotactl(), Q_XQUOTASYNC still checks sb_rdonly(sb) instead of
sb_rdonly_any(sb), returning 0 rather than -EROFS in emergency read-only
mode:

fs/quota/quota.c:do_quotactl() {
    ...
	case Q_XQUOTASYNC:
		if (sb_rdonly(sb))
			return -EROFS;
		/* XFS quotas are fully coherent now, making this call a noop */
		return 0;
    ...
}

Q_SYNC and quota_sync_one() also call ->quota_sync() directly, relying on
dquot_quota_sync() rather than do_quotactl() to reject emergency read-only
superblocks.

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