Changeset 32710 in vbox for trunk/src/VBox/HostDrivers/VBoxNetFlt

Timestamp:

Sep 23, 2010 11:44:42 AM (14 years ago)

Author:

vboxsync

Message:

Solaris/VBoxNetFlt: fix deadlock with PromiscReq and ModWritePut (#5262)

File:

• trunk/src/VBox/HostDrivers/VBoxNetFlt/solaris/VBoxNetFlt-solaris.c (modified) (11 diffs)

trunk/src/VBox/HostDrivers/VBoxNetFlt/solaris/VBoxNetFlt-solaris.c

@@ -216 +216 @@
     ddi_prop_op,                    /* property ops */
     &g_VBoxNetFltSolarisStreamTab,
-    D_NEW | D_MP | D_MTQPAIR,       /* compat. flag */
+    D_NEW | D_MP | D_MTQPAIR | D_MTOUTPERIM | D_MTOCEXCL, /* compat. flag */
     CB_REV                          /* revision */
 };
@@ -255 +255 @@
     DEVICE_NAME,
     &g_VBoxNetFltSolarisStreamTab,
-    D_NEW | D_MP | D_MTQPAIR
+    D_NEW | D_MP | D_MTQPAIR | D_MTOUTPERIM | D_MTOCEXCL
 };
 
@@ -344 +344 @@
 #endif
     size_t cLoopback;                     /* loopback queue size list */
-    timeout_id_t TimeoutId;               /* timeout id of promisc. req */
+    timeout_id_t volatile TimeoutId;      /* timeout id of promisc. req */
     PVBOXNETFLTPACKETID pHead;            /* loopback packet identifier head */
     PVBOXNETFLTPACKETID pTail;            /* loopback packet identifier tail */
@@ -897 +897 @@
     if (pStream->Type == kPromiscStream)
     {
+        /*
+         * If there are any timeout scheduled, we need to make sure they are cancelled.
+         */
+        vboxnetflt_promisc_stream_t *pPromiscStream = (vboxnetflt_promisc_stream_t *)pStream;
+        timeout_id_t TimeoutId = ASMAtomicReadPtr(&pPromiscStream->TimeoutId);
+        if (TimeoutId)
+        {
+            quntimeout(WR(pPromiscStream->Stream.pReadQueue), TimeoutId);
+            ASMAtomicWritePtr(&pPromiscStream->TimeoutId, NULL);
+        }
+
         flushq(pQueue, FLUSHALL);
         flushq(WR(pQueue), FLUSHALL);
@@ -1299 +1310 @@
 
 /**
- * Send fake promiscous mode requests downstream.
- *
- * @param   pQueue          Pointer to the read queue.
- * @param   fPromisc        Whether to enable promiscous mode or not.
- * @param   PromiscLevel    Promiscous level; DL_PROMISC_PHYS/SAP/MULTI.
+ * Callback function for qwriter to send promiscuous request messages
+ * downstream.
+ *
+ * @param   pQueue          Pointer to the write queue.
+ * @param   fPromisc        Whether to send promiscuous ON or OFF requests.
  *
  * @returns VBox status code.
@@ -1346 +1357 @@
     }
 
-    qreply(pQueue, pPromiscPhysMsg);
-    qreply(pQueue, pPromiscSapMsg);
+    putnext(pQueue, pPromiscPhysMsg);
+    putnext(pQueue, pPromiscSapMsg);
 
     return VINF_SUCCESS;
@@ -1353 +1364 @@
 
 
-/*
- * Callback wrapper for qtimeout to safely send promiscuous off request.
- *
- * @param   pvData      Pointer to a vboxnetflt_promisc_params_t structure, will be freed by us.
+/**
+ * Callback wrapper for qwriter() to safely send promiscuous requests. This is
+ * called at the outer perimeter with exclusive lock held.
+ *
+ * @param pQueue            Pointer to the write queue.
+ * @param pMsg              A one byte message indicates a Promisc ON, otherwise
+ *                          a promiscuous OFF request. See
+ *                          vboxNetFltSolarisPromiscReqWrap().
+ */
+static void vboxNetFltSolarisPromiscReqWrapExcl(queue_t *pQueue, mblk_t *pMsg)
+{
+    /*
+     * Paranoia.
+     */
+    AssertReturnVoid(pQueue);
+    if (RT_UNLIKELY(!pMsg))
+        LogRel((DEVICE_NAME ":VBoxNetFltSolarisPromiscReqWrapExcl pQueue=%p missing message!\n", pQueue));
+
+    bool fPromisc = (MBLKL(pMsg) == 1);
+    freemsg(pMsg);
+    pMsg = NULL;
+    int rc = vboxNetFltSolarisPromiscReq(pQueue, fPromisc);
+    if (RT_FAILURE(rc))
+        LogRel((DEVICE_NAME ":VBoxNetFltSolarisPromiscReqWrapExcl vboxNetFltSolarisPromiscReq failed. rc=%d\n", rc));
+    else
+        LogRel((DEVICE_NAME ":VBoxNetFltSolarisPromiscReqWrapExcl success fPromisc=%d\n", fPromisc));
+}
+
+
+/**
+ * Callback wrapper for qtimeout() to safely send promiscuous requests. This is
+ * called at the inner perimenter with shared lock.
+ *
+ * @param pvData            Pointer to vboxnetflt_promisc_params_t. See
+ *                          vboxNetFltPortOsSetActive().
  */
 static void vboxNetFltSolarisPromiscReqWrap(void *pvData)
@@ -1368 +1410 @@
             && pPromiscStream->Stream.pReadQueue)
         {
-            pPromiscStream->TimeoutId = 0;
-            vboxNetFltSolarisPromiscReq(pPromiscStream->Stream.pReadQueue, pParams->fPromiscOn);
+            /*
+             * Use size of message to indicate to qwriter callback whether it must send
+             * promiscuous On or Off messages. This is ugly but easier and more efficient than
+             * scheduling two separate qwriter callbacks with prepared messages to putnext.
+             */
+            size_t cbMsg = pParams->fPromiscOn ? 1 : 2;
+            mblk_t *pMsg = allocb(cbMsg, BPRI_HI);
+            if (RT_UNLIKELY(!pMsg))
+            {
+                LogRel((DEVICE_NAME ":Failed to alloc message of %u bytes\n", cbMsg));
+                return;
+            }
+
+            /*
+             * Move the data pointer so we can use MBLKL, as MBLKSIZE gets the db_lim which is
+             * always aligned.
+             */
+            pMsg->b_wptr += cbMsg;
+
+            /*
+             * Upgrade inner perimeter lock to exclusive outer perimeter lock and
+             * then call putnext while we are at the outer perimeter.
+             */
+            qwriter(WR(pPromiscStream->Stream.pReadQueue), pMsg, vboxNetFltSolarisPromiscReqWrapExcl, PERIM_OUTER);
+            ASMAtomicWritePtr(&pPromiscStream->TimeoutId, NULL);
         }
         RTMemFree(pParams);
