On Tue, Jul 2, 2013 at 12:02 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Mon, 1 Jul 2013, Ming Lei wrote: > >> 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. > > There is indeed such a guarantee, provided by the spinlock. CPU1 > acquires urb_list_lock after CPU0 releases it, right? Therefore, all > stores to memory performed by CPU0 before releasing the lock will > be visible to CPU1 after it acquires the lock. Therefore CPU1 will see > that ep->enabled is 0. OK, I got it, but looks it is a bit tricky and maybe some comment is helpful, thanks for your explanation. > >> >> 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. > > I don't mind -- but I don't see how it will help. > > Alan Stern 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
