diff --git a/libvirt.changes b/libvirt.changes index 699fafe..00abc4b 100644 --- a/libvirt.changes +++ b/libvirt.changes @@ -1,3 +1,10 @@ +------------------------------------------------------------------- +Tue Jun 1 13:52:21 MDT 2010 - jfehlig@novell.com + +- Fix race in VNC port reservation with qemu/KVM domains + bnc#594024 + vnc-race-{1,2,3}.patch + ------------------------------------------------------------------- Wed May 12 11:29:44 MDT 2010 - jfehlig@novell.com diff --git a/libvirt.spec b/libvirt.spec index b00bf18..0f1992a 100644 --- a/libvirt.spec +++ b/libvirt.spec @@ -143,6 +143,9 @@ Source0: %{name}-%{version}.tar.bz2 Source1: libvirtd.init # Upstream patches Patch0: remote-rm-unused-field.patch +Patch1: vnc-race-1.patch +Patch2: vnc-race-2.patch +Patch3: vnc-race-3.patch # Need to go upstream Patch100: xen-name-for-devid.patch Patch101: socat.patch @@ -257,6 +260,9 @@ Authors: %prep %setup -q %patch0 -p1 +%patch1 -p1 +%patch2 -p1 +%patch3 -p1 %patch100 -p1 %patch101 -p1 %patch102 diff --git a/vnc-race-1.patch b/vnc-race-1.patch new file mode 100644 index 0000000..32d7aca --- /dev/null +++ b/vnc-race-1.patch @@ -0,0 +1,281 @@ +commit 2f32d7afd5bd71f79c0e756c87813702065c6d1a +Author: Jim Fehlig +Date: Thu May 20 22:23:48 2010 -0600 + + Add simple bitmap operations to utils + + V2: + - Move bitmap impl to src/util/bitmap.[ch] + - Use CHAR_BIT instead of explicit '8' + - Use size_t instead of unsigned int + - Fix calculation of bitmap size in virBitmapAlloc + - Ensure bit is within range of map in the set, clear, and get + operations + - Use bool in virBitmapGetBit + - Add virBitmapFree to free-like funcs in cfg.mk + + V3: + - Check for overflow in virBitmapAlloc + - Fix copy and paste bug in virBitmapAlloc + - Use size_t in prototypes + - Add ATTRIBUTE_NONNULL in prototypes where appropriate + and remove NULL check from impl + + V4: + - Add ATTRIBUTE_RETURN_CHECK in prototypes where appropriate. + +Index: libvirt-0.8.1/src/Makefile.am +=================================================================== +--- libvirt-0.8.1.orig/src/Makefile.am ++++ libvirt-0.8.1/src/Makefile.am +@@ -50,6 +50,7 @@ augeastest_DATA = + # helper APIs for various purposes + UTIL_SOURCES = \ + util/authhelper.c util/authhelper.h \ ++ util/bitmap.c util/bitmap.h \ + util/bridge.c util/bridge.h \ + util/buf.c util/buf.h \ + util/conf.c util/conf.h \ +Index: libvirt-0.8.1/src/libvirt_private.syms +=================================================================== +--- libvirt-0.8.1.orig/src/libvirt_private.syms ++++ libvirt-0.8.1/src/libvirt_private.syms +@@ -4,6 +4,14 @@ + # + + ++# bitmap.h ++virBitmapAlloc; ++virBitmapFree; ++virBitmapSetBit; ++virBitmapClearBit; ++virBitmapGetBit; ++ ++ + # buf.h + virBufferVSprintf; + virBufferEscapeString; +Index: libvirt-0.8.1/src/util/bitmap.c +=================================================================== +--- /dev/null ++++ libvirt-0.8.1/src/util/bitmap.c +@@ -0,0 +1,151 @@ ++/* ++ * bitmap.h: Simple bitmap operations ++ * ++ * Copyright (C) 2010 Novell, Inc. ++ * ++ * This library is free software; you can redistribute it and/or ++ * modify it under the terms of the GNU Lesser General Public ++ * License as published by the Free Software Foundation; either ++ * version 2.1 of the License, or (at your option) any later version. ++ * ++ * This library is distributed in the hope that it will be useful, ++ * but WITHOUT ANY WARRANTY; without even the implied warranty of ++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU ++ * Lesser General Public License for more details. ++ * ++ * You should have received a copy of the GNU Lesser General Public ++ * License along with this library; if not, write to the Free Software ++ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA ++ * ++ * Author: Jim Fehlig ++ */ ++ ++#include ++ ++#include ++#include ++#include ++#include ++#include ++#include ++ ++#include "bitmap.h" ++#include "memory.h" ++ ++ ++struct _virBitmap { ++ size_t size; ++ uint32_t *map; ++}; ++ ++ ++#define VIR_BITMAP_BITS_PER_UNIT (sizeof(uint32_t) * CHAR_BIT) ++#define VIR_BITMAP_UNIT_OFFSET(b) ((b) / VIR_BITMAP_BITS_PER_UNIT) ++#define VIR_BITMAP_BIT_OFFSET(b) ((b) % VIR_BITMAP_BITS_PER_UNIT) ++ ++ ++/** ++ * virBitmapAlloc: ++ * @size: number of bits ++ * ++ * Allocate a bitmap capable of containing @size bits. ++ * ++ * Returns a pointer to the allocated bitmap or NULL if ++ * memory cannot be allocated. ++ */ ++virBitmapPtr virBitmapAlloc(size_t size) ++{ ++ virBitmapPtr bitmap; ++ size_t sz; ++ ++ if (SIZE_MAX - VIR_BITMAP_BITS_PER_UNIT < size) ++ return NULL; ++ ++ sz = (size + VIR_BITMAP_BITS_PER_UNIT - 1) / ++ VIR_BITMAP_BITS_PER_UNIT; ++ ++ if (VIR_ALLOC(bitmap) < 0) ++ return NULL; ++ ++ if (VIR_ALLOC_N(bitmap->map, sz) < 0) { ++ VIR_FREE(bitmap); ++ return NULL; ++ } ++ ++ return bitmap; ++} ++ ++/** ++ * virBitmapFree: ++ * @bitmap: previously allocated bitmap ++ * ++ * Free @bitmap previously allocated by virBitmapAlloc. ++ */ ++void virBitmapFree(virBitmapPtr bitmap) ++{ ++ if (bitmap) { ++ VIR_FREE(bitmap->map); ++ VIR_FREE(bitmap); ++ } ++} ++ ++/** ++ * virBitmapSetBit: ++ * @bitmap: Pointer to bitmap ++ * @b: bit position to set ++ * ++ * Set bit position @b in @bitmap ++ * ++ * Returns 0 on if bit is successfully set, -1 on error. ++ */ ++int virBitmapSetBit(virBitmapPtr bitmap, size_t b) ++{ ++ if (b > bitmap->size - 1) ++ return -1; ++ ++ bitmap->map[VIR_BITMAP_UNIT_OFFSET(b)] |= (1 << VIR_BITMAP_BIT_OFFSET(b)); ++ return 0; ++} ++ ++/** ++ * virBitmapClearBit: ++ * @bitmap: Pointer to bitmap ++ * @b: bit position to clear ++ * ++ * Clear bit position @b in @bitmap ++ * ++ * Returns 0 on if bit is successfully clear, -1 on error. ++ */ ++int virBitmapClearBit(virBitmapPtr bitmap, size_t b) ++{ ++ if (b > bitmap->size - 1) ++ return -1; ++ ++ bitmap->map[VIR_BITMAP_UNIT_OFFSET(b)] &= ~(1 << VIR_BITMAP_BIT_OFFSET(b)); ++ return 0; ++} ++ ++/** ++ * virBitmapGetBit: ++ * @bitmap: Pointer to bitmap ++ * @b: bit position to get ++ * @result: bool pointer to receive bit setting ++ * ++ * Get setting of bit position @b in @bitmap and store in @result ++ * ++ * On success, @result will contain the setting of @b and 0 is ++ * returned. On failure, -1 is returned and @result is unchanged. ++ */ ++int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) ++{ ++ uint32_t bit; ++ ++ if (b > bitmap->size - 1) ++ return -1; ++ ++ bit = bitmap->map[VIR_BITMAP_UNIT_OFFSET(b)] & ++ (1 << VIR_BITMAP_BIT_OFFSET(b)); ++ ++ *result = bit != 0; ++ return 0; ++} +Index: libvirt-0.8.1/src/util/bitmap.h +=================================================================== +--- /dev/null ++++ libvirt-0.8.1/src/util/bitmap.h +@@ -0,0 +1,63 @@ ++/* ++ * bitmap.h: Simple bitmap operations ++ * ++ * Copyright (C) 2010 Novell, Inc. ++ * ++ * This library is free software; you can redistribute it and/or ++ * modify it under the terms of the GNU Lesser General Public ++ * License as published by the Free Software Foundation; either ++ * version 2.1 of the License, or (at your option) any later version. ++ * ++ * This library is distributed in the hope that it will be useful, ++ * but WITHOUT ANY WARRANTY; without even the implied warranty of ++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU ++ * Lesser General Public License for more details. ++ * ++ * You should have received a copy of the GNU Lesser General Public ++ * License along with this library; if not, write to the Free Software ++ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA ++ * ++ * Author: Jim Fehlig ++ */ ++ ++#ifndef __BITMAP_H__ ++# define __BITMAP_H__ ++ ++#include "internal.h" ++ ++#include ++#include ++ ++ ++typedef struct _virBitmap virBitmap; ++typedef virBitmap *virBitmapPtr; ++ ++/* ++ * Allocate a bitmap capable of containing @size bits. ++ */ ++virBitmapPtr virBitmapAlloc(size_t size) ATTRIBUTE_RETURN_CHECK; ++ ++/* ++ * Free previously allocated bitmap ++ */ ++void virBitmapFree(virBitmapPtr bitmap); ++ ++/* ++ * Set bit position @b in @bitmap ++ */ ++int virBitmapSetBit(virBitmapPtr bitmap, size_t b) ++ ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; ++ ++/* ++ * Clear bit position @b in @bitmap ++ */ ++int virBitmapClearBit(virBitmapPtr bitmap, size_t b) ++ ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; ++ ++/* ++ * Get setting of bit position @b in @bitmap and store in @result ++ */ ++int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result) ++ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; ++ ++#endif diff --git a/vnc-race-2.patch b/vnc-race-2.patch new file mode 100644 index 0000000..67a9d16 --- /dev/null +++ b/vnc-race-2.patch @@ -0,0 +1,29 @@ +commit c020f6203e3735a531135bc4321415ce5520fbde +Author: Jim Fehlig +Date: Thu May 20 22:25:16 2010 -0600 + + Add defines for QEMU_VNC_PORT_{MIN,MAX} and use them + +Index: libvirt-0.8.1/src/qemu/qemu_driver.c +=================================================================== +--- libvirt-0.8.1.orig/src/qemu/qemu_driver.c ++++ libvirt-0.8.1/src/qemu/qemu_driver.c +@@ -89,6 +89,9 @@ + + #define VIR_FROM_THIS VIR_FROM_QEMU + ++#define QEMU_VNC_PORT_MIN 5900 ++#define QEMU_VNC_PORT_MAX 65535 ++ + /* Only 1 job is allowed at any time + * A job includes *all* monitor commands, even those just querying + * information, not merely actions */ +@@ -2607,7 +2610,7 @@ qemuInitPCIAddresses(struct qemud_driver + static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { + int i; + +- for (i = 5900 ; i < 65535 ; i++) { ++ for (i = QEMU_VNC_PORT_MIN; i < QEMU_VNC_PORT_MAX; i++) { + int fd; + int reuse = 1; + struct sockaddr_in addr; diff --git a/vnc-race-3.patch b/vnc-race-3.patch new file mode 100644 index 0000000..e083557 --- /dev/null +++ b/vnc-race-3.patch @@ -0,0 +1,130 @@ +commit ba196952f57f2cb22be75fa5a4f363035a111103 +Author: Jim Fehlig +Date: Fri May 21 07:52:09 2010 -0600 + + 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 + + V4: + - Check for failure of virBitmap{Set,Clear}Bit + +Index: libvirt-0.8.1/src/qemu/qemu_conf.h +=================================================================== +--- libvirt-0.8.1.orig/src/qemu/qemu_conf.h ++++ libvirt-0.8.1/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; +Index: libvirt-0.8.1/src/qemu/qemu_driver.c +=================================================================== +--- libvirt-0.8.1.orig/src/qemu/qemu_driver.c ++++ libvirt-0.8.1/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); +@@ -2607,13 +2613,22 @@ qemuInitPCIAddresses(struct qemud_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); +@@ -2629,6 +2644,12 @@ static int qemudNextFreeVNCPort(struct q + if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0) { + /* Not in use, lets grab it */ + close(fd); ++ /* Add port to bitmap of reserved ports */ ++ if (virBitmapSetBit(driver->reservedVNCPorts, ++ i - QEMU_VNC_PORT_MIN) < 0) { ++ VIR_DEBUG("virBitmapSetBit failed on bit %d", ++ i - QEMU_VNC_PORT_MIN); ++ } + return i; + } + close(fd); +@@ -3608,6 +3629,21 @@ 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) { ++ if (virBitmapClearBit(driver->reservedVNCPorts, ++ vm->def->graphics[0]->data.vnc.port - \ ++ QEMU_VNC_PORT_MIN) < 0) { ++ VIR_DEBUG("virBitmapClearBit failed on bit %d", ++ vm->def->graphics[0]->data.vnc.port - QEMU_VNC_PORT_MIN); ++ } ++ } ++ + vm->pid = -1; + vm->def->id = -1; + vm->state = VIR_DOMAIN_SHUTOFF;