Re: [libvirt PATCH v2 1/2] util: replace macvtap name reservation bitmap with a simple counter

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(a)redhat.com&gt; > --- >   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