Re: [PATCH] ocfs2: fix kernel BUG in ocfs2_page_mkwrite() when write_end fails

[Heming Zhao <heming.zhao@suse.com>]

On 11/11/26 10:34, Zhiyuan Zhang wrote:
> From: Zhiyuan Zhang <zhiyuanzhang912@gmail.com>
> 
> syzbot reported a kernel BUG in the ocfs2 write fault path:
> 
>    (syz.0.17,5578,0):ocfs2_write_end_nolock:1945 ERROR: status = -30
>    kernel BUG at fs/ocfs2/mmap.c:106!
>    Call Trace:
>     do_page_mkwrite+0x147/0x310 mm/memory.c:3879
>     do_shared_fault mm/memory.c:6145 [inline]
>     handle_mm_fault+0x1a53/0x3750 mm/memory.c:6943
> 
> __ocfs2_page_mkwrite() calls ocfs2_write_begin_nolock() and then
> ocfs2_write_end_nolock(), and uses BUG_ON() to assert that the latter
> returns the full length.  But ocfs2_write_end_nolock() can fail: since
> commit 7f27ec978b0e ("ocfs2: call ocfs2_journal_access_di() before
> ocfs2_journal_dirty() in ocfs2_write_end_nolock()") it gets journal
> access to the inode buffer again, because the handle may have been
> restarted since ocfs2_write_begin_nolock().  That fails with -EROFS or
> -EIO once the journal has been aborted or jbd2 has seen write errors
> on the device.  The syzbot reproducer gets there by shrinking the
> backing loop device to zero with LOOP_SET_STATUS while writing to a
> shared file mapping.
> 
> Handle the failure instead of crashing.  ocfs2_write_end_nolock()
> leaves the mmap folio locked, because ocfs2_unlock_folios() skips it
> when w_target_locked is set so that it can be returned with
> VM_FAULT_LOCKED, and do_page_mkwrite() does not unlock it either when
> ->page_mkwrite() returns an error.  So unlock it and fail the fault;
> the faulting task gets SIGBUS, like for the other errors in this
> function.
> 
> For an mmap write ocfs2_write_end_nolock() returns either len or a
> negative errno: inline data has already been converted to extents by
> ocfs2_write_begin_nolock(), and copied is equal to len.
> 
> The BUG matters for local mounts, where the filesystem is expected to
> go read-only and keep running after a journal abort.  On clustered
> mounts ocfs2_abort() panics the node anyway as soon as it notices the
> aborted journal.
> 
> Fixes: 7f27ec978b0e ("ocfs2: call ocfs2_journal_access_di() before ocfs2_journal_dirty() in ocfs2_write_end_nolock()")
> Reported-by: syzbot+42984127d45ad96219de@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=42984127d45ad96219de
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-fable-5-2
> Signed-off-by: Zhiyuan Zhang <zhiyuanzhang912@gmail.com>

looks good to me.
Reviewed-by: Heming Zhao <heming.zhao@suse.com>
> ---
> 
> Notes:
>      Testing: syzbot C reproducer in QEMU (x86_64) on a defconfig-based
>      kernel with KASAN, lockdep, DEBUG_VM and hung task detection enabled
>      (not syzbot's own config).
>      
>       - Unpatched: BUG in 3 of 3 runs, within 22 iterations.
>       - Patched: 437 iterations over 3 runs hit the failing write_end 21
>         times (11 x -EROFS, 10 x -EIO), all ending in SIGBUS, with no BUG,
>         KASAN, lockdep, DEBUG_VM or hung task report.
>       - Negative control: without the folio_unlock(), inode eviction hangs
>         on the locked folio in truncate_inode_pages_range() in 3 of 3 runs
>         that hit the error.
>      
>      Not tested on a clustered mount.
>      
>      AI assistants were used to analyse the report, to write the patch
>      and the changelog, and to run the tests above.
>      
>      ocfs2_dio_wr_get_block() has the same BUG_ON(ret != len).  It is left
>      alone here because the pending "migrates ocfs2 DIO from buffer_head to
>      iomap" series removes that function.
> 
>   fs/ocfs2/mmap.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/mmap.c b/fs/ocfs2/mmap.c
> index 6c570157caf1..1c03f45e8965 100644
> --- a/fs/ocfs2/mmap.c
> +++ b/fs/ocfs2/mmap.c
> @@ -103,7 +103,17 @@ static vm_fault_t __ocfs2_page_mkwrite(struct file *file,
>   		goto out;
>   	}
>   	err = ocfs2_write_end_nolock(mapping, pos, len, len, fsdata);
> -	BUG_ON(err != len);
> +	if (err != len) {
> +		/*
> +		 * ocfs2_write_end_nolock() keeps the mmap folio locked so
> +		 * that it can be returned to the VM with VM_FAULT_LOCKED.
> +		 * On failure nobody else will unlock it, so do it here.
> +		 */
> +		folio_unlock(locked_folio);
> +		mlog_errno(err);
> +		ret = vmf_error(err);
> +		goto out;
> +	}
>   	ret = VM_FAULT_LOCKED;
>   out:
>   	return ret;
> 
> base-commit: 067c5c24cef23d913b67eaaf71ae67e7169a6620