> Date: Fri, 10 Aug 2018 19:11:54 +0100 > From: Nick Hudson <nick.hud...@gmx.co.uk> > > On 10/08/2018 19:08, Taylor R Campbell wrote: > >> Date: Fri, 10 Aug 2018 18:54:10 +0100 > >> From: Nick Hudson <nick.hud...@gmx.co.uk> > >> > >> The second bug should never occur as the intention was for drivers to > >> always use xfers until detach and detach would abort correctly. > > Who waits for the callout to complete? > > The abort routine does it now, right?
Ah, yes, I had missed that. In HEAD, *hci_abort_xfer unconditionally waits for the callout and task to complete, so as long as it is always called before usbd_free_xfer, it avoids this use-after-free bug I hypothesized. (In that case the #if DIAGNOSTIC in usbd_free_xfer is unnecessary -- you could KASSERT(!callout_pending(ch)) instead, but callout_destroy already does that.) In the patch I proposed, I made all three paths -- hardware completion, software abort, timeout -- use effectively the same logic, all under the lock with no waits: /* If the status is already set, do nothing. */ if (xfer->ux_status != USBD_IN_PROGRESS) return; if (status != USBD_TIMEOUT) ehci_cancel_timeout(); xfer->ux_status = status; With this structure, it is necessary for usbd_free_xfer to wait for the callout and task. I did draft a version in which *hci_abort_xfer still does callout_halt, but it had even more code that wasn't obvious how to deduplicate -- ehci_cancel_timeout would be copied with slight differences in *hci_done and *hci_abort_xfer. The patch I sent seemed simpler.