Re: [PATCH RFC 11/28] affs: 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] affs: affs_error() logic short-circuit prevents setting emergency RO if already SB_RDONLY
- [High] affs: mmap writes and writeback bypass AFFS_STATE_EMERGENCY_RO and corrupt filesystem
--

commit 1ab95d009ae4a762fda73d7b1a5276c439494d6a
Author: Christian Brauner <brauner@kernel.org>
Subject: affs: don't set SB_RDONLY after filesystem errors

This commit stops setting SB_RDONLY directly in affs_error() without
holding sb->s_umount, which previously caused deadlocks during freezing.
Instead, it introduces an AFFS_STATE_EMERGENCY_RO bit to gracefully
refuse modifications with -EROFS.

> diff --git a/fs/affs/amigaffs.c b/fs/affs/amigaffs.c
> --- a/fs/affs/amigaffs.c
> +++ b/fs/affs/amigaffs.c
> @@ -479,9 +479,15 @@ affs_error(struct super_block *sb, const char *function, const char *fmt, ...)
>  	vaf.fmt = fmt;
>  	vaf.va = &args;
>  	pr_crit("error (device %s): %s(): %pV\n", sb->s_id, function, &vaf);
> -	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.
> +	 */
> +	if (!sb_rdonly(sb) &&
> +	    !test_and_set_bit(AFFS_STATE_EMERGENCY_RO, &AFFS_SB(sb)->s_state))

[Severity: High]
Does this short-circuit logic prevent the emergency RO bit from being set
if the filesystem is already read-only?

If a user mounts the filesystem RO, hits a read or checksum error (triggering
affs_error()), the AFFS_STATE_EMERGENCY_RO bit is completely skipped because
sb_rdonly(sb) is true.

When the user later remounts RW, affs_reconfigure() finds
affs_emergency_ro(sb) to be false and permits the RW remount. Could this
allow a corrupted filesystem to become writable, violating the intent to
refuse modifications after an error?

>  		pr_warn("Remounting filesystem read-only\n");
> -	sb->s_flags |= SB_RDONLY;
>  	va_end(args);
>  }

> diff --git a/fs/affs/file.c b/fs/affs/file.c
> --- a/fs/affs/file.c
> +++ b/fs/affs/file.c
[ ... ]
> @@ -1000,10 +1006,18 @@ int affs_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
>  	inode_unlock(inode);
>  	return ret;
>  }
> +
> +static ssize_t affs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> +{
> +	if (affs_emergency_ro(file_inode(iocb->ki_filp)->i_sb))
> +		return -EROFS;
> +	return generic_file_write_iter(iocb, from);
> +}

[Severity: High]
Since this patch removes SB_RDONLY from the error path, do we now need
checks in the mmap and writeback paths to prevent arbitrary data and
metadata modifications?

By no longer setting SB_RDONLY, the VFS filemap_page_mkwrite (used by
generic_file_mmap_prepare) permits dirtying pages without resistance.
During writeback, affs_writepages() calls affs_get_block():

fs/affs/file.c:affs_writepages() {
    return mpage_writepages(mapping, wbc, affs_get_block);
}

fs/affs/file.c:affs_get_block() {
    ...
    if (create) {
        u32 blocknr = affs_alloc_block(inode, ext_bh->b_blocknr);
    ...
}

If the write targets a hole, affs_alloc_block() allocates blocks on the
physical disk and dirties metadata buffers. Because neither affs_writepages()
nor affs_get_block() check affs_emergency_ro(), could a user memory map an
opened file, write to it after an error, and have background writeback flush
the pages to disk, thereby corrupting the filesystem?

> +
>  const struct file_operations affs_file_operations = {
>  	.llseek		= generic_file_llseek,
>  	.read_iter	= generic_file_read_iter,
> -	.write_iter	= generic_file_write_iter,
> +	.write_iter	= affs_file_write_iter,
>  	.mmap_prepare	= generic_file_mmap_prepare,
>  	.open		= affs_file_open,
>  	.release	= affs_file_release,

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