bridge: re-generate a bridge MAC when the kernel has zeroed it out

The kernel zeroes a bridge's MAC address when its last port is removed.
The ensureAddr function pins the bridge's MAC address using the
address read when the bridge was looked up, which is before the veth
is attached. So for an existing bridge that had previously lost all its
ports, the address it tried to pin is all zeros, and pinning an all-zero
address fails with EINVAL.

In fact, this can happen in practice: an ADD that fails has its veth
cleaned up, which, if it was the first ADD, leaves the bridge portless
and hence its MAC zeroed, so the next ADD fails to pin it.

Detect that case and regenerate a random MAC address the same way the
kernel does. This is fine as it only happens if the bridge has no ports,
so no peer can hold stale state about the old address.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>

Author: twz123

plugins/main/bridge/bridge.go

@@ -15,6 +15,7 @@
 package main
 
 import (
+	"crypto/rand"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -303,8 +304,27 @@ func ensureAddr(br netlink.Link, family int, ipn *net.IPNet, forceAddress bool)
 	}
 
 	// Set the bridge's MAC to itself. Otherwise, the bridge will take the
-	// lowest-numbered mac on the bridge, and will change as ifs churn
-	if err := netlink.LinkSetHardwareAddr(br, br.Attrs().HardwareAddr); err != nil {
+	// lowest-numbered mac on the bridge, and will change as ifs churn. The
+	// cached attrs hold the MAC the bridge had before any veths were attached
+	// during this invocation.
+	hwAddr := br.Attrs().HardwareAddr
+
+	// Check whether the kernel has zeroed the bridge's MAC.
+	// This only happens when the bridge's last port was detached.
+	if isZeroMAC(hwAddr) {
+		// There are no neighbors that could hold stale state about the previous
+		// MAC, so it's fine to generate a suitable stand-in the same way the
+		// kernel does.
+		// https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/include/linux/etherdevice.h?h=v7.1#n237
+		hwAddr = make(net.HardwareAddr, 6)
+		if _, err := rand.Read(hwAddr); err != nil {
+			panic(fmt.Sprintf("failed to generate random MAC address: %v", err))
+		}
+		hwAddr[0] &= 0xfe // clear multicast bit
+		hwAddr[0] |= 0x02 // set local assignment bit (IEEE802)
+	}
+
+	if err := netlink.LinkSetHardwareAddr(br, hwAddr); err != nil {
 		return fmt.Errorf("could not set bridge's mac: %v", err)
 	}
 
@@ -676,6 +696,10 @@ func cmdAdd(args *skel.CmdArgs) error {
 						if err != nil {
 							return fmt.Errorf("failed to set bridge addr: %v", err)
 						}
+						// Reload the bridge so it reflects the MAC that ensureAddr just pinned.
+						if br, err = bridgeByName(n.BrName); err != nil {
+							return err
+						}
 					}
 				}
 
@@ -1120,3 +1144,16 @@ func cmdStatus(args *skel.CmdArgs) error {
 
 	return nil
 }
+
+// Indicates whether a hardware address is nil, empty, or consists solely of
+// zeros. Technically, the kernel will always report such addresses as an array
+// of zeros, but vishvananda/netlink will "normalize" them to nil.
+func isZeroMAC(mac net.HardwareAddr) bool {
+	for _, b := range mac {
+		if b != 0 {
+			return false
+		}
+	}
+
+	return true
+}

plugins/main/bridge/bridge_test.go

@@ -17,6 +17,7 @@ package main
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net"
 	"os"
@@ -27,6 +28,8 @@ import (
 	"github.com/networkplumbing/go-nft/nft"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	gomegaformat "github.com/onsi/gomega/format"
+	gomegatypes "github.com/onsi/gomega/types"
 	"github.com/vishvananda/netlink"
 	"github.com/vishvananda/netlink/nl"
 	"sigs.k8s.io/knftables"
@@ -2445,6 +2448,70 @@ var _ = Describe("bridge Operations", func() {
 			})
 		}
 
+		It(fmt.Sprintf("[%s] keeps a stable MAC even if the bridge previously lost all its ports", ver), func() {
+			err := originalNS.Do(func(ns.NetNS) error {
+				defer GinkgoRecover()
+
+				// Test dual-stack here: A dual-stack ADD configures a
+				// gateway per family, so it sets the bridge MAC more than
+				// once in a single invocation.
+				tc := testCase{
+					cniVersion: ver,
+					ranges: []rangeInfo{
+						{subnet: "10.1.2.0/24"},
+						{subnet: "2001:db8:42::/64"},
+					},
+					expGWCIDRs: []string{"10.1.2.1/24", "2001:db8:42::1/64"},
+				}
+
+				br, _, err := setupBridge(tc.netConf())
+				Expect(err).NotTo(HaveOccurred())
+
+				// Attach and remove a port so the kernel first generates,
+				// and then zeroes the bridge's MAC.
+				cleanupVethPort, err := attachVethPort(br, "")
+				Expect(err).NotTo(HaveOccurred())
+				defer func() { Expect(cleanupVethPort()).To(Succeed()) }()
+				link, err := netlinksafe.LinkByName(BRNAME)
+				Expect(err).NotTo(HaveOccurred())
+				kernelMAC := link.Attrs().HardwareAddr
+				Expect(kernelMAC).NotTo(beAZeroMAC(), "kernel didn't generate a MAC")
+				Expect(kernelMAC[0]&0x01).To(BeZero(), "kernel-generated MAC is not unicast")
+				Expect(kernelMAC[0]&0x02).NotTo(BeZero(), "kernel-generated MAC is not locally administered")
+				Expect(cleanupVethPort()).To(Succeed())
+				link, err = netlinksafe.LinkByName(BRNAME)
+				Expect(err).NotTo(HaveOccurred())
+				// If the expectation below fails, it may be because the
+				// kernel behavior has changed and it keeps the original
+				// address.
+				Expect(link.Attrs().HardwareAddr).To(beAZeroMAC(), "kernel should have zeroed the MAC after the bridge lost its last port")
+
+				cmdAddDelTest(originalNS, targetNS, tc, dataDir)
+
+				// The plugin must have generated a valid, locally administered
+				// unicast MAC address. This MAC address must have survived the
+				// removal of the container veth during DEL.
+				link, err = netlinksafe.LinkByName(BRNAME)
+				Expect(err).NotTo(HaveOccurred())
+				bridgeMAC := link.Attrs().HardwareAddr
+				Expect(bridgeMAC).NotTo(beAZeroMAC(), "bridge plugin should have generated a MAC")
+				Expect(bridgeMAC).NotTo(Equal(kernelMAC), "generated MAC is the same as the kernel-generated one")
+				Expect(bridgeMAC[0]&0x01).To(BeZero(), "generated MAC should be unicast")
+				Expect(bridgeMAC[0]&0x02).NotTo(BeZero(), "generated MAC should be locally administered")
+
+				// A second pod's ADD on the same bridge must keep that MAC.
+				// Every pod on the node shares the bridge. Between the two
+				// ADDs, it briefly had no ports. Because the generated MAC
+				// address was pinned, the kernel does not zero it.
+				cmdAddDelTest(originalNS, targetNS, tc, dataDir)
+				link, err = netlinksafe.LinkByName(BRNAME)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(link.Attrs().HardwareAddr).To(Equal(bridgeMAC), "generated MAC changed, but it should have been pinned")
+				return nil
+			})
+			Expect(err).NotTo(HaveOccurred())
+		})
+
 		It(fmt.Sprintf("[%s] uses an explicit MAC addresses for the container iface (from CNI_ARGS)", ver), func() {
 			err := originalNS.Do(func(ns.NetNS) error {
 				defer GinkgoRecover()
@@ -2823,3 +2890,25 @@ func assertMacSpoofCheckRules(assert func(actual interface{}, expectedLen int))
 		"macspoofchk-dummy-0-eth0",
 	)), 2)
 }
+
+func beAZeroMAC() gomegatypes.GomegaMatcher {
+	return zeroMACMatcher{}
+}
+
+type zeroMACMatcher struct{}
+
+func (zeroMACMatcher) Match(actual any) (bool, error) {
+	if addr, ok := actual.(net.HardwareAddr); ok {
+		return isZeroMAC(addr), nil
+	}
+
+	return false, errors.New("expected a net.HardwareAddr")
+}
+
+func (zeroMACMatcher) FailureMessage(actual any) string {
+	return gomegaformat.Message(fmt.Sprint(actual), "to be a zero MAC")
+}
+
+func (zeroMACMatcher) NegatedFailureMessage(actual any) string {
+	return gomegaformat.Message(fmt.Sprint(actual), "not to be a zero MAC")
+}