[PATCH RFC 00/28] fs: don't set SB_RDONLY after filesystem errors

[Jan Kara]

On Mon 12-10-26 12:20:45, Christian Brauner wrote:
> A lot of filesystems handle errors by setting SB_RDONLY on the
> superblock. They do that from whatever context found the error and
> without holding s_umount. That's never been okay. SB_RDONLY is supposed
> to only change under s_umount through a proper remount.
> 
> And it breaks freezing. freeze_super() and thaw_super() both look at
> sb_rdonly() to figure out whether the s_writers rwsems need to be taken
> or released. So if an error sets SB_RDONLY while the filesystem is
> frozen thaw_super() takes the read-only shortcut, leaves the rwsems
> write-locked and marks the superblock unfrozen. Every writer after that
> hangs in sb_start_write() and umount warns when it frees the locked
> rwsems. jfs even oopses on umount. And the unlocked update of
> sb->s_flags races with everyone else who writes it.
> 
> Karl noticed and proposed teaching thaw_super() to remember whether the
> freeze took the rwsems (cf. [1]). But a filesystem changing SB_RDONLY
> behind the VFS' back causes more problems than just confused freezing.
> Jan preferred fixing the filesystems instead (cf. [2]) and I agree. ext4
> and f2fs already stopped doing it. So this converts all the others to
> what ext4 does:
> 
> - the error is recorded in filesystem private state and the filesystem
>   refuses modifications with EROFS itself
> - the superblock stays read-write and "emergency_ro" is shown in the
>   mount options ("withdrawn" for gfs2, "ro_ops" for efivarfs)
> - a remount to read-write fails with EROFS until umount
> - a remount to read-only works so emergency remount (sysrq u) doesn't
>   WARN, and it doesn't mark the filesystem clean
> 
> That's ext2, jfs, nilfs2, fat, exfat, hfs, affs, ufs, hpfs, ntfs, ocfs2,
> zonefs, btrfs, gfs2, efivarfs and ubifs. For gfs2 it's the withdraw and
> for efivarfs it's the notifier that flips SB_RDONLY when the efivar ops
> switch between ops that can and can't set variables. ubifs can't be
> frozen but it flips SB_RDONLY from any context all the same. hfsplus
> only did it when it created the attributes tree, where it can't trigger,
> so that check is simply limited to mount.
> 
> fat, exfat, hpfs, ntfs and ubifs keep setting SB_RDONLY for errors while
> the filesystem is still being mounted. Until SB_BORN the mounter holds
> s_umount and nothing can freeze the superblock so that's fine.
> 
> A few patches make the read-only remount work in the first place. exfat
> and ocfs2 didn't inherit their mount options on remount so emergency
> remount, which passes none, could fail on them. And gfs2 has a BUG_ON()
> when it's remounted read-only while a withdraw is still pending that
> this makes easier to hit. Those come before the conversion of the
> respective filesystem. ext4 and xfs have the same problem with their own
> error states. ext4 refused to go read-only in its emergency state at all
> and a shut down xfs couldn't be remounted read-only either. So sysrq u
> WARNed for every ext4 that hit an error and for every shut down xfs.
> Both are fixed at the end of the series.
> 
> ecryptfs opens its lower files read-write unless the lower superblock is
> read-only. On top of one of these filesystems every lookup of a regular
> file would fail with EROFS after an error and leak the inode. umount
> then reports busy inodes. ext4 already behaves like that and so does a
> read-only bind mount underneath. The first two patches make ecryptfs
> fall back to a read-only lower file and plug the leak.
> 
> There are user-visible changes but I think they're all fine. The big one
> is that "mount -o remount,rw" doesn't make the filesystem writable again
> after an error anymore. That's how ext4 behaves since
> commit d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem
> errors"). statfs() doesn't report ST_RDONLY after an error and a second
> read-only mount of the device fails with EBUSY. Writes through file
> descriptors opened before the error fail with EROFS now where they used
> to succeed. ext2, fat, hfs, affs, ufs and ubifs fail fsync() with EROFS
> after an error. Writeback drops what they can't write anymore so fsync()
> would report success otherwise. And remount helpers that pass the shown
> options back, busybox mount for example, trip over "emergency_ro".
> That's already the case on ext4.
> 
> Every patch has a reproducer in the commit message. I ran all of them
> in a VM against the unpatched and the patched kernel, the latter with
> KASAN and kmemleak. There are selftests for ext2, jfs, nilfs2, gfs2 and
> the ext4 and xfs remount changes.
> 
> This depends on three fixes that aren't upstream yet:
> 
>   super: don't put the remount context under s_umount
>   super: keep the read-only state if a reconfiguration doesn't change it
>   nilfs2: don't reconfigure a frozen superblock on mount
> 
> The first one matters most. This series makes errored filesystems
> reachable for sysrq u and without it a concurrent umount deadlocks on
> s_umount. The second one makes fc->sb_flags carry the current read-only
> state for fspick() reconfigurations, which ->reconfigure() of fat, affs,
> hfs, ufs, hpfs and btrfs rely on. The third one closes the last way
> nilfs2 changes SB_RDONLY on a frozen superblock, through a mount that
> reconfigures it.
> 
> With this no filesystem changes SB_RDONLY outside of mount and remount
> anymore.
> 
> [1]: https://lore.kernel.org/r/20260903060855.4610-1-kmehltretter@gmail.com
> [2]: https://lore.kernel.org/r/hyhuckyohsabx2suvq3z7uy3pbgmihva6wgzxr3op7pzcrlu33@p7zzlgp2ehjf
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner at kernel.org>

I'm in favor of this. Just after checking couple of filesystem changes -
won't we be better off by using a generic flag in super_block for this so
that VFS can check for it? Then each filesystem won't have to implement
its own wrapper for simple_fsync(), quota operations, and whatever other
generic operations that need to bail on shutdown filesystem...

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