From: Zizhi Wo <wozizhi@huawei.com>

In __cachefiles_write(), if the return value of the write operation > 0, it
is set to 0. This makes it impossible to distinguish scenarios where a
partial write has occurred, and will affect the outer calling functions:

 1) cachefiles_write_complete() will call "term_func" such as
netfs_write_subrequest_terminated(). When "ret" in __cachefiles_write()
is used as the "transferred_or_error" of this function, it can not
distinguish the amount of data written, makes the WARN meaningless.

 2) cachefiles_ondemand_fd_write_iter() can only assume all writes were
successful by default when "ret" is 0, and unconditionally return the full
length specified by user space.

Fix it by modifying "ret" to reflect the actual number of bytes written.
Furthermore, returning a value greater than 0 from __cachefiles_write()
does not affect other call paths, such as cachefiles_issue_write() and
fscache_write().

Fixes: 047487c947e8 ("cachefiles: Implement the I/O routines")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/cachefiles/io.c       | 2 --
 fs/cachefiles/ondemand.c | 4 +---
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
index c08e4a66ac07..3e0576d9db1d 100644
--- a/fs/cachefiles/io.c
+++ b/fs/cachefiles/io.c
@@ -347,8 +347,6 @@ int __cachefiles_write(struct cachefiles_object *object,
 	default:
 		ki->was_async = false;
 		cachefiles_write_complete(&ki->iocb, ret);
-		if (ret > 0)
-			ret = 0;
 		break;
 	}
 
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index d9bc67176128..a7ed86fa98bb 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -83,10 +83,8 @@ static ssize_t cachefiles_ondemand_fd_write_iter(struct kiocb *kiocb,
 
 	trace_cachefiles_ondemand_fd_write(object, file_inode(file), pos, len);
 	ret = __cachefiles_write(object, file, pos, iter, NULL, NULL);
-	if (!ret) {
-		ret = len;
+	if (ret > 0)
 		kiocb->ki_pos += ret;
-	}
 
 out:
 	fput(file);
-- 
2.46.1

On Thu, 03 Jul 2025 10:44:18 +0800, Zizhi Wo wrote:
> In __cachefiles_write(), if the return value of the write operation > 0, it
> is set to 0. This makes it impossible to distinguish scenarios where a
> partial write has occurred, and will affect the outer calling functions:
> 
>  1) cachefiles_write_complete() will call "term_func" such as
> netfs_write_subrequest_terminated(). When "ret" in __cachefiles_write()
> is used as the "transferred_or_error" of this function, it can not
> distinguish the amount of data written, makes the WARN meaningless.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] cachefiles: Fix the incorrect return value in __cachefiles_write()
      https://git.kernel.org/vfs/vfs/c/6b89819b06d8

I think this should only affect erofs, right?

David

在 2025/7/10 0:03, David Howells 写道:
> I think this should only affect erofs, right?
> 
> David
> 
> 

Yes, currently other callers don't rely on the return value of
__cachefiles_write(); instead, they determine success or failure through
cachefiles_write_complete().

Therefore, resetting "ret" to 0 in __cachefiles_write() might be
unnecessary? When this step is removed, the outer
cachefiles_ondemand_fd_write_iter() can also correctly update the offset
based on ret.

Thanks,
Zizhi Wo

在 2025/7/3 10:44, Zizhi Wo 写道:
> From: Zizhi Wo <wozizhi@huawei.com>
> 
> In __cachefiles_write(), if the return value of the write operation > 0, it
> is set to 0. This makes it impossible to distinguish scenarios where a
> partial write has occurred, and will affect the outer calling functions:
> 
>   1) cachefiles_write_complete() will call "term_func" such as
> netfs_write_subrequest_terminated(). When "ret" in __cachefiles_write()
> is used as the "transferred_or_error" of this function, it can not
> distinguish the amount of data written, makes the WARN meaningless.
> 

Sorry, I was negligent. The first error actually doesn't exist because
ret=0 was set after cachefiles_write_complete(), but the second error
still exists.

Thanks,
Zizhi Wo

>   2) cachefiles_ondemand_fd_write_iter() can only assume all writes were
> successful by default when "ret" is 0, and unconditionally return the full
> length specified by user space.
> 
> Fix it by modifying "ret" to reflect the actual number of bytes written.
> Furthermore, returning a value greater than 0 from __cachefiles_write()
> does not affect other call paths, such as cachefiles_issue_write() and
> fscache_write().
> 
> Fixes: 047487c947e8 ("cachefiles: Implement the I/O routines")
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> ---
>   fs/cachefiles/io.c       | 2 --
>   fs/cachefiles/ondemand.c | 4 +---
>   2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
> index c08e4a66ac07..3e0576d9db1d 100644
> --- a/fs/cachefiles/io.c
> +++ b/fs/cachefiles/io.c
> @@ -347,8 +347,6 @@ int __cachefiles_write(struct cachefiles_object *object,
>   	default:
>   		ki->was_async = false;
>   		cachefiles_write_complete(&ki->iocb, ret);
> -		if (ret > 0)
> -			ret = 0;
>   		break;
>   	}
>   
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index d9bc67176128..a7ed86fa98bb 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -83,10 +83,8 @@ static ssize_t cachefiles_ondemand_fd_write_iter(struct kiocb *kiocb,
>   
>   	trace_cachefiles_ondemand_fd_write(object, file_inode(file), pos, len);
>   	ret = __cachefiles_write(object, file, pos, iter, NULL, NULL);
> -	if (!ret) {
> -		ret = len;
> +	if (ret > 0)
>   		kiocb->ki_pos += ret;
> -	}
>   
>   out:
>   	fput(file);