public inbox for stable@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: 9p: usbg: clear stale client pointer on close
@ 2026-03-13 17:16 Hyungjung Joo
  2026-03-13 18:06 ` Michael Grzeschik
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Hyungjung Joo @ 2026-03-13 17:16 UTC (permalink / raw)
  To: ericvh, lucho, asmadeus
  Cc: linux_oss, m.grzeschik, gregkh, v9fs, linux-usb, linux-kernel,
	stable, Hyungjung Joo

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) {
+		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);
 
 	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)
 {
 	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);
 
-	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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-13 17:16 [PATCH] net: 9p: usbg: clear stale client pointer on close Hyungjung Joo
@ 2026-03-13 18:06 ` Michael Grzeschik
  2026-03-14  6:01 ` Dominique Martinet
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Michael Grzeschik @ 2026-03-13 18:06 UTC (permalink / raw)
  To: Hyungjung Joo
  Cc: ericvh, lucho, asmadeus, linux_oss, gregkh, v9fs, linux-usb,
	linux-kernel, stable

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

Hi

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>

I was just preparing a similar change in an series of patches. Let me
come back with a test of your patch and the comparison to my changes.

Thanks!
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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-13 17:16 [PATCH] net: 9p: usbg: clear stale client pointer on close Hyungjung Joo
  2026-03-13 18:06 ` Michael Grzeschik
@ 2026-03-14  6:01 ` Dominique Martinet
  2026-03-14  7:24   ` Hyungjung Joo
  2026-03-18 14:33 ` Michael Grzeschik
  2026-03-18 23:24 ` Michael Grzeschik
  3 siblings, 1 reply; 11+ messages in thread
From: Dominique Martinet @ 2026-03-14  6:01 UTC (permalink / raw)
  To: Hyungjung Joo
  Cc: ericvh, lucho, linux_oss, m.grzeschik, gregkh, v9fs, linux-usb,
	linux-kernel, stable

Hyungjung Joo wrote on Sat, Mar 14, 2026 at 02:16:59AM +0900:
> 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.

Just to make sure the problem is the usb9pfs struct being freed, not the
p9_client itself which is still alive after the usb device is gone
(until umount)?

I'm surprised free_func isn't called after unbind, which should stop the
queues (through disable_usb9pfs)?
or are the ep being disabled not enough to ensure the callbacks are not
in use? (e.g. disabling prevents further calls, but doesn't wait for
currently running/queued requests?)


