[f2fs-dev] [PATCH] f2fs: fix compressed file start atomic write may cause data corruption

[f2fs-dev] [PATCH] f2fs: fix compressed file start atomic write may cause data corruption

[Fengnan Chang via Linux-f2fs-devel]

When compressed file has blocks, f2fs_ioc_start_atomic_write will succeed,
but compressed flag will be remained in inode. If write partial compreseed
cluster and commit atomic write will cause data corruption.

This is the reproduction process:
Step 1:
create a compressed file ,write 64K data , call fsync(), then the blocks
are write as compressed cluster.
Step2:
iotcl(F2FS_IOC_START_ATOMIC_WRITE)  --- this should be fail, but not.
write page 0 and page 3.
iotcl(F2FS_IOC_COMMIT_ATOMIC_WRITE)  -- page 0 and 3 write as normal file,
Step3:
drop cache.
read page 0-4   -- Since page 0 has a valid block address, read as
non-compressed cluster, page 1 and 2 will be filled with compressed data
or zero.

And before f2fs: compress: remove unneeded read when rewrite whole cluster,
even Step 2 is not right, but whole cluster will mark dirty in write_begin,
and whole cluster will be rewrite as no-compressed cluster, so it's ok.

Fixes: 4c8ff7095bef (f2fs: support data compression)
Fixes: 7eab7a696827 (f2fs: compress: remove unneeded read when rewrite whole
cluster)
Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
---
 fs/f2fs/data.c | 2 +-
 fs/f2fs/file.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index b09f401f8960..5675af1b6916 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3363,7 +3363,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,

 		*fsdata = NULL;

-		if (len == PAGE_SIZE)
+		if (len == PAGE_SIZE && !(f2fs_is_atomic_file(inode)))
 			goto repeat;

 		ret = f2fs_prepare_compress_overwrite(inode, pagep,
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index cfdc41f87f5d..2a07568ee61e 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2009,7 +2009,8 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)

 	inode_lock(inode);

-	f2fs_disable_compressed_file(inode);
+	if (!f2fs_disable_compressed_file(inode))
+		return -EINVAL;

 	if (f2fs_is_atomic_file(inode)) {
 		if (is_inode_flag_set(inode, FI_ATOMIC_REVOKE_REQUEST))
--
2.32.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: fix compressed file start atomic write may cause data corruption

[Chao Yu]

On 2022/3/9 17:37, Fengnan Chang wrote:
> When compressed file has blocks, f2fs_ioc_start_atomic_write will succeed,
> but compressed flag will be remained in inode. If write partial compreseed
> cluster and commit atomic write will cause data corruption.
> 
> This is the reproduction process:
> Step 1:
> create a compressed file ,write 64K data , call fsync(), then the blocks
> are write as compressed cluster.
> Step2:
> iotcl(F2FS_IOC_START_ATOMIC_WRITE)  --- this should be fail, but not.
> write page 0 and page 3.
> iotcl(F2FS_IOC_COMMIT_ATOMIC_WRITE)  -- page 0 and 3 write as normal file,
> Step3:
> drop cache.
> read page 0-4   -- Since page 0 has a valid block address, read as
> non-compressed cluster, page 1 and 2 will be filled with compressed data
> or zero.
> 
> And before f2fs: compress: remove unneeded read when rewrite whole cluster,
> even Step 2 is not right, but whole cluster will mark dirty in write_begin,
Nitpick here... could you please update as below:

The root cause is, after commit 7eab7a696827 ("f2fs: compress: remove unneeded
read when rewrite whole cluster"), in step 2, f2fs_write_begin() only set target
page dirty, and in f2fs_commit_inmem_pages(), we will write partial raw pages
into compressed cluster, result in corrupting compressed cluster layout.

> and whole cluster will be rewrite as no-compressed cluster, so it's ok.
> 
> Fixes: 4c8ff7095bef (f2fs: support data compression)

Fixes: 4c8ff7095bef ("f2fs: support data compression")

> Fixes: 7eab7a696827 (f2fs: compress: remove unneeded read when rewrite whole
> cluster)

Fixes: 7eab7a696827 ("f2fs: compress: remove unneeded read when rewrite whole cluster")

Thanks,

> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> ---
>   fs/f2fs/data.c | 2 +-
>   fs/f2fs/file.c | 3 ++-
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index b09f401f8960..5675af1b6916 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3363,7 +3363,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
> 
>   		*fsdata = NULL;
> 
> -		if (len == PAGE_SIZE)
> +		if (len == PAGE_SIZE && !(f2fs_is_atomic_file(inode)))
>   			goto repeat;
> 
>   		ret = f2fs_prepare_compress_overwrite(inode, pagep,
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index cfdc41f87f5d..2a07568ee61e 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2009,7 +2009,8 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
> 
>   	inode_lock(inode);
> 
> -	f2fs_disable_compressed_file(inode);
> +	if (!f2fs_disable_compressed_file(inode))
> +		return -EINVAL;
> 
>   	if (f2fs_is_atomic_file(inode)) {
>   		if (is_inode_flag_set(inode, FI_ATOMIC_REVOKE_REQUEST))
> --
> 2.32.0
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel