Re: kern/57965: urtwn(4) command queue can overflow and deadlock

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

The attached patch is a candidate workaround, not yet tested.

From 491269b43e8cdbe70b174550cbd70fe8369baacf Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Mon, 26 Feb 2024 19:51:13 +0000
Subject: [PATCH] urtwn(4): Ditch old queued commands on overflow.

Don't increment ring->queued past what the task will decrement.

This is a stop-gap measure; really, we should just have one task for
each operation that is deferred to the task thread.

PR kern/57965
---
 sys/dev/usb/if_urtwn.c | 47 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/sys/dev/usb/if_urtwn.c b/sys/dev/usb/if_urtwn.c
index 7e7c6c176d6d..f9fd12143fca 100644
--- a/sys/dev/usb/if_urtwn.c
+++ b/sys/dev/usb/if_urtwn.c
@@ -871,6 +871,24 @@ urtwn_tx_beacon(struct urtwn_softc *sc, struct mbuf *m,
 	return urtwn_tx(sc, m, ni, data);
 }
 
+static void
+urtwn_cmdq_invariants(struct urtwn_softc *sc)
+{
+	struct urtwn_host_cmd_ring *const ring = &sc->cmdq;
+
+	KASSERT(mutex_owned(&sc->sc_task_mtx));
+	KASSERTMSG((ring->cur >= 0 && ring->cur < URTWN_HOST_CMD_RING_COUNT),
+	    "%s: cur=%d next=%d queued=%d",
+	    device_xname(sc->sc_dev), ring->cur, ring->next, ring->queued);
+	KASSERTMSG((ring->next >= 0 && ring->next < URTWN_HOST_CMD_RING_COUNT),
+	    "%s: cur=%d next=%d queued=%d",
+	    device_xname(sc->sc_dev), ring->cur, ring->next, ring->queued);
+	KASSERTMSG((ring->queued >= 0 &&
+		ring->queued <= URTWN_HOST_CMD_RING_COUNT),
+	    "%s: %d commands queued",
+	    device_xname(sc->sc_dev), ring->queued);
+}
+
 static void
 urtwn_task(void *arg)
 {
@@ -903,7 +921,11 @@ urtwn_task(void *arg)
 	/* Process host commands. */
 	s = splusb();
 	mutex_spin_enter(&sc->sc_task_mtx);
+	urtwn_cmdq_invariants(sc);
 	while (ring->next != ring->cur) {
+		KASSERTMSG(ring->queued > 0, "%s: cur=%d next=%d queued=%d",
+		    device_xname(sc->sc_dev),
+		    ring->cur, ring->next, ring->queued);
 		cmd = &ring->cmd[ring->next];
 		mutex_spin_exit(&sc->sc_task_mtx);
 		splx(s);
@@ -911,6 +933,10 @@ urtwn_task(void *arg)
 		cmd->cb(sc, cmd->data);
 		s = splusb();
 		mutex_spin_enter(&sc->sc_task_mtx);
+		urtwn_cmdq_invariants(sc);
+		KASSERTMSG(ring->queued > 0, "%s: cur=%d next=%d queued=%d",
+		    device_xname(sc->sc_dev),
+		    ring->cur, ring->next, ring->queued);
 		ring->queued--;
 		ring->next = (ring->next + 1) % URTWN_HOST_CMD_RING_COUNT;
 	}
@@ -925,6 +951,7 @@ urtwn_do_async(struct urtwn_softc *sc, void (*cb)(struct urtwn_softc *, void *),
 {
 	struct urtwn_host_cmd_ring *ring = &sc->cmdq;
 	struct urtwn_host_cmd *cmd;
+	bool schedtask = false;
 	int s;
 
 	URTWNHIST_FUNC();
@@ -933,19 +960,27 @@ urtwn_do_async(struct urtwn_softc *sc, void (*cb)(struct urtwn_softc *, void *),
 
 	s = splusb();
 	mutex_spin_enter(&sc->sc_task_mtx);
+	urtwn_cmdq_invariants(sc);
 	cmd = &ring->cmd[ring->cur];
 	cmd->cb = cb;
 	KASSERT(len <= sizeof(cmd->data));
 	memcpy(cmd->data, arg, len);
 	ring->cur = (ring->cur + 1) % URTWN_HOST_CMD_RING_COUNT;
 
-	/* If there is no pending command already, schedule a task. */
-	if (!sc->sc_dying && ++ring->queued == 1) {
-		mutex_spin_exit(&sc->sc_task_mtx);
-		usb_add_task(sc->sc_udev, &sc->sc_task, USB_TASKQ_DRIVER);
-	} else
-		mutex_spin_exit(&sc->sc_task_mtx);
+	/*
+	 * Schedule a task to process the command if need be.
+	 */
+	if (!sc->sc_dying) {
+		if (ring->queued == URTWN_HOST_CMD_RING_COUNT)
+			device_printf(sc->sc_dev, "command queue overflow\n");
+		else if (ring->queued++ == 0)
+			schedtask = true;
+	}
+	mutex_spin_exit(&sc->sc_task_mtx);
 	splx(s);
+
+	if (schedtask)
+		usb_add_task(sc->sc_udev, &sc->sc_task, USB_TASKQ_DRIVER);
 }
 
 static void