Re: [PATCH v2 28/34] ubifs: don't set SB_RDONLY after errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ubifs: writeback and ubifs_remount_ro() run inconsistently after ro_error
- [Medium] ubifs: unguarded assert in ubifs_reconfigure() when remounting after ro_error
- [Medium] ubifs: setting SB_RDONLY before SB_BORN in ubifs_ro_mode() desyncs ro state
--

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

ubifs: don't set SB_RDONLY after errors

This commit stops setting SB_RDONLY in ubifs_ro_mode() once SB_BORN is set on
the superblock, calling sb_set_emergency_ro() instead. It also allows
read-only remounts to proceed through ubifs_remount_ro() after an error and
checks c->ro_error in the xattr setters.

> diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
> index 6c6d682427793..7a59aa1ea5ce9 100644
> --- a/fs/ubifs/io.c
> +++ b/fs/ubifs/io.c
> @@ -72,7 +72,14 @@ 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;
> +		/*
> +		 * Before SB_BORN the mounter holds s_umount, later see
> +		 * sb_set_emergency_ro().
> +		 */
> +		if (!(c->vfs_sb->s_flags & SB_BORN))
> +			c->vfs_sb->s_flags |= SB_RDONLY;
> +		else
> +			sb_set_emergency_ro(c->vfs_sb);

[Severity: Medium]
Can setting SB_RDONLY instead of calling sb_set_emergency_ro() before SB_BORN
is set race on sb->s_flags and leave c->ro_mount and SB_STATE_EMERGENCY_RO out
of sync with sb->s_flags?

In mount_ubifs(), c->ro_mount is latched once at entry and the background
thread c->bgt is started before vfs_get_tree() sets SB_BORN:

