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

[Michal Privoznik]

On 8/26/20 4:21 PM, Ján Tomko wrote:
> On a Wednesday in 2020, Laine Stump wrote:
>> 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?
>>
> 
> libxml2 is linking to it, at least.
> 
> Anyway, we already use ldexp in virrandom and isnan in virxml.c so
> I'd consider the -lm change a separate issue from this commit.

It is, but we are not linking libxml2 to virt_util_lib rather than 
libvirt.so. Hence, when linking virt_util_lib we get this linking error.

I'm okay with making the change in a separate commit.

Michal