Re: [PATCHv2 2/2] util: set bridge device MAC address explicitly during virNetDevBridgeCreate

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 17, 2019 at 21:59:49 -0400, Laine Stump wrote:
> From: Laine Stump <laine@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux