Re: [PATCH RFC 04/28] jfs: don't set SB_RDONLY after filesystem errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] jfs: mmap writes bypass emergency read-only state
- [High] jfs: remounting with nointegrity bypasses emergency RO protections
- [High] jfs: double log shutdown oops on jfs_unfreeze error path
- [High] jfs: extRecord() omits synchronous commit of synclist anonymous tlocks
--

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

jfs: don't set SB_RDONLY after filesystem errors

The commit avoids setting SB_RDONLY on errors to prevent freeze deadlocks,
introducing a private JFS_FLAGS_EMERGENCY_RO flag instead. It updates various
entry points to check this flag before modifying the filesystem.

Link: https://lore.kernel.org/r/20260903060855.4610-1-kmehltretter@gmail.com

> diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> --- a/fs/jfs/file.c
> +++ b/fs/jfs/file.c

[ ... ]

> @@ -146,7 +160,7 @@ const struct file_operations jfs_file_operations = {
>  	.open		= jfs_open,
>  	.llseek		= generic_file_llseek,
>  	.read_iter	= generic_file_read_iter,
> -	.write_iter	= generic_file_write_iter,
> +	.write_iter	= jfs_file_write_iter,
>  	.mmap_prepare	= generic_file_mmap_prepare,

[Severity: High]
Does using the generic VFS VM operations here allow memory-mapped writes to
bypass the new emergency read-only state?

Since SB_RDONLY is no longer set on errors, the generic fault handler
filemap_page_mkwrite will succeed because it only checks sb_rdonly(sb).
This allows dirtying the page. When the VFS writeback daemon processes the
dirty inode, it calls jfs_writepages.

Because jfs_writepages maps already-allocated blocks via jfs_get_block
without calling extAlloc or extRecord, it completely bypasses the new
emergency read-only checks, allowing modifications to be written to disk.

> diff --git a/fs/jfs/jfs_extent.c b/fs/jfs/jfs_extent.c
> --- a/fs/jfs/jfs_extent.c
> +++ b/fs/jfs/jfs_extent.c

[ ... ]

> @@ -270,6 +276,10 @@ int extRecord(struct inode *ip, xad_t * xp)
>  	/* update the extent */
>  	rc = xtUpdate(0, ip, xp);
>  
> +	/* get the anonymous tlocks committed, like extAlloc() does */
> +	if (!rc)
> +		mark_inode_dirty(ip);
> +

[Severity: High]
Does this omit the synchronous commit of synclist anonymous tlocks that
extAlloc() performs?

When xtUpdate() initiates an anonymous transaction, it can flag the inode
with COMMIT_Synclist and set nohomeok > 0 if the metapage is already on
the journal's synclist.

While extAlloc() explicitly tests for COMMIT_Synclist and calls
jfs_commit_inode() synchronously to clear this state, extRecord() does
not. This leaves the metapages pinned in memory. If a concurrent operation
calls jfs_flush_journal(), it can stall indefinitely waiting for the
synclist to empty, leading to a hang or bug in the journal thread.

> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> --- a/fs/jfs/super.c
> +++ b/fs/jfs/super.c

[ ... ]

> @@ -367,6 +388,11 @@ static int jfs_reconfigure(struct fs_context *fc)
>  	int flag = ctx->flag;
>  	int ret;
>  
> +	/* An error made the filesystem read-only until it is unmounted. */
> +	if (jfs_emergency_ro(sb) && (ctx->resize || (sb_rdonly(sb) && !readonly)))
> +		return -EROFS;
> +

[Severity: High]
Can a remount command that toggles nointegrity bypass this read-only guard?

If the filesystem was originally mounted read-write, sb_rdonly(sb) is
false. A remount command that only toggles nointegrity evaluates to false
in this guard and proceeds down the function.

Further down in jfs_reconfigure:

	if ((JFS_SBI(sb)->flag & JFS_NOINTEGRITY) != (flag & JFS_NOINTEGRITY)) {
		if (!sb_rdonly(sb)) {
			rc = jfs_umount_rw(sb);

This block would be erroneously executed, calling jfs_umount_rw(). That
function flushes block and inode maps to disk and closes the log, which
violates the guarantee that nothing may be written after an error, and
risks severe metadata corruption.

[ ... ]

> @@ -616,15 +657,19 @@ static int jfs_unfreeze(struct super_block *sb)
>  	struct jfs_log *log = sbi->log;
>  	int rc = 0;
>  
> -	if (!sb_rdonly(sb)) {
> +	/* Undo what jfs_freeze() did, also if an error happened meanwhile. */
> +	if (test_bit(JFS_FLAGS_LOG_SHUTDOWN, &sbi->jfs_flags)) {
>  		rc = updateSuper(sb, FM_MOUNT);
>  		if (rc) {
>  			jfs_error(sb, "updateSuper failed\n");
>  			goto out;
>  		}
>  		rc = lmLogInit(log);
> -		if (rc)
> +		if (rc) {
>  			jfs_error(sb, "lmLogInit failed\n");
> +			goto out;
> +		}
> +		clear_bit(JFS_FLAGS_LOG_SHUTDOWN, &sbi->jfs_flags);
>  out:
>  		txResume(sb);
>  	}

[Severity: High]
If lmLogInit() or updateSuper() fails, does this leave the log state
desynchronized?

By jumping to out without clearing JFS_FLAGS_LOG_SHUTDOWN, the flag
remains set while the log is still shut down (log->bp is NULL).

Later, when the filesystem is unmounted, jfs_umount calls lmLogClose(),
which unconditionally calls lmLogShutdown() if the log is inactive.
lmLogShutdown() will then dereference the NULL pointer, triggering
the exact null pointer dereference this patch originally intended
to fix.

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