[libvirt] [PATCH 0/3 V2] Fix race in qemu driver wrt vnc port allocation
Sorry for the delay ... V2 changes noted in the patches. 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. The issue was briefly discussed on IRC and the consensus there was to track port allocation with a bitmap. The first patch adds some simple bitmap operations to utils. Second patch defines VNC_PORT_{MIN,MAX} and uses them in place of explicit values. Third patch makes use of the bitmap impl to track vnc port allocations. The race was consistently encountered in HA environments [1]. This patch series has been successfully tested in such environment. [1] http://www.linux-ha.org/wiki/VirtualDomain_%28resource_agent%29#VNC_port_aut... Jim Fehlig (3): Add simple bitmap operations to utils Add defines for QEMU_VNC_PORT_{MIN,MAX} and use them Fix race in finding available vnc port cfg.mk | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 8 +++ src/qemu/qemu_conf.h | 3 + src/qemu/qemu_driver.c | 39 +++++++++++- src/util/bitmap.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/bitmap.h | 60 ++++++++++++++++++ 7 files changed, 264 insertions(+), 3 deletions(-) create mode 100644 src/util/bitmap.c create mode 100644 src/util/bitmap.h
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 --- cfg.mk | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 8 +++ src/util/bitmap.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/bitmap.h | 60 ++++++++++++++++++ 5 files changed, 225 insertions(+), 0 deletions(-) create mode 100644 src/util/bitmap.c create mode 100644 src/util/bitmap.h diff --git a/cfg.mk b/cfg.mk index 04719d4..18a83df 100644 --- a/cfg.mk +++ b/cfg.mk @@ -67,6 +67,7 @@ local-checks-to-skip = \ useless_free_options = \ --name=VIR_FREE \ --name=sexpr_free \ + --name=virBitmapFree \ --name=virCPUDefFree \ --name=virCapabilitiesFree \ --name=virCapabilitiesFreeGuest \ diff --git a/src/Makefile.am b/src/Makefile.am index 889de8e..fad1bc4 100644 --- a/src/Makefile.am +++ b/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 \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bdeab0f..8149719 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -4,6 +4,14 @@ # +# bitmap.h +virBitmapAlloc; +virBitmapFree; +virBitmapSetBit; +virBitmapClearBit; +virBitmapGetBit; + + # buf.h virBufferVSprintf; virBufferEscapeString; diff --git a/src/util/bitmap.c b/src/util/bitmap.c new file mode 100644 index 0000000..d733373 --- /dev/null +++ b/src/util/bitmap.c @@ -0,0 +1,155 @@ +/* + * 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 <jfehlig@novell.com> + */ + +#include <config.h> + +#include <limits.h> +#include <stdint.h> +#include <stdio.h> +#include <string.h> +#include <stdlib.h> +#include <sys/types.h> + +#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 = (size + VIR_BITMAP_BITS_PER_UNIT - 1) / + VIR_BITMAP_BITS_PER_UNIT; + + if (VIR_ALLOC_N(bitmap, sizeof(virBitmap)) < 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, unsigned int b) +{ + if (bitmap == NULL) + return -1; + + 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, unsigned int b) +{ + if (bitmap == NULL) + return -1; + + 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, unsigned int b, bool *result) +{ + uint32_t bit; + + if (bitmap == NULL) + return -1; + + 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; +} diff --git a/src/util/bitmap.h b/src/util/bitmap.h new file mode 100644 index 0000000..0143b0e --- /dev/null +++ b/src/util/bitmap.h @@ -0,0 +1,60 @@ +/* + * 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 <jfehlig@novell.com> + */ + +#ifndef __BITMAP_H__ +# define __BITMAP_H__ + +#include "internal.h" + +#include <stdbool.h> +#include <sys/types.h> + + +typedef struct _virBitmap virBitmap; +typedef virBitmap *virBitmapPtr; + +/* + * Allocate a bitmap capable of containing @size bits. + */ +virBitmapPtr virBitmapAlloc(size_t size); + +/* + * Free previously allocated bitmap + */ +void virBitmapFree(virBitmapPtr bitmap); + +/* + * Set bit position @b in @bitmap + */ +int virBitmapSetBit(virBitmapPtr bitmap, unsigned int b); + +/* + * Clear bit position @b in @bitmap + */ +int virBitmapClearBit(virBitmapPtr bitmap, unsigned int b); + +/* + * Get setting of bit position @b in @bitmap and store in @result + */ +int virBitmapGetBit(virBitmapPtr bitmap, unsigned int b, bool *result); + +#endif -- 1.6.0.2
On 05/20/2010 02:45 PM, Jim Fehlig wrote:
+virBitmapPtr virBitmapAlloc(size_t size) +{ + virBitmapPtr bitmap; + size_t sz = (size + VIR_BITMAP_BITS_PER_UNIT - 1) / + VIR_BITMAP_BITS_PER_UNIT;
Check for overflow: if (SIZE_MAX - VIR_BITMAP_BITS_PER_UNIT < size) return NULL;
+ + if (VIR_ALLOC_N(bitmap, sizeof(virBitmap)) < 0)
Use VIR_ALLOC(bitmap) - you want to allocate one bitmap object, not an array of bitmaps of length 'sizeof(virBitmap)'. But VIR_ALLOC_N is correct for bitmap->map.
+ +/** + * 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, unsigned int b)
size_t, not unsigned int, to avoid type mismatch when comparing against _virBitmap.size.
+ +/* + * Set bit position @b in @bitmap + */ +int virBitmapSetBit(virBitmapPtr bitmap, unsigned int b);
Add ATTRIBUTE_NONNULL(1) here, and on all subsequent prototypes that take virBitmapPtr, and you can get rid of the bitmap==NULL check in the implementations (but keep the virBitmapFree semantics of allowing NULL, as it simplifies cleanup paths). s/unsigned int/size_t/.
+ +/* + * Clear bit position @b in @bitmap + */ +int virBitmapClearBit(virBitmapPtr bitmap, unsigned int b);
s/unsigned int/size_t/
+ +/* + * Get setting of bit position @b in @bitmap and store in @result + */ +int virBitmapGetBit(virBitmapPtr bitmap, unsigned int b, bool *result);
s/unsigned int/size_t/. ATTRIBUTE_NONNULL(3) as well. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/20/2010 02:45 PM, Jim Fehlig wrote:
+virBitmapPtr virBitmapAlloc(size_t size) +{ + virBitmapPtr bitmap; + size_t sz = (size + VIR_BITMAP_BITS_PER_UNIT - 1) / + VIR_BITMAP_BITS_PER_UNIT;
Check for overflow: if (SIZE_MAX - VIR_BITMAP_BITS_PER_UNIT < size) return NULL;
+ + if (VIR_ALLOC_N(bitmap, sizeof(virBitmap)) < 0)
Use VIR_ALLOC(bitmap) - you want to allocate one bitmap object, not an array of bitmaps of length 'sizeof(virBitmap)'.
Opps. Copy and paste from bitmap->map. This and your other comments are addressed in V3. Regards, Jim
On 05/21/2010 09:29 AM, Jim Fehlig wrote:
This and your other comments are addressed in V3.
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 form impl
If you keep that as part of the commit message, s/form/from/ (Personally, I put my comments about intra-version diffs...
---
...here, after the --- (by using git send-email --annotate), so that they are part of the email trail but not the actual commit message. After all, at the end of the day, the email trail shows how we got the the end result, but the commit history need only show the end result if we aren't committing the intermediate results. But aside from my nitpicking on the commit message, the patch itself looks good! ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/21/2010 09:29 AM, Jim Fehlig wrote:
This and your other comments are addressed in V3.
Oops, just realized we missed one thing - if you haven't pushed yet, then please squash in a change to use ATTRIBUTE_RETURN_CHECK in bitmap.h for the three functions that return an int. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/21/2010 09:29 AM, Jim Fehlig wrote:
This and your other comments are addressed in V3.
Oops, just realized we missed one thing - if you haven't pushed yet, then please squash in a change to use ATTRIBUTE_RETURN_CHECK in bitmap.h for the three functions that return an int.
Done. Also checked for failure of these function in qemu_driver.c and pushed the series. Thanks for the review and comments! Regards, Jim
--- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65ca117..06cf660 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -88,6 +88,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 */ @@ -2632,7 +2635,7 @@ qemuInitPCIAddresses(struct qemud_driver *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; -- 1.6.0.2
On 05/20/2010 02:45 PM, Jim Fehlig wrote:
--- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65ca117..06cf660 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -88,6 +88,9 @@
#define VIR_FROM_THIS VIR_FROM_QEMU
+#define QEMU_VNC_PORT_MIN 5900 +#define QEMU_VNC_PORT_MAX 65535
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 --- src/qemu/qemu_conf.h | 3 +++ src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index a101e47..d7d4c57 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 06cf660..01867db 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,19 @@ 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; + + virBitmapGetBit(driver->reservedVNCPorts, i - QEMU_VNC_PORT_MIN, &used); + if (used) + continue; + addr.sin_family = AF_INET; addr.sin_port = htons(i); addr.sin_addr.s_addr = htonl(INADDR_ANY); @@ -2654,6 +2666,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); @@ -3582,8 +3596,13 @@ cleanup: qemuRemoveCgroup(driver, vm, 1); 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.autoport) { + virBitmapClearBit(driver->reservedVNCPorts, + vm->def->graphics[0]->data.vnc.port - \ + QEMU_VNC_PORT_MIN); vm->def->graphics[0]->data.vnc.port = -1; + } + if (logfile != -1) close(logfile); vm->def->id = -1; @@ -3716,6 +3735,17 @@ 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) { + 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
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? (Hmm, maybe an addendum to my comments on 1/3, that the bitmap.h file should mark functions as ATTRIBUTE_RETURN_CHECK?) I could go either way, so for now, I'll wait for other comments (and for the fixes to bitmap.h) before giving an ack. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
On 05/21/2010 10:00 AM, Jim Fehlig wrote:
+ 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);
We might as well be consistent, and check for failure here, too (adding ATTRIBUTE_RETURN_CHECK in bitmap.h would have caught this automatically). ACK with that squashed in. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Jim Fehlig