Re: [PATCH] net: 9p: usbg: clear stale client pointer on close

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7303 bytes --]

Hi

I have some review feedback.

On Sat, Mar 14, 2026 at 02:16:59AM +0900, Hyungjung Joo wrote:
>p9_usbg_close() tears down the client transport, but usb9pfs keeps
>using usb9pfs->client from asynchronous TX and RX completion handlers.
>A late completion can therefore dereference a client that has already
>been freed during mount teardown.
>
>Clear usb9pfs->client under usb9pfs->lock when closing the transport,
>detach any pending TX request from in_req->context, and make the TX/RX
>completion handlers bail out once the transport has been detached. This
>keeps late completions from touching a freed or rebound p9_client.
>
>Fixes: a3be076dc174 ("net/9p/usbg: Add new usb gadget function transport")
>Cc: stable@vger.kernel.org
>Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Signed-off-by: Hyungjung Joo <jhj140711@gmail.com>
>---
> net/9p/trans_usbg.c | 63 +++++++++++++++++++++++++++++++++------------
> 1 file changed, 47 insertions(+), 16 deletions(-)
>
>diff --git a/net/9p/trans_usbg.c b/net/9p/trans_usbg.c
>index 1ce70338999c..3c2aa1943f93 100644
>--- a/net/9p/trans_usbg.c
>+++ b/net/9p/trans_usbg.c
>@@ -149,7 +149,8 @@ static void usb9pfs_tx_complete(struct usb_ep *ep, struct usb_request *req)
> {
> 	struct f_usb9pfs *usb9pfs = ep->driver_data;
> 	struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
>-	struct p9_req_t *p9_tx_req = req->context;
>+	struct p9_client *client;
>+	struct p9_req_t *p9_tx_req;
> 	unsigned long flags;
>
> 	/* reset zero packages */
>@@ -165,18 +166,27 @@ static void usb9pfs_tx_complete(struct usb_ep *ep, struct usb_request *req)
> 		ep->name, req->status, req->actual, req->length);
>
> 	spin_lock_irqsave(&usb9pfs->lock, flags);
>-	WRITE_ONCE(p9_tx_req->status, REQ_STATUS_SENT);
>+	client = usb9pfs->client;
>+	p9_tx_req = req->context;
>+	req->context = NULL;
>
>-	p9_req_put(usb9pfs->client, p9_tx_req);
>+	if (!client || !p9_tx_req) {

Just goto unlock_complete;

>+		spin_unlock_irqrestore(&usb9pfs->lock, flags);
>+		complete(&usb9pfs->send);
>+		return;
>+	}
>
>-	req->context = NULL;
>+	WRITE_ONCE(p9_tx_req->status, REQ_STATUS_SENT);
>+
>+	p9_req_put(client, p9_tx_req);
>

unlock_complete:
> 	spin_unlock_irqrestore(&usb9pfs->lock, flags);
>
> 	complete(&usb9pfs->send);
> }
>
>-static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs, void *buf)
>+static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs,
>+					  struct p9_client *client, void *buf)

I like this change but not in this patch. Since with your patch,
rx_header will be called under locking context, this is not
really a necessary change.

I would keep it in another patch, though and move it before your change.

