Give each virtual network bridge its own fixed MAC address

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=609463

The problem was that, since a bridge always acquires the MAC address
of the connected interface with the numerically lowest MAC, as guests
are started and stopped, it was possible for the MAC address to change
over time, and this change in the network was being detected by
Windows 7 (it sees the MAC of the default route change), so on each
reboot it would bring up a dialog box asking about this "new network".

The solution is to create a dummy tap interface with a MAC guaranteed
to be lower than any guest interface's MAC, and attach that tap to the
bridge as soon as it's created. Since all guest MAC addresses start
with 0xFE, we can just generate a MAC with the standard "0x52, 0x54,
0" prefix, and it's guaranteed to always win (physical interfaces are
never connected to these bridges, so we don't need to worry about
competing numerically with them).

Note that the dummy tap is never set to IFF_UP state - that's not
necessary in order for the bridge to take its MAC, and not setting it
to UP eliminates the clutter of having an (eg) "virbr0-nic" displayed
in the output of the ifconfig command.

I chose to not auto-generate the MAC address in the network XML
parser, as there are likely to be consumers of that API that don't
need or want to have a MAC address associated with the
bridge.

Instead, in bridge_driver.c when the network is being defined, if
there is no MAC, one is generated. To account for virtual network
configs that already exist when upgrading from an older version of
libvirt, I've added a %post script to the specfile that searches for
all network definitions in both the config directory
(/etc/libvirt/qemu/networks) and the state directory
(/var/lib/libvirt/network) that are missing a mac address, generates a
random address, and adds it to the config (and a matching address to
the state file, if there is one).

docs/formatnetwork.html.in: document <mac address.../>
docs/schemas/network.rng: add nac address to schema
libvirt.spec.in: %post script to update existing networks
src/conf/network_conf.[ch]: parse and format <mac address.../>
src/libvirt_private.syms: export a couple private symbols we need
src/network/bridge_driver.c:
    auto-generate mac address when needed,
    create dummy interface if mac address is present.
tests/networkxml2xmlin/isolated-network.xml
tests/networkxml2xmlin/routed-network.xml
tests/networkxml2xmlout/isolated-network.xml
tests/networkxml2xmlout/routed-network.xml: add mac address to some tests

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index b1b04859ba2bfb2a8ab513bd258ebf87f99fd113..c6969eb1a3c66e09aa45cc3905087776902f7771 100644 (file)
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -105,12 +105,15 @@
     <h3><a name="elementsAddress">Addressing</a></h3>
 
     <p>
-      The final set of elements define the IPv4 address range available,
-      and optionally enable DHCP sevices.
+      The final set of elements define the addresses (IPv4 and/or
+      IPv6, as well as MAC) to be assigned to the bridge device
+      associated with the virtual network, and optionally enable DHCP
+      services.
     </p>
 
     <pre>
         ...
+        &lt;mac address='00:16:3E:5D:C7:9E'/&gt;
         &lt;ip address="192.168.122.1" netmask="255.255.255.0"&gt;
           &lt;dhcp&gt;
             &lt;range start="192.168.122.100" end="192.168.122.254" /&gt;
@@ -121,6 +124,20 @@
       &lt;/network&gt;</pre>
 
     <dl>
+      <dt><code>mac</code></dt>
+      <dd>The <code>address</code> attribute defines a MAC
+        (hardware) address formatted as 6 groups of 2-digit
+        hexadecimal numbers, the groups separated by colons
+        (eg, <code>"52:54:00:1C:DA:2F"</code>).  This MAC address is
+        assigned to the bridge device when it is created.  Generally
+        it is best to not specify a MAC address when creating a
+        network - in this case, if a defined MAC address is needed for
+        proper operation, libvirt will automatically generate a random
+        MAC address and save it in the config. Allowing libvirt to
+        generate the MAC address will assure that it is compatible
+        with the idiosyncrasies of the platform where libvirt is
+        running. <span class="since">Since 0.8.8</span>
+      </dd>
       <dt><code>ip</code></dt>
       <dd>The <code>address</code> attribute defines an IPv4 address in
         dotted-decimal format, or an IPv6 address in standard

diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 4252f30f56454cb219234f94acf7851bcc3fd13f..6d01b06082bc169970c44e9b4f3cabd8d2007077 100644 (file)
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -50,6 +50,14 @@
           </element>
         </optional>
 
+        <!-- <mac> element -->
+        <optional>
+          <element name="mac">
+            <attribute name="address"><ref name="mac-addr"/></attribute>
+            <empty/>
+          </element>
+        </optional>
+
         <!-- <forward> element -->
         <optional>
           <!-- The device through which the bridge is connected to the

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 095139103e46f90b9dcb36417830d16eb8b250b9..632edf32aaae694c80d601ce2db42ad80d461814 100644 (file)
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -751,6 +751,46 @@ then
          > %{_sysconfdir}/libvirt/qemu/networks/default.xml
     ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml
 fi
+
+# All newly defined networks will have a mac address for the bridge
+# auto-generated, but networks already existing at the time of upgrade
+# will not. We need to go through all the network configs, look for
+# those that don't have a mac address, and add one.
+
+network_files=$( (cd %{_localstatedir}/lib/libvirt/network && \
+                  grep -L "mac address" *.xml; \
+                  cd %{_sysconfdir}/libvirt/qemu/networks && \
+                  grep -L "mac address" *.xml) 2>/dev/null \
+                | sort -u)
+
+for file in $network_files
+do
+   # each file exists in either the config or state directory (or both) and
+   # does not have a mac address specified in either. We add the same mac
+   # address to both files (or just one, if the other isn't there)
+
+   mac4=`printf '%X' $(($RANDOM % 256))`
+   mac5=`printf '%X' $(($RANDOM % 256))`
+   mac6=`printf '%X' $(($RANDOM % 256))`
+   for dir in %{_localstatedir}/lib/libvirt/network \
+              %{_sysconfdir}/libvirt/qemu/networks
+   do
+      if test -f $dir/$file
+      then
+         sed -i.orig -e \
+           "s|\(<bridge.*$\)|\0\n  <mac address='52:54:00:$mac4:$mac5:$mac6'/>|" \
+           $dir/$file
+         if test $? != 0
+         then
+             echo "failed to add <mac address='52:54:00:$mac4:$mac5:$mac6'/>" \
+                  "to $dir/$file"
+             mv -f $dir/$file.orig $dir/$file
+         else
+             rm -f $dir/$file.orig
+         fi
+      fi
+   done
+done
 %endif
 
 %if %{with_cgconfig}

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 28a3ee832534806cfa7c580c9794b496da72b445..dcab9de21dbf477d4b5de65dbfa001b0fe4ac516 100644 (file)
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -628,6 +628,19 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
     if (virXPathULong("string(./bridge[1]/@delay)", ctxt, &def->delay) < 0)
         def->delay = 0;
 
+    tmp = virXPathString("string(./mac[1]/@address)", ctxt);
+    if (tmp) {
+        if (virParseMacAddr(tmp, def->mac) < 0) {
+            virNetworkReportError(VIR_ERR_XML_ERROR,
+                                  _("Invalid bridge mac address '%s' in network '%s'"),
+                                  tmp, def->name);
+            VIR_FREE(tmp);
+            goto error;
+        }
+        VIR_FREE(tmp);
+        def->mac_specified = true;
+    }
+
     nIps = virXPathNodeSet("./ip", ctxt, &ipNodes);
     if (nIps > 0) {
         int ii;
@@ -856,6 +869,11 @@ char *virNetworkDefFormat(const virNetworkDefPtr def)
     virBufferVSprintf(&buf, " stp='%s' delay='%ld' />\n",
                       def->stp ? "on" : "off",
                       def->delay);
+    if (def->mac_specified) {
+        char macaddr[VIR_MAC_STRING_BUFLEN];
+        virFormatMacAddr(def->mac, macaddr);
+        virBufferVSprintf(&buf, "  <mac address='%s'/>\n", macaddr);
+    }
 
     if (def->domain)
         virBufferVSprintf(&buf, "  <domain name='%s'/>\n", def->domain);
@@ -1165,6 +1183,18 @@ error:
 }
 
 
+void virNetworkSetBridgeMacAddr(virNetworkDefPtr def)
+{
+    if (!def->mac_specified) {
+        /* if the bridge doesn't have a mac address explicitly defined,
+         * autogenerate a random one.
+         */
+        virGenerateMacAddr((unsigned char[]){ 0x52, 0x54, 0 },
+                           def->mac);
+        def->mac_specified = true;
+    }
+}
+
 /*
  * virNetworkObjIsDuplicate:
  * @doms : virNetworkObjListPtr to search

diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index fd96c367b1c88bf50199cba8aa74ee3cc62e8aa6..281124bff64d04f6291ddc689e6fefbf341cbf1d 100644 (file)
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -31,6 +31,7 @@
 # include "internal.h"
 # include "threads.h"
 # include "network.h"
+# include "util.h"
 
 /* 2 possible types of forwarding */
 enum virNetworkForwardType {
@@ -92,6 +93,8 @@ struct _virNetworkDef {
     char *domain;
     unsigned long delay;   /* Bridge forward delay (ms) */
     unsigned int stp :1; /* Spanning tree protocol */
+    unsigned char mac[VIR_MAC_BUFLEN]; /* mac address of bridge device */
+    bool mac_specified;
 
     int forwardType;    /* One of virNetworkForwardType constants */
     char *forwardDev;   /* Destination device for forwarding */
@@ -191,6 +194,8 @@ int virNetworkSetBridgeName(const virNetworkObjListPtr nets,
                             virNetworkDefPtr def,
                             int check_collision);
 
+void virNetworkSetBridgeMacAddr(virNetworkDefPtr def);
+
 int virNetworkObjIsDuplicate(virNetworkObjListPtr doms,
                              virNetworkDefPtr def,
                              unsigned int check_active);

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 301eaefd883f83cf81b24a427f7e36702fa2e434..797a6709f53dc31c3a7507b8037f789659a4a818 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -608,6 +608,7 @@ virNetworkObjLock;
 virNetworkObjUnlock;
 virNetworkRemoveInactive;
 virNetworkSaveConfig;
+virNetworkSetBridgeMacAddr;
 virNetworkSetBridgeName;
 
 
@@ -877,6 +878,7 @@ virFileWriteStr;
 virFindFileInPath;
 virFork;
 virFormatMacAddr;
+virGenerateMacAddr;
 virGetGroupID;
 virGetHostname;
 virGetUserDirectory;

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c4ee1e83c059f4dc1badeab5d944aa229866a944..98c59c9ac50a332bc94c6ede37c586b8748b0576 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -128,6 +128,15 @@ networkRadvdConfigFileName(const char *netname)
     return configfile;
 }
 
+static char *
+networkBridgeDummyNicName(const char *brname)
+{
+    char *nicname;
+
+    virAsprintf(&nicname, "%s-nic", brname);
+    return nicname;
+}
+
 static void
 networkFindActiveConfigs(struct network_driver *driver) {
     unsigned int i;
@@ -1566,6 +1575,7 @@ networkStartNetworkDaemon(struct network_driver *driver,
     bool v4present = false, v6present = false;
     virErrorPtr save_err = NULL;
     virNetworkIpDefPtr ipdef;
+    char *macTapIfName;
 
     if (virNetworkObjIsActive(network)) {
         networkReportError(VIR_ERR_OPERATION_INVALID,
@@ -1585,6 +1595,30 @@ networkStartNetworkDaemon(struct network_driver *driver,
         return -1;
     }
 
+    if (network->def->mac_specified) {
+        /* To set a mac for the bridge, we need to define a dummy tap
+         * device, set its mac, then attach it to the bridge. As long
+         * as its mac address is lower than any other interface that
+         * gets attached, the bridge will always maintain this mac
+         * address.
+         */
+        macTapIfName = networkBridgeDummyNicName(network->def->bridge);
+        if (!macTapIfName) {
+            virReportOOMError();
+            goto err0;
+        }
+        if ((err = brAddTap(driver->brctl, network->def->bridge,
+                            &macTapIfName, network->def->mac, 0, false, NULL))) {
+            virReportSystemError(err,
+                                 _("cannot create dummy tap device '%s' to set mac"
+                                   " address on bridge '%s'"),
+                                 macTapIfName, network->def->bridge);
+            VIR_FREE(macTapIfName);
+            goto err0;
+        }
+        VIR_FREE(macTapIfName);
+    }
+
     /* Set bridge options */
     if (brSetForwardDelay(driver->brctl, network->def->bridge,
                           network->def->delay)) {
@@ -1693,6 +1727,17 @@ networkStartNetworkDaemon(struct network_driver *driver,
     networkRemoveIptablesRules(driver, network);
 
  err1:
+    if (!save_err)
+        save_err = virSaveLastError();
+
+    if ((err = brDeleteTap(driver->brctl, macTapIfName))) {
+        char ebuf[1024];
+        VIR_WARN("Failed to delete dummy tap device '%s' on bridge '%s' : %s",
+                 macTapIfName, network->def->bridge,
+                 virStrerror(err, ebuf, sizeof ebuf));
+    }
+
+ err0:
     if (!save_err)
         save_err = virSaveLastError();
     if ((err = brDeleteBridge(driver->brctl, network->def->bridge))) {
@@ -1714,6 +1759,7 @@ static int networkShutdownNetworkDaemon(struct network_driver *driver,
 {
     int err;
     char *stateFile;
+    char *macTapIfName;
 
     VIR_INFO(_("Shutting down network '%s'"), network->def->name);
 
@@ -1744,6 +1790,21 @@ static int networkShutdownNetworkDaemon(struct network_driver *driver,
         kill(network->dnsmasqPid, SIGTERM);
 
     char ebuf[1024];
+
+    if (network->def->mac_specified) {
+        macTapIfName = networkBridgeDummyNicName(network->def->bridge);
+        if (!macTapIfName) {
+            virReportOOMError();
+        } else {
+            if ((err = brDeleteTap(driver->brctl, macTapIfName))) {
+                VIR_WARN("Failed to delete dummy tap device '%s' on bridge '%s' : %s",
+                         macTapIfName, network->def->bridge,
+                         virStrerror(err, ebuf, sizeof ebuf));
+            }
+            VIR_FREE(macTapIfName);
+        }
+    }
+
     if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) {
         VIR_WARN("Failed to bring down bridge '%s' : %s",
                  network->def->bridge, virStrerror(err, ebuf, sizeof ebuf));
@@ -1988,6 +2049,8 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) {
     if (virNetworkSetBridgeName(&driver->networks, def, 1))
         goto cleanup;
 
+    virNetworkSetBridgeMacAddr(def);
+
     if (!(network = virNetworkAssignDef(&driver->networks,
                                         def)))
         goto cleanup;
@@ -2029,6 +2092,8 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
     if (virNetworkSetBridgeName(&driver->networks, def, 1))
         goto cleanup;
 
+    virNetworkSetBridgeMacAddr(def);
+
     if (!(network = virNetworkAssignDef(&driver->networks,
                                         def)))
         goto cleanup;

diff --git a/tests/networkxml2xmlin/isolated-network.xml b/tests/networkxml2xmlin/isolated-network.xml
index 507e3bb905f38c038e94f329501e2ec13aa46eaa..0d562ea2021bd7d6ca7dfb639dda75c883e15b84 100644 (file)
--- a/tests/networkxml2xmlin/isolated-network.xml
+++ b/tests/networkxml2xmlin/isolated-network.xml
@@ -2,6 +2,7 @@
   <name>private</name>
   <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
   <bridge name="virbr2" />
+  <mac address='52:54:00:17:3F:37'/>
   <ip address="192.168.152.1" netmask="255.255.255.0">
     <dhcp>
       <range start="192.168.152.2" end="192.168.152.254" />

diff --git a/tests/networkxml2xmlin/routed-network.xml b/tests/networkxml2xmlin/routed-network.xml
index 6634ee87e22093d71e66f62513f8c1619d9858f9..61d73c0b865fcb2c46374e5f7e0b53ed88812bf7 100644 (file)
--- a/tests/networkxml2xmlin/routed-network.xml
+++ b/tests/networkxml2xmlin/routed-network.xml
@@ -2,6 +2,7 @@
   <name>local</name>
   <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
   <bridge name="virbr1" />
+  <mac address='12:34:56:78:9A:BC'/>
   <forward mode="route" dev="eth1"/>
   <ip address="192.168.122.1" netmask="255.255.255.0">
   </ip>

diff --git a/tests/networkxml2xmlout/isolated-network.xml b/tests/networkxml2xmlout/isolated-network.xml
index 1d06f19bb1df58e32241973575de757c05f15dac..cc320a9d22b9f6c08dcc1ea10ecbe66b6405eb81 100644 (file)
--- a/tests/networkxml2xmlout/isolated-network.xml
+++ b/tests/networkxml2xmlout/isolated-network.xml
@@ -2,6 +2,7 @@
   <name>private</name>
   <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
   <bridge name='virbr2' stp='on' delay='0' />
+  <mac address='52:54:00:17:3F:37'/>
   <ip address='192.168.152.1' netmask='255.255.255.0'>
     <dhcp>
       <range start='192.168.152.2' end='192.168.152.254' />

diff --git a/tests/networkxml2xmlout/routed-network.xml b/tests/networkxml2xmlout/routed-network.xml
index 8f11166b941d00fb2db7aa2dd7a6b3efb8b25b8d..3aa810929fd2c6fed863402d91589f50a4a38fd4 100644 (file)
--- a/tests/networkxml2xmlout/routed-network.xml
+++ b/tests/networkxml2xmlout/routed-network.xml
@@ -3,6 +3,7 @@
   <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
   <forward dev='eth1' mode='route'/>
   <bridge name='virbr1' stp='on' delay='0' />
+  <mac address='12:34:56:78:9A:BC'/>
   <ip address='192.168.122.1' netmask='255.255.255.0'>
   </ip>
 </network>