Re: CVS commit: src/sys

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

> Date: Fri, 10 Aug 2018 19:11:54 +0100
> From: Nick Hudson <nick.hudson%gmx.co.uk@localhost>
> 
> On 10/08/2018 19:08, Taylor R Campbell wrote:
> >> Date: Fri, 10 Aug 2018 18:54:10 +0100
> >> From: Nick Hudson <nick.hudson%gmx.co.uk@localhost>
> >>
> >> 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.