Re: [PATCH RFC 23/28] ubifs: don't set SB_RDONLY after errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] fs/ubifs: Data race on bitfields ro_error and ro_mount causes lost updates
--

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

ubifs: don't set SB_RDONLY after errors

The commit stops UBIFS from asynchronously setting SB_RDONLY on the VFS
superblock when an error occurs, to avoid racing with VFS operations.
Instead, it relies on the c->ro_error flag to reject writes, returning
-EROFS from various file operations after an error.

> diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
> index 6c6d682427793..3d01529a07540 100644
> --- a/fs/ubifs/io.c
> +++ b/fs/ubifs/io.c
> @@ -72,7 +72,12 @@ void ubifs_ro_mode(struct ubifs_info *c, int err)
>  	if (!c->ro_error) {
>  		c->ro_error = 1;
>  		c->no_chk_data_crc = 0;
> -		c->vfs_sb->s_flags |= SB_RDONLY;
> +		/*
> +		 * Only a mount in progress may set SB_RDONLY, later that needs
> +		 * s_umount. UBIFS refuses modifications itself afterwards.
> +		 */
> +		if (!(c->vfs_sb->s_flags & SB_BORN))
> +			c->vfs_sb->s_flags |= SB_RDONLY;

[Severity: High]
Does this change risk losing the ro_error flag update due to a data race on
adjacent bitfields?

In struct ubifs_info, ro_error, ro_media, and ro_mount are adjacent bitfields
sharing the same unsigned int.

ubifs_ro_mode() locklessly sets c->ro_error = 1 from any context, including
hardware interrupt or softirq contexts during I/O error completions.

Concurrently, ubifs_remount_ro() modifies c->ro_mount while holding
c->umount_mutex:

fs/ubifs/super.c:ubifs_remount_ro() {
    ...
    ubifs_lpt_free(c, 1);
    c->ro_mount = 1;
    err = dbg_check_space_info(c);
    ...
}

Because bitfield assignments are implemented as read-modify-write operations
on the entire word, an interrupt firing during the RMW cycle of
ubifs_remount_ro() will cause the ro_error update to be silently clobbered
and lost.

Now that the asynchronous setting of SB_RDONLY is removed, the filesystem
is entirely dependent on c->ro_error to reject subsequent writes. If ro_error
is lost, could the filesystem erroneously continue attempting writes to a
broken medium during a concurrent remount?

[ ... ]

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