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

["Taylor R Campbell via gnats" <gnats-admin%NetBSD.org@localhost>]

The following reply was made to PR kern/56194; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Michael van Elst <mlelstv%NetBSD.org@localhost>
Cc: source-changes-d%NetBSD.org@localhost, gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost,
	Nick Hudson <skrll%NetBSD.org@localhost>
Subject: Re: kern/56194: xhci panic: kernel diagnostic assertion "xs->xs_xr[dci] == NULL" failed
Date: Sun, 11 Jan 2026 16:22:31 +0000

 > 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.