Changeset 86405 in vbox for trunk

Timestamp:

Oct 2, 2020 5:58:42 AM (4 years ago)

Author:

vboxsync

Message:

Fixed problem with host crashing with DevVirtioNet_1_0.cpp R3 virtual address access from R0 when SMAP kernel parameter is enabled

Location:

trunk/src/VBox/Devices

Files:

• Network/DevVirtioNet_1_0.cpp (modified) (20 diffs)
• VirtIO/VirtioCore.h (modified) (2 diffs)

trunk/src/VBox/Devices/Network/DevVirtioNet_1_0.cpp

@@ -304 +304 @@
 typedef struct VIRTIONETVIRTQ
 {
-    struct VIRTIONETWORKER         *pWorker;                    /**< Pointer to R0 worker struct                    */
-    struct VIRTIONETWORKERR3       *pWorkerR3;                  /**< Pointer to R3 worker struct                    */
     uint16_t                       uIdx;                        /**< Index of this queue                            */
     uint16_t                       align;
@@ -321 +319 @@
 {
     SUPSEMEVENT                     hEvtProcess;                /**< handle of associated sleep/wake-up semaphore   */
-    PVIRTIONETVIRTQ                 pVirtq;                     /**< pointer to queue                               */
     uint16_t                        uIdx;                       /**< Index of this worker                           */
     bool volatile                   fSleeping;                  /**< Flags whether worker thread is sleeping or not */
@@ -337 +334 @@
 {
     R3PTRTYPE(PPDMTHREAD)           pThread;                    /**< pointer to worker thread's handle              */
-    PVIRTIONETVIRTQ                 pVirtq;                     /**< pointer to queue                               */
     uint16_t                        uIdx;                       /**< Index of this worker                           */
     uint16_t                        pad;
@@ -603 +599 @@
 }
 
+
+
 /**
  * @callback_method_impl{VIRTIOCORER0,pfnVirtqNotified}
@@ -612 +610 @@
 
     PVIRTIONETVIRTQ  pVirtq  = &pThis->aVirtqs[uVirtqNbr];
-    PVIRTIONETWORKER pWorker = pVirtq->pWorker;
+    PVIRTIONETWORKER pWorker = &pThis->aWorkers[uVirtqNbr];
 
 #if defined (IN_RING3) && defined (LOG_ENABLED)
+
      RTLogFlush(NULL);
+
 #endif
 
@@ -624 +624 @@
         if (cBufsAvailable)
         {
-            Log10Func(("%s %u empty bufs added to %s by guest (notifying leaf device)\n",
-                       pThis->szInst, cBufsAvailable, pVirtq->szName));
+            VIRTIOLOGLEVEL10("%s %u empty bufs added to %s by guest (notifying leaf device)\n",
+                       pThis->szInst, cBufsAvailable, pVirtq->szName);
             virtioNetWakeupRxBufWaiter(pDevIns);
         }
         else
-            LogRel(("%s \n\n***WARNING: %s notified but no empty bufs added by guest! (skip notifying of leaf device)\n\n",
-                    pThis->szInst, pVirtq->szName));
+            VIRTIOLOGLEVEL10("%s \n\n***WARNING: %s notified but no empty bufs added by guest! (skip notifying of leaf device)\n\n",
+                    pThis->szInst, pVirtq->szName);
     }
     else if (IS_TX_VIRTQ(uVirtqNbr) || IS_CTRL_VIRTQ(uVirtqNbr))
@@ -639 +639 @@
             if (ASMAtomicReadBool(&pWorker->fSleeping))
             {
-                Log10Func(("%s %s has available buffers - waking worker.\n", pThis->szInst, pVirtq->szName));
+                VIRTIOLOGLEVEL10("%s %s has available buffers - waking worker.\n", pThis->szInst, pVirtq->szName);
+
                 int rc = PDMDevHlpSUPSemEventSignal(pDevIns, pWorker->hEvtProcess);
                 AssertRC(rc);
@@ -645 +646 @@
             else
             {
-                Log10Func(("%s %s has available buffers - worker already awake\n", pThis->szInst, pVirtq->szName));
+                VIRTIOLOGLEVEL10("%s %s has available buffers - worker already awake\n", pThis->szInst, pVirtq->szName);
             }
         }
         else
         {
-            Log10Func(("%s %s has available buffers - waking worker.\n", pThis->szInst, pVirtq->szName));
+            VIRTIOLOGLEVEL10("%s %s has available buffers - waking worker.\n", pThis->szInst, pVirtq->szName);
         }
     }
@@ -838 +839 @@
             if (pVirtq->fHasWorker)
             {
-                PVIRTIONETWORKER   pWorker   = pVirtq->pWorker;
-                PVIRTIONETWORKERR3 pWorkerR3 = pVirtq->pWorkerR3;
+                PVIRTIONETWORKER   pWorker   = &pThis->aWorkers[uVirtqNbr];
+                PVIRTIONETWORKERR3 pWorkerR3 = &pThisCC->aWorkers[uVirtqNbr];
+
+                Assert((pWorker->uIdx == pVirtq->uIdx));
+                Assert((pWorkerR3->uIdx == pVirtq->uIdx));
 
                 if (pWorker->fAssigned)
@@ -1134 +1138 @@
     {
         PVIRTIONETWORKER pWorker = &pThis->aWorkers[uIdxWorker];
-        PVIRTIONETVIRTQ  pVirtq  = pWorker->pVirtq;
+        PVIRTIONETVIRTQ  pVirtq  = &pThis->aVirtqs[uIdxWorker];
         if (pVirtq->fAttachedToVirtioCore)
         {
@@ -2589 +2593 @@
 }
 
-static int virtioNetR3CreateOneWorkerThread(PPDMDEVINS pDevIns, PVIRTIONET pThis, uint16_t uIdxWorker,
+static int virtioNetR3CreateOneWorkerThread(PPDMDEVINS pDevIns, PVIRTIONET pThis,
                                             PVIRTIONETWORKER pWorker, PVIRTIONETWORKERR3 pWorkerR3,
                                             PVIRTIONETVIRTQ pVirtq)
@@ -2597 +2601 @@
 
     int rc = PDMDevHlpSUPSemEventCreate(pDevIns, &pWorker->hEvtProcess);
+    LogFunc(("PDMDevHlpSUPSemEventCreate(pDevIns, &pWorker->hEvtProcess=%p)", &pWorker->hEvtProcess));
+    LogFunc(("pWorker->hEvtProcess = %x\n", pWorker->hEvtProcess));
 
     if (RT_FAILURE(rc))
@@ -2611 +2617 @@
                                    N_("Error creating thread for Virtual Virtq %s\n"), pVirtq->uIdx);
 
-    pWorker->pVirtq    = pWorkerR3->pVirtq   = pVirtq;
-    pWorker->uIdx      = pWorkerR3->uIdx     = uIdxWorker;
-    pVirtq->pWorker    = pWorker;
-    pVirtq->pWorkerR3  = pWorkerR3;
     pWorker->fAssigned = true;  /* Because worker's state held in fixed-size array w/empty slots */
 
@@ -2625 +2627 @@
 {
 
-#define CTRLWIDX 0 /* First worker is for the control queue */
 
     Log10Func(("%s\n", pThis->szInst));
 
     PVIRTIONETVIRTQ pCtlVirtq = &pThis->aVirtqs[CTRLQIDX];
-    int rc = virtioNetR3CreateOneWorkerThread(pDevIns, pThis, CTRLQIDX /* uIdxWorker */,
-                                              &pThis->aWorkers[CTRLWIDX], &pThisCC->aWorkers[CTRLWIDX], pCtlVirtq);
+    int rc = virtioNetR3CreateOneWorkerThread(pDevIns, pThis,
+                                              &pThis->aWorkers[CTRLQIDX], &pThisCC->aWorkers[CTRLQIDX], pCtlVirtq);
     AssertRCReturn(rc, rc);
 
     pCtlVirtq->fHasWorker = true;
 
-    uint16_t uIdxWorker = CTRLWIDX + 1;
-    for (uint16_t uVirtqPair = pThis->cInitializedVirtqPairs; uVirtqPair < pThis->cVirtqPairs; uVirtqPair++, uIdxWorker++)
+    for (uint16_t uVirtqPair = pThis->cInitializedVirtqPairs; uVirtqPair < pThis->cVirtqPairs; uVirtqPair++)
     {
         PVIRTIONETVIRTQ pTxVirtq = &pThis->aVirtqs[TXQIDX(uVirtqPair)];
         PVIRTIONETVIRTQ pRxVirtq = &pThis->aVirtqs[RXQIDX(uVirtqPair)];
 
-        rc = virtioNetR3CreateOneWorkerThread(pDevIns, pThis, uIdxWorker, &pThis->aWorkers[uIdxWorker],
-                                              &pThisCC->aWorkers[uIdxWorker], pTxVirtq);
+        rc = virtioNetR3CreateOneWorkerThread(pDevIns, pThis, &pThis->aWorkers[TXQIDX(uVirtqPair)],
+                                                              &pThisCC->aWorkers[TXQIDX(uVirtqPair)], pTxVirtq);
         AssertRCReturn(rc, rc);
 
@@ -2665 +2665 @@
     PVIRTIONETCC       pThisCC   = PDMDEVINS_2_DATA_CC(pDevIns, PVIRTIONETCC);
     PVIRTIONETWORKER   pWorker   = (PVIRTIONETWORKER)pThread->pvUser;
-    PVIRTIONETVIRTQ    pVirtq    = pWorker->pVirtq;
+    PVIRTIONETVIRTQ    pVirtq    = &pThis->aVirtqs[pWorker->uIdx];
+    uint16_t           uIdx      = pWorker->uIdx;
+
+    ASMAtomicWriteBool(&pWorker->fSleeping, false);
+
+    Assert(pWorker->uIdx == pVirtq->uIdx);
 
     if (pThread->enmState == PDMTHREADSTATE_INITIALIZING)
         return VINF_SUCCESS;
 
-    LogFunc(("%s worker thread started for %s\n", pThis->szInst, pVirtq->szName));
+    LogFunc(("%s worker thread idx=%d started for %s (virtq idx=%d)\n", pThis->szInst,  pWorker->uIdx, pVirtq->szName, pVirtq->uIdx));
 
     /** @todo Race w/guest enabling/disabling guest notifications cyclically.
               See BugRef #8651, Comment #82 */
-    virtioCoreVirtqEnableNotify(&pThis->Virtio, pVirtq->uIdx, true /* fEnable */);
+    virtioCoreVirtqEnableNotify(&pThis->Virtio, uIdx, true /* fEnable */);
 
     while (   pThread->enmState != PDMTHREADSTATE_TERMINATING
            && pThread->enmState != PDMTHREADSTATE_TERMINATED)
     {
-        if (IS_VIRTQ_EMPTY(pDevIns, &pThis->Virtio, pVirtq->uIdx))
+        if (IS_VIRTQ_EMPTY(pDevIns, &pThis->Virtio,  pVirtq->uIdx))
         {
             /* Atomic interlocks avoid missing alarm while going to sleep & notifier waking the awoken */
             ASMAtomicWriteBool(&pWorker->fSleeping, true);
+
             bool fNotificationSent = ASMAtomicXchgBool(&pWorker->fNotified, false);
             if (!fNotificationSent)
@@ -2688 +2694 @@
                 Log10Func(("%s %s worker sleeping...\n\n", pThis->szInst, pVirtq->szName));
                 Assert(ASMAtomicReadBool(&pWorker->fSleeping));
+
                 int rc = PDMDevHlpSUPSemEventWaitNoResume(pDevIns, pWorker->hEvtProcess, RT_INDEFINITE_WAIT);
                 STAM_COUNTER_INC(&pThis->StatTransmitByThread);
@@ -2698 +2705 @@
             }
             ASMAtomicWriteBool(&pWorker->fSleeping, false);
+
         }
 
@@ -2760 +2768 @@
         {
             PVIRTIONETVIRTQ pVirtq = &pThis->aVirtqs[uVirtqNbr];
-            pVirtq->uIdx = uVirtqNbr;
+            PVIRTIONETWORKER pWorker = &pThis->aWorkers[uVirtqNbr];
+
+            Assert(pWorker->uIdx == uVirtqNbr);
+            Assert(pVirtq->uIdx == pWorker->uIdx);
+
             (void) virtioCoreR3VirtqAttach(&pThis->Virtio, pVirtq->uIdx, pVirtq->szName);
             pVirtq->fAttachedToVirtioCore = true;
@@ -3032 +3044 @@
     virtioNetR3SetVirtqNames(pThis);
     pThis->aVirtqs[CTRLQIDX].fCtlVirtq = true;
+    for (unsigned uVirtqNbr = 0; uVirtqNbr < pThis->cVirtVirtqs; uVirtqNbr++)
+    {
+        PVIRTIONETVIRTQ pVirtq = &pThis->aVirtqs[uVirtqNbr];
+        PVIRTIONETWORKER pWorker = &pThis->aWorkers[uVirtqNbr];
+        PVIRTIONETWORKERR3 pWorkerR3 = &pThisCC->aWorkers[uVirtqNbr];
+        pVirtq->uIdx = uVirtqNbr;
+        pWorker->uIdx = uVirtqNbr;
+        pWorkerR3->uIdx = uVirtqNbr;
+    }
 
     /*
@@ -3039 +3060 @@
     if (RT_FAILURE(rc))
         return PDMDEV_SET_ERROR(pDevIns, rc, N_("Failed to create worker threads"));
+
 
     /*

trunk/src/VBox/Devices/VirtIO/VirtioCore.h

@@ -35 +35 @@
 # define VIRTIO_HEX_DUMP(logLevel, pv, cb, base, title) do { } while (0)
 #endif
+
+#ifdef IN_RING3
+#   define VIRTIOLOG(format...)    LogFunc((format))
+#   define VIRTIOLOGREL(format...) LogRelFunc((format))
+#else
+#   define VIRTIOLOG(format...)    SUPR0Printf(format)
+#   define VIRTIOLOGREL(format...) SUPR0Printf(format)
+#endif
+
+#define VIRTIOLOGLEVEL2(format...)  virtioLogLevel(2,  format)
+#define VIRTIOLOGLEVEL3(format...)  virtioLogLevel(3,  format)
+#define VIRTIOLOGLEVEL4(format...)  virtioLogLevel(4,  format)
+#define VIRTIOLOGLEVEL5(format...)  virtioLogLevel(5,  format)
+#define VIRTIOLOGLEVEL6(format...)  virtioLogLevel(6,  format)
+#define VIRTIOLOGLEVEL7(format...)  virtioLogLevel(7,  format)
+#define VIRTIOLOGLEVEL8(format...)  virtioLogLevel(8,  format)
+#define VIRTIOLOGLEVEL9(format...)  virtioLogLevel(9,  format)
+#define VIRTIOLOGLEVEL10(format...) virtioLogLevel(10, format)
+#define VIRTIOLOGLEVEL11(format...) virtioLogLevel(11, format)
+#define VIRTIOLOGLEVEL12(format...) virtioLogLevel(12, format)
+
+DECLINLINE(void) virtioLogLevel(int logLevel, const char *format...)
+{
+    va_list arglist;
+    va_start(arglist, format);
+    if (LogIsItEnabled(logLevel, LOG_GROUP))
+        VIRTIOLOG(format);
+    va_end(arglist);
+}
 
 /** Pointer to the shared VirtIO state. */
@@ -362 +391 @@
     R3PTRTYPE(PVIRTIO_PCI_CAP_T)        pDeviceCap;                /**< Pointer to struct in PCI config area.     */
 
-    uint32_t                    cbDevSpecificCfg;                  /**< Size of client's dev-specific config data */
-    R3PTRTYPE(uint8_t *)        pbDevSpecificCfg;                  /**< Pointer to client's struct                */
-    R3PTRTYPE(uint8_t *)        pbPrevDevSpecificCfg;              /**< Previous read dev-specific cfg of client  */
-    bool                        fGenUpdatePending;                 /**< If set, update cfg gen after driver reads */
-    char                        pcszMmioName[MAX_NAME];            /**< MMIO mapping name                         */
+    uint32_t                            cbDevSpecificCfg;          /**< Size of client's dev-specific config data */
+    R3PTRTYPE(uint8_t *)                pbDevSpecificCfg;          /**< Pointer to client's struct                */
+    R3PTRTYPE(uint8_t *)                pbPrevDevSpecificCfg;      /**< Previous read dev-specific cfg of client  */
+    bool                                fGenUpdatePending;         /**< If set, update cfg gen after driver reads */
+    char                                pcszMmioName[MAX_NAME];    /**< MMIO mapping name                         */
 } VIRTIOCORER3;