Re: [PATCH RFC 12/28] ufs: 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:
- [High] ufs: writeback, mmap, and eviction bypass emergency read-only state
--

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

ufs: don't set SB_RDONLY after filesystem errors

This commit replaces the setting of the VFS SB_RDONLY flag on filesystem
errors with a custom UFS_STATE_EMERGENCY_RO state flag. This prevents
thaw_super() from hanging, while preventing new modifications by returning
-EROFS in VFS-level UFS functions.

> diff --git a/fs/ufs/super.c b/fs/ufs/super.c
> index 3569ac92b065b..ba2dd4502869c 100644
> --- a/fs/ufs/super.c
> +++ b/fs/ufs/super.c
> @@ -280,11 +280,17 @@ void ufs_error (struct super_block * sb, const char * function,
>  	uspi = UFS_SB(sb)->s_uspi;
>  	usb1 = ubh_get_usb_first(uspi);
>  	
> -	if (!sb_rdonly(sb)) {
> +	/*
> +	 * Don't set SB_RDONLY: that requires sb->s_umount and a proper
> +	 * remount. Set while the superblock is frozen it makes thaw_super()
> +	 * skip releasing s_writers and every later writer hangs. Refuse
> +	 * modifications until umount instead.
> +	 * The superblock buffer marked bad goes out with the block device.
> +	 */
> +	if (!sb_rdonly(sb) &&
> +	    !test_and_set_bit(UFS_STATE_EMERGENCY_RO, &UFS_SB(sb)->s_state)) {
>  		usb1->fs_clean = UFS_FSBAD;
>  		ubh_mark_buffer_dirty(USPI_UBH(uspi));
> -		ufs_mark_sb_dirty(sb);
> -		sb->s_flags |= SB_RDONLY;

[Severity: High]
With the removal of the VFS SB_RDONLY flag here, are background VFS operations
now able to bypass the emergency read-only state?

Since the VFS is no longer aware of the read-only state, it appears that
background writeback and eviction operations will continue to run.

For example, ufs_writepages() in fs/ufs/inode.c lacks a check for
ufs_emergency_ro(). Dirty pages dirtied before the error occurred could still
be flushed to disk, and mpage_writepages() could allocate new blocks by
calling ufs_getfrag_block() with create=1.

Memory-mapped writes use filemap_page_mkwrite(), which invokes
ufs_write_begin(). Without an emergency RO check in ufs_write_begin(),
userspace could continue to dirty mapped pages and allocate new blocks.

Additionally, ufs_evict_inode() processes unlinked files when they are closed.
It unconditionally calls ufs_truncate_blocks(), ufs_update_inode(), and
ufs_free_inode(). These functions modify block/inode bitmaps and call
ubh_mark_buffer_dirty().

Because the block layer's flusher will sync these dirty buffer heads to the
underlying block device, could this silently write data and metadata
modifications to a corrupted disk while the filesystem is in the emergency
read-only state?

>  	}

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