[libvirt] [PATCHv2 2/2] util: set bridge device MAC address explicitly during virNetDevBridgeCreate
Jiri Denemark
jdenemar at redhat.com
Thu Nov 7 13:04:32 UTC 2019
On Thu, Oct 17, 2019 at 21:59:49 -0400, Laine Stump wrote:
> From: Laine Stump <laine at redhat.com>
>
> Remember when the MAC address of libvirt-created bridges weren't
> stable, and changed as guests were started and stopped? Pepperidge
> Farms remembers.
>
> (No, I would never actually push a comment like that! Just wanted to
> get your attention).
>
> When libvirt first implemented a stable and configurable MAC address
> for the bridges created for libvirt virtual networks (commit
> 5754dbd56d, in libvirt v8.8.0) most distro stable releases didn't
Looks like we can finally travel in time :-) v0.8.8 is the correct
version.
> support explicitly setting the MAC address of a bridge; the bridge
> just always assumed the lowest numbered MAC of all attached
> interfaces. Because of this, we stabilized the bridge MAC address by
> creating a "dummy" tap interface with a MAC address guaranteed to be
> lower than any of the guest tap devices' MACs (which all started with
> 0xFE, so it's not difficult to do) and attached it to the bridge -
> this was the inception of the "virbr0-nic" device that has confused so
> many people over the years.
>
> Even though the linux kernel had recently gained support for
> explicitly setting a bridge MAC, we deemed it unnecessary to set the
> MAC that way, because the other (indirect) method worked everywhere.
>
> But recently there have been reports that the bridge MAC address was
> not following the setting in the network config, and mismatched the
> MAC of the dummy tap device (which was still correct). It turns out
> that this is due to a change in systemd/udev that persists whatever
> MAC address is set for a bridge when it's initially started:
>
> https://github.com/systemd/systemd/blob/master/NEWS#L499
>
> which was the result of:
>
> https://github.com/systemd/systemd/issues/3374
>
> (apparently if there is no MAC saved for a bridge by the name of a
> bridge being created, the random MAC generated during creation is
> saved, and then that same MAC is used to explicitly set the MAC each
> time it is created). Once a bridge has an explicitly set MAC, the "use
> the lowest numbered MAC of attached devices" rule is ignored, so our
> dummy tap device is like the goggles - it does nothing! (well, almost).
>
> We could whine about changes in default behavior, etc. etc., but
> because the change was in response to actual user problems, that seems
> likely a fruitless task. Fortunately, time has marched on, and even
> distro releases that are old enough that they are no longer supported
> by upstream libvirt (e.g. RHEL6) have support for explicitly setting a
> bridge device MAC address, either during creation or with a separate
> ioctl after creation, so we can now do that.
>
> The method is to add a mac arg to virNetDevBridgeCreate(). In the case
> of platforms where the bridge is created with a netlink RTM_NEWLINK
> message, we just add the desired mac to the message. For platforms
> that still use an ioctl (either SIOCBRADDBR or SIOCIFCREATE2), we make
> a separate call to virNetDevSetMAC() after creating the bridge.
>
> This makes the dummy tap pointless for purposes of setting the MAC
> address, but it is still useful for IPv6 DAD initialization (which
> apparently requires at least one interface to be attached to the
> bridge and online), so it hasn't been removed. (I'm considered making
> another patch to add the dummy tap device only if the network needs
> IPv6 DAD, but haven't decided yet if it's worthwhile).
>
> (NB: we can safely *always* call virNetDevBridgeCreate() with
> &def->mac from the network driver because, in spite of the existence
> of a "mac_specified" bool in the config suggesting that it may not
> always be present, in reality a mac address will always be added to
> any network that doesn't have one - this is guaranteed in all cases by
> commit a47ae7c004)
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1760851
> Signed-off-by: Laine Stump <laine at redhat.com>
> ---
> NB: I was unable to test the calling of virNetDevSetMAC() from the
> SIOCIFCREATE2 (BSD) version of virNetDevBridgeCreate(); even though I
> managed to get a FreeBSD system setup and libvirt built there, when I
> tried to start the default network the SIOCIFCREATE2 ioctl itself
> failed, so it never even got to the virNetDevSetMAC(). I would
> sincerely appreciate if someone more up to speed with libvirt on
> FreeBSD/OpenBSD could check that out and let me know if it works
> properly.
>
> src/network/bridge_driver.c | 2 +-
> src/util/virnetdevbridge.c | 43 ++++++++++++++++++++++++++++++-------
> src/util/virnetdevbridge.h | 2 +-
> 3 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6862818698..4f01bf6664 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2498,7 +2498,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
> def->name);
> return -1;
> }
> - if (virNetDevBridgeCreate(def->bridge) < 0)
> + if (virNetDevBridgeCreate(def->bridge, &def->mac) < 0)
> return -1;
>
> if (def->mac_specified) {
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index edf4cc6236..d3a1e3c13e 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -379,7 +379,7 @@ virNetDevBridgePortSetUnicastFlood(const char *brname G_GNUC_UNUSED,
> */
> #if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR)
> static int
> -virNetDevBridgeCreateWithIoctl(const char *brname)
> +virNetDevBridgeCreateWithIoctl(const char *brname, const virMacAddr *mac)
Could you move the new parameter on its own line?
> {
> VIR_AUTOCLOSE fd = -1;
>
> @@ -392,22 +392,35 @@ virNetDevBridgeCreateWithIoctl(const char *brname)
> return -1;
> }
>
> + if (virNetDevSetMAC(brname, mac) < 0) {
> + virErrorPtr savederr;
> +
> + virErrorPreserveLast(&savederr);
> + ignore_value(ioctl(fd, SIOCBRDELBR, brname));
> + virErrorRestore(&savederr);
> + return -1;
> + }
> +
> return 0;
> }
> #endif
>
> #if defined(__linux__) && defined(HAVE_LIBNL)
> int
> -virNetDevBridgeCreate(const char *brname)
> +virNetDevBridgeCreate(const char *brname, const virMacAddr *mac)
And the same hare.
> {
> /* use a netlink RTM_NEWLINK message to create the bridge */
> int error = 0;
> + virNetlinkNewLinkData data = {
> + .mac = mac,
> + };
> +
>
> - if (virNetlinkNewLink(brname, "bridge", NULL, &error) < 0) {
> + if (virNetlinkNewLink(brname, "bridge", &data, &error) < 0) {
> # if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR)
> if (error == -EOPNOTSUPP) {
> /* fallback to ioctl if netlink doesn't support creating bridges */
> - return virNetDevBridgeCreateWithIoctl(brname);
> + return virNetDevBridgeCreateWithIoctl(brname, mac);
> }
> # endif
> if (error < 0)
> @@ -423,15 +436,17 @@ virNetDevBridgeCreate(const char *brname)
>
> #elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR)
> int
> -virNetDevBridgeCreate(const char *brname)
> +virNetDevBridgeCreate(const char *brname,
> + const virMacAddr *mac)
So that all prototypes look like this one.
> {
> - return virNetDevBridgeCreateWithIoctl(brname);
> + return virNetDevBridgeCreateWithIoctl(brname, mac);
> }
>
>
> #elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCIFCREATE2)
> int
> -virNetDevBridgeCreate(const char *brname)
> +virNetDevBridgeCreate(const char *brname,
> + const virMacAddr *mac)
> {
> struct ifreq ifr;
> VIR_AUTOCLOSE s = -1;
> @@ -448,10 +463,21 @@ virNetDevBridgeCreate(const char *brname)
> if (virNetDevSetName(ifr.ifr_name, brname) == -1)
> return -1;
>
> + if (virNetDevSetMAC(brname, mac) < 0) {
> + virErrorPtr savederr;
> +
> + virErrorPreserveLast(&savederr);
> + ignore_value(virNetDevBridgeDelete(brname));
> + virErrorRestore(&savederr);
> + return -1;
> + }
> +
> return 0;
> }
> #else
> -int virNetDevBridgeCreate(const char *brname)
> +int
> +virNetDevBridgeCreate(const char *brname,
> + const virMacAddr *mac ATTRIBUTE_UNUSED)
> {
> virReportSystemError(ENOSYS,
> _("Unable to create bridge %s"), brname);
> @@ -528,6 +554,7 @@ virNetDevBridgeDelete(const char *brname)
> _("Unable to remove bridge %s"),
> brname);
> return -1;
> +
> }
>
> return 0;
Drop this hunk, please.
> diff --git a/src/util/virnetdevbridge.h b/src/util/virnetdevbridge.h
> index 88284d6bed..c47597dec9 100644
> --- a/src/util/virnetdevbridge.h
> +++ b/src/util/virnetdevbridge.h
> @@ -21,7 +21,7 @@
> #include "internal.h"
> #include "virmacaddr.h"
>
> -int virNetDevBridgeCreate(const char *brname)
> +int virNetDevBridgeCreate(const char *brname, const virMacAddr *mac)
> ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT;
> int virNetDevBridgeDelete(const char *brname)
> ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT;
Reviewed-by: Jiri Denemark <jdenemar at redhat.com>
More information about the libvir-list
mailing list