@@ -2022 +2087 @@
     if (pThis->u.s.hIface)
     {
-        /*
-         * If there are any timeout scheduled, we need to make sure they are cancelled.
-         */
-        vboxnetflt_promisc_stream_t *pPromiscStream = ASMAtomicUoReadPtrT(&pThis->u.s.pPromiscStream, vboxnetflt_promisc_stream_t *);
-        if (   pPromiscStream
-            && pPromiscStream->TimeoutId)
-        {
-            quntimeout(WR(pPromiscStream->Stream.pReadQueue), pPromiscStream->TimeoutId);
-        }
-
         ldi_close(pThis->u.s.hIface, FREAD | FWRITE, kcred);
         pThis->u.s.hIface = NULL;
@@ -3623 +3678 @@
     if (RT_LIKELY(pData))
     {
+        /*
+         * See #5262 as to why we need to do all this qtimeout/qwriter tricks.
+         */
         vboxnetflt_promisc_stream_t *pPromiscStream = ASMAtomicUoReadPtrT(&pThis->u.s.pPromiscStream, vboxnetflt_promisc_stream_t *);
         if (   pPromiscStream
@@ -3629 +3687 @@
             pData->pThis      = pThis;
             pData->fPromiscOn = fActive;
-            if (pPromiscStream->TimeoutId != 0)
+            if (ASMAtomicReadPtr(&pPromiscStream->TimeoutId))
                 quntimeout(WR(pPromiscStream->Stream.pReadQueue), pPromiscStream->TimeoutId);
-            pPromiscStream->TimeoutId = qtimeout(WR(pPromiscStream->Stream.pReadQueue), vboxNetFltSolarisPromiscReqWrap, pData, 1 /* ticks */);
-            return;
-        }
+            timeout_id_t TimeoutId = qtimeout(WR(pPromiscStream->Stream.pReadQueue), vboxNetFltSolarisPromiscReqWrap, pData, 1 /* ticks */);
+            ASMAtomicWritePtr(&pPromiscStream->TimeoutId, TimeoutId);
+            return; /* pData will be freed by vboxNetFltSolarisPromiscReqWrap() */
+        }
+        else
+            LogRel((DEVICE_NAME ":vboxNetFltPortOsSetActive pThis=%p fActive=%d missing stream!\n", pThis, fActive));
         RTMemFree(pData);
     }