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

[Dominique Martinet <asmadeus@codewreck.org>]

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