Re: CVS commit: src/sys

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

> Date: Fri, 10 Aug 2018 18:54:10 +0100
> From: Nick Hudson <nick.hudson%gmx.co.uk@localhost>
> 
> On 10/08/2018 07:31, Taylor R Campbell wrote:
> > 2. The callout can run after usbd_free_xfer -- use-after-free.
> > 
> > The fix for the second bug is simple: use callout_halt (and
> > usb_rem_task_wait) in usbd_free_xfer, rather than the #if DIAGNOSTIC
> > callout_stop bogosity that it does right now.
> 
> 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?

Note that in the story I related, nobody cancels the xfer: it's either
completed by hardware, or timed out.  Neither path calls callout_halt.

> > (a) The hardware completion interrupt handler can't sleep, so it can't
> >      use callout_halt.  Now, maybe you could argue that callout_halt on
> >      a callout whose action only acquires locks is OK for a softintr.
> >      But it _certainly_ can't use usb_rem_task_wait which does a
> >      cv_wait on some hardware condition.
> 
> All completions are done in softint, so I'm not sure this part applies.

My point is that we can't wait for the callout and task to complete in
the harware completion (soft) interrupt handler.  Callout, maybe, but
task, no.  So we can't fix this by changing (e.g.) ehci_idone to use
callout_halt and usb_rem_task_wait where it currently uses
callout_stop and usb_rem_task.

> Perhaps the fix is still appropriate despite my comments?

I think so.