> Wow, very thorough research! Thanks for figuring this out!
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.
> I'm leaving the country in 48 hours and may not be able to review this
> before I go. If anyone else wants to take a crack at it so it can go
> into this release, that would be wonderful. Otherwise I'll get to it as
> soon as I possibly can.
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@xxxxxxxxxxxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list