(Also, thanks Michael for looking -- I'll let you do a first review
before looking deeper)
-- 
Dominique

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-14  6:01 ` Dominique Martinet
@ 2026-03-14  7:24   ` Hyungjung Joo
  2026-03-14 12:30     ` Dominique Martinet
  0 siblings, 1 reply; 11+ messages in thread
From: Hyungjung Joo @ 2026-03-14  7:24 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: ericvh, lucho, linux_oss, m.grzeschik, gregkh, v9fs, linux-usb,
	linux-kernel, stable

Thanks for the careful review.

> Just to make sure the problem is the usb9pfs struct being freed, not the
> p9_client itself which is still alive after the usb device is gone
> (until umount)?

The issue I am addressing is the stale use of the usb9pfs->client
association after the transport has been closed. I am not relying on
early free of struct f_usb9pfs for this bug.

The lifetime mismatch here is that struct f_usb9pfs belongs to the
gadget function and can be reused across mount sessions, while
struct p9_client is per-mount. On the close path, p9_usbg_close() did
not detach usb9pfs->client, so late TX/RX completions could still
follow that pointer after close, including into a later remount that
rebinds it.

> I'm surprised free_func isn't called after unbind, which should stop the
> queues (through disable_usb9pfs)?
> or are the ep being disabled not enough to ensure the callbacks are not
> in use? (e.g. disabling prevents further calls, but doesn't wait for
> currently running/queued requests?)

My understanding is that the unbind/free_func path is different from the
close path at issue here. This patch is not trying to change or rely on
gadget teardown ordering; it addresses the close-side race where
usb9pfs->client remained attached and the completion paths still
dereferenced it.

That is why the patch:
- clears usb9pfs->client under usb9pfs->lock on close,
- detaches any pending TX request from in_req->context, and
- makes TX/RX completions bail out once the transport has been detached.

So the intent is to prevent late completions from using a stale or
rebound client association after close, rather than to claim an early
free of the gadget object itself.

Thanks,

2026년 3월 14일 (토) PM 3:01, Dominique Martinet <asmadeus@codewreck.org>님이 작성:
>
> Hyungjung Joo wrote on Sat, Mar 14, 2026 at 02:16:59AM +0900:
> > 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.
>
> Just to make sure the problem is the usb9pfs struct being freed, not the
> p9_client itself which is still alive after the usb device is gone
> (until umount)?
>
> I'm surprised free_func isn't called after unbind, which should stop the
> queues (through disable_usb9pfs)?
> or are the ep being disabled not enough to ensure the callbacks are not
> in use? (e.g. disabling prevents further calls, but doesn't wait for
> currently running/queued requests?)
>
>
> (Also, thanks Michael for looking -- I'll let you do a first review
> before looking deeper)
> --
> Dominique

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-14  7:24   ` Hyungjung Joo
@ 2026-03-14 12:30     ` Dominique Martinet
  0 siblings, 0 replies; 11+ messages in thread
From: Dominique Martinet @ 2026-03-14 12:30 UTC (permalink / raw)
  To: Hyungjung Joo
  Cc: ericvh, lucho, linux_oss, m.grzeschik, gregkh, v9fs, linux-usb,
	linux-kernel, stable

Hyungjung Joo wrote on Sat, Mar 14, 2026 at 04:24:19PM +0900:
> Thanks for the careful review.
> 
> > Just to make sure the problem is the usb9pfs struct being freed, not the
> > p9_client itself which is still alive after the usb device is gone
> > (until umount)?
> 
> The issue I am addressing is the stale use of the usb9pfs->client
> association after the transport has been closed. I am not relying on
> early free of struct f_usb9pfs for this bug.
> 
> The lifetime mismatch here is that struct f_usb9pfs belongs to the
> gadget function and can be reused across mount sessions, while
> struct p9_client is per-mount. On the close path, p9_usbg_close() did
> not detach usb9pfs->client, so late TX/RX completions could still
> follow that pointer after close, including into a later remount that
> rebinds it.

Ah, you did write p9_usbg_close() in the commit message and I skipped
over it too fast...

I also misread your locking, I thought you were just getting a client
pointer under lock, but you're also using it only within the lock (so
there is no benefit from using a local variable as far as locking goes,
but releasing the lock early would be invalid as the client itself is
freed immediately after close...)

From a performance point of view I think making close() stop the ep and
wait until any in flight calls would be better than taking the lock all
the time, but I'm not sure how visible that'd actually be on real
workloads... close() can take as long as you want, it's perfectly fine
to sleep there until the usb side has finished flushing if you want.
I'm curious what approach Michael picked?


> > I'm surprised free_func isn't called after unbind, which should stop the
> > queues (through disable_usb9pfs)?
> > or are the ep being disabled not enough to ensure the callbacks are not
> > in use? (e.g. disabling prevents further calls, but doesn't wait for
> > currently running/queued requests?)
> 
> My understanding is that the unbind/free_func path is different from the
> close path at issue here. This patch is not trying to change or rely on
> gadget teardown ordering; it addresses the close-side race where
> usb9pfs->client remained attached and the completion paths still
> dereferenced it.

Yes, transport close corresponds to a umount call, and is unrelated to
usb lifetime; I was thinking of the other way around (usb device
disappearing while the mount is still alive)

> That is why the patch:
> - clears usb9pfs->client under usb9pfs->lock on close,
> - detaches any pending TX request from in_req->context, and
> - makes TX/RX completions bail out once the transport has been detached.
> 
> So the intent is to prevent late completions from using a stale or
> rebound client association after close, rather than to claim an early
> free of the gadget object itself.

Thank you for the details (and the patch!); I'll have a deeper look
after hearing back from Michael

-- 
Dominique Martinet | Asmadeus

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-13 17:16 [PATCH] net: 9p: usbg: clear stale client pointer on close Hyungjung Joo
  2026-03-13 18:06 ` Michael Grzeschik
  2026-03-14  6:01 ` Dominique Martinet
@ 2026-03-18 14:33 ` Michael Grzeschik
  2026-03-18 14:49   ` Hyungjung Joo
  2026-03-18 23:24 ` Michael Grzeschik
  3 siblings, 1 reply; 11+ messages in thread
From: Michael Grzeschik @ 2026-03-18 14:33 UTC (permalink / raw)
  To: Hyungjung Joo
  Cc: ericvh, lucho, asmadeus, linux_oss, gregkh, v9fs, linux-usb,
	linux-kernel, stable

