Re: kern/56194: xhci panic: kernel diagnostic assertion "xs->xs_xr[dci] == NULL" failed

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Module Name:    src
> Committed By:   mlelstv
> Date:           Sat Jan 10 08:54:08 UTC 2026
> 
> Modified Files:
>         src/sys/dev/usb: uaudio.c
> 
> Log Message:
> Also use atomic operation to store pipe pointers, not just to clear them.
> 
> Probably fixes PR 56194.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.184 -r1.185 src/sys/dev/usb/uaudio.c

I think this cannot be correct.

There are several possible scenarios:

1. Concurrent calls to uaudio_chan_open and/or uaudio_chan_close with
   the same channel are NOT possible.  I.e., all such calls are
   serialized.

   => In this case, the atomic_swap_ptr can't make any difference, so
      this patch can't fix anything.

2. Concurrent calls to uaudio_chan_open with the same channel are
   possible.

   => In this case, the following sequence is possible:

        thread0                 thread1
        -------                 -------
        usbd_open_pipe(&pipe0)  usbd_open_pipe(&pipe1)
        pipe0 = atomic_swap_ptr(&ch->pipe, pipe0)
        KASSERT(pipe0 == NULL)
                                pipe1 = atomic_swap_ptr(&ch->pipe, pipe1)
                                KASSERT(pipe1 == NULL)

      The KASSERT in thread0 succeeds but the KASSERT in thread1 fails
      (or, in non-DIAGNOSTIC builds, this leaks a pipe).  So this is
      still broken.

3. A call to uaudio_chan_open can happen concurrently with a call to
   uaudio_chan_close with the same channel.

   => In this case, the following sequence is possible:

        thread0                 thread1
        -------                 -------
                                uaudio_trigger_input(...)
                                  uaudio_chan_open(sc, ch)
                                    usbd_open_pipe(&pipe)
                                    atomic_swap_ptr(&ch->pipe, pipe)
        uaudio_chan_close(sc, ch)
          pipe = atomic_swap_ptr(&ch->pipe, NULL)
          usbd_close_pipe(pipe)
                                  uaudio_chan_alloc_buffers(sc, ch)
                                    usbd_create_xfer(ch->pipe, ...)

      At this point in thread1, ch->pipe is null, so we have a null
      pointer dereference.  (Another possible ordering is that thread1
      loads ch->pipe before thread0 nulls it out, but then thread0
      closes the pipe before thread1 enters usbd_create_xfer, and so
      it'll be use-after-free instead of null pointer dereference.)
      So again, this is still broken.

I suspect that both (2) and (3) are true, but that really (1) _ought_
to be true.

If so, perhaps we can enforce it by a per-channel mutex taken by

uaudio_halt_in_dma_unlocked
uaudio_halt_out_dma_unlocked
uaudio_trigger_input
uaudio_trigger_output

and asserted held by

uaudio_chan_set_param
uaudio_chan_open
uaudio_chan_alloc_buffers
uaudio_chan_free_buffers
uaudio_chan_close

and then dispense with the atomic_swap_ptrs altogether.