On 8/11/23 17:07, Sam Edwards wrote:
stub_recv_cmd_submit() allocates a `priv` structure, which is freed by
the TX thread after all URBs in the `priv` complete and are handled.
This means that stub_recv_cmd_submit() effectively loses ownership of
`priv` once the final URB is submitted: in the worst case, the RX
thread will be preempted before usb_submit_urb() returns, and the TX
thread will do all handling and cleanup before the RX thread resumes.
How did you find this problem? Please add the details in the change
log and also please describe the fix in detail.
This patch changes for loop from looping on priv->num_urbs to
num_urbs. Change looks okay to me. I do want to know how you found
the problem.
We don't lose ownership if usb_submit_urb() returns an error value,
since that means it won't eventually call stub_complete(), so the
error-handling `usbip_dump_urb(priv->urbs[i])` is still safe.
Did you happen to track down where this urb gets free'd after
usbip_dump_urb(priv->urbs[i])
Signed-off-by: Sam Edwards <CFSworks@xxxxxxxxx>
---
drivers/usb/usbip/stub_rx.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index fc01b31bbb87..dba9a64830e6 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -592,8 +592,11 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
if (usbip_recv_iso(ud, priv->urbs[0]) < 0)
return;
- /* urb is now ready to submit */
- for (i = 0; i < priv->num_urbs; i++) {
+ /*
+ * URB(s) are now ready to submit.
+ * Note: priv is freed after the last URB is (successfully) submitted.
+ */
+ for (i = 0; i < num_urbs; i++) {
ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
if (ret == 0)
thanks,
-- Shuah
