Re: [PATCH v2 16/34] hpfs: 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] hpfs: remount rw after error succeeds and writes spare block in mark_dirty()
--

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

hpfs: don't set SB_RDONLY after filesystem errors

This commit updates hpfs_error() to call sb_set_emergency_ro() instead of
setting SB_RDONLY directly once SB_BORN is set, avoiding a hang in
thaw_super() when an error occurs while the filesystem is frozen. It also
updates hpfs_trim_fs(), hpfs_lookup(), hpfs_map_dnode(), and unmark_dirty()
to check sb_emergency_ro().

> diff --git a/fs/hpfs/super.c b/fs/hpfs/super.c
> index 8fbdbf0806276..ab89e624cd517 100644
> --- a/fs/hpfs/super.c
> +++ b/fs/hpfs/super.c
[ ... ]
> @@ -81,7 +81,14 @@ 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;
> +				/*
> +				 * Before SB_BORN the mounter holds s_umount,
> +				 * later see sb_set_emergency_ro().
> +				 */
> +				if (s->s_flags & SB_BORN)
> +					sb_set_emergency_ro(s);
> +				else
> +					s->s_flags |= SB_RDONLY;

[Severity: Medium]
When a runtime error occurs on a read-write mount after SB_BORN is set,
hpfs_error() calls sb_set_emergency_ro(s) without setting SB_RDONLY in
s->s_flags, leaving sb_rdonly(s) false.

If userspace then attempts to remount read-write (or reconfigures mount
options without SB_RDONLY), reconfigure_super() in fs/super.c checks:

	remount_rw = !(fc->sb_flags & SB_RDONLY) && sb_rdonly(sb);
	remount_ro = (fc->sb_flags & SB_RDONLY) && !sb_rdonly(sb);
    ...
	if (remount_rw && sb_emergency_ro(sb))
		return -EROFS;

Because sb_rdonly(sb) is false, remount_rw evaluates to false and bypasses
the -EROFS check. reconfigure_super() then proceeds into hpfs_reconfigure()
in fs/hpfs/super.c:

	if (!(fc->sb_flags & SB_RDONLY)) mark_dirty(s, 1);

	hpfs_unlock(s);
	return 0;

And mark_dirty() in fs/hpfs/super.c checks:

	if (hpfs_sb(s)->sb_chkdsk && (remount || !sb_rdonly(s))) {
		struct buffer_head *bh;
		struct hpfs_spare_block *sb;
		if ((sb = hpfs_map_sector(s, 17, &bh, 0))) {
			sb->dirty = 1;
			sb->old_wrote = 0;
			mark_buffer_dirty(bh);
			sync_dirty_buffer(bh);
			brelse(bh);
		}
	}

Unlike unmark_dirty(), neither hpfs_reconfigure() nor mark_dirty() checks
sb_emergency_ro(s).

Does this allow a read-write remount or reconfigure after an error to
succeed and synchronously write the spare block at sector 17 via
mark_dirty()?

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