> {
> 	struct p9_req_t *p9_rx_req;
> 	struct p9_fcall	rc;
>@@ -202,7 +212,7 @@ static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs, void *buf)
> 		 "mux %p pkt: size: %d bytes tag: %d\n",
> 		 usb9pfs, rc.size, rc.tag);
>
>-	p9_rx_req = p9_tag_lookup(usb9pfs->client, rc.tag);
>+	p9_rx_req = p9_tag_lookup(client, rc.tag);
> 	if (!p9_rx_req || p9_rx_req->status != REQ_STATUS_SENT) {
> 		p9_debug(P9_DEBUG_ERROR, "Unexpected packet tag %d\n", rc.tag);
> 		return NULL;
>@@ -212,7 +222,7 @@ static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs, void *buf)
> 		p9_debug(P9_DEBUG_ERROR,
> 			 "requested packet size too big: %d for tag %d with capacity %zd\n",
> 			 rc.size, rc.tag, p9_rx_req->rc.capacity);
>-		p9_req_put(usb9pfs->client, p9_rx_req);
>+		p9_req_put(client, p9_rx_req);
> 		return NULL;
> 	}
>
>@@ -220,7 +230,7 @@ static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs, void *buf)
> 		p9_debug(P9_DEBUG_ERROR,
> 			 "No recv fcall for tag %d (req %p), disconnecting!\n",
> 			 rc.tag, p9_rx_req);
>-		p9_req_put(usb9pfs->client, p9_rx_req);
>+		p9_req_put(client, p9_rx_req);
> 		return NULL;
> 	}
>
>@@ -231,8 +241,10 @@ static void usb9pfs_rx_complete(struct usb_ep *ep, struct usb_request *req)
> {
> 	struct f_usb9pfs *usb9pfs = ep->driver_data;
> 	struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
>+	struct p9_client *client;
> 	struct p9_req_t *p9_rx_req;
> 	unsigned int req_size = req->actual;
>+	unsigned long flags;
> 	int status = REQ_STATUS_RCVD;
>
> 	if (req->status) {
>@@ -241,9 +253,16 @@ static void usb9pfs_rx_complete(struct usb_ep *ep, struct usb_request *req)
> 		return;
> 	}
>
>-	p9_rx_req = usb9pfs_rx_header(usb9pfs, req->buf);
>-	if (!p9_rx_req)
>+	spin_lock_irqsave(&usb9pfs->lock, flags);
>+	client = usb9pfs->client;
>+	if (!client) {
>+		spin_unlock_irqrestore(&usb9pfs->lock, flags);
> 		return;
>+	}
>+
>+	p9_rx_req = usb9pfs_rx_header(usb9pfs, client, req->buf);
>+	if (!p9_rx_req)
>+		goto out_unlock;
>
> 	if (req_size > p9_rx_req->rc.capacity) {
> 		dev_err(&cdev->gadget->dev,
>@@ -257,8 +276,11 @@ static void usb9pfs_rx_complete(struct usb_ep *ep, struct usb_request *req)
>
> 	p9_rx_req->rc.size = req_size;
>
>-	p9_client_cb(usb9pfs->client, p9_rx_req, status);
>-	p9_req_put(usb9pfs->client, p9_rx_req);
>+	p9_client_cb(client, p9_rx_req, status);
>+	p9_req_put(client, p9_rx_req);
>+
>+out_unlock:
>+	spin_unlock_irqrestore(&usb9pfs->lock, flags);
>
> 	complete(&usb9pfs->received);
> }
>@@ -416,7 +438,9 @@ static int p9_usbg_create(struct p9_client *client, struct fs_context *fc)
> 		client->status = Disconnected;
> 	else
> 		client->status = Connected;
>+	spin_lock_irq(&usb9pfs->lock);
> 	usb9pfs->client = client;
>+	spin_unlock_irq(&usb9pfs->lock);
>
> 	client->trans_mod->maxsize = usb9pfs->buflen;
>
>@@ -427,18 +451,25 @@ static int p9_usbg_create(struct p9_client *client, struct fs_context *fc)
>
> static void usb9pfs_clear_tx(struct f_usb9pfs *usb9pfs)
> {
>+	struct p9_client *client;
> 	struct p9_req_t *req;
>+	unsigned long flags;
>
>-	guard(spinlock_irqsave)(&usb9pfs->lock);
>+	spin_lock_irqsave(&usb9pfs->lock, flags);
>+	client = usb9pfs->client;
>+	usb9pfs->client = NULL;
>+	req = usb9pfs->in_req ? usb9pfs->in_req->context : NULL;
>+	if (usb9pfs->in_req)
>+		usb9pfs->in_req->context = NULL;
>+	spin_unlock_irqrestore(&usb9pfs->lock, flags);

Same applies here, it would be nice to seperate the changes of
using the extra variable to a second patch.
>
>-	req = usb9pfs->in_req->context;
>-	if (!req)
>+	if (!req || !client)
> 		return;
>
> 	if (!req->t_err)
> 		req->t_err = -ECONNRESET;
>
>-	p9_client_cb(usb9pfs->client, req, REQ_STATUS_ERROR);
>+	p9_client_cb(client, req, REQ_STATUS_ERROR);
> }
>
> static void p9_usbg_close(struct p9_client *client)
>-- 
>2.34.1

I tested your patch and like the changes, however I have some extra
patches on top. Would you mind if I take your changes and include my
suggestions. I would still keep you as author though. These changes would
then be included in my series I will send today.

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]