fs/ubifs/super.c:mount_ubifs() {
	c->ro_mount = !!sb_rdonly(c->vfs_sb);
	...
	if (!c->ro_mount) {
		/* Create background thread */
		c->bgt = kthread_run(ubifs_bg_thread, c, "%s", c->bgt_name);
}

Before vfs_get_tree() sets SB_BORN, ubifs_ro_mode() can be called without
failing the mount if an assertion fails under the default ASSACT_RO action in
ubifs_assert_failed() during mount_ubifs() or ubifs_iget(sb, UBIFS_ROOT_INO),
or if ubifs_bg_thread() wakes up after ubifs_fill_super() unlocks
c->umount_mutex and hits an error in ubifs_bg_wbufs_sync() or run_bg_commit().

If c->bgt calls ubifs_ro_mode() before SB_BORN is set, modifying
c->vfs_sb->s_flags |= SB_RDONLY happens without holding s_umount while the
mounter thread is modifying sb->s_flags in vfs_get_tree().

In addition, the mount finishes with SB_RDONLY set on sb->s_flags,
c->ro_error == 1, c->ro_mount == 0, and SB_STATE_EMERGENCY_RO clear:

fs/ubifs/super.c:ubifs_reconfigure() {
	if (c->ro_mount && !(fc->sb_flags & SB_RDONLY)) {
		if (c->ro_error) {
			ubifs_msg(c, "cannot re-mount R/W due to prior errors");
			return -EROFS;
		}
		...
	} else if (!c->ro_mount && (fc->sb_flags & SB_RDONLY)) {
		/* After an error ubifs_remount_ro() can't write but still works. */
		ubifs_remount_ro(c);
	}
}

Because sb_emergency_ro(sb) is false and c->ro_mount is 0, a subsequent
remount to read-write passes both reconfigure_super() and ubifs_reconfigure()
and clears SB_RDONLY, while a remount to read-only sees sb_rdonly(sb) as true
in reconfigure_super() and skips sb_prepare_remount_readonly() and
sb_start_ro_state_change() before calling ubifs_remount_ro().

Should ubifs_ro_mode() call sb_set_emergency_ro(c->vfs_sb) unconditionally?

[ ... ]

> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 38972786817e7..ef39da4e780b7 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1977,10 +1977,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. */

[Severity: High]
Does allowing ubifs_remount_ro() to run after c->ro_error is set cause a NULL
pointer dereference on c->write_reserve_buf or trigger assertions in
reserve_space() and ubifs_wbuf_sync_nolock()?

Previously, ubifs_ro_mode() set SB_RDONLY on sb->s_flags (causing
sync_filesystem() to return early via sb_rdonly(sb)), and ubifs_reconfigure()
refused to call ubifs_remount_ro(c) when c->ro_error was set.

With SB_RDONLY no longer set on error, writeback still runs
ubifs_writepages() -> ubifs_writepage(). When a dirty folio extends past
ui->synced_i_size, ubifs_writepage() calls ubifs_write_inode():

fs/ubifs/file.c:ubifs_writepage() {
		if (folio_pos(folio) + len > synced_i_size) {
			err = inode->i_sb->s_op->write_inode(inode, NULL);
			if (err)
				goto out_redirty;
		}
		return do_writepage(folio, len);
}

In ubifs_write_inode(), ubifs_jnl_write_inode() fails with -EROFS when
c->ro_error is set without updating ui->synced_i_size, but ubifs_write_inode()
still clears ui->dirty = 0:

fs/ubifs/super.c:ubifs_write_inode() {
	if (inode->i_nlink) {
		err = ubifs_jnl_write_inode(c, inode);
		if (err)
			ubifs_err(c, "can't write inode %llu, error %d",
				  inode->i_ino, err);
		else
			err = dbg_check_inode_size(c, inode, ui->ui_size);
	}

	ui->dirty = 0;
	mutex_unlock(&ui->ui_mutex);
	ubifs_release_dirty_inode_budget(c, ui);
	return err;
}

ubifs_writepage() then jumps to out_redirty and calls
folio_redirty_for_writepage(), which re-marks the folio PAGECACHE_TAG_DIRTY
and marks the inode I_DIRTY_PAGES without calling ubifs_dirty_inode(), leaving
ui->dirty == 0 while requeue_inode() puts the inode back onto wb->b_dirty.

If sync_inodes_sb() during ubifs_reconfigure() is the first writeback pass to
hit c->ro_error on that inode (for example, if writeback_inodes_sb() stopped
earlier after exhausting work->nr_pages or its time slice in
writeback_sb_inodes(), or skipped the inode while I_SYNC was set),
sync_filesystem() finishes with the redirtied folio still on wb->b_dirty and
ui->dirty == 0.

ubifs_reconfigure() ignores the return value of sync_filesystem() and calls
ubifs_remount_ro(), which frees c->write_reserve_buf and sets c->ro_mount = 1:

fs/ubifs/super.c:ubifs_remount_ro() {
	kfree(c->write_reserve_buf);
	c->write_reserve_buf = NULL;
	vfree(c->ileb_buf);
	c->ileb_buf = NULL;
	ubifs_lpt_free(c, 1);
	c->ro_mount = 1;
}

When background writeback (wb_workfn) subsequently flushes wb->b_dirty,
ubifs_writepage() calls ubifs_write_inode() again. Because ui->dirty is 0,
ubifs_write_inode() returns 0 immediately, and ubifs_writepage() proceeds into
do_writepage() -> ubifs_jnl_write_data():

fs/ubifs/journal.c:ubifs_jnl_write_data() {
	data = kmalloc(dlen + auth_len, GFP_NOFS | __GFP_NOWARN);
	if (!data) {
		allocated = 0;
		mutex_lock(&c->write_reserve_mutex);
		data = c->write_reserve_buf;
	}

	data->ch.node_type = UBIFS_DATA_NODE;
}

If kmalloc() fails under memory pressure, data = c->write_reserve_buf is NULL
and data->ch.node_type dereferences NULL. Even if kmalloc() succeeds,
make_reservation() -> reserve_space() hits
ubifs_assert(c, !c->ro_media && !c->ro_mount) before checking c->ro_error:

fs/ubifs/journal.c:reserve_space() {
	ubifs_assert(c, !c->ro_media && !c->ro_mount);
	squeeze = (jhead == BASEHD);
again:
	mutex_lock_nested(&wbuf->io_mutex, wbuf->jhead);

	if (c->ro_error) {
		err = -EROFS;
		goto out_unlock;
	}
}

Furthermore, unlike ubifs_put_super() (which guards ubifs_wbuf_sync() with
if (!c->ro_error)), ubifs_remount_ro() and ubifs_sync_fs() call
ubifs_wbuf_sync() -> ubifs_wbuf_sync_nolock(), which evaluates wbuf geometry
assertions before checking c->ro_error:

fs/ubifs/io.c:ubifs_wbuf_sync_nolock() {
	ubifs_assert(c, !(wbuf->avail & 7));
	ubifs_assert(c, wbuf->offs + wbuf->size <= c->leb_size);
	ubifs_assert(c, wbuf->size >= c->min_io_size);
	ubifs_assert(c, wbuf->size <= c->max_write_size);
	ubifs_assert(c, wbuf->size % c->min_io_size == 0);
	ubifs_assert(c, !c->ro_media && !c->ro_mount);
	if (c->leb_size - wbuf->offs >= c->max_write_size)
		ubifs_assert(c, !((wbuf->offs + wbuf->size) % c->max_write_size));

	if (c->ro_error)
		return -EROFS;
}

If c->ro_error was triggered by a mid-write failure in
ubifs_wbuf_write_nolock() after advancing wbuf->offs while wbuf->used > 0 and
before updating wbuf->size, those assertions can fire during
ubifs_remount_ro() or ubifs_sync_fs().

>  		ubifs_remount_ro(c);

[Severity: Medium]
Can remounting read-only after a garbage collection error trigger the
c->lst.taken_empty_lebs > 0 assertion at the end of ubifs_reconfigure()?

At the end of ubifs_reconfigure(), the code checks:

fs/ubifs/super.c:ubifs_reconfigure() {
	if (!c->need_recovery)
		ubifs_assert(c, c->lst.taken_empty_lebs > 0);
}

When c->lst.taken_empty_lebs is 1 (holding only the reserved GC LEB
c->gc_lnum), ubifs_garbage_collect() -> ubifs_garbage_collect_leb() ->
move_nodes() -> switch_gc_head() consumes c->gc_lnum and sets c->gc_lnum = -1.
Once the first node is written to the GC write-buffer, bud_wbuf_callback() ->
ubifs_update_one_lp() -> ubifs_change_lp() decrements c->lst.taken_empty_lebs
to 0.

If an error then occurs in move_nodes() or gc_sync_wbufs() before a new
c->gc_lnum is retained, ubifs_garbage_collect() switches to read-only error
mode with c->ro_error == 1, c->gc_lnum == -1, and
c->lst.taken_empty_lebs == 0:

fs/ubifs/gc.c:ubifs_garbage_collect() {
out:
	ubifs_assert(c, ret < 0);
	ubifs_assert(c, ret != -ENOSPC && ret != -EAGAIN);
	ubifs_wbuf_sync_nolock(wbuf);
	ubifs_ro_mode(c, ret);
	mutex_unlock(&wbuf->io_mutex);
	if (lp.lnum != -1)
		ubifs_return_leb(c, lp.lnum);
	return ret;
}

Now that ubifs_reconfigure() no longer returns -EROFS on a read-only remount
when c->ro_error is set, should the assertion at the end of
ubifs_reconfigure() also be guarded by !c->ro_error, similar to
ubifs_put_super()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-work-fs-sb_rdonly-v2-0-2f44f64f0b66@kernel.org?part=28

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/