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

[Laine Stump]

On 8/26/20 9:00 AM, Michal Privoznik wrote:
> 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:


Dan had said yesterday in IRC that we already link in libm, and it's 
been building correctly. Are there other targets where that isn't the 
case, and I'm just lucky?


>
> 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
>