[-- 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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-18 14:33 ` Michael Grzeschik
@ 2026-03-18 14:49   ` Hyungjung Joo
  0 siblings, 0 replies; 11+ messages in thread
From: Hyungjung Joo @ 2026-03-18 14:49 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: ericvh, lucho, asmadeus, linux_oss, gregkh, v9fs, linux-usb,
	linux-kernel, stable

Hi,
Thank you for the feedback

2026년 3월 18일 (수) PM 11:33, Michael Grzeschik <mgr@pengutronix.de>님이 작성:

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

That sounds good to me.
Feel free to take my changes and include your suggestions and thanks
for keeping me as the author.

Best regards,
Hyungjung Joo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-13 17:16 [PATCH] net: 9p: usbg: clear stale client pointer on close Hyungjung Joo
                   ` (2 preceding siblings ...)
  2026-03-18 14:33 ` Michael Grzeschik
@ 2026-03-18 23:24 ` Michael Grzeschik
  2026-03-19  0:30   ` Dominique Martinet
  3 siblings, 1 reply; 11+ messages in thread
From: Michael Grzeschik @ 2026-03-18 23:24 UTC (permalink / raw)
  To: Hyungjung Joo
  Cc: ericvh, lucho, asmadeus, linux_oss, gregkh, v9fs, linux-usb,
	linux-kernel, stable

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

Hi

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>

I wonder where greg did this review? Was this in another thread?

I will have to drop this anyway, when splitting it appart.

>Signed-off-by: Hyungjung Joo <jhj140711@gmail.com>
>---
> net/9p/trans_usbg.c | 63 +++++++++++++++++++++++++++++++++------------
> 1 file changed, 47 insertions(+), 16 deletions(-)
>

...

-- 
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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-18 23:24 ` Michael Grzeschik
@ 2026-03-19  0:30   ` Dominique Martinet
  2026-03-19  9:16     ` Michael Grzeschik
  0 siblings, 1 reply; 11+ messages in thread
From: Dominique Martinet @ 2026-03-19  0:30 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: Hyungjung Joo, ericvh, lucho, linux_oss, gregkh, v9fs, linux-usb,
	linux-kernel, stable

Michael Grzeschik wrote on Thu, Mar 19, 2026 at 12:24:51AM +0100:
> On Sat, Mar 14, 2026 at 02:16:59AM +0900, Hyungjung Joo wrote:
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> I wonder where greg did this review? Was this in another thread?

Yes, it was when Hyungjung Joo sent this to security@, so the review was
not public

-- 
Dominique Martinet | Asmadeus

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-19  0:30   ` Dominique Martinet
@ 2026-03-19  9:16     ` Michael Grzeschik
  2026-03-19  9:22       ` Greg KH
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Grzeschik @ 2026-03-19  9:16 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Hyungjung Joo, ericvh, lucho, linux_oss, gregkh, v9fs, linux-usb,
	linux-kernel, stable

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

On Thu, Mar 19, 2026 at 09:30:36AM +0900, Dominique Martinet wrote:
>Michael Grzeschik wrote on Thu, Mar 19, 2026 at 12:24:51AM +0100:
>> On Sat, Mar 14, 2026 at 02:16:59AM +0900, Hyungjung Joo wrote:
>> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> I wonder where greg did this review? Was this in another thread?
>
>Yes, it was when Hyungjung Joo sent this to security@, so the review was
>not public

Since the patch was already reviewed by greg, I decided to keep it
as much as it is and include it in one in my series. This way I can keep
the Reviewed-by and the Fixes tag for stable.

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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: 9p: usbg: clear stale client pointer on close
  2026-03-19  9:16     ` Michael Grzeschik
@ 2026-03-19  9:22       ` Greg KH
  0 siblings, 0 replies; 11+ messages in thread
From: Greg KH @ 2026-03-19  9:22 UTC (permalink / raw)
  To: Michael Grzeschik
  Cc: Dominique Martinet, Hyungjung Joo, ericvh, lucho, linux_oss, v9fs,
	linux-usb, linux-kernel, stable

On Thu, Mar 19, 2026 at 10:16:05AM +0100, Michael Grzeschik wrote:
> On Thu, Mar 19, 2026 at 09:30:36AM +0900, Dominique Martinet wrote:
> > Michael Grzeschik wrote on Thu, Mar 19, 2026 at 12:24:51AM +0100:
> > > On Sat, Mar 14, 2026 at 02:16:59AM +0900, Hyungjung Joo wrote:
> > > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > 
> > > I wonder where greg did this review? Was this in another thread?
> > 
> > Yes, it was when Hyungjung Joo sent this to security@, so the review was
> > not public
> 
> Since the patch was already reviewed by greg, I decided to keep it
> as much as it is and include it in one in my series. This way I can keep
> the Reviewed-by and the Fixes tag for stable.

Hey, I just did a cursory review, feel free to drop mine if the patch
has changed at all, doesn't bother me in the least.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-03-19  9:22 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-13 17:16 [PATCH] net: 9p: usbg: clear stale client pointer on close Hyungjung Joo
2026-03-13 18:06 ` Michael Grzeschik
2026-03-14  6:01 ` Dominique Martinet
2026-03-14  7:24   ` Hyungjung Joo
2026-03-14 12:30     ` Dominique Martinet
2026-03-18 14:33 ` Michael Grzeschik
2026-03-18 14:49   ` Hyungjung Joo
2026-03-18 23:24 ` Michael Grzeschik
2026-03-19  0:30   ` Dominique Martinet
2026-03-19  9:16     ` Michael Grzeschik
2026-03-19  9:22       ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox