[PATCH 0/5] 9p: bound zero-copy error replies and keep flushed requests until the device returns them

[PATCH 0/5] 9p: bound zero-copy error replies and keep flushed requests until the device returns them

[Lianjie Wang]

This series fixes memory errors caused by a misbehaving virtio-9p device
in zero-copy requests and in the flush of an interrupted request. A
correct device can also cause a double put, which is fixed in patch 3. A
zero-copy write interrupted by a fatal signal can also overwrite a later
write to the same file, which is fixed in patch 4.

The patches were tested with QEMU modified to send the malformed or slow
replies.

Lianjie Wang (5):
  9p/virtio: clamp the reply size of a zero-copy request
  9p: ignore a zero-copy reply that arrives after a fatal signal
  9p/virtio: keep a request until the device returns it
  9p: take the tag of a flushed request from the request
  9p/virtio: keep zero-copy buffers until the device returns them

 include/net/9p/client.h |   6 ++
 net/9p/client.c         |  19 +++---
 net/9p/trans_virtio.c   | 130 ++++++++++++++++++++++++++++------------
 3 files changed, 106 insertions(+), 49 deletions(-)


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.56.0

[PATCH 1/5] 9p/virtio: clamp the reply size of a zero-copy request

[Lianjie Wang]

req_done() takes the used length from the device as the reply size
without checking it against the posted buffers, and p9_check_errors()
skips its size check for zero-copy requests, so reading an oversized
error reply to a zero-copy request can overrun the posted buffers:

  BUG: KASAN: slab-out-of-bounds in handle_rerror+0x144/0x230
  Read of size 8 at addr ffff888003065768 by task zc9p/157
  Call Trace:
   handle_rerror+0x144/0x230
   p9_virtio_zc_request+0xb8e/0xd70
   p9_client_zc_rpc+0x21c/0x970
   p9_client_read_once+0x237/0x6c0
  The buggy address is located 0 bytes to the right of
   allocated 8-byte region [ffff888003065760, ffff888003065768)

Also, handle_rerror() returns before truncating the reply size of a
zero-copy TWRITE, which posts no reply pages, so p9_check_errors() can
parse the error string of an oversized error reply past the end of
rc.sdata.

Fix this by clamping the reply size to the posted buffers upon getting
the reply.

This patch is assisted by Claude Opus 5.5.

Reported-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
Closes: https://lore.kernel.org/all/20260607140603.24342-1-zhaoyz24@mails.tsinghua.edu.cn/
Suggested-by: Dominique Martinet <asmadeus@codewreck.org>
Fixes: f615625a44c4 ("9p: handling Rerror without copy_from_iter_full()")
Cc: stable@vger.kernel.org
Signed-off-by: Lianjie Wang <lian@is.s.u-tokyo.ac.jp>
---
 net/9p/trans_virtio.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 9fa63e3ebf673..53046668c16b1 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -537,10 +537,14 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	 * threads - echoes to wmb() in the callback
 	 */
 	smp_rmb();
-	// RERROR needs reply (== error string) in static data
-	if (READ_ONCE(req->status) == REQ_STATUS_RCVD &&
-	    unlikely(req->rc.sdata[4] == P9_RERROR))
-		handle_rerror(req, in_hdr_len, offs, in_pages);
+	if (READ_ONCE(req->status) == REQ_STATUS_RCVD) {
+		// The device may report more than the buffers we posted
+		req->rc.size = umin(req->rc.size, in_hdr_len + inlen);
+
+		// RERROR needs reply (== error string) in static data
+		if (unlikely(req->rc.sdata[4] == P9_RERROR))
+			handle_rerror(req, in_hdr_len, offs, in_pages);
+	}
 
 	/*
 	 * Non kernel buffers are pinned, unpin them
-- 
2.56.0

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

[Lianjie Wang]

When a fatal signal ends the wait of p9_virtio_zc_request() for a
zero-copy TREAD or TREADDIR, the device can still answer the request
before the RFLUSH, and p9_client_zc_rpc() parses that reply.
p9_virtio_zc_request() has returned by then, so it neither bounds the
reply size nor moves the error string of an RERROR from the reply pages
into rc.sdata, and p9_check_errors() skips its size check for zero-copy
requests, so it parses the error string of an oversized reply past the
end of rc.sdata:

  BUG: KASAN: slab-out-of-bounds in p9pdu_readf+0x151e/0x1c50
  Read of size 8179 at addr ffff888004724009 by task zc9p/155
  Call Trace:
   p9pdu_readf+0x151e/0x1c50
   p9_check_errors+0x23d/0x510
   p9_client_zc_rpc+0x4f0/0x970
   p9_client_read_once+0x237/0x6c0

Also, the error string that fits within rc.sdata is parsed from
rc.sdata, but the device wrote only the header there, so
p9_errstr2errno() can print the error from uninitialized memory.

Fix this by ignoring the reply when a fatal signal ends the wait of a
zero-copy request, since p9_client_zc_rpc() already returns -ERESTARTSYS
when the server cancels such a request.

This patch is assisted by Claude Opus 5.5.

Fixes: f615625a44c4 ("9p: handling Rerror without copy_from_iter_full()")
Cc: stable@vger.kernel.org
Signed-off-by: Lianjie Wang <lian@is.s.u-tokyo.ac.jp>
---
 net/9p/client.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

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.
+		 */
 	}
 recalc_sigpending:
 	if (sigpending) {
-- 
2.56.0

[PATCH 3/5] 9p/virtio: keep a request until the device returns it

[Lianjie Wang]

When the RFLUSH of a pending request arrives, p9_virtio_cancelled()
drops the virtio transport's reference to it. However,
p9_virtio_zc_request() has already dropped that reference if a fatal
signal ended its wait for room in the ring, since the request then never
reached the device. Such a request is still flushed, so the reference is
dropped twice. This can happen when concurrent requests fill the ring
and a waiting process is killed:

  refcount_t: underflow; use-after-free.
  WARNING: lib/refcount.c:37 at refcount_warn_saturate+0x84/0xd0, CPU#3: dd/293
  Call Trace:
   p9_req_put+0xe5/0x190
   p9_client_zc_rpc+0x52e/0x970
   p9_client_read_once+0x237/0x6c0
   p9_client_read+0x9d/0x140

Additionally, a misbehaving device can return the request after the
RFLUSH, so req_done() writes into the freed request:

  BUG: KASAN: slab-use-after-free in req_done+0x1d9/0x280
  Write of size 4 at addr ffff88800392d758 by task swapper/1/0
  Call Trace:
   req_done+0x1d9/0x280
   vring_interrupt+0x32a/0x540
  The buggy address belongs to the object at ffff88800392d708
   which belongs to the cache p9_req_t of size 136
  The buggy address is located 80 bytes inside of
   freed 136-byte region [ffff88800392d708, ffff88800392d790)

Fix this by dropping the reference in req_done() for a buffer without a
reply as well, since the device can return a flushed request without
one. p9_virtio_cancelled() is no longer needed.

This patch is assisted by Claude Opus 5.5.

Fixes: 728356dedeff ("9p: Add refcount to p9_req_t")
Cc: stable@vger.kernel.org
Signed-off-by: Lianjie Wang <lian@is.s.u-tokyo.ac.jp>
---
 net/9p/trans_virtio.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 53046668c16b1..68c3457bec401 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -145,6 +145,9 @@ static void req_done(struct virtqueue *vq)
 		if (len) {
 			req->rc.size = len;
 			p9_client_cb(chan->client, req, REQ_STATUS_RCVD);
+		} else {
+			/* No reply, so put the ref that p9_client_cb() puts */
+			p9_req_put(chan->client, req);
 		}
 	}
 	spin_unlock_irqrestore(&chan->lock, flags);
@@ -195,13 +198,6 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req)
 	return 1;
 }
 
