Re: [PATCH] f2fs: check cp_error before committing atomic writes

[Chao Yu <chao@kernel.org>]

On 8/6/26 14:18, Wenjie Qi wrote:
> F2FS checks cp_error before entering the ioctl path, but cp_error can be
> set after that and before atomic commit starts replacing blocks.
> 
> In that case, F2FS_IOC_COMMIT_ATOMIC_WRITE returns -EIO, but the current
> mount can still expose the new data before remount.
> 
> Check cp_error again in f2fs_commit_atomic_write() after
> filemap_write_and_wait_range() and before block replacement starts.
> 
> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
> Cc: stable@kernel.org
> Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
> ---
>  fs/f2fs/segment.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> QEMU validation:
> - before fix, FAULT_CHECKPOINT made COMMIT_ATOMIC_WRITE fail with -EIO,
>   while the current mount still exposed the new data; after remount, the
>   file reverted to the old data.
> - after fix, the same fault still failed with -EIO, and both the current
>   mount and the remounted filesystem kept the old data.
> - no-fault smoke still committed the new data successfully and preserved it
>   across remount.
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 0b706568b034..e6384dd2e916 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -408,6 +408,9 @@ int f2fs_commit_atomic_write(struct inode *inode)
>  	if (err)
>  		return err;
>  
> +	if (unlikely(f2fs_cp_error(sbi)))
> +		return -EIO;

I suspect even we add cp_error in more places, we may suffer both conditions:
1) expose new data to userspace if cp_error occurs before f2fs_do_sync_file()
after f2fs_commit_atomic_write().
2) not expose new data to userspace if we cp_error occurs before or during
__f2fs_commit_atomic_write().

Thanks,

> +
>  	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>  	f2fs_lock_op(sbi, &lc);
>