Changeset 62294 in vbox for trunk/src/VBox

Timestamp:

Jul 18, 2016 10:12:57 AM (9 years ago)

Author:

vboxsync

Message:

VUSB: Add reference couting to the USB device structure to prevent races between EMT and the worker threads when the device is attached leading to crashes

Location:

trunk/src/VBox/Devices/USB

Files:

• DrvVUSBRootHub.cpp (modified) (7 diffs)
• VUSBDevice.cpp (modified) (6 diffs)
• VUSBInternal.h (modified) (3 diffs)

trunk/src/VBox/Devices/USB/DrvVUSBRootHub.cpp

@@ -279 +279 @@
         pUsbIns->pvVUsbDev2 = NULL;
     }
-    vusbDevDestroy(pDev);
-    RTMemFree(pDev);
+    vusbDevRelease(pDev);
     return rc;
 }
@@ -290 +289 @@
     PVUSBDEV pDev = (PVUSBDEV)pUsbIns->pvVUsbDev2;
     Assert(pDev);
-    vusbDevDestroy(pDev);
-    RTMemFree(pDev);
-    pUsbIns->pvVUsbDev2 = NULL;
+
+    /*
+     * Deal with pending async reset.
+     * (anything but reset)
+     */
+    vusbDevSetStateCmp(pDev, VUSB_DEVICE_STATE_DEFAULT, VUSB_DEVICE_STATE_RESET);
+
+    /*
+     * Detach and free resources.
+     */
+    if (pDev->pHub)
+        vusbDevDetach(pDev);
+
+    vusbDevRelease(pDev);
     return VINF_SUCCESS;
 }
@@ -322 +332 @@
 {
     unsigned iHash = vusbHashAddress(Address);
-    for (PVUSBDEV pDev = pRh->apAddrHash[iHash]; pDev; pDev = pDev->pNextHash)
-        if (pDev->u8Address == Address)
-            return pDev;
-    return NULL;
+    PVUSBDEV pDev = NULL;
+
+    RTCritSectEnter(&pRh->CritSectDevices);
+    for (PVUSBDEV pCur = pRh->apAddrHash[iHash]; pCur; pCur = pCur->pNextHash)
+        if (pCur->u8Address == Address)
+        {
+            pDev = pCur;
+            break;
+        }
+
+    if (pDev)
+        vusbDevRetain(pDev);
+    RTCritSectLeave(&pRh->CritSectDevices);
+    return pDev;
 }
 
@@ -355 +375 @@
     /* The URB comes from the roothub if there is no device (invalid address). */
     if (pUrb->pVUsb->pDev)
+    {
         vusbUrbPoolFree(&pUrb->pVUsb->pDev->UrbPool, pUrb);
+        vusbDevRelease(pUrb->pVUsb->pDev);
+    }
     else
         vusbUrbPoolFree(&pRh->Hub.Dev.UrbPool, pUrb);
@@ -371 +394 @@
     if (!pDev)
         pDev = vusbRhFindDevByAddress(pRh, DstAddress);
+    else
+        vusbDevRetain(pDev);
 
     if (pDev)
@@ -706 +731 @@
     else
     {
+        vusbDevRetain(&pRh->Hub.Dev);
         pUrb->pVUsb->pDev = &pRh->Hub.Dev;
         Log(("vusb: pRh=%p: SUBMIT: Address %i not found!!!\n", pRh, pUrb->DstAddress));
@@ -1235 +1261 @@
     pThis->Hub.Dev.u8NewAddress         = VUSB_INVALID_ADDRESS;
     pThis->Hub.Dev.i16Port              = -1;
+    pThis->Hub.Dev.cRefs                = 1;
     pThis->Hub.Dev.IDevice.pfnReset     = vusbRhDevReset;
     pThis->Hub.Dev.IDevice.pfnPowerOn   = vusbRhDevPowerOn;

trunk/src/VBox/Devices/USB/VUSBDevice.cpp

@@ -966 +966 @@
     pDev->u8NewAddress = VUSB_INVALID_ADDRESS;
 
+    RTCritSectEnter(&pDev->pHub->pRootHub->CritSectDevices);
     PVUSBDEV pCur = pDev->pHub->pRootHub->apAddrHash[u8Hash];
     if (pCur == pDev)
@@ -989 +990 @@
         }
     }
+    RTCritSectLeave(&pDev->pHub->pRootHub->CritSectDevices);
 }
 
@@ -1264 +1266 @@
  *
  * @param   pDev    The device.
- * @thread  EMT
+ * @thread any.
  */
 void vusbDevDestroy(PVUSBDEV pDev)
@@ -1270 +1272 @@
     LogFlow(("vusbDevDestroy: pDev=%p[%s] enmState=%d\n", pDev, pDev->pUsbIns->pszName, pDev->enmState));
 
-    /*
-     * Deal with pending async reset.
-     * (anything but reset)
-     */
-    vusbDevSetStateCmp(pDev, VUSB_DEVICE_STATE_DEFAULT, VUSB_DEVICE_STATE_RESET);
-
-    /*
-     * Detach and free resources.
-     */
-    if (pDev->pHub)
-        vusbDevDetach(pDev);
     RTMemFree(pDev->paIfStates);
     TMR3TimerDestroy(pDev->pResetTimer);
@@ -1307 +1298 @@
     /* Not using vusbDevSetState() deliberately here because it would assert on the state. */
     pDev->enmState = VUSB_DEVICE_STATE_DESTROYED;
+    pDev->pUsbIns->pvVUsbDev2 = NULL;
+    RTMemFree(pDev);
 }
 
@@ -1766 +1759 @@
     pDev->pHub = NULL;
     pDev->enmState = VUSB_DEVICE_STATE_DETACHED;
+    pDev->cRefs = 1;
     pDev->u8Address = VUSB_INVALID_ADDRESS;
     pDev->u8NewAddress = VUSB_INVALID_ADDRESS;

trunk/src/VBox/Devices/USB/VUSBInternal.h

@@ -227 +227 @@
     /** The device state. */
     VUSBDEVICESTATE volatile enmState;
+    /** Reference counter to protect the device structure from going away. */
+    uint32_t volatile        cRefs;
 
     /** The device address. */
@@ -254 +256 @@
     /** List of active async URBs. */
     RTLISTANCHOR        LstAsyncUrbs;
-#if HC_ARCH_BITS == 32
-    /** Align the size to a 8 byte boundary. */
-    uint32_t            u32Alignment0;
-#endif
 
     /** Dumper state. */
@@ -750 +748 @@
 }
 
+/**
+ * Retains the given VUSB device pointer.
+ *
+ * @returns New reference count.
+ * @param   pThis          The VUSB device pointer.
+ */
+DECLINLINE(uint32_t) vusbDevRetain(PVUSBDEV pThis)
+{
+    AssertPtrReturn(pThis, UINT32_MAX);
+
+    uint32_t cRefs = ASMAtomicIncU32(&pThis->cRefs);
+    AssertMsg(cRefs > 1 && cRefs < _1M, ("%#x %p\n", cRefs, pThis));
+    return cRefs;
+}
+
+/**
+ * Releases the given VUSB device pointer.
+ *
+ * @returns New reference count.
+ * @retval 0 if no onw is holding a reference anymore causing the device to be destroyed.
+ */
+DECLINLINE(uint32_t) vusbDevRelease(PVUSBDEV pThis)
+{
+    AssertPtrReturn(pThis, UINT32_MAX);
+
+    uint32_t cRefs = ASMAtomicDecU32(&pThis->cRefs);
+    AssertMsg(cRefs < _1M, ("%#x %p\n", cRefs, pThis));
+    if (cRefs == 0)
+        vusbDevDestroy(pThis);
+    return cRefs;
+}
+
 /** Strings for the CTLSTAGE enum values. */
 extern const char * const g_apszCtlStates[4];