On 8/24/20 6:23 AM, Laine Stump wrote: Hopefully, we won't use any of these in a forked off process because these are not async-signal safe anymore. Michal

There have been some reports that, due to libvirt always trying to assign the lowest numbered macvtap / tap device name possible, a new guest would sometimes be started using the same tap device name as previously used by another guest that is in the process of being destroyed *as the new guest is starting. In some cases this has led to, for example, the old guest's qemuProcessStop() code deleting a port from an OVS switch that had just been re-added by the new guest (because the port name is based on only the device name using the port). Similar problems can happen (and I believe have) with nwfilter rules and bandwidth rules (which are both instantiated based on the name of the tap device). A couple patches have been previously proposed to change the ordering of startup and shutdown processing, or to put a mutex around everything related to the tap/macvtap device name usage, but in the end no matter what you do there will still be possible holes, because the device could be deleted outside libvirt's control (for example, regular tap devices are automatically deleted when the qemu process terminates, and that isn't always initiated by libvirt but could instead happen completely asynchronously - libvirt then has no control over the ordering of shutdown operations, and no opportunity to protect it with a mutex.) But this only happens if a new device is created at the same time as one is being deleted. We can effectively eliminate the chance of this happening if we end the practice of always looking for the lowest numbered available device name, and instead just keep an integer that is incremented each time we need a new device name. At some point it will need to wrap back around to 0 (in order to avoid the IFNAMSIZ 15 character limit if nothing else), and we can't guarantee that the new name really will be the *least* recently used name, but "math" suggests that it will be *much* less common that we'll try to re-use the *most* recently used name. This patch implements such a counter for macvtap/macvlan only. It does it on top of the existing "ID reservation" system (I'm thinking about making a followup that gets rid of the bitmap, as it's now overkill and just serves to make the code more confusing). Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/util/virnetdevmacvlan.c | 67 +++++++++++++++++++++++++++---------- 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 69a9c784bb..c086aa3eb0 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -74,6 +74,8 @@ VIR_LOG_INIT("util.netdevmacvlan"); virMutex virNetDevMacVLanCreateMutex = VIR_MUTEX_INITIALIZER; virBitmapPtr macvtapIDs = NULL; virBitmapPtr macvlanIDs = NULL; +static int macvtapLastID = -1; +static int macvlanLastID = -1; static int virNetDevMacVLanOnceInit(void) @@ -108,12 +110,18 @@ virNetDevMacVLanReserveID(int id, unsigned int flags, bool quietFail, bool nextFree) { virBitmapPtr bitmap; + int *lastID; if (virNetDevMacVLanInitialize() < 0) return -1; - bitmap = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? - macvtapIDs : macvlanIDs; + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { + bitmap = macvtapIDs; + lastID = &macvtapLastID; + } else { + bitmap = macvlanIDs; + lastID = &macvlanLastID; + } if (id > MACVLAN_MAX_ID) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -122,24 +130,49 @@ virNetDevMacVLanReserveID(int id, unsigned int flags, return -1; } - if ((id < 0 || nextFree) && - (id = virBitmapNextClearBit(bitmap, id)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("no unused %s names available"), - VIR_NET_GENERATED_PREFIX); - return -1; - } + if (id < 0 || nextFree) { + /* starting with *lastID + 1, do a loop looking for an unused + * device name, wrapping around at MACVLAN_MAX_ID. + */ + int start = (++(*lastID)) % (MACVLAN_MAX_ID + 1); + bool found = false; - if (virBitmapIsBitSet(bitmap, id)) { - if (quietFail) { - VIR_INFO("couldn't reserve name %s%d - already in use", - VIR_NET_GENERATED_PREFIX, id); - } else { + for (id = start; + id + 1 != start; + id = (++(*lastID)) % (MACVLAN_MAX_ID + 1)) { + + if (!virBitmapIsBitSet(bitmap, id)) { + found = true; + break; + } + } + if (!found) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("couldn't reserve name %s%d - already in use"), - VIR_NET_GENERATED_PREFIX, id); + _("no unused %s names available"), + VIR_NET_GENERATED_PREFIX); + return -1; } - return -1; + } else { + /* A specific ID was requested, we just fail if that + * ID isn't available + */ + if (virBitmapIsBitSet(bitmap, id)) { + if (quietFail) { + VIR_INFO("couldn't reserve name %s%d - already in use", + VIR_NET_GENERATED_PREFIX, id); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("couldn't reserve name %s%d - already in use"), + VIR_NET_GENERATED_PREFIX, id); + } + return -1; + } + /* adjust lastID to not look below this ID (even though + * eventually we will wrap around and look below it - this is + * just a delay tactic + */ + if (*lastID % (MACVLAN_MAX_ID + 1) < id) + *lastID = id; } if (virBitmapSetBit(bitmap, id) < 0) { -- 2.26.2

When creating a standard tap device, if provided with an ifname that contains "%d", rather than taking that literally as the name to use for the new device, the kernel will instead use that string as a template, and search for the lowest number that could be put in place of %d and produce an otherwise unused and unique name for the new device. For example, if there is no tap device name given in the XML, libvirt will always send "vnet%d" as the device name, and the kernel will create new devices named "vnet0", "vnet1", etc. If one of those devices is deleted, creating a "hole" in the name list, the kernel will always attempt to reuse the name in the hole first before using a name with a higher number (i.e. it finds the lowest possible unused number). The problem with this, as described in the previous patch dealing with macvtap device naming, is that it makes "immediate reuse" of a newly freed tap device name *much* more common, and in the aftermath of deleting a tap device, there is some other necessary cleanup of things which are named based on the device name (nwfilter rules, bandwidth rules, OVS switch ports, to name a few) that could end up stomping over the top of the setup of a new device of the same name for a different guest. Since the kernel "create a name based on a template" functionality for tap devices doesn't exist for macvtap, this patch is a bit different from the previous patch - we look for a requested ifname of "vnet%d", and when we see that, we use it as a sprintf format string to find an unused device name ourselves, then pass that exact name on to the kernel; in this way we can avoid the perils of the kernel's "lowest number first" algorithm, and instead use a "hopefully never-before used number" algorithm. (NB: It is still possible for a user to provide their own parameterized template name (e.g. "mytap%d") in the XML, and libvirt will just pass that through to the kernel as it always has.) Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 22 ++++++++++- src/util/virnetdevtap.c | 79 +++++++++++++++++++++++++++++++++++++++- src/util/virnetdevtap.h | 4 ++ 4 files changed, 102 insertions(+), 4 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 01c2e710cd..a9d5af9dde 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2674,6 +2674,7 @@ virNetDevTapGetName; virNetDevTapGetRealDeviceName; virNetDevTapInterfaceStats; virNetDevTapReattachBridge; +virNetDevTapReserveName; # util/virnetdevveth.h diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 832b2e6870..2a1c1a3732 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3319,8 +3319,26 @@ qemuProcessNotifyNets(virDomainDefPtr def) * domain to be unceremoniously killed, which would be *very* * impolite. */ - if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) - ignore_value(virNetDevMacVLanReserveName(net->ifname, false)); + switch (virDomainNetGetActualType(net)) { + case VIR_DOMAIN_NET_TYPE_DIRECT: + ignore_value(virNetDevMacVLanReserveName(net->ifname, false)); + break; + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + virNetDevTapReserveName(net->ifname); + break; + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_LAST: + break; + } if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!conn && !(conn = virGetConnectNetwork())) diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index c0a7c3019e..4c3b68e582 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -54,6 +54,45 @@ VIR_LOG_INIT("util.netdevtap"); +#define VIR_TAP_MAX_ID 99999 + +virMutex virNetDevTapCreateMutex = VIR_MUTEX_INITIALIZER; +static int virNetDevTapLastID = -1; + + +/** + * virNetDevTapReserveName: + * @name: name of an existing tap device + * + * Set the value of virNetDevTapLastID to assure that any new tap + * device created with an autogenerated name will use a number higher + * than the number in the given tap device name. + * + * Returns nothing. + */ +void +virNetDevTapReserveName(const char *name) +{ + unsigned int id; + const char *idstr = NULL; + + + if (STRPREFIX(name, VIR_NET_GENERATED_TAP_PREFIX)) { + + idstr = name + strlen(VIR_NET_GENERATED_TAP_PREFIX); + + if (virStrToLong_ui(idstr, NULL, 10, &id) >= 0) { + virMutexLock(&virNetDevTapCreateMutex); + + if (virNetDevTapLastID % (VIR_TAP_MAX_ID + 1) < (int)id) + virNetDevTapLastID = id; + + virMutexUnlock(&virNetDevTapCreateMutex); + } + } +} + + /** * virNetDevTapGetName: * @tapfd: a tun/tap file descriptor @@ -230,10 +269,45 @@ int virNetDevTapCreate(char **ifname, size_t tapfdSize, unsigned int flags) { - size_t i; + size_t i = 0; struct ifreq ifr; int ret = -1; - int fd; + int fd = 0; + + virMutexLock(&virNetDevTapCreateMutex); + + /* if ifname is "vnet%d", then create the actual name to use by + * replacing %d with ++virNetDevTapLastID. Keep trying new values + * until one is found that doesn't already exist, or we've + * completed one full circle of the number space. + * + */ + + if (STREQ(*ifname, VIR_NET_GENERATED_TAP_PREFIX "%d")) { + int id; + int start = ++virNetDevTapLastID % (VIR_TAP_MAX_ID + 1); + bool found = false; + + for (id = start; + id + 1 != start; + id = ++virNetDevTapLastID % (VIR_TAP_MAX_ID + 1)) { + + g_autofree char *try = g_strdup_printf(*ifname, id); + + if (!virNetDevExists(try)) { + g_free(*ifname); + *ifname = g_steal_pointer(&try); + found = true; + break; + } + } + if (!found) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no unused %s names available"), + VIR_NET_GENERATED_TAP_PREFIX); + goto cleanup; + } + } if (!tunpath) tunpath = "/dev/net/tun"; @@ -302,6 +376,7 @@ int virNetDevTapCreate(char **ifname, ret = 0; cleanup: + virMutexUnlock(&virNetDevTapCreateMutex); if (ret < 0) { VIR_FORCE_CLOSE(fd); while (i--) diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index c6bd9285ba..dea8aec3af 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -29,6 +29,10 @@ # define VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP 1 #endif +void +virNetDevTapReserveName(const char *name) + ATTRIBUTE_NONNULL(1); + int virNetDevTapCreate(char **ifname, const char *tunpath, int *tapfd, -- 2.26.2