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

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

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

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: netbsd-bugs%NetBSD.org@localhost
Subject: Re: kern/57965: urtwn(4) command queue can overflow and deadlock
Date: Mon, 26 Feb 2024 20:24:18 +0000

 This is a multi-part message in MIME format.
 --=_QJyEfqlAfncoM8fB56hLDQdX5vxg3caH
 
 The attached patch is a candidate workaround, not yet tested.
 
 --=_QJyEfqlAfncoM8fB56hLDQdX5vxg3caH
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr57965-urtwncmdqoverflow"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr57965-urtwncmdqoverflow.patch"
 
 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);
  }
 =20
 +static void
 +urtwn_cmdq_invariants(struct urtwn_softc *sc)
 +{
 +	struct urtwn_host_cmd_ring *const ring =3D &sc->cmdq;
 +
 +	KASSERT(mutex_owned(&sc->sc_task_mtx));
 +	KASSERTMSG((ring->cur >=3D 0 && ring->cur < URTWN_HOST_CMD_RING_COUNT),
 +	    "%s: cur=3D%d next=3D%d queued=3D%d",
 +	    device_xname(sc->sc_dev), ring->cur, ring->next, ring->queued);
 +	KASSERTMSG((ring->next >=3D 0 && ring->next < URTWN_HOST_CMD_RING_COUNT),
 +	    "%s: cur=3D%d next=3D%d queued=3D%d",
 +	    device_xname(sc->sc_dev), ring->cur, ring->next, ring->queued);
 +	KASSERTMSG((ring->queued >=3D 0 &&
 +		ring->queued <=3D 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 =3D splusb();
  	mutex_spin_enter(&sc->sc_task_mtx);
 +	urtwn_cmdq_invariants(sc);
  	while (ring->next !=3D ring->cur) {
 +		KASSERTMSG(ring->queued > 0, "%s: cur=3D%d next=3D%d queued=3D%d",
 +		    device_xname(sc->sc_dev),
 +		    ring->cur, ring->next, ring->queued);
  		cmd =3D &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 =3D splusb();
  		mutex_spin_enter(&sc->sc_task_mtx);
 +		urtwn_cmdq_invariants(sc);
 +		KASSERTMSG(ring->queued > 0, "%s: cur=3D%d next=3D%d queued=3D%d",
 +		    device_xname(sc->sc_dev),
 +		    ring->cur, ring->next, ring->queued);
  		ring->queued--;
  		ring->next =3D (ring->next + 1) % URTWN_HOST_CMD_RING_COUNT;
  	}
 @@ -925,6 +951,7 @@ urtwn_do_async(struct urtwn_softc *sc, void (*cb)(struc=
 t urtwn_softc *, void *),
  {
  	struct urtwn_host_cmd_ring *ring =3D &sc->cmdq;
  	struct urtwn_host_cmd *cmd;
 +	bool schedtask =3D false;
  	int s;
 =20
  	URTWNHIST_FUNC();
 @@ -933,19 +960,27 @@ urtwn_do_async(struct urtwn_softc *sc, void (*cb)(str=
 uct urtwn_softc *, void *),
 =20
  	s =3D splusb();
  	mutex_spin_enter(&sc->sc_task_mtx);
 +	urtwn_cmdq_invariants(sc);
  	cmd =3D &ring->cmd[ring->cur];
  	cmd->cb =3D cb;
  	KASSERT(len <=3D sizeof(cmd->data));
  	memcpy(cmd->data, arg, len);
  	ring->cur =3D (ring->cur + 1) % URTWN_HOST_CMD_RING_COUNT;
 =20
 -	/* If there is no pending command already, schedule a task. */
 -	if (!sc->sc_dying && ++ring->queued =3D=3D 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 =3D=3D URTWN_HOST_CMD_RING_COUNT)
 +			device_printf(sc->sc_dev, "command queue overflow\n");
 +		else if (ring->queued++ =3D=3D 0)
 +			schedtask =3D true;
 +	}
 +	mutex_spin_exit(&sc->sc_task_mtx);
  	splx(s);
 +
 +	if (schedtask)
 +		usb_add_task(sc->sc_udev, &sc->sc_task, USB_TASKQ_DRIVER);
  }
 =20
  static void
 
 --=_QJyEfqlAfncoM8fB56hLDQdX5vxg3caH--