On Mon, Jul 1, 2013 at 10:44 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Mon, 1 Jul 2013, Ming Lei wrote: > >> >> Currently, URB might be probably submitted to HCD too even after >> >> usb_hcd_flush_endpoint() completes since both accesses to dev->ep_in[epnum] >> >> and ep->enabled aren't protected by effective locks. >> > >> > The urb_list_lock in hcd.c serves to synchronize changes to >> > ep->enabled. An URB might be submitted after usb_hcd_flush_endpoint() >> > completes, but the submission will fail in usb_hcd_link_urb_to_ep(). >> >> But the lock isn't held when setting and clearing ep->enabled in >> usb_enable_endpoint and usb_disable_endpoint, is it? > > No. But it is held in usb_hcd_flush_endpoint() and > usb_hcd_link_urb_to_ep(). Therefore, if a thread clears ep->enabled > and then flushes the endpoint, further submissions will fail. I am wondering if holding the lock in usb_hcd_flush_endpoint() can avoid the race completely. Suppose usb_hcd_link_urb_to_ep() in submit path runs on CPU1 just when usb_hcd_flush_endpoint()(called from usb_disable_endpoint()) completes on CPU0, there is no any guarantee that CPU1 can see the up-to-date value of ep->enabled, so the urb might be submitted to HCD successfully. > > However, this does still leave one possible race: On the first > submission to an isochronous endpoint, itd_submit() and sitd_submit() > will allocate a new ehci_iso_stream before calling > usb_hcd_link_urb_to_ep(). If the endpoint has been flushed and > disabled, the enqueue will fail but the new iso_stream will not be > destroyed -- it will leak. But this is an old failure mode, not > related to the new changes. > >> >> So how about not adding the WARN_ON(!list_empty(&qh->qtd_list) >> >> now when qh->qh_state is QH_STATE_LINKED? >> > >> > I think we should add the WARN_ON. Or jump to the default case in that >> > switch statement -- maybe the error message there should include a >> > WARN_ON. >> >> How about just adding WARN_ON(!list_empty(&qh->qtd_list) but >> continue unlinking the qh like current code, which should be >> harmless and avoid the QH leak? > > I think the end result would be the same. It wouldn't hurt and it > wouldn't help. We'd still end up at the "default" case. It may help to avoid one QH leak, even though the problem is caused by usbcore, so I plan to do that if you have no objection. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
