[PATCH v2 06/34] nilfs2: don't set SB_RDONLY after filesystem errors
Ryusuke Konishi
konishi.ryusuke at gmail.com
Tue Nov 3 09:32:10 PST 2026
On Sat, Oct 31, 2026 at 1:09 AM <sashiko-bot at kernel.org> wrote:
>
> 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 at 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.
I have reviewed the related code paths across all patterns regarding
Sashiko's concerns. Regarding this first point, there is no need for
concern.
Skip check in nilfs_reconfigure():
if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
goto out;
Skipping this block when the requested state matches the current state
is valid because no RO/RW transition is being requested. In particular,
with the workaround patch ("super: keep the read-only state if a
reconfiguration doesn't change it") queued in the vfs-7.5.super branch,
reconfigure_super() ensures that fc->sb_flags inherits SB_RDONLY bit
from sb->s_flags even when "rw" or "ro" option is not specified.
When called via nilfs_get_tree() -> nilfs_reconfigure():
- If only a read-only snapshot mount exists and a read-only current
mount is added, skipping via this check is correct.
- If a read-write current mount is requested instead, this skip check
is bypassed (since sb_rdonly(s) is true for snapshot mounts while
fc->sb_flags does not have SB_RDONLY), and the downstream
sb_emergency_ro() check safely rejects it with -EROFS if the
superblock has entered the emergency RO state.
> 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's point here has a valid aspect.
This inconsistency can be resolved by inserting the following handling
logic if sb_emergency_ro(s) is true:
} else if (!ctx->cno) {
if (nilfs_tree_is_busy(s->s_root)) {
if (sb_emergency_ro(s)) {
if (!(fc->sb_flags & SB_RDONLY)) {
err = -EROFS;
goto failed_super;
} else {
s->s_flags |= SB_RDONLY;
}
} else 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;
}
} else {
…
}
}
This makes all transitions behave consistently under emergency RO:
* RW (degraded) --- RW mount request --> Rejected with -EROFS
|
`- RO mount request --> RO (degraded)
* RO (degraded) --- RW mount request --> Rejected with -EROFS
|
`- RO mount request --> RO (degraded)
* Snapshot-only RO --- RW mount request --> Rejected with -EROFS
(degraded) | [Blocked by emergency_ro check
| in nilfs_reconfigure()]
`- RO mount request --> RO (degraded)
[Reused via nilfs_reconfigure()]
The current v2 patch is safe as-is since write paths are fully
protected by nilfs_sb_rdonly()
even if a degraded RW-mount is reused, but including this change
handles secondary
mounts under emergency RO much more cleanly.
Thanks,
Ryusuke Konishi
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261029-work-fs-sb_rdonly-v2-0-2f44f64f0b66@kernel.org?part=6
More information about the linux-mtd
mailing list