Re: [PATCH 2/5] 9p: ignore a zero-copy reply that arrives after a fatal signal

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] p9_client_zc_rpc() (net/9p/client.c) no longer runs `if
  (READ_ONCE(req->status) == REQ_STATUS_RCVD) err = 0;` after the flush.

--

9p: ignore a zero-copy reply that arrives after a fatal signal

This patch changes p9_client_zc_rpc() so that err stays -ERESTARTSYS
after a fatal signal ends the transport wait, even if the reply arrived
before the RFLUSH. It does this to avoid parsing an unbounded or
uninitialized RERROR string from a zero-copy TREAD or TREADDIR reply that
p9_virtio_zc_request() never post-processed.

> diff --git a/net/9p/client.c b/net/9p/client.c
> index e63e8c2a853df..66980ad52bca8 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -706,10 +706,11 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
>  
>  		if (c->trans_mod->cancel(c, req))
>  			p9_client_flush(c, req);
> -
> -		/* if we received the response anyway, don't signal error */
> -		if (READ_ONCE(req->status) == REQ_STATUS_RCVD)
> -			err = 0;
> +		/*
> +		 * err stays -ERESTARTSYS even if the reply has arrived, since
> +		 * the transport moves the error string of a zero-copy reply
> +		 * into rc only while it waits.
> +		 */
>  	}

[Severity: Medium]

This change applies to every zero-copy request type, not only TREAD and
TREADDIR. Does it also drop a completed zero-copy TWRITE?

On virtio, p9_client_write() and p9_client_write_subreq() both send
P9_TWRITE through p9_client_zc_rpc() when the write is over 1024 bytes:

net/9p/client.c:p9_client_write_subreq() {
    ...
	if (clnt->trans_mod->zc_request && len > 1024) {
		req = p9_client_zc_rpc(clnt, P9_TWRITE, NULL, &subreq->io_iter,
				       0, wreq->len, P9_ZC_HDR_SZ, "dqd",
				       fid->fid, start, len);
    ...
}

Here is the sequence. A fatal signal ends io_wait_event_killable() in
p9_virtio_zc_request(). The server then finishes the write and sends
RWRITE before it handles the TFLUSH. p9_client_flush() returns with
req->status == REQ_STATUS_RCVD.

Before this patch, err was reset to 0 at that point. Now the RWRITE is
dropped and the caller gets -ERESTARTSYS, even though the server state has
changed. Under 9P flush semantics, a reply that arrives before Rflush
should be honoured.

On the netfs path, v9fs_issue_write() then sees len == 0:

fs/9p/vfs_addr.c:v9fs_issue_write() {
    ...
	netfs_write_subrequest_terminated(subreq, len ?: err);
}

So it skips the i_size/remote_i_size update and does not set
NETFS_SREQ_MADE_PROGRESS. netfs_collect_write_results() then calls
mapping_set_error(wreq->mapping, front->error) with -ERESTARTSYS.

Could a later fsync() from another opener then report a writeback error
for data the server already has? In the same way, v9fs_xattr_set() via
p9_client_write() would report failure for an xattr value that was
written.

The new comment says the transport moves the error string into rc only
while it waits. For TWRITE, inlen is 0 and no reply pages are posted. Isn't
the whole reply, including any RERROR string, already in rc.sdata then?

The commit message says:

  "since p9_client_zc_rpc() already returns -ERESTARTSYS when the server
  cancels such a request."

A cancelled write leaves the server unchanged, but a completed write does
not, so this reasoning doesn't seem to cover TWRITE.

For TWRITE, the only late-reply risk seems to be an rc.size that was never
clamped, because p9_check_errors() skips its size check when rc.zc is set.
On the late REQ_STATUS_RCVD path, would checking rc.size against
rc.capacity (or the posted in_hdr_len + inlen) fix the KASAN report while
keeping a valid RWRITE?

Also, the later commit "9p: take the tag of a flushed request from the
request" in this series makes a zero-copy TWRITE actually send a TFLUSH and
wait for RFLUSH. That widens the window for an RWRITE to arrive first, and
p9_client_zc_rpc() at the end of the series still leaves err as
-ERESTARTSYS in this case.

>  recalc_sigpending:
>  	if (sigpending) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109105251.2545764-1-lian%40is.s.u-tokyo.ac.jp