-/* Reply won't come, so drop req ref */
-static int p9_virtio_cancelled(struct p9_client *client, struct p9_req_t *req)
-{
-	p9_req_put(client, req);
-	return 0;
-}
-
 /**
  * pack_sg_list_p - Just like pack_sg_list. Instead of taking a buffer,
  * this takes a list of pages.
@@ -806,7 +802,6 @@ static struct p9_trans_module p9_virtio_trans = {
 	.request = p9_virtio_request,
 	.zc_request = p9_virtio_zc_request,
 	.cancel = p9_virtio_cancel,
-	.cancelled = p9_virtio_cancelled,
 	/*
 	 * We leave one entry for input and one entry for response
 	 * headers. We also skip one more entry to accommodate, address
-- 
2.56.0

[PATCH 4/5] 9p: take the tag of a flushed request from the request

[Lianjie Wang]

p9_client_flush() finds the tag of the request to cancel by parsing the
header of oldreq->tc. For a zero-copy TWRITE, however,
p9_virtio_zc_request() adds the payload length to the size in the header
after marshalling, so p9_parse_header() fails on the mismatch with
tc.size and no TFLUSH is sent. As a result, a zero-copy write
interrupted by a fatal signal returns without waiting for the RFLUSH,
and the server can still write the data after the writer is killed,
overwriting a later write to the same file.

Use the tag in oldreq->tc.tag and drop the parse, so a TFLUSH is sent.

This patch is assisted by Claude Opus 5.5.

Fixes: d28c756caee6 ("9p/net: Fix zero-copy path in the 9p virtio transport")
Cc: stable@vger.kernel.org
Signed-off-by: Lianjie Wang <lian@is.s.u-tokyo.ac.jp>
---
 net/9p/client.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index 66980ad52bca8..15bb939087165 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -477,12 +477,7 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...);
 static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
 {
 	struct p9_req_t *req;
-	s16 oldtag;
-	int err;
-
-	err = p9_parse_header(&oldreq->tc, NULL, NULL, &oldtag, 1);
-	if (err)
-		return err;
+	s16 oldtag = oldreq->tc.tag;
 
 	p9_debug(P9_DEBUG_9P, ">>> TFLUSH tag %d\n", oldtag);
 
-- 
2.56.0

[PATCH 5/5] 9p/virtio: keep zero-copy buffers until the device returns them

[Lianjie Wang]

When a fatal signal ends the wait of p9_virtio_zc_request() while the
device still holds the request, the payload pages are released, and
p9_client_zc_rpc() returns to its caller once the RFLUSH arrives. Since
the caller may free the buffers, a misbehaving device can answer the
request after the RFLUSH and write the reply into freed memory. With the
payload bounced through swiotlb, the guest itself copies the reply into
the freed memory when req_done() unmaps the returned buffers:

  BUG: KASAN: use-after-free in swiotlb_bounce+0x236/0x270
  Write of size 4080 at addr ffff888004800010 by task swapper/1/0
  Call Trace:
   swiotlb_bounce+0x236/0x270
   req_done+0xb2/0x280

Fix this by releasing the pages in req_done() once the device returns
the request, when the wait ends without a reply. req_done() can find the
pages only through the request, so the page arrays, their page counts
and need_drop are gathered into struct p9_zc_pages allocated for each
zero-copy request. p9_client_zc_rpc() now waits until the device returns
the request, since the caller may reuse or free the buffers after
p9_client_zc_rpc() returns.

This patch is assisted by Claude Opus 5.5.

Fixes: abfa034e4b8e ("fs/9p: Update zero-copy implementation in 9p")
Cc: stable@vger.kernel.org
Signed-off-by: Lianjie Wang <lian@is.s.u-tokyo.ac.jp>
---
 include/net/9p/client.h |   6 +++
 net/9p/client.c         |   3 ++
 net/9p/trans_virtio.c   | 109 +++++++++++++++++++++++++++++-----------
 3 files changed, 90 insertions(+), 28 deletions(-)

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index e4e5cb102fb18..8f9074e69c54a 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -71,6 +71,8 @@ enum p9_req_status_t {
 	REQ_STATUS_ERROR,
 };
 
+struct p9_zc_pages;
+
 /**
  * struct p9_req_t - request slots
  * @status: status of this request slot
@@ -79,6 +81,9 @@ enum p9_req_status_t {
  * @tc: the request fcall structure
  * @rc: the response fcall structure
  * @req_list: link for higher level objects to chain requests
+ * @zc_pages: the zero-copy payload pages, set by the transport while the
+ *            device may access them and cleared with a wakeup on @wq when
+ *            the device returns them
  */
 struct p9_req_t {
 	int status;
@@ -88,6 +93,7 @@ struct p9_req_t {
 	struct p9_fcall tc;
 	struct p9_fcall rc;
 	struct list_head req_list;
+	struct p9_zc_pages *zc_pages;
 };
 
 /**
diff --git a/net/9p/client.c b/net/9p/client.c
index 15bb939087165..34be0d2652d4e 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -188,6 +188,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, uint t_size, uint r_size,
 	refcount_set(&req->refcount, 0);
 	init_waitqueue_head(&req->wq);
 	INIT_LIST_HEAD(&req->req_list);
+	req->zc_pages = NULL;
 
 	idr_preload(GFP_NOFS);
 	spin_lock_irq(&c->lock);
@@ -707,6 +708,8 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
 		 * into rc only while it waits.
 		 */
 	}
+	/* The caller may reuse the payload once the transport releases it */
+	wait_event_idle(req->wq, !smp_load_acquire(&req->zc_pages));
 recalc_sigpending:
 	if (sigpending) {
 		spin_lock_irqsave(&current->sighand->siglock, flags);
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 68c3457bec401..d6914e5b36972 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -42,6 +42,43 @@ static DEFINE_MUTEX(virtio_9p_lock);
 static DECLARE_WAIT_QUEUE_HEAD(vp_wq);
 static atomic_t vp_pinned = ATOMIC_INIT(0);
 
+/**
+ * struct p9_zc_pages - the pages mapped for the device by a zero-copy request
+ * @in_pages: the pages of the reply payload
+ * @out_pages: the pages of the request payload
+ * @in_nr_pages: the number of reply pages
+ * @out_nr_pages: the number of request pages
+ * @need_drop: whether p9_zc_release() drops a reference to each page
+ * @held: whether req_done() releases this structure
+ */
+struct p9_zc_pages {
+	struct page **in_pages;
+	struct page **out_pages;
+	int in_nr_pages;
+	int out_nr_pages;
+	int need_drop;
+	bool held;
+};
+
+static void p9_zc_release(struct p9_zc_pages *zc)
+{
+	if (zc->need_drop) {
+		if (zc->in_pages) {
+			p9_release_pages(zc->in_pages, zc->in_nr_pages);
+			atomic_sub(zc->in_nr_pages, &vp_pinned);
+		}
+		if (zc->out_pages) {
+			p9_release_pages(zc->out_pages, zc->out_nr_pages);
+			atomic_sub(zc->out_nr_pages, &vp_pinned);
+		}
+		/* wakeup anybody waiting for slots to pin pages */
+		wake_up(&vp_wq);
+	}
+	kvfree(zc->in_pages);
+	kvfree(zc->out_pages);
+	kfree(zc);
+}
+
 /**
  * struct virtio_chan - per-instance transport information
  * @inuse: whether the channel is in use
@@ -142,6 +179,15 @@ static void req_done(struct virtqueue *vq)
 			need_wakeup = true;
 		}
 
+		/* The device has returned the buffers of a zero-copy request */
+		if (req->zc_pages) {
+			if (req->zc_pages->held)
+				p9_zc_release(req->zc_pages);
+			/* pairs with smp_load_acquire() in p9_client_zc_rpc() */
+			smp_store_release(&req->zc_pages, NULL);
+			wake_up(&req->wq);
+		}
+
 		if (len) {
 			req->rc.size = len;
 			p9_client_cb(chan->client, req, REQ_STATUS_RCVD);
@@ -419,25 +465,30 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 {
 	int in, out, err, out_sgs, in_sgs;
 	unsigned long flags;
-	int in_nr_pages = 0, out_nr_pages = 0;
-	struct page **in_pages = NULL, **out_pages = NULL;
+	struct p9_zc_pages *zc;
 	struct virtio_chan *chan = client->trans;
 	struct scatterlist *sgs[4];
 	size_t offs = 0;
-	int need_drop = 0;
 	int kicked = 0;
+	bool held;
 
 	p9_debug(P9_DEBUG_TRANS, "virtio request\n");
 
+	zc = kzalloc_obj(*zc, GFP_NOFS);
+	if (!zc) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	if (uodata) {
 		__le32 sz;
-		int n = p9_get_mapped_pages(chan, &out_pages, uodata,
-					    outlen, &offs, &need_drop);
+		int n = p9_get_mapped_pages(chan, &zc->out_pages, uodata,
+					    outlen, &offs, &zc->need_drop);
 		if (n < 0) {
 			err = n;
 			goto err_out;
 		}
-		out_nr_pages = DIV_ROUND_UP(n + offs, PAGE_SIZE);
+		zc->out_nr_pages = DIV_ROUND_UP(n + offs, PAGE_SIZE);
 		if (n != outlen) {
 			__le32 v = cpu_to_le32(n);
 			memcpy(&req->tc.sdata[req->tc.size - 4], &v, 4);
@@ -450,13 +501,13 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 		sz = cpu_to_le32(req->tc.size + outlen);
 		memcpy(&req->tc.sdata[0], &sz, sizeof(sz));
 	} else if (uidata) {
-		int n = p9_get_mapped_pages(chan, &in_pages, uidata,
-					    inlen, &offs, &need_drop);
+		int n = p9_get_mapped_pages(chan, &zc->in_pages, uidata,
+					    inlen, &offs, &zc->need_drop);
 		if (n < 0) {
 			err = n;
 			goto err_out;
 		}
-		in_nr_pages = DIV_ROUND_UP(n + offs, PAGE_SIZE);
+		zc->in_nr_pages = DIV_ROUND_UP(n + offs, PAGE_SIZE);
 		if (n != inlen) {
 			__le32 v = cpu_to_le32(n);
 			memcpy(&req->tc.sdata[req->tc.size - 4], &v, 4);
@@ -476,10 +527,10 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	if (out)
 		sgs[out_sgs++] = chan->sg;
 
-	if (out_pages) {
+	if (zc->out_pages) {
 		sgs[out_sgs++] = chan->sg + out;
 		out += pack_sg_list_p(chan->sg, out, VIRTQUEUE_NUM,
-				      out_pages, out_nr_pages, offs, outlen);
+				      zc->out_pages, zc->out_nr_pages, offs, outlen);
 	}
 
 	/*
@@ -494,10 +545,10 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	if (in)
 		sgs[out_sgs + in_sgs++] = chan->sg + out;
 
-	if (in_pages) {
+	if (zc->in_pages) {
 		sgs[out_sgs + in_sgs++] = chan->sg + out + in;
 		pack_sg_list_p(chan->sg, out + in, VIRTQUEUE_NUM,
-			       in_pages, in_nr_pages, offs, inlen);
+			       zc->in_pages, zc->in_nr_pages, offs, inlen);
 	}
 
 	BUG_ON(out_sgs + in_sgs > ARRAY_SIZE(sgs));
@@ -522,6 +573,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 			goto err_out;
 		}
 	}
+	req->zc_pages = zc;
 	virtqueue_kick(chan->vq);
 	spin_unlock_irqrestore(&chan->lock, flags);
 	kicked = 1;
@@ -539,27 +591,28 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 
 		// RERROR needs reply (== error string) in static data
 		if (unlikely(req->rc.sdata[4] == P9_RERROR))
-			handle_rerror(req, in_hdr_len, offs, in_pages);
+			handle_rerror(req, in_hdr_len, offs, zc->in_pages);
+	} else {
+		/*
+		 * A fatal signal ended the wait, and the device may still hold
+		 * the request, so let req_done() release the pages unless the
+		 * device has already returned the request.
+		 */
+		spin_lock_irqsave(&chan->lock, flags);
+		held = req->zc_pages;
+		if (held)
+			zc->held = true;
+		spin_unlock_irqrestore(&chan->lock, flags);
+		if (held)
+			return err;
 	}
 
 	/*
 	 * Non kernel buffers are pinned, unpin them
 	 */
 err_out:
-	if (need_drop) {
-		if (in_pages) {
-			p9_release_pages(in_pages, in_nr_pages);
-			atomic_sub(in_nr_pages, &vp_pinned);
-		}
-		if (out_pages) {
-			p9_release_pages(out_pages, out_nr_pages);
-			atomic_sub(out_nr_pages, &vp_pinned);
-		}
-		/* wakeup anybody waiting for slots to pin pages */
-		wake_up(&vp_wq);
-	}
-	kvfree(in_pages);
-	kvfree(out_pages);
+	p9_zc_release(zc);
+out:
 	if (!kicked) {
 		/* reply won't come */
 		p9_req_put(client, req);
-- 
2.56.0

Re: [PATCH 1/5] 9p/virtio: clamp the reply size of a zero-copy request

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The clamp added to p9_virtio_zc_request()
  (net/9p/trans_virtio.c) is `req->rc.size = umin(req->rc.size,…

--

9p/virtio: clamp the reply size of a zero-copy request

This commit clamps req->rc.size in p9_virtio_zc_request() to the total
size of the buffers posted to the device (in_hdr_len + inlen). The clamp
happens before handle_rerror() runs. The aim is to stop oversized error
replies to zero-copy requests from being parsed past the posted buffers.

Closes: https://lore.kernel.org/all/20260607140603.24342-1-zhaoyz24@mails.tsinghua.edu.cn/

> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 9fa63e3ebf673..53046668c16b1 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -537,10 +537,14 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
>  	 * threads - echoes to wmb() in the callback
>  	 */
>  	smp_rmb();
> -	// RERROR needs reply (== error string) in static data
> -	if (READ_ONCE(req->status) == REQ_STATUS_RCVD &&
> -	    unlikely(req->rc.sdata[4] == P9_RERROR))
> -		handle_rerror(req, in_hdr_len, offs, in_pages);
> +	if (READ_ONCE(req->status) == REQ_STATUS_RCVD) {
> +		// The device may report more than the buffers we posted
> +		req->rc.size = umin(req->rc.size, in_hdr_len + inlen);
> +
> +		// RERROR needs reply (== error string) in static data
> +		if (unlikely(req->rc.sdata[4] == P9_RERROR))
> +			handle_rerror(req, in_hdr_len, offs, in_pages);
> +	}

[Severity: High]
Can an oversized P9_RLERROR reply to a zero-copy TREAD still make
p9_check_errors() read past the end of rc.sdata?

For a zero-copy TREAD, p9_client_read_once() passes in_hdr_len = 11 and
inlen = rsize, which can be up to msize - 24. So the new bound can be
much larger than rc.capacity. rc.sdata is a P9_ZC_HDR_SZ (4096) byte
allocation, and only its first 11 bytes are posted to the device.

The only truncation to the rc.sdata size is in handle_rerror():

    if (unlikely(req->rc.size > P9_ZC_HDR_SZ))
        req->rc.size = P9_ZC_HDR_SZ;

That runs only when sdata[4] == P9_RERROR. p9_check_errors() skips its
capacity check for zc replies, and it handles both error types:

    if (req->rc.size > req->rc.capacity && !req->rc.zc) {
    ...
    if (type != P9_RERROR && type != P9_RLERROR)
        return 0;

On a non-.L client, both types are parsed with "s?d". The client can be
non-.L because of the mount option. It can also be non-.L because the
device answers RVERSION with "9P2000" or "9P2000.u", which
p9_client_version() accepts.

Here is the case. The device answers a zero-copy TREAD with an 11-byte
header that has type P9_RLERROR and size = 11 + inlen, plus a large
string length, and it reports used len = 11 + inlen. Then:

    p9_virtio_zc_request()
        rc.size stays 11 + inlen, handle_rerror() is skipped
    p9_check_errors()
        p9pdu_readf(&req->rc, ..., "s?d", &ename, &ecode)
            pdu_read()
                len = min(pdu->size - pdu->offset, size);
                memcpy(data, &pdu->sdata[pdu->offset], len);

pdu_read() is bounded by pdu->size, not pdu->capacity. Could that read
tens of KiB past the 4096-byte rc.sdata object?

With legacy 9P2000, the string then goes to p9_errstr2errno(), which
calls pr_err() for unknown error strings. Would that put adjacent slab
contents into the kernel log?

The commit message says the patch prevents p9_check_errors() from
parsing "the error string of an oversized error reply past the end of
rc.sdata". This case still seems reachable after the patch.

Clamping rc.size to rc.capacity for every zc reply would break
successful reads. p9_client_read_once() bounds the received count by
rc.size via "D". Would it work better to give P9_RLERROR the same
truncation and copy that P9_RERROR gets in handle_rerror()? Another
option is to bound the error-string parse in p9_check_errors() by
rc.capacity for zc replies.

>  
>  	/*
>  	 * Non kernel buffers are pinned, unpin them

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

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

[netdev-bot+sashiko]

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

Re: [PATCH 3/5] 9p/virtio: keep a request until the device returns it

[netdev-bot+sashiko]

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

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

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).

- [High] Client teardown still frees requests the virtio device has not
  returned.

Pre-existing issues:
- [Critical] At this commit, p9_virtio_zc_request() releases the zero-
  copy payload pages while the device may still own them.
- [Low] The refcount comment in p9_tag_alloc() (net/9p/client.c:205-214)
  says that for virtio 'p9_client_cb eats the second ref there as the…

--

9p/virtio: keep a request until the device returns it

This patch removes p9_virtio_cancelled(), so a flushed request no longer
loses its virtio transport reference twice. Instead, req_done() drops
that reference when the device returns a buffer with no reply. The aim is
to keep the request alive until the device hands its buffer back, which
fixes a refcount underflow and a use-after-free on a late return.

> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 53046668c16b1..68c3457bec401 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -145,6 +145,9 @@ static void req_done(struct virtqueue *vq)
>  		if (len) {
>  			req->rc.size = len;
>  			p9_client_cb(chan->client, req, REQ_STATUS_RCVD);
> +		} else {
> +			/* No reply, so put the ref that p9_client_cb() puts */
> +			p9_req_put(chan->client, req);
>  		}

[Severity: High]
Is the request still kept until the device returns it when the client
is torn down?

On umount, p9_client_destroy() calls ->close() and then frees the
outstanding requests and the client:

net/9p/client.c:p9_client_destroy() {
	if (clnt->trans_mod)
		clnt->trans_mod->close(clnt);
	...
	p9_tag_cleanup(clnt);

	kmem_cache_destroy(clnt->fcall_cache);
	kfree(clnt);
}

p9_virtio_close() only clears inuse:

net/9p/trans_virtio.c:p9_virtio_close() {
	mutex_lock(&virtio_9p_lock);
	if (chan)
		chan->inuse = false;
	mutex_unlock(&virtio_9p_lock);
}

It does not reset the device, detach posted buffers, wait for
outstanding tokens, or clear chan->client. Its kernel-doc also says it
frees the channel's resources, which doesn't match the code.

p9_tag_cleanup() then calls p9_req_put() on every request still in the
IDR. That includes a flushed request whose last reference now belongs
to the device. So the request and its tc/rc sdata are freed while the
vring still points at them.

If the device returns that buffer later, vring_interrupt()->req_done()
gets the stale pointer from virtqueue_get_buf():

  len == 0: the new branch calls p9_req_put(chan->client, req) on freed
            memory
  len != 0: p9_client_cb() writes req->rc.size and the status, wakes
            req->wq and drops a reference

chan->client is then either the freed client or, if the tag was
remounted, a new client. p9_virtio_create() reassigns chan->client
without taking chan->lock.

p9_req_cache is a single SLAB_TYPESAFE_BY_RCU cache, so the freed
request may already be a live request of the new mount. Can the stray
put then decrement a live refcount and p9_tag_remove() a live tag from
the new client's IDR?

This looks like the case the commit message describes:

  "a misbehaving device can return the request after the RFLUSH, so
  req_done() writes into the freed request"

Here it is reached by unmounting between the RFLUSH and the late return.

A well-behaved device can hit the same problem if a request is abandoned
without a completed flush and the device is still working on it at
umount. This happens when the client is no longer Connected (for
example, BeginDisconnect after umount -f) or when a fatal signal aborts
the TFLUSH wait.

The later patch "9p/virtio: keep zero-copy buffers until the device
returns them" adds wait_event_idle() on req->zc_pages in
p9_client_zc_rpc(). That only covers zero-copy requests. Requests sent
through p9_client_rpc(), TFLUSH included, still return without waiting
for the device. Neither p9_virtio_close() nor p9_tag_cleanup() changes
in the series.

Before the request pool and the client are freed, should the close path
take back the posted buffers, either by resetting the device and
detaching them or by waiting for outstanding tokens? Or should
chan->client be cleared, or kept valid, under chan->lock?

[Severity: Low]
This is a pre-existing issue, but the refcount comment in p9_tag_alloc()
still names p9_client_cb() as the only place that drops the second
virtio reference:

net/9p/client.c:p9_tag_alloc() {
	...
	 * (the write happens synchronously in the request/zc_request
	 * callback), so p9_client_cb eats the second ref there
	 * as the pointer is duplicated directly by virtqueue_add_sgs()
	 */
	refcount_set(&req->refcount, 2);
	...
}

After this patch, req_done() also drops that reference directly when
len == 0. The comment also leaves out existing puts: in
p9_virtio_zc_request() when the request was not kicked, and in
p9_client_rpc() when ->request() fails. It never mentioned
p9_virtio_cancelled() either, and it is unchanged at the end of the
series.

Could it be updated to list the current places that drop this
reference?

>  	}
>  	spin_unlock_irqrestore(&chan->lock, flags);
> @@ -195,13 +198,6 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req)
>  	return 1;
>  }
>  
> -/* Reply won't come, so drop req ref */
> -static int p9_virtio_cancelled(struct p9_client *client, struct p9_req_t *req)
> -{
> -	p9_req_put(client, req);
> -	return 0;
> -}
> -

[Severity: Critical]
This isn't a bug introduced by this patch, but it is in the same fatal
signal handling in p9_virtio_zc_request(). After the kick, a fatal
signal during the reply wait falls through to err_out. That path then
releases the zero-copy pages while the device may still own them:

net/9p/trans_virtio.c:p9_virtio_zc_request() {
	...
	virtqueue_kick(chan->vq);
	spin_unlock_irqrestore(&chan->lock, flags);
	kicked = 1;
	...
	err = io_wait_event_killable(req->wq,
				     READ_ONCE(req->status) >= REQ_STATUS_RCVD);
	...
err_out:
	if (need_drop) {
		if (in_pages) {
			p9_release_pages(in_pages, in_nr_pages);
			atomic_sub(in_nr_pages, &vp_pinned);
		}
	...
	kvfree(in_pages);
	kvfree(out_pages);
	...
}

TFLUSH has not been sent at this point. Can the device still DMA the
read payload into pages that are unpinned and possibly reused? Or into
a kvec buffer that the caller frees once the call returns?

The later patch in this series, "9p/virtio: keep zero-copy buffers until
the device returns them", appears to fix this. It attaches the pages to
req->zc_pages before the kick and leaves p9_zc_release() to req_done()
after a fatal signal. p9_client_zc_rpc() then waits with
wait_event_idle() until the device returns the buffers.

[ ... ]

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

Re: [PATCH 5/5] 9p/virtio: keep zero-copy buffers until the device returns them

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The patch adds an unconditional `wait_event_idle(req->wq,
  !smp_load_acquire(&req->zc_pages))` to p9_client_zc_rpc()…

--

9p/virtio: keep zero-copy buffers until the device returns them

This patch fixes a use-after-free. If a fatal signal ends the wait in
p9_virtio_zc_request() while the device still holds the request, a
misbehaving device can later write into zero-copy buffers that have
already been freed. Freeing of the pinned pages moves to req_done() via
a per-request struct p9_zc_pages, and p9_client_zc_rpc() now waits until
the device returns the request.

> diff --git a/net/9p/client.c b/net/9p/client.c
> index 15bb939087165..34be0d2652d4e 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c

[ ... ]

> @@ -707,6 +708,8 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
>  		 * into rc only while it waits.
>  		 */
>  	}
> +	/* The caller may reuse the payload once the transport releases it */
> +	wait_event_idle(req->wq, !smp_load_acquire(&req->zc_pages));

[Severity: High]

Can this wait leave a killed task stuck forever?

wait_event_idle() sleeps in TASK_IDLE and has no timeout, so SIGKILL
cannot end it. khungtaskd also skips TASK_NOLOAD tasks, so the hang
would never be reported.

The only code that clears req->zc_pages and wakes req->wq is in
req_done(). It runs only after virtqueue_get_buf() returns this exact
request:

net/9p/trans_virtio.c:req_done() {
    ...
	while ((req = virtqueue_get_buf(chan->vq, &len)) != NULL) {
		...
		if (req->zc_pages) {
			if (req->zc_pages->held)
				p9_zc_release(req->zc_pages);
			smp_store_release(&req->zc_pages, NULL);
			wake_up(&req->wq);
		}
    ...
}

On a fatal signal, p9_virtio_zc_request() hands the pages to req_done()
and returns with req->zc_pages still set:

		held = req->zc_pages;
		if (held)
			zc->held = true;
		spin_unlock_irqrestore(&chan->lock, flags);
		if (held)
			return err;

p9_client_zc_rpc() sends a TFLUSH only when c->status == Connected.
There are two cases where no flush is sent but the new wait still runs:

  - after umount -f, via
    v9fs_session_begin_cancel()->p9_client_begin_disconnect()
  - after an earlier -EIO has set Disconnected

If a flush is sent, p9_client_rpc(TFLUSH) can itself return early on the
fatal signal. In every case the wait above runs.

If the device never returns the original descriptor, nothing else clears
zc_pages. That can happen in several ways:

  - virtqueue_kick() fails and sets vq->broken. Its return value is
    ignored right after req->zc_pages = zc in p9_virtio_zc_request().
  - surprise PCI removal. virtio_pci_remove() calls
    virtio_break_device(), and vring_interrupt() then stops calling the
    callback.
  - a hung backend, or a device that answers the TFLUSH but keeps the
    original descriptor. This is the misbehaving-device case the commit
    message describes.

Before this patch, a killed task always left this path.

Once stuck, the task keeps any VFS locks its caller holds and cannot
finish exiting or coredumping. Its pinned pages stay counted in
vp_pinned, which throttles p9_get_mapped_pages() on other mounts. It
also keeps the mount busy, so p9_virtio_close() never runs and
chan->inuse stays true. p9_virtio_remove() then loops here and never
reaches virtio_reset_device()/del_vqs():

	while (chan->inuse) {
		mutex_unlock(&virtio_9p_lock);
		msleep(250);

That is a circular wait. Remove waits for umount, umount waits for the
task, and the task waits for a req_done() that never runs for this
request.

For pinned user pages (need_drop == 1), the zc->held hand-off already
keeps the pages alive until req_done(), so the unconditional wait seems
to be needed only for kvec buffers. Could the wait be limited to that
case, or bounded or made killable? Alternatively, could the transport
clear zc_pages and wake waiters when the vq is broken or torn down?

The commit message also does not mention that killed tasks can now
block on the device with no time limit. Should it?

>  recalc_sigpending:
>  	if (sigpending) {
>  		spin_lock_irqsave(&current->sighand->siglock, flags);

[ ... ]

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