[libvirt PATCH v2 1/2] util: replace macvtap name reservation bitmap with a simple counter
Ján Tomko
jtomko at redhat.com
Wed Aug 26 14:21:38 UTC 2020
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.
Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200826/7ae54ca6/attachment-0001.sig>
More information about the libvir-list
mailing list