[libvirt PATCH v2 1/2] util: replace macvtap name reservation bitmap with a simple counter
Michal Privoznik
mprivozn at redhat.com
Wed Aug 26 13:00:03 UTC 2020
On 8/26/20 7:22 AM, Laine Stump wrote:
> 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, replacing
> the existing, and much more complicated, "ID reservation" system. The
> counter is set according to whatever macvtap/macvlan devices are
> already in use by guests when libvirtd is started, incremented each
> time a new device name is needed, and wraps back to 0 when either
> INT_MAX is reached, or when the resulting device name would be longer
> than IFNAMSIZ-1 characters (which actually is what happens when the
> template for the device name is "maccvtap%d"). The result is that no
> macvtap name will be re-used until the host has created (and possibly
> destroyed) 99,999,999 devices.
>
> Signed-off-by: Laine Stump <laine at redhat.com>
> ---
> src/libvirt_private.syms | 1 -
> src/libxl/libxl_driver.c | 2 +-
> src/lxc/lxc_process.c | 2 +-
> src/qemu/qemu_process.c | 2 +-
> src/util/virnetdevmacvlan.c | 402 +++++++++++++-----------------------
> src/util/virnetdevmacvlan.h | 6 +-
> 6 files changed, 145 insertions(+), 270 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index dcea93a5fe..dc4db2c844 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> +static int
> +virNetDevMacVLanGenerateName(char **ifname, unsigned int flags)
> {
> - unsigned int id;
> - unsigned int flags = 0;
> - const char *idstr = NULL;
> -
> - if (virNetDevMacVLanInitialize() < 0)
> - return -1;
> + const char *prefix;
> + const char *iftemplate;
> + int *lastID;
> + int id;
> + double maxIDd;
> + int maxID = INT_MAX;
> + int attempts = 0;
>
> - if (STRPREFIX(name, VIR_NET_GENERATED_MACVTAP_PREFIX)) {
> - idstr = name + strlen(VIR_NET_GENERATED_MACVTAP_PREFIX);
> - flags |= VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
> - } else if (STRPREFIX(name, VIR_NET_GENERATED_MACVLAN_PREFIX)) {
> - idstr = name + strlen(VIR_NET_GENERATED_MACVLAN_PREFIX);
> + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) {
> + prefix = VIR_NET_GENERATED_MACVTAP_PREFIX;
> + iftemplate = VIR_NET_GENERATED_MACVTAP_PREFIX "%d";
> + lastID = &virNetDevMacVTapLastID;
> } else {
> - return -2;
> + prefix = VIR_NET_GENERATED_MACVLAN_PREFIX;
> + iftemplate = VIR_NET_GENERATED_MACVLAN_PREFIX "%d";
> + lastID = &virNetDevMacVLanLastID;
> }
>
> - if (virStrToLong_ui(idstr, NULL, 10, &id) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("couldn't get id value from macvtap device name %s"),
> - name);
> - return -1;
> - }
> - return virNetDevMacVLanReserveID(id, flags, quietFail, false);
> -}
> + maxIDd = pow(10, IFNAMSIZ - 1 - strlen(prefix));
> + if (maxIDd <= (double)INT_MAX)
> + maxID = (int)maxIDd;
pow() requires -lm. We need this to be squashed in:
diff --git i/meson.build w/meson.build
index dabd4196e6..81668a6681 100644
--- i/meson.build
+++ w/meson.build
@@ -1176,6 +1176,9 @@ endif
libxml_version = '2.9.1'
libxml_dep = dependency('libxml-2.0', version: '>=' + libxml_version)
+cc = meson.get_compiler('c')
+m_dep = cc.find_library('m', required : false)
+
use_macvtap = false
if not get_option('macvtap').disabled()
if (cc.has_header_symbol('linux/if_link.h', 'MACVLAN_MODE_BRIDGE') and
diff --git i/src/util/meson.build w/src/util/meson.build
index a7017f459f..f7092cc3f1 100644
--- i/src/util/meson.build
+++ w/src/util/meson.build
@@ -188,6 +188,7 @@ virt_util_lib = static_library(
devmapper_dep,
gnutls_dep,
libnl_dep,
+ m_dep,
numactl_dep,
secdriver_dep,
src_dep,
NOTE: Doesn't come from my head.
https://mesonbuild.com/howtox.html#add-math-library-lm-portably
Michal
More information about the libvir-list
mailing list