Re: [PATCH RFC 13/28] hpfs: don't set SB_RDONLY after filesystem errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vfs: hpfs: writeback, mmap, and eviction bypass emergency RO state
- [Medium] vfs: hpfs: read-write remount bypasses -EROFS rejection
--

commit fff4d175276350a79a5a1bcb72eb2e0bb0259054
Author: Christian Brauner <brauner@kernel.org>
hpfs: don't set SB_RDONLY after filesystem errors

This commit replaces setting SB_RDONLY with a new HPFS_STATE_EMERGENCY_RO
flag during runtime filesystem errors. This avoids blocking on s_writers
locks if the filesystem is frozen.

> diff --git a/fs/hpfs/super.c b/fs/hpfs/super.c
> index 8fbdbf0806276..cff963cabbb63 100644
> --- a/fs/hpfs/super.c
> +++ b/fs/hpfs/super.c
> @@ -81,7 +81,18 @@ void hpfs_error(struct super_block *s, const char *fmt, ...)
>  			else {
>  				pr_cont("; remounting read-only\n");
>  				mark_dirty(s, 0);
> -				s->s_flags |= SB_RDONLY;
> +				/*
> +				 * Only a mount in progress may still set
> +				 * SB_RDONLY: later that needs s->s_umount,
> +				 * and on a frozen superblock it makes
> +				 * thaw_super() leave s_writers locked.
> +				 * Refuse modifications until umount instead.
> +				 */
> +				if (s->s_flags & SB_BORN)
> +					set_bit(HPFS_STATE_EMERGENCY_RO,
> +						&hpfs_sb(s)->sb_state);
> +				else
> +					s->s_flags |= SB_RDONLY;

[Severity: High]
Because the VFS SB_RDONLY flag is no longer set here, the VFS background
writeback thread and generic memory-mapped page faults will no longer
automatically skip the filesystem.

While the namespace operations were appropriately updated, could this allow
dirty pages from mmap to be written out since hpfs_writepages() doesn't
check the emergency read-only state? Similarly, could hpfs_evict_inode()
mutate the btree and sector bitmaps on a corrupted filesystem when an
unlinked inode is evicted?

[ ... ]

> @@ -426,6 +437,11 @@ static int hpfs_reconfigure(struct fs_context *fc)
>  	struct super_block *s = fc->root->d_sb;
>  	struct hpfs_sb_info *sbi = hpfs_sb(s);
>  
> +	/* After an error, refuse going back to read-write until umount. */
> +	if (hpfs_emergency_ro(s) && sb_rdonly(s) &&
> +	    !(fc->sb_flags & SB_RDONLY))
> +		return -EROFS;

[Severity: Medium]
The commit message states "A remount to read-write fails with -EROFS until
umount". However, if the filesystem experienced the runtime error while
originally mounted read-write, hpfs_error() sets HPFS_STATE_EMERGENCY_RO
but not SB_RDONLY.

In that scenario, sb_rdonly(s) is false. When a user subsequently executes
a read-write remount, does this check inadvertently bypass the -EROFS
rejection and allow the remount to succeed since sb_rdonly(s) evaluates
to false?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-work-fs-sb_rdonly-v1-0-60743a3dc326@kernel.org?part=13