kern/57783: usbd_set_polling calls ubm_softint with polling enabled but bus lock held

[campbell+netbsd%mumble.net@localhost]

>Number:         57783
>Category:       kern
>Synopsis:       usbd_set_polling calls ubm_softint with polling enabled but bus lock held
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Dec 18 21:40:00 +0000 2023
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, 8
>Organization:
The NetBSD Pollingation
>Environment:
>Description:
[ 61805.181248] fatal breakpoint trap in supervisor mode
[ 61805.181248] trap type 1 code 0 rip 0xffffffff80235415 cs 0x8 rflags 0x202 cr2 0x78d8318c6000 ilevel 0x6 rsp 0xffff949093d1aab0
[ 61805.181248] curlwp 0xffff8a75ebc1dc00 pid 0.5 lowest kstack 0xffff949093d162c0
[ 61805.181248] Mutex error: mutex_vector_enter,551: locking against myself

[ 61805.181248] lock address : ffff8a66f5e3a9c8
[ 61805.181248] current cpu  :                  0
[ 61805.181248] current lwp  : 0xffff8a75ebc1dc00
[ 61805.181248] owner field  : 0xffff8a75ebc1dc00 wait/spin:                0/0

[ 61805.181248] panic: lock error: Mutex: mutex_vector_enter,551: locking against myself: lock 0xffff8a66f5e3a9c8 cpu 0 lwp 0xffff8a75ebc1dc00
[ 61805.181248] cpu0: Begin traceback...
[ 61805.181248] vpanic() at netbsd:vpanic+0x171
[ 61805.181248] panic() at netbsd:panic+0x3c
[ 61805.181248] lockdebug_abort() at netbsd:lockdebug_abort+0x114
[ 61805.181248] mutex_vector_enter() at netbsd:mutex_vector_enter+0x381
[ 61805.181248] usbd_transfer() at netbsd:usbd_transfer+0xdb
[ 61805.181248] ucomsubmitread() at netbsd:ucomsubmitread+0x54
[ 61805.181248] ucom_read_complete() at netbsd:ucom_read_complete+0x105
[ 61805.181248] ucomreadcb() at netbsd:ucomreadcb+0x392
[ 61805.181248] usb_transfer_complete() at netbsd:usb_transfer_complete+0x536
[ 61805.181248] xhci_softintr() at netbsd:xhci_softintr+0xef8
[ 61805.181248] usbd_set_polling() at netbsd:usbd_set_polling+0x3f
[ 61805.181248] ukbd_cnpollc() at netbsd:ukbd_cnpollc+0x6c
[ 61805.181248] cnpollc() at netbsd:cnpollc+0x71
[ 61805.181248] kdb_trap() at netbsd:kdb_trap+0xfd
[ 61805.181248] trap() at netbsd:trap+0x1bc
[ 61805.181248] --- trap (number 1) ---
[ 61805.181248] breakpoint() at netbsd:breakpoint+0x5
[ 61805.181248] wskbd_translate() at netbsd:wskbd_translate+0xf22
[ 61805.181248] wskbd_input() at netbsd:wskbd_input+0xbe
[ 61805.181248] ukbd_decode() at netbsd:ukbd_decode+0x27e
[ 61805.181248] ukbd_delayed_decode() at netbsd:ukbd_delayed_decode+0x3e
[ 61805.181248] callout_softclock() at netbsd:callout_softclock+0x490
[ 61805.181248] softint_dispatch() at netbsd:softint_dispatch+0x100
[ 61805.181248] cpu0: End traceback...

What happened is:

1. usbd_set_polling incremented bus->ub_usepolling
2. usbd_set_polling acquired bus->ub_lock and called xhci_softintr
3. xhci_softintr processed the queue and called xfer callbacks _without_ releasing the lock because bus->ub_usepolling is nonzero
4. ucomreadcb called usbd_transfer
5. usbd_transfer unconditionally tried mutex_enter(bus->ub_lock) via usbd_lock_pipe and crashed because the lock was already held from(2)

There are two problems:

(a) xhci_softintr is calling xfer callbacks with the lock held, which it should never do
(b) usbd_transfer is taking the lock while polling is enabled and maybe it shouldn't do that
>How-To-Repeat:
1. Attach USB serial port and do I/O on it in background.
2. Type C-A-ESC at USB keyboard to trigger ddb.
>Fix:
1. Change usbd_set_polling sequence from:

        if (on)
                bus->ub_usepolling++;
        else
                bus->ub_usepolling--;

        mutex_enter(ub_lock);
        bus->ub_methods->ubm_softint(bus);
        mutex_exit(ub_lock);

   to:

        mutex_enter(ub_lock);
        if (on) {
                if (bus->ub_usepolling == 0)
                        bus->ub_methods->ubm_softint(bus);
                bus->ub_usepolling++;
        } else {
                bus->ub_usepolling--;
                if (bus->ub_usepolling == 0)
                        bus->ub_methods->ubm_softint(bus);
        }
        mutex_exit(ub_lock);

   This way we we avoid calling ubm_softint with the lock held but polling enabled.

2. Maybe make usbd_lock_pipe or usbd_transfer conditional on bus->ub_usepolling.  This way it is safe to invoke when polling is enabled.  Unclear if this is sensible or not -- need to think more about it.