Re: PROBLEM: [REGRESSION 7.1-rc4 -> 7.1-rc5] 9p: silent NUL corruption of file data on cache modes with CACHE_WRITEBACK

[Dominique Martinet <asmadeus@codewreck.org>]

David Howells wrote on Fri, Sep 11, 2026 at 10:25:04AM +0100:
> commit a7f0af2e23c3ab191c08c1041dc67afd971a8c49
> Author: David Howells <dhowells@redhat.com>
> Date:   Fri Sep 11 10:15:43 2026 +0100
> 
>     9p: Fix v9fs_issue_write() to update i_size and remote_i_size
>     
>     Fix v9fs_issue_write() to update i_size and remote_i_size to the new size
>     of the server file if we made it larger, using the start fpos and the count
>     returned by p9_client_write() to calculate the new minimum file size.
>     
>     This assumes that if the 9P server makes a short write (say it hits
>     ENOSPC), a reduced count is returned.
>     
>     Fixes: 5fb70e7275a6 ("netfs, 9p: Implement helpers for new write code")
>     Reported-by: Michael Mulqueen <mike@method-b.uk>
>     Closes: https://lore.kernel.org/r/fbb9e395-1e07-4212-8f70-23f3cd498074@method-b.uk/
>     Signed-off-by: David Howells <dhowells@redhat.com>

Thanks for the patch, I sent it to lists as a proper patch so sashiko
process it (please send patches and write your comments after the `---`
line or just reply to the patches, it's not just for sashiko, it's also
much easier to pick up patches rather than chunks like that -- I had to
save the mail, manually edit and apply...)

Anyway, quoting the report
https://sashiko.dev/#/patchset/20260912071940.3-2-asmadeus%40codewreck.org
----
> diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
> index 2b6ca573f955a..52c4cf54b833c 100644
> --- a/fs/9p/vfs_addr.c
> +++ b/fs/9p/vfs_addr.c
> @@ -54,11 +54,37 @@ static void v9fs_begin_writeback(struct netfs_io_request *wreq)
>  static void v9fs_issue_write(struct netfs_io_subrequest *subreq)
>  {
>  	struct p9_fid *fid = subreq->rreq->netfs_priv;
> +	struct inode *inode = subreq->rreq->inode;
> +	struct netfs_inode *ictx = netfs_inode(inode);
>  	int err, len;
>  
>  	len = p9_client_write(fid, subreq->start, &subreq->io_iter, &err);
This is a pre-existing issue, but does using subreq->start here corrupt data
on retry?
If a network error or short write causes a subrequest to partially complete
and be marked for retry, the netfs retry logic resets the io_iter but does
not adjust subreq->start.
When v9fs_issue_write() is re-entered, writing the remaining data at the
original subreq->start offset rather than subreq->start + subreq->transferred
would overwrite previously written data.
Similarly, the new size calculation below propagates this flaw by failing to
include subreq->transferred, which underestimates the file size:
    uoff_t end = subreq->start + len, i_size, remote, zp;

> -	if (len > 0)
> +	if (len > 0) {
> +		uoff_t end = subreq->start + len, i_size, remote, zp;
> +		bool set = false;
> +
> +		spin_lock(&inode->i_lock);
> +
> +		/* We can read the sizes directly as we hold i_lock. */
> +		i_size = inode->i_size;
Does reading and updating i_size from this asynchronous writeback path race
with concurrent truncate operations?
If a concurrent truncate shrinks the file while this async writeback is in
flight, overriding i_size to the write's end position without holding i_rwsem
would undo the truncate's effect locally.
Also, reading inode->i_size directly without i_size_read() bypasses
i_size_seqcount, which can cause torn reads on 32-bit platforms.

(+ something about breaking builds, I guess sashiko's tree is missing
2c8f4742bb76 netfs: Fix potential for tearing in ->remote_i_size and
->zero_point ?
this patch does require it but it seems fine as that also got backported
to 7.0, so this fix can be picked up fine)
---

I don't want to think about what happens with parallel truncates so I'll
ignore this one, but the other one looks correct (checked AFS and CIFS
for comparison)

That can wait for the next cycle and I still indend of sending this
patch to Linus in a day or two, but please send a follow-up patch if you
have time (or I will when I can find time)

Thanks,
-- 
Dominique