Re: [libvirt] [PATCH] Fix dnsmasq/radvd on bridges not being able to be bound to IPv6 address on "recent" kernels

Thanks. It took me almost two days to figure this out and find a working solution. Interesting debugging, though, as I learned quite a lot on bridges and IPv6 DAD. Thanks. And I've found a solution for radvd: use the “IgnoreIfMissing” flag in its config so that it continues running even if it find the interface down on startup. BTW, I'm also hesitating to set the VIR_NETDEV_TAP_CREATE_PERSIST flag automatically when no tapfd pointer is given, as currently, with my patch, if you call virNetDevTapCreate() without VIR_NETDEV_TAP_CREATE_PERSIST and without tapfd argument, the device will disappear immediately. I don't know if implicitly setting a flag is a good idea, though. Anyway, here's the updated patch (without mangled newlines, hopefully). A summary for this patch would be: When creating a network bridge with an IPv6 address on kernel >= 2.6.39 (because of kernel's commit 1faa4356a3bd89ea11fb92752d897cff3a20ec0e), dnsmasq and radvd fail to bind to this address as the interface is not IFF_RUNNING and thus DAD (Duplicate Address Detection) cannot happen. This keeps the address flagged “tentative” and no daemon can bind to it (RFC 2462). This happens because the dummy TAP interface, linked to the bridge to set its MAC address, has its file descriptor closed and this prevents the interface from being IFF_RUNNING. This patch makes the bridge creation code keep the dummy TAP file descriptor open until DAD has happened on the bridge, allowing dnsmasq to bind to the address (and we tell radvd to ignore the interface if it is “missing” on startup), and then sets the interface down and closes the fd, as before. Oh, and please Cc me, I'm not subscribed. --- src/network/bridge_driver.c | 20 ++++++++++++++++++-- src/uml/uml_conf.c | 2 +- src/util/virnetdevtap.c | 24 ++++++++++++++---------- src/util/virnetdevtap.h | 2 ++ 4 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 79d3010..b21d86b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -62,6 +62,7 @@ #include "virnetdev.h" #include "virnetdevbridge.h" #include "virnetdevtap.h" +#include "virfile.h" #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network" #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network" @@ -840,6 +841,7 @@ networkStartRadvd(virNetworkObjPtr network) " AdvSendAdvert on;\n" " AdvManagedFlag off;\n" " AdvOtherConfigFlag off;\n" + " IgnoreIfMissing on;\n" "\n", network->def->bridge); for (ii = 0; @@ -1744,6 +1746,7 @@ networkStartNetworkVirtual(struct network_driver *driver, virErrorPtr save_err = NULL; virNetworkIpDefPtr ipdef; char *macTapIfName = NULL; + int tapfd = -1; /* Check to see if any network IP collides with an existing route */ if (networkCheckRouteCollision(network) < 0) @@ -1765,10 +1768,13 @@ networkStartNetworkVirtual(struct network_driver *driver, virReportOOMError(); goto err0; } + /* Keep tun fd open and interface up to allow for IPv6 DAD to happen */ if (virNetDevTapCreateInBridgePort(network->def->bridge, &macTapIfName, network->def->mac, - NULL, NULL, NULL, - VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE) < 0) { + NULL, &tapfd, NULL, + VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE + |VIR_NETDEV_TAP_CREATE_IFUP + |VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { VIR_FREE(macTapIfName); goto err0; } @@ -1828,6 +1834,16 @@ networkStartNetworkVirtual(struct network_driver *driver, if (v6present && networkStartRadvd(network) < 0) goto err4; + /* DAD has happened (dnsmasq waits for it), dnsmasq is now bound to the + * bridge's IPv6 address, and radvd to the interface, so we can now set the + * dummy tun down. + */ + if (tapfd >= 0) { + if (virNetDevSetOnline(macTapIfName, false) < 0) + goto err4; + VIR_FORCE_CLOSE(tapfd); + } + if (virNetDevBandwidthSet(network->def->bridge, network->def->bandwidth) < 0) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot set bandwidth limits on %s"), diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 79b249d..c9b5044 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -141,7 +141,7 @@ umlConnectTapDevice(virConnectPtr conn, if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, net->mac, vm->uuid, NULL, virDomainNetGetActualVirtPortProfile(net), - VIR_NETDEV_TAP_CREATE_IFUP) < 0) { + VIR_NETDEV_TAP_CREATE_IFUP|VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { if (template_ifname) VIR_FREE(net->ifname); goto error; diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 5d21164..45ff20f 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -118,18 +118,20 @@ virNetDevProbeVnetHdr(int tapfd) * * VIR_NETDEV_TAP_CREATE_VNET_HDR * - Enable IFF_VNET_HDR on the tap device + * VIR_NETDEV_TAP_CREATE_PERSIST + * - The device will persist after the file descriptor is closed * * Creates a tap interface. - * If the @tapfd parameter is supplied, the open tap device file - * descriptor will be returned, otherwise the TAP device will be made - * persistent and closed. The caller must use virNetDevTapDelete to - * remove a persistent TAP devices when it is no longer needed. + * If the @tapfd parameter is supplied, the open tap device file descriptor + * will be returned, otherwise the TAP device will be closed. The caller must + * use virNetDevTapDelete to remove a persistent TAP device when it is no + * longer needed. * * Returns 0 in case of success or an errno code in case of failure. */ int virNetDevTapCreate(char **ifname, int *tapfd, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { int fd; struct ifreq ifr; @@ -166,7 +168,7 @@ int virNetDevTapCreate(char **ifname, goto cleanup; } - if (!tapfd && + if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) && (errno = ioctl(fd, TUNSETPERSIST, 1))) { virReportSystemError(errno, _("Unable to set tap device %s to persistent"), @@ -267,14 +269,16 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) * - Enable IFF_VNET_HDR on the tap device * VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE * - Set this interface's MAC as the bridge's MAC address + * VIR_NETDEV_TAP_CREATE_PERSIST + * - The device will persist after the file descriptor is closed * * This function creates a new tap device on a bridge. @ifname can be either * a fixed name or a name template with '%d' for dynamic name allocation. * in either case the final name for the bridge will be stored in @ifname. - * If the @tapfd parameter is supplied, the open tap device file - * descriptor will be returned, otherwise the TAP device will be made - * persistent and closed. The caller must use virNetDevTapDelete to remove - * a persistent TAP devices when it is no longer needed. + * If the @tapfd parameter is supplied, the open tap device file descriptor + * will be returned, otherwise the TAP device will be closed. The caller must + * use virNetDevTapDelete to remove a persistent TAP device when it is no + * longer needed. * * Returns 0 in case of success or -1 on failure */ diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index d9a3593..f1355ba 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -42,6 +42,8 @@ typedef enum { VIR_NETDEV_TAP_CREATE_VNET_HDR = 1 << 1, /* Set this interface's MAC as the bridge's MAC address */ VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE = 1 << 2, + /* The device will persist after the file descriptor is closed */ + VIR_NETDEV_TAP_CREATE_PERSIST = 1 << 3, } virNetDevTapCreateFlags; int virNetDevTapCreateInBridgePort(const char *brname, -- Benjamin Cama <benjamin.cama(a)telecom-bretagne.eu&gt;