Re: [PATCH v2 06/34] nilfs2: don't set SB_RDONLY after filesystem errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nilfs2: nilfs_reconfigure() and nilfs_get_tree() bypass sb_emergency_ro()
--

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

nilfs2: don't set SB_RDONLY after filesystem errors

This commit replaces setting SB_RDONLY in __nilfs_error() with
sb_set_emergency_ro() and introduces nilfs_sb_rdonly() to guard write paths
after filesystem errors without taking s_umount.

Link: https://lore.kernel.org/r/20260903060855.4610-1-kmehltretter@gmail.com

> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index 2e3b591c117c9..edb7d379ecbdf 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -137,12 +137,13 @@ void __nilfs_error(struct super_block *sb, const char *function,
>  
>  	va_end(args);
>  
> -	if (!sb_rdonly(sb)) {
> +	if (!nilfs_sb_rdonly(sb)) {
>  		nilfs_set_error(sb);
>  
>  		if (nilfs_test_opt(nilfs, ERRORS_RO)) {
>  			printk(KERN_CRIT "Remounting filesystem read-only\n");
> -			sb->s_flags |= SB_RDONLY;
> +			/* SB_RDONLY needs s_umount, see sb_set_emergency_ro(). */
> +			sb_set_emergency_ro(sb);
>  		}
>  	}

[ ... ]

> @@ -1141,6 +1146,10 @@ static int nilfs_reconfigure(struct fs_context *fc)
>  	if (fc->sb_flags & SB_RDONLY) {
>  		sb->s_flags |= SB_RDONLY;
>  
> +		/* After an error nothing may be written, just go read-only. */
> +		if (sb_emergency_ro(sb))
> +			goto out;
> +
>  		/*
>  		 * Remounting a valid RW partition RDONLY, so set
>  		 * the RDONLY flag and then mark the partition as valid again.
> @@ -1152,6 +1161,12 @@ static int nilfs_reconfigure(struct fs_context *fc)
>  		__u64 features;
>  		struct nilfs_root *root;
>  
> +		/* The VFS doesn't refuse this when nilfs_get_tree() calls us. */
> +		if (sb_emergency_ro(sb)) {
> +			err = -EROFS;
> +			goto ignore_opts;
> +		}
> +

[Severity: Medium]
Can this sb_emergency_ro(sb) check in the else branch of nilfs_reconfigure()
be bypassed after an error on a read-write mount?

When __nilfs_error() calls sb_set_emergency_ro(sb) on a read-write mount,
SB_RDONLY is not set in sb->s_flags, so sb_rdonly(sb) remains false.

In nilfs_reconfigure(), right before the if (fc->sb_flags & SB_RDONLY) block:

	if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
		goto out;

When a read-write mount or remount is requested (!(fc->sb_flags & SB_RDONLY))
on a superblock where sb_emergency_ro(sb) is true and sb_rdonly(sb) is still
false (for example, via nilfs_get_tree() -> nilfs_reconfigure() when the
superblock was kept alive by a snapshot after the corrupted read-write mount
was unmounted, or via reconfigure_super() -> nilfs_reconfigure() on a direct
remount,rw without an intermediate remount,ro), this comparison evaluates
false == false and jumps straight to out, overwriting nilfs->ns_mount_opt and
returning 0 instead of -EROFS.

Also, in nilfs_get_tree(), when nilfs_tree_is_busy(s->s_root) is true:

	} else if (!ctx->cno) {
		if (nilfs_tree_is_busy(s->s_root)) {
			if ((fc->sb_flags ^ s->s_flags) & SB_RDONLY) {
				nilfs_err(s,
					  "the device already has a %s mount.",
					  sb_rdonly(s) ? "read-only" : "read/write");
				err = -EBUSY;
				goto failed_super;
			}

Because s->s_flags does not have SB_RDONLY set after sb_set_emergency_ro(s),
doesn't this treat the emergency read-only superblock as read/write, rejecting
a secondary read-only mount with -EBUSY while allowing a secondary read-write
mount?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-work-fs-sb_rdonly-v2-0-2f44f64f0b66@kernel.org?part=6