Re: [PATCH 3/3] Fix race in finding available vnc port

[Jim Fehlig <jfehlig@xxxxxxxxxx>]

Eric Blake wrote:
> On 05/20/2010 02:45 PM, Jim Fehlig wrote:
>   
>>      for (i = QEMU_VNC_PORT_MIN; i < QEMU_VNC_PORT_MAX; i++) {
>>          int fd;
>>          int reuse = 1;
>>          struct sockaddr_in addr;
>> +        bool used = false;
>> +
>> +        virBitmapGetBit(driver->reservedVNCPorts, i - QEMU_VNC_PORT_MIN, &used);
>>     
>
> For now, we're safe not checking for failure here.  But maybe we should
> add an error check and a VIR_DEBUG to be proactive in case the size of
> driver->reservedVNCPorts is ever accidentally changed?

I've added this suggestion and rebased against cleanup label changes in
qemudStartVMDaemon.

Regards,
Jim

>From 7eb16f0237971b3dc337684cfa43dbf6e4bf0bc6 Mon Sep 17 00:00:00 2001
Message-Id: <7eb16f0237971b3dc337684cfa43dbf6e4bf0bc6.1274457350.git.jfehlig@xxxxxxxxxx>
In-Reply-To: <cover.1274457350.git.jfehlig@xxxxxxxxxx>
References: <cover.1274457350.git.jfehlig@xxxxxxxxxx>
From: Jim Fehlig <jfehlig@xxxxxxxxxx>
Date: Fri, 21 May 2010 07:52:09 -0600
Subject: [PATCH 3/3] Fix race in finding available vnc port

The qemu driver contains a subtle race in the logic to find next
available vnc port.  Currently it iterates through all available ports
and returns the first for which bind(2) succeeds.  However it is possible
that a previously issued port has not yet been bound by qemu, resulting
in the same port used for a subsequent domain.

This patch addresses the race by using a simple bitmap to "reserve" the
ports allocated by libvirt.

V2:
  - Put port bitmap in struct qemud_driver
  - Initialize bitmap in qemudStartup

V3:
  - Check for failure of virBitmapGetBit
  - Additional check for port != -1 before calling virbitmapClearBit
---
 src/qemu/qemu_conf.h   |    3 +++
 src/qemu/qemu_driver.c |   31 ++++++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 11f8dcd..8fd8d79 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -39,6 +39,7 @@
 # include "pci.h"
 # include "cpu_conf.h"
 # include "driver.h"
+# include "bitmap.h"
 
 # define qemudDebug(fmt, ...) do {} while(0)
 
@@ -153,6 +154,8 @@ struct qemud_driver {
     char *saveImageFormat;
 
     pciDeviceList *activePciHostdevs;
+
+    virBitmapPtr reservedVNCPorts;
 };
 
 typedef struct _qemuDomainPCIAddressSet qemuDomainPCIAddressSet;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 265067c..d497393 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1479,6 +1479,11 @@ qemudStartup(int privileged) {
          virEventAddTimeout(-1, qemuDomainEventFlush, qemu_driver, NULL)) < 0)
         goto error;
 
+    /* Allocate bitmap for vnc port reservation */
+    if ((qemu_driver->reservedVNCPorts =
+         virBitmapAlloc(QEMU_VNC_PORT_MAX - QEMU_VNC_PORT_MIN)) == NULL)
+        goto out_of_memory;
+
     if (privileged) {
         if (virAsprintf(&qemu_driver->logDir,
                         "%s/log/libvirt/qemu", LOCAL_STATE_DIR) == -1)
@@ -1775,6 +1780,7 @@ qemudShutdown(void) {
     virCapabilitiesFree(qemu_driver->caps);
 
     virDomainObjListDeinit(&qemu_driver->domains);
+    virBitmapFree(qemu_driver->reservedVNCPorts);
 
     VIR_FREE(qemu_driver->securityDriverName);
     VIR_FREE(qemu_driver->logDir);
@@ -2632,13 +2638,22 @@ qemuInitPCIAddresses(struct qemud_driver *driver,
     return ret;
 }
 
-static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) {
+static int qemudNextFreeVNCPort(struct qemud_driver *driver) {
     int i;
 
     for (i = QEMU_VNC_PORT_MIN; i < QEMU_VNC_PORT_MAX; i++) {
         int fd;
         int reuse = 1;
         struct sockaddr_in addr;
+        bool used = false;
+
+        if (virBitmapGetBit(driver->reservedVNCPorts,
+                            i - QEMU_VNC_PORT_MIN, &used) < 0)
+            VIR_DEBUG("virBitmapGetBit failed on bit %d", i - QEMU_VNC_PORT_MIN);
+
+        if (used)
+            continue;
+
         addr.sin_family = AF_INET;
         addr.sin_port = htons(i);
         addr.sin_addr.s_addr = htonl(INADDR_ANY);
@@ -2654,6 +2669,8 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) {
         if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0) {
             /* Not in use, lets grab it */
             close(fd);
+            /* Add port to bitmap of reserved ports */
+            virBitmapSetBit(driver->reservedVNCPorts, i - QEMU_VNC_PORT_MIN);
             return i;
         }
         close(fd);
@@ -3694,6 +3711,18 @@ retry:
 
     qemudRemoveDomainStatus(driver, vm);
 
+    /* Remove VNC port from port reservation bitmap, but only if it was
+       reserved by the driver (autoport=yes)
+    */
+    if ((vm->def->ngraphics == 1) &&
+        vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+        vm->def->graphics[0]->data.vnc.autoport &&
+        vm->def->graphics[0]->data.vnc.port != -1) {
+        virBitmapClearBit(driver->reservedVNCPorts,
+                          vm->def->graphics[0]->data.vnc.port - \
+                          QEMU_VNC_PORT_MIN);
+    }
+
     vm->pid = -1;
     vm->def->id = -1;
     vm->state = VIR_DOMAIN_SHUTOFF;
-- 
1.6.0.2

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list