Re: [PATCH RFC 06/28] fat: 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] fat: logical bypass of emergency RO flag on read-only mounts
- [Medium] fat: TOCTOU race in directory operations bypasses emergency RO state
--

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

    fat: don't set SB_RDONLY after filesystem errors

This commit introduces a new FAT_FLAGS_EMERGENCY_RO bit to handle filesystem
errors instead of setting SB_RDONLY directly. This prevents hangs when errors
occur while the filesystem is frozen, and various filesystem operations are
updated to check this new flag to refuse modifications.

> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index e79762cf19754..ecba57df83ca2 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
[ ... ]
> @@ -32,10 +33,22 @@ void __fat_fs_error(struct super_block *sb, int report, const char *fmt, ...)
>  		va_end(args);
>  	}
>  
> -	if (opts->errors == FAT_ERRORS_PANIC)
> +	if (opts->errors == FAT_ERRORS_PANIC) {
>  		panic("FAT-fs (%s): fs panic from previous error\n", sb->s_id);
> -	else if (opts->errors == FAT_ERRORS_RO && !sb_rdonly(sb)) {
> -		sb->s_flags |= SB_RDONLY;
> +	} else if (opts->errors == FAT_ERRORS_RO && !sb_rdonly(sb)) {
> +		/*
> +		 * Only a mount in progress may still set SB_RDONLY: the
> +		 * mounter holds s_umount and nothing can freeze the superblock
> +		 * before SB_BORN. Later, setting it without s_umount on a
> +		 * frozen superblock makes thaw_super() mark it unfrozen but
> +		 * skip sb_freeze_unlock(), so the s_writers rwsems stay
> +		 * write-locked and every writer hangs. Refuse modifications
> +		 * until umount instead.
> +		 */
> +		if (!(sb->s_flags & SB_BORN))
> +			sb->s_flags |= SB_RDONLY;
> +		else if (test_and_set_bit(FAT_FLAGS_EMERGENCY_RO, &sbi->flags))
> +			return;

[Severity: High]
Does this logic bypass setting the emergency read-only flag if the filesystem
is already mounted read-only when the error occurs?

If an error happens on a read-only mount, the !sb_rdonly(sb) condition skips
setting the FAT_FLAGS_EMERGENCY_RO bit, meaning the filesystem won't
persistently record its corrupted emergency state.

If the administrator later remounts the filesystem as read-write,
fat_reconfigure() allows the remount because fat_emergency_ro(sb) will return
false. Could this allow modifying a known-corrupted filesystem?

> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index da3e89c0b16ac..73f7f11b05907 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -791,6 +791,9 @@ static int vfat_rmdir(struct inode *dir, struct dentry *dentry)
>  	struct fat_slot_info sinfo;
>  	int err;
>  
> +	if (unlikely(fat_emergency_ro(sb)))
> +		return -EROFS;
> +
>  	mutex_lock(&MSDOS_SB(sb)->s_lock);
>  
>  	err = fat_dir_empty(inode);

[Severity: Medium]
Could this lockless check introduce a TOCTOU race?

If a separate thread encounters an error and locklessly sets
FAT_FLAGS_EMERGENCY_RO, and concurrently a thread executing vfat_rmdir() passes
this check and waits on s_lock, the rmdir thread will proceed with the
operation once it acquires the lock.

Since fat_remove_entries() is called later inside s_lock but doesn't perform
its own internal check of the emergency read-only flag (unlike
fat_add_entries()), could directory modifications be written to disk despite
the filesystem having entered an emergency read-only state?

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