From: VMware, Inc <>
Date: Tue, 26 Apr 2011 20:36:26 +0000 (-0700)
Subject: VMCIQPair_Detach() should not leak when it fails.
X-Git-Tag: 2011.04.25-402641~62
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=bcc970017bc734a337825e209fa5599e174fd0cb;p=thirdparty%2Fopen-vm-tools.git

VMCIQPair_Detach() should not leak when it fails.

We're hitting an ASSERT on shutdown. Code can
still have a socket open very late in the OS
shutdown sequence.  VMCISockets forcibly closes
this socket so that it can safely unload.  The
device I/O space is already gone at this point,
and with it the queuepairs.  So we fail to
detach the queuepair, but we still ASSERT at
the VMCISocket level that we released it.
This occurs in both the common code (ESX,
Mac OS and Windows) and the Linux code.

At the VMCI level, we're actually leaking
when it fails, because we only release the
entry and qpair struct on success.  There's
nothing a caller can do at that point, so we
should always release the entry and qpair
struct.

Signed-off-by: Marcelo Vanzin <mvanzin@vmware.com>
---

diff --git a/open-vm-tools/modules/linux/vmci/common/vmciQPair.c b/open-vm-tools/modules/linux/vmci/common/vmciQPair.c
index 2a62b4182..5c4bc92bf 100644
--- a/open-vm-tools/modules/linux/vmci/common/vmciQPair.c
+++ b/open-vm-tools/modules/linux/vmci/common/vmciQPair.c
@@ -209,22 +209,19 @@ VMCIQPair_Detach(VMCIQPair **qpair) // IN/OUT
    oldQPair = *qpair;
    result = VMCIQueuePair_Detach(oldQPair->handle, oldQPair->guestEndpoint);
 
-   if  (result >= VMCI_SUCCESS) {
-#ifdef DEBUG
-      oldQPair->handle = VMCI_INVALID_HANDLE;
-      oldQPair->produceQ = NULL;
-      oldQPair->consumeQ = NULL;
-      oldQPair->produceQSize = 0;
-      oldQPair->consumeQSize = 0;
-      oldQPair->flags = 0;
-      oldQPair->privFlags = 0;
-      oldQPair->peer = VMCI_INVALID_ID;
-#endif
-
-      VMCI_FreeKernelMem(oldQPair, sizeof *oldQPair);
-
-      *qpair = NULL;
-   }
+   /*
+    * The guest can fail to detach for a number of reasons, and if it does so,
+    * it will cleanup the entry (if there is one).  The host can fail too, but
+    * it won't cleanup the entry immediately, it will do that later when the
+    * context is freed.  Either way, we need to release the qpair struct here;
+    * there isn't much the caller can do, and we don't want to leak.
+    */
+
+   memset(oldQPair, 0, sizeof *oldQPair);
+   oldQPair->handle = VMCI_INVALID_HANDLE;
+   oldQPair->peer = VMCI_INVALID_ID;
+   VMCI_FreeKernelMem(oldQPair, sizeof *oldQPair);
+   *qpair = NULL;
 
    return result;
 }
diff --git a/open-vm-tools/modules/linux/vmci/common/vmciQueuePair.c b/open-vm-tools/modules/linux/vmci/common/vmciQueuePair.c
index 0a476b4c9..a01b6ea6d 100644
--- a/open-vm-tools/modules/linux/vmci/common/vmciQueuePair.c
+++ b/open-vm-tools/modules/linux/vmci/common/vmciQueuePair.c
@@ -2212,8 +2212,8 @@ VMCIQueuePairDetachGuestWork(VMCIHandle handle)   // IN
 
    entry = (QPGuestEndpoint *)QueuePairList_FindEntry(&qpGuestEndpoints, handle);
    if (!entry) {
-      result = VMCI_ERROR_NOT_FOUND;
-      goto out;
+      VMCIQPLock_Release(&qpGuestEndpoints.lock);
+      return VMCI_ERROR_NOT_FOUND;
    }
 
    ASSERT(entry->qp.refCount >= 1);
@@ -2223,9 +2223,11 @@ VMCIQueuePairDetachGuestWork(VMCIHandle handle)   // IN
 
       if (entry->qp.refCount > 1) {
          result = QueuePairNotifyPeerLocal(FALSE, handle);
-         if (result < VMCI_SUCCESS) {
-            goto out;
-         }
+         /*
+          * We can fail to notify a local queuepair because we can't allocate.
+          * We still want to release the entry if that happens, so don't bail
+          * out yet.
+          */
       }
    } else {
       result = VMCIQueuePairDetachHypercall(handle);
@@ -2249,15 +2251,28 @@ VMCIQueuePairDetachGuestWork(VMCIHandle handle)   // IN
             VMCIQPUnmarkHibernateFailed(entry);
          }
       }
+      if (result < VMCI_SUCCESS) {
+         /*
+          * We failed to notify a non-local queuepair.  That other queuepair
+          * might still be accessing the shared memory, so don't release the
+          * entry yet.  It will get cleaned up by VMCIQueuePair_Exit()
+          * if necessary (assuming we are going away, otherwise why did this
+          * fail?).
+          */
+
+         VMCIQPLock_Release(&qpGuestEndpoints.lock);
+         return result;
+      }
    }
 
-out:
-   if (result >= VMCI_SUCCESS) {
-      entry->qp.refCount--;
+   /*
+    * If we get here then we either failed to notify a local queuepair, or
+    * we succeeded in all cases.  Release the entry if required.
+    */
 
-      if (entry->qp.refCount == 0) {
-         QueuePairList_RemoveEntry(&qpGuestEndpoints, &entry->qp);
-      }
+   entry->qp.refCount--;
+   if (entry->qp.refCount == 0) {
+      QueuePairList_RemoveEntry(&qpGuestEndpoints, &entry->qp);
    }
 
    /* If we didn't remove the entry, this could change once we unlock. */
@@ -2269,7 +2284,7 @@ out:
 
    VMCIQPLock_Release(&qpGuestEndpoints.lock);
 
-   if (result >= VMCI_SUCCESS && refCount == 0) {
+   if (refCount == 0) {
       QPGuestEndpointDestroy(entry);
    }
    return result;