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

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

The following reply was made to PR kern/57783; 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, mrg%NetBSD.org@localhost, skrll%NetBSD.org@localhost
Subject: Re: kern/57783: usbd_set_polling calls ubm_softint with polling enabled but bus lock held
Date: Mon, 18 Dec 2023 22:29:09 +0000

 This is a multi-part message in MIME format.
 --=_nsoQjY3CD7bBvFEdm/FiyAa+axP8majm
 
 The attached patch series attempts to address this issue -- untested.
 Each patch should independently resolve this specific panic, but I
 think they may both generally be needed.
 
 --=_nsoQjY3CD7bBvFEdm/FiyAa+axP8majm
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr57783-usbpollingsoftintxfer"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr57783-usbpollingsoftintxfer.patch"
 
 From db0cfab258b4fd1a691082195a2541988f11ab52 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Mon, 18 Dec 2023 22:17:13 +0000
 Subject: [PATCH 1/2] usbdi(9): Avoid calling ubm_softint with lock held and
  polling on.
 
 PR kern/57783
 
 XXX pullup-10
 XXX pullup-9
 XXX pullup-8
 ---
  sys/dev/usb/usbdi.c | 32 ++++++++++++++++++++++++++------
  1 file changed, 26 insertions(+), 6 deletions(-)
 
 diff --git a/sys/dev/usb/usbdi.c b/sys/dev/usb/usbdi.c
 index fac06b94df44..125ef3b6ba63 100644
 --- a/sys/dev/usb/usbdi.c
 +++ b/sys/dev/usb/usbdi.c
 @@ -1362,14 +1362,34 @@ usbd_dopoll(struct usbd_interface *iface)
  void
  usbd_set_polling(struct usbd_device *dev, int on)
  {
 -	if (on)
 -		dev->ud_bus->ub_usepolling++;
 -	else
 -		dev->ud_bus->ub_usepolling--;
 =20
 -	/* Kick the host controller when switching modes */
  	mutex_enter(dev->ud_bus->ub_lock);
 -	dev->ud_bus->ub_methods->ubm_softint(dev->ud_bus);
 +	if (on) {
 +		/*
 +		 * Enabling polling.  If we're enabling for the first
 +		 * time, call the softint routine on transition while
 +		 * we hold the lock and polling is still disabled, and
 +		 * then enable polling -- once polling is enabled, we
 +		 * must not hold the lock when we call the softint
 +		 * routine.
 +		 */
 +		KASSERT(dev->ud_bus->ub_usepolling < __type_max(char));
 +		if (dev->ud_bus->ub_usepolling =3D=3D 0)
 +			dev->ud_bus->ub_methods->ubm_softint(dev->ud_bus);
 +		dev->ud_bus->ub_usepolling++;
 +	} else {
 +		/*
 +		 * Disabling polling.  If we're disabling polling for
 +		 * the last time, disable polling first and then call
 +		 * the softint routine while we hold the lock -- until
 +		 * polling is disabled, we must not hold the lock when
 +		 * we call the softint routine.
 +		 */
 +		KASSERT(dev->ud_bus->ub_usepolling > 0);
 +		dev->ud_bus->ub_usepolling--;
 +		if (dev->ud_bus->ub_usepolling =3D=3D 0)
 +			dev->ud_bus->ub_methods->ubm_softint(dev->ud_bus);
 +	}
  	mutex_exit(dev->ud_bus->ub_lock);
  }
 =20
 
 From c8553615bd0ddac1eab7133e0c334a76f3482eb8 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Mon, 18 Dec 2023 22:19:13 +0000
 Subject: [PATCH 2/2] usbdi(9): Avoid taking locks in usbd_transfer while
  polling.
 
 PR kern/57783
 
 XXX pullup-10
 XXX pullup-9
 XXX pullup-8
 ---
  sys/dev/usb/usbdi.c | 23 ++++++++++++++++-------
  1 file changed, 16 insertions(+), 7 deletions(-)
 
 diff --git a/sys/dev/usb/usbdi.c b/sys/dev/usb/usbdi.c
 index 125ef3b6ba63..80e432a22bc2 100644
 --- a/sys/dev/usb/usbdi.c
 +++ b/sys/dev/usb/usbdi.c
 @@ -410,14 +410,18 @@ usbd_transfer(struct usbd_xfer *xfer)
  		}
  	}
 =20
 -	usbd_lock_pipe(pipe);
 +	if (pipe->up_dev->ud_bus->ub_usepolling =3D=3D 0)
 +		usbd_lock_pipe(pipe);
  	if (pipe->up_aborting) {
  		/*
  		 * XXX For synchronous transfers this is fine.  What to
  		 * do for asynchronous transfers?  The callback is
  		 * never run, not even with status USBD_CANCELLED.
 +		 *
 +		 * XXX Does it make sense to abort while polling?
  		 */
 -		usbd_unlock_pipe(pipe);
 +		if (pipe->up_dev->ud_bus->ub_usepolling =3D=3D 0)
 +			usbd_unlock_pipe(pipe);
  		USBHIST_LOG(usbdebug, "<- done xfer %#jx, aborting",
  		    (uintptr_t)xfer, 0, 0, 0);
  		SDT_PROBE2(usb, device, xfer, done,  xfer, USBD_CANCELLED);
 @@ -443,7 +447,8 @@ usbd_transfer(struct usbd_xfer *xfer)
  	} while (0);
  	SDT_PROBE3(usb, device, pipe, transfer__done,  pipe, xfer, err);
 =20
 -	usbd_unlock_pipe(pipe);
 +	if (pipe->up_dev->ud_bus->ub_usepolling =3D=3D 0)
 +		usbd_unlock_pipe(pipe);
 =20
  	if (err !=3D USBD_IN_PROGRESS && err) {
  		/*
 @@ -453,7 +458,8 @@ usbd_transfer(struct usbd_xfer *xfer)
  		 */
  		USBHIST_LOG(usbdebug, "xfer failed: %jd, reinserting",
  		    err, 0, 0, 0);
 -		usbd_lock_pipe(pipe);
 +		if (pipe->up_dev->ud_bus->ub_usepolling =3D=3D 0)
 +			usbd_lock_pipe(pipe);
  		SDT_PROBE1(usb, device, xfer, preabort,  xfer);
  #ifdef DIAGNOSTIC
  		xfer->ux_state =3D XFER_BUSY;
 @@ -461,7 +467,8 @@ usbd_transfer(struct usbd_xfer *xfer)
  		SIMPLEQ_REMOVE_HEAD(&pipe->up_queue, ux_next);
  		if (pipe->up_serialise)
  			usbd_start_next(pipe);
 -		usbd_unlock_pipe(pipe);
 +		if (pipe->up_dev->ud_bus->ub_usepolling =3D=3D 0)
 +			usbd_unlock_pipe(pipe);
  	}
 =20
  	if (!(flags & USBD_SYNCHRONOUS)) {
 @@ -480,7 +487,8 @@ usbd_transfer(struct usbd_xfer *xfer)
  	}
 =20
  	/* Sync transfer, wait for completion. */
 -	usbd_lock_pipe(pipe);
 +	if (pipe->up_dev->ud_bus->ub_usepolling =3D=3D 0)
 +		usbd_lock_pipe(pipe);
  	while (!xfer->ux_done) {
  		if (pipe->up_dev->ud_bus->ub_usepolling)
  			panic("usbd_transfer: not done");
 @@ -503,7 +511,8 @@ usbd_transfer(struct usbd_xfer *xfer)
  	}
  	err =3D xfer->ux_status;
  	SDT_PROBE2(usb, device, xfer, done,  xfer, err);
 -	usbd_unlock_pipe(pipe);
 +	if (pipe->up_dev->ud_bus->ub_usepolling =3D=3D 0)
 +		usbd_unlock_pipe(pipe);
  	return err;
  }
 =20
 
 --=_nsoQjY3CD7bBvFEdm/FiyAa+axP8majm--