Changeset 61978 in vbox for trunk

Timestamp:

Jul 1, 2016 1:42:28 PM (8 years ago)

Author:

vboxsync

Message:

Storage/iSCSI: Fix a possible crash during VM resume when connectin to the target fails in iscsiSetOpenFlags (readonly -> read/write transition). Failing the connection attempt here leaves the image data in an inconsistent state leading to memory corruption later on if iscsiWrite/iscsiRead is called. Don't do any communication with the target in iscsiSetOpenFlags but use the remembered data from the first login to decide whether the transition is allowed. Introduce a new flag to indicate whether a reconnect attempt should be made from the I/O worker thread before processing new requests to try reconnecting after a VM resume which was suspended due to an unresponsive target

File:

• trunk/src/VBox/Storage/ISCSI.cpp (modified) (4 diffs)

trunk/src/VBox/Storage/ISCSI.cpp

@@ -573 +573 @@
     /** Flag whether to dump malformed packets in the release log. */
     bool                fDumpMalformedPackets;
+    /** Flag whtether the target is readonly. */
+    bool                fTargetReadOnly;
+    /** Flag whether to retry the connection before processing new requests. */
+    bool                fTryReconnect;
 
     /** Head of request queue */
@@ -3436 +3440 @@
                     case ISCSICMDTYPE_REQ:
                     {
+                        if (   !iscsiIsClientConnected(pImage)
+                            && pImage->fTryReconnect)
+                        {
+                            pImage->fTryReconnect = false;
+                            iscsiReattach(pImage);
+                        }
+    
                         /* If there is no connection complete the command with an error. */
                         if (RT_LIKELY(iscsiIsClientConnected(pImage)))
@@ -4275 +4286 @@
     if (RT_SUCCESS(rc))
     {
-        if (!(uOpenFlags & VD_OPEN_FLAGS_READONLY) && data4[2] & 0x80)
+        pImage->fTargetReadOnly = !!(data4[2] & 0x80);
+        if (!(uOpenFlags & VD_OPEN_FLAGS_READONLY) && pImage->fTargetReadOnly)
         {
             rc = VERR_VD_IMAGE_READ_ONLY;
@@ -5163 +5175 @@
 static DECLCALLBACK(int) iscsiSetOpenFlags(void *pBackendData, unsigned uOpenFlags)
 {
-    LogFlowFunc(("pBackendData=%#p\n uOpenFlags=%#x", pBackendData, uOpenFlags));
+    LogFlowFunc(("pBackendData=%#p uOpenFlags=%#x\n", pBackendData, uOpenFlags));
     PISCSIIMAGE pImage = (PISCSIIMAGE)pBackendData;
-    int rc;
+    int rc = VINF_SUCCESS;
 
     /* Image must be opened and the new flags must be valid. */
-    if (!pImage || (uOpenFlags & ~(  VD_OPEN_FLAGS_READONLY | VD_OPEN_FLAGS_INFO
-                                   | VD_OPEN_FLAGS_ASYNC_IO | VD_OPEN_FLAGS_SHAREABLE
-                                   | VD_OPEN_FLAGS_SEQUENTIAL | VD_OPEN_FLAGS_SKIP_CONSISTENCY_CHECKS)))
-    {
-        rc = VERR_INVALID_PARAMETER;
-        goto out;
-    }
-
-    /* Implement this operation via reopening the image if we actually need
-     * to do something. A read/write -> readonly transition doesn't need a
-     * reopen. In the other direction we don't have the necessary information
-     * as the "disk is readonly" flag is thrown away. Can be optimized too,
-     * but it's not worth the effort at the moment. */
+    AssertReturn(pImage && !(uOpenFlags & ~(  VD_OPEN_FLAGS_READONLY | VD_OPEN_FLAGS_INFO
+                                            | VD_OPEN_FLAGS_ASYNC_IO | VD_OPEN_FLAGS_SHAREABLE
+                                            | VD_OPEN_FLAGS_SEQUENTIAL | VD_OPEN_FLAGS_SKIP_CONSISTENCY_CHECKS)),
+                 VERR_INVALID_PARAMETER);
+
+    /*
+     * A read/write -> readonly transition is always possible,
+     * for the reverse direction check that the target didn't present itself
+     * as readonly during the first attach.
+     */
     if (   !(uOpenFlags & VD_OPEN_FLAGS_READONLY)
-        && (pImage->uOpenFlags & VD_OPEN_FLAGS_READONLY))
-    {
-        iscsiFreeImage(pImage, false);
-        rc = iscsiOpenImage(pImage, uOpenFlags);
-    }
+        && (pImage->uOpenFlags & VD_OPEN_FLAGS_READONLY)
+        && pImage->fTargetReadOnly)
+        rc = VERR_VD_IMAGE_READ_ONLY;
     else
     {
         pImage->uOpenFlags = uOpenFlags;
-        rc = VINF_SUCCESS;
-    }
-out:
+        pImage->fTryReconnect = true;
+    }
+
     LogFlowFunc(("returns %Rrc\n", rc));
     return rc;