Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/trunk]: src/sys Fix steady state of root intr xfers.
details: https://anonhg.NetBSD.org/src/rev/803d2677f94f
branches: trunk
changeset: 848868:803d2677f94f
user: riastradh <riastradh%NetBSD.org@localhost>
date: Wed Feb 12 16:02:01 2020 +0000
description:
Fix steady state of root intr xfers.
Why?
- Avoid completing a root intr xfer multiple times in races.
- Avoid potential use-after-free in poll_hub callouts (uhci, ahci).
How?
- Use sc->sc_intr_xfer or equivalent to store only a pending xfer
that has not yet completed -- whether successfully, by timeout, or
by synchronous abort. When any of those happens, set it to null
under the lock, so the xfer is completed only once.
- For hci drivers that use a callout to poll the root hub (uhci, ahci):
. Pass the softc pointer, not the xfer, to the callout, so the
callout is not even tempted to use xfer after free -- if the
callout fires, but the xfer is synchronously aborted before the
callout can do anything, the xfer might be freed by the time the
callout starts to examine it.
. Teach the callout to do nothing if it is callout_pending after it
has fired. This way:
1. completion or synchronous abort can just callout_stop
2. start can just callout_schedule
If the callout had already fired before (1), and doesn't acquire
the bus lock until after (2), it may be tempted to abort the new
root intr xfer just after submission, which would be wrong -- so
instead we just have the callout do nothing if it notices it has
been rescheduled, since it will fire again after the appropriate
time has elapsed.
diffstat:
sys/arch/mips/adm5120/dev/ahci.c | 100 +++++++++++++++++++++++++++----
sys/dev/usb/ehci.c | 36 +++++++++--
sys/dev/usb/ohci.c | 30 +++++++-
sys/dev/usb/uhci.c | 122 ++++++++++++++++++++++++++++++--------
sys/dev/usb/vhci.c | 30 +++++++-
sys/dev/usb/xhci.c | 48 +++++++++++---
sys/external/bsd/dwc2/dwc2.c | 30 +++++++-
7 files changed, 320 insertions(+), 76 deletions(-)
diffs (truncated from 813 to 300 lines):
diff -r 1a64b1dee4e2 -r 803d2677f94f sys/arch/mips/adm5120/dev/ahci.c
--- a/sys/arch/mips/adm5120/dev/ahci.c Wed Feb 12 16:01:00 2020 +0000
+++ b/sys/arch/mips/adm5120/dev/ahci.c Wed Feb 12 16:02:01 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ahci.c,v 1.18 2020/02/12 16:01:00 riastradh Exp $ */
+/* $NetBSD: ahci.c,v 1.19 2020/02/12 16:02:01 riastradh Exp $ */
/*-
* Copyright (c) 2007 Ruslan Ermilov and Vsevolod Lobko.
@@ -64,7 +64,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ahci.c,v 1.18 2020/02/12 16:01:00 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ahci.c,v 1.19 2020/02/12 16:02:01 riastradh Exp $");
#include <sys/param.h>
#include <sys/systm.h>
@@ -283,6 +283,7 @@
SIMPLEQ_INIT(&sc->sc_free_xfers);
callout_init(&sc->sc_poll_handle, 0);
+ callout_setfunc(&sc->sc_poll_handle, ahci_poll_hub, sc);
mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_SOFTUSB);
mutex_init(&sc->sc_intr_lock, MUTEX_DEFAULT, IPL_SCHED /* XXXNH */);
@@ -422,13 +423,39 @@
void
ahci_poll_hub(void *arg)
{
- struct usbd_xfer *xfer = arg;
- struct ahci_softc *sc = AHCI_XFER2SC(xfer);
+ struct ahci_softc *sc = arg;
+ struct usbd_xfer *xfer;
u_char *p;
static int p0_state=0;
static int p1_state=0;
- callout_reset(&sc->sc_poll_handle, sc->sc_interval, ahci_poll_hub, xfer);
+ mutex_enter(&sc->sc_lock);
+
+ /*
+ * If the intr xfer has completed or been synchronously
+ * aborted, we have nothing to do.
+ */
+ xfer = sc->sc_intr_xfer;
+ if (xfer == NULL)
+ goto out;
+
+ /*
+ * If the intr xfer for which we were scheduled is done, and
+ * another intr xfer has been submitted, let that one be dealt
+ * with when the callout fires again.
+ *
+ * The call to callout_pending is racy, but the the transition
+ * from pending to invoking happens atomically. The
+ * callout_ack ensures callout_invoking does not return true
+ * due to this invocation of the callout; the lock ensures the
+ * next invocation of the callout cannot callout_ack (unless it
+ * had already run to completion and nulled sc->sc_intr_xfer,
+ * in which case would have bailed out already).
+ */
+ callout_ack(&sc->sc_poll_handle);
+ if (callout_pending(&sc->sc_poll_handle) ||
+ callout_invoking(&sc->sc_poll_handle))
+ goto out;
/* USB spec 11.13.3 (p.260) */
p = KERNADDR(&xfer->ux_dmabuf, 0);
@@ -444,15 +471,23 @@
p1_state=(REG_READ(ADMHCD_REG_PORTSTATUS1) & ADMHCD_CCS);
};
- /* no change, return NAK */
- if (p[0] == 0)
- return;
+ /* no change, return NAK and try again later */
+ if (p[0] == 0) {
+ callout_schedule(&sc->sc_poll_handle, sc->sc_interval);
+ goto out;
+ }
+ /*
+ * Interrupt completed, and the xfer has not been completed or
+ * synchronously aborted. Complete the xfer now.
+ *
+ * XXX Set ux_isdone if DIAGNOSTIC?
+ */
xfer->ux_actlen = 1;
xfer->ux_status = USBD_NORMAL_COMPLETION;
- mutex_enter(&sc->sc_lock);
usb_transfer_complete(xfer);
- mutex_exit(&sc->sc_lock);
+
+out: mutex_exit(&sc->sc_lock);
}
struct usbd_xfer *
@@ -719,8 +754,10 @@
DPRINTF(D_TRACE, ("SLRIstart "));
+ KASSERT(sc->sc_intr_xfer == NULL);
+
sc->sc_interval = MS_TO_TICKS(xfer->ux_pipe->up_endpoint->ue_edesc->bInterval);
- callout_reset(&sc->sc_poll_handle, sc->sc_interval, ahci_poll_hub, xfer);
+ callout_schedule(&sc->sc_poll_handle, sc->sc_interval);
sc->sc_intr_xfer = xfer;
return USBD_IN_PROGRESS;
}
@@ -728,24 +765,59 @@
static void
ahci_root_intr_abort(struct usbd_xfer *xfer)
{
+ struct ahci_softc *sc = AHCI_XFER2SC(xfer);
+
DPRINTF(D_TRACE, ("SLRIabort "));
+
+ KASSERT(mutex_owned(&sc->sc_lock));
+ KASSERT(xfer->ux_pipe->up_intrxfer == xfer);
+
+ /*
+ * Try to stop the callout before it starts. If we got in too
+ * late, too bad; but if the callout had yet to run and time
+ * out the xfer, cancel it ourselves.
+ */
+ callout_stop(&sc->sc_poll_handle);
+ if (sc->sc_intr_xfer == NULL)
+ return;
+
+ KASSERT(sc->sc_intr_xfer == xfer);
+ xfer->ux_status = USBD_CANCELLED;
+ usb_transfer_complete(xfer);
}
static void
ahci_root_intr_close(struct usbd_pipe *pipe)
{
- struct ahci_softc *sc = AHCI_PIPE2SC(pipe);
+ struct ahci_softc *sc __diagused = AHCI_PIPE2SC(pipe);
DPRINTF(D_TRACE, ("SLRIclose "));
- callout_stop(&sc->sc_poll_handle);
- sc->sc_intr_xfer = NULL;
+ KASSERT(mutex_owned(&sc->sc_lock));
+
+ /*
+ * The caller must arrange to have aborted the pipe already, so
+ * there can be no intr xfer in progress. The callout may
+ * still be pending from a prior intr xfer -- if it has already
+ * fired, it will see there is nothing to do, and do nothing.
+ */
+ KASSERT(sc->sc_intr_xfer == NULL);
+ KASSERT(!callout_pending(&sc->sc_poll_handle));
}
static void
ahci_root_intr_done(struct usbd_xfer *xfer)
{
+ struct ahci_softc *sc = AHCI_XFER2SC(xfer);
+
//DPRINTF(D_XFER, ("RIdn "));
+
+ KASSERT(mutex_owned(&sc->sc_lock));
+
+ /* Claim the xfer so it doesn't get completed again. */
+ KASSERT(sc->sc_intr_xfer == xfer);
+ KASSERT(xfer->ux_status != USBD_IN_PROGRESS);
+ sc->sc_intr_xfer = NULL;
}
static usbd_status
diff -r 1a64b1dee4e2 -r 803d2677f94f sys/dev/usb/ehci.c
--- a/sys/dev/usb/ehci.c Wed Feb 12 16:01:00 2020 +0000
+++ b/sys/dev/usb/ehci.c Wed Feb 12 16:02:01 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ehci.c,v 1.271 2020/02/12 16:01:00 riastradh Exp $ */
+/* $NetBSD: ehci.c,v 1.272 2020/02/12 16:02:01 riastradh Exp $ */
/*
* Copyright (c) 2004-2012 The NetBSD Foundation, Inc.
@@ -53,7 +53,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ehci.c,v 1.271 2020/02/12 16:01:00 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ehci.c,v 1.272 2020/02/12 16:02:01 riastradh Exp $");
#include "ohci.h"
#include "uhci.h"
@@ -2722,11 +2722,13 @@
if (!polling)
mutex_enter(&sc->sc_lock);
+ KASSERT(sc->sc_intrxfer == NULL);
sc->sc_intrxfer = xfer;
if (!polling)
mutex_exit(&sc->sc_lock);
- return USBD_IN_PROGRESS;
+ xfer->ux_status = USBD_IN_PROGRESS;
+ return xfer->ux_status;
}
/* Abort a root interrupt request. */
@@ -2738,8 +2740,16 @@
KASSERT(mutex_owned(&sc->sc_lock));
KASSERT(xfer->ux_pipe->up_intrxfer == xfer);
- sc->sc_intrxfer = NULL;
-
+ /* If xfer has already completed, nothing to do here. */
+ if (sc->sc_intrxfer == NULL)
+ return;
+
+ /*
+ * Otherwise, sc->sc_intrxfer had better be this transfer.
+ * Cancel it.
+ */
+ KASSERT(sc->sc_intrxfer == xfer);
+ KASSERT(xfer->ux_status == USBD_IN_PROGRESS);
xfer->ux_status = USBD_CANCELLED;
usb_transfer_complete(xfer);
}
@@ -2748,18 +2758,30 @@
Static void
ehci_root_intr_close(struct usbd_pipe *pipe)
{
- ehci_softc_t *sc = EHCI_PIPE2SC(pipe);
+ ehci_softc_t *sc __diagused = EHCI_PIPE2SC(pipe);
EHCIHIST_FUNC(); EHCIHIST_CALLED();
KASSERT(mutex_owned(&sc->sc_lock));
- sc->sc_intrxfer = NULL;
+ /*
+ * Caller must guarantee the xfer has completed first, by
+ * closing the pipe only after normal completion or an abort.
+ */
+ KASSERT(sc->sc_intrxfer == NULL);
}
Static void
ehci_root_intr_done(struct usbd_xfer *xfer)
{
+ struct ehci_softc *sc = EHCI_XFER2SC(xfer);
+
+ KASSERT(mutex_owned(&sc->sc_lock));
+
+ /* Claim the xfer so it doesn't get completed again. */
+ KASSERT(sc->sc_intrxfer == xfer);
+ KASSERT(xfer->ux_status != USBD_IN_PROGRESS);
+ sc->sc_intrxfer = NULL;
}
/************************/
diff -r 1a64b1dee4e2 -r 803d2677f94f sys/dev/usb/ohci.c
--- a/sys/dev/usb/ohci.c Wed Feb 12 16:01:00 2020 +0000
+++ b/sys/dev/usb/ohci.c Wed Feb 12 16:02:01 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ohci.c,v 1.293 2020/02/12 16:01:00 riastradh Exp $ */
+/* $NetBSD: ohci.c,v 1.294 2020/02/12 16:02:01 riastradh Exp $ */
/*
* Copyright (c) 1998, 2004, 2005, 2012 The NetBSD Foundation, Inc.
@@ -41,7 +41,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ohci.c,v 1.293 2020/02/12 16:01:00 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ohci.c,v 1.294 2020/02/12 16:02:01 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -1708,6 +1708,7 @@
p[i/8] |= 1 << (i%8);
}
DPRINTF("change=0x%02jx", *p, 0, 0, 0);
+ KASSERT(xfer->ux_status == USBD_IN_PROGRESS);
xfer->ux_actlen = xfer->ux_length;
xfer->ux_status = USBD_NORMAL_COMPLETION;
@@ -1721,7 +1722,9 @@
KASSERT(mutex_owned(&sc->sc_lock));
+ /* Claim the xfer so it doesn't get completed again. */
KASSERT(sc->sc_intrxfer == xfer);
+ KASSERT(xfer->ux_status != USBD_IN_PROGRESS);
sc->sc_intrxfer = NULL;
}
@@ -2516,18 +2519,29 @@
if (!polling)
mutex_exit(&sc->sc_lock);
- return USBD_IN_PROGRESS;
+ xfer->ux_status = USBD_IN_PROGRESS;
Home |
Main Index |
Thread Index |
Old Index