Re: [PATCH v3 30/33] ubifs: don't set SB_RDONLY after errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] ubifs: false space accounting error in ubifs_remount_ro()
  after error
- [Medium] ubifs: ubifs_fsync() trips !c->ro_mount assert after error
  remount
- [Medium] ubifs: race between in-flight writeback and
  ubifs_remount_ro() after error
--

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

ubifs: don't set SB_RDONLY after errors

This commit switches ubifs_ro_mode() from setting SB_RDONLY directly on
s_flags to calling sb_set_emergency_ro(), allows remounting read-only after
an error, and drops dirty folios and inodes during writeback once
c->ro_error is set.

> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 38972786817e7..2d71274e89ad0 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
[ ... ]
> @@ -1839,19 +1846,26 @@ static void ubifs_remount_ro(struct ubifs_info *c)
>  
>  	dbg_save_space_info(c);
>  
> -	for (i = 0; i < c->jhead_cnt; i++) {
> -		err = ubifs_wbuf_sync(&c->jheads[i].wbuf);
> +	/* Like ubifs_put_super(), don't write anything after an error. */
> +	if (!c->ro_error) {
> +		for (i = 0; i < c->jhead_cnt; i++) {
> +			err = ubifs_wbuf_sync(&c->jheads[i].wbuf);
> +			if (err)
> +				ubifs_ro_mode(c, err);
> +		}
> +
> +		c->mst_node->flags &= ~cpu_to_le32(UBIFS_MST_DIRTY);
> +		c->mst_node->flags |= cpu_to_le32(UBIFS_MST_NO_ORPHS);
> +		c->mst_node->gc_lnum = cpu_to_le32(c->gc_lnum);
> +		err = ubifs_write_master(c);
>  		if (err)
>  			ubifs_ro_mode(c, err);
> +	} else {
> +		/* Syncing the write-buffers would have canceled them. */
> +		for (i = 0; i < c->jhead_cnt; i++)
> +			hrtimer_cancel(&c->jheads[i].wbuf.timer);
>  	}

[Severity: Medium]
Can an in-flight ubifs_fsync() trip the !c->ro_mount assertion in
ubifs_wbuf_sync_nolock() when racing with a read-only remount after an
error?

When c->ro_error is set, skipping ubifs_wbuf_sync() leaves wbuf->used and
wbuf->inodes[] populated while ubifs_remount_ro() sets c->ro_mount = 1.
Because vfs_fsync_range() does not hold s_umount or mnt_want_write(), an
fsync() on an inode whose node is already buffered in a wbuf (with no dirty
folios and ui->dirty == 0) can interleave with ubifs_remount_ro():

CPU0                              CPU1
ubifs_fsync()
  if (c->ro_mount)
    return 0;
  file_write_and_wait_range()
  ubifs_write_inode() -> 0
                                  ubifs_remount_ro()
                                    hrtimer_cancel(&wbuf->timer)
                                    c->ro_mount = 1
  ubifs_sync_wbufs_by_inode()
    ubifs_wbuf_sync_nolock(wbuf)
      ubifs_assert(c, !c->ro_media && !c->ro_mount)
        <-- assertion failure

In ubifs_wbuf_sync_nolock(), the !c->ro_mount assertion runs before the
if (c->ro_error) return -EROFS check, causing a warning and stack dump (or
a panic when mounted with assert=panic).

> -
> -	c->mst_node->flags &= ~cpu_to_le32(UBIFS_MST_DIRTY);
> -	c->mst_node->flags |= cpu_to_le32(UBIFS_MST_NO_ORPHS);
> -	c->mst_node->gc_lnum = cpu_to_le32(c->gc_lnum);
> -	err = ubifs_write_master(c);
> -	if (err)
> -		ubifs_ro_mode(c, err);
> -
>  	vfree(c->orph_buf);
>  	c->orph_buf = NULL;
>  	kfree(c->write_reserve_buf);

[Severity: Medium]
Can ubifs_remount_ro() free c->write_reserve_buf and set c->ro_mount = 1
while an in-flight do_writepage() is still running?

Once ubifs_ro_mode() calls sb_set_emergency_ro(), sync_filesystem(sb) in
ubifs_reconfigure() returns immediately at sb_rdonly_any(sb) without
calling sync_inodes_sb() or waiting for in-flight writeback.  A writeback
caller that does not take mnt_want_write() or s_umount (such as
sync_file_range()) can pass the unlocked c->ro_error check in
do_writepage() just before an error occurs and then race with
ubifs_remount_ro():

CPU0                              CPU1
do_writepage()
  if (unlikely(c->ro_error))
  ubifs_jnl_write_data()
                                  ubifs_ro_mode()
                                    c->ro_error = 1
                                    sb_set_emergency_ro(sb)
                                  ubifs_reconfigure()
                                    sync_filesystem(sb) -> 0
                                    ubifs_remount_ro()
                                      kfree(c->write_reserve_buf)
                                      c->write_reserve_buf = NULL
                                      c->ro_mount = 1
    mutex_lock(&c->write_reserve_mutex)
    data = c->write_reserve_buf
    data->ch.node_type = ...
      <-- NULL dereference or use-after-free

Even when kmalloc() succeeds in ubifs_jnl_write_data(), make_reservation()
subsequently calls reserve_space(), tripping
ubifs_assert(c, !c->ro_media && !c->ro_mount).

> @@ -1977,10 +1991,7 @@ static int ubifs_reconfigure(struct fs_context *fc)
>  		if (err)
>  			return err;
>  	} else if (!c->ro_mount && (fc->sb_flags & SB_RDONLY)) {
> -		if (c->ro_error) {
> -			ubifs_msg(c, "cannot re-mount R/O due to prior errors");
> -			return -EROFS;
> -		}
> +		/* After an error ubifs_remount_ro() can't write but still works. */
>  		ubifs_remount_ro(c);
>  	}

[Severity: Medium]
Should ubifs_remount_ro() skip dbg_save_space_info() and
dbg_check_space_info() when c->ro_error is set, as ubifs_put_super() and
the taken_empty_lebs check in ubifs_reconfigure() do?

Because sync_filesystem() is a no-op once sb_set_emergency_ro() is set,
dirty folios and dirty inodes remain pending during ubifs_remount_ro().
dbg_save_space_info() snapshots ubifs_get_free_space_nolock(), which
includes c->bi.data_growth and c->bi.dd_growth.  If a concurrent writeback
or folio release drops a dirty folio via do_writepage() or a dirty inode
via ubifs_write_inode() before dbg_check_space_info() runs at the end of
ubifs_remount_ro(), releasing that page or inode budget changes the
reported free space and causes dbg_check_space_info() to log a false "free
space changed" error and call dump_stack().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-work-fs-sb_rdonly-v3-0-0be92bb73709@kernel.org?part=30