From 21ea3ff25ffb5f7e841cd0dd2481c67387cb8d66 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Tue, 28 Jul 2026 13:43:47 +0200 Subject: [PATCH] Add patches for bridge MAC pinning Signed-off-by: Tom Wieczorek --- .dockerignore | 1 + .github/workflows/ci.yaml | 1 + Dockerfile | 10 +- ...ge-always-pin-the-bridge-MAC-address.patch | 149 ++++++++++++ ...te-a-bridge-MAC-when-the-kernel-has-.patch | 218 ++++++++++++++++++ 5 files changed, 377 insertions(+), 2 deletions(-) create mode 100644 files/patches/0001-bridge-always-pin-the-bridge-MAC-address.patch create mode 100644 files/patches/0002-bridge-re-generate-a-bridge-MAC-when-the-kernel-has-.patch diff --git a/.dockerignore b/.dockerignore index 6ffc39e..49aa35d 100644 --- a/.dockerignore +++ b/.dockerignore @@ -3,3 +3,4 @@ # Keep in sync with the paths in .github/workflows/ci.yaml. !src/ +!files/ diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 6c22a67..dd02b26 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -11,6 +11,7 @@ on: - .dockerignore # keep in sync with .dockerignore - src/** + - files/** push: *triggers jobs: diff --git a/Dockerfile b/Dockerfile index d06b9dd..e43e432 100644 --- a/Dockerfile +++ b/Dockerfile @@ -5,14 +5,20 @@ ARG \ HASH=34bd82d47e981940751619c9cc44c095bb90bfcaf8d71865cbb822c37690a764 FROM --platform=$BUILDPLATFORM $GOLANG_IMAGE AS sources +RUN apk add --no-cache patch ENV GOTOOLCHAIN=local GOTELEMETRY=off WORKDIR /go/src/containernetworking-plugins ARG VERSION HASH -RUN --mount=type=tmpfs,target=/tmp \ +RUN --mount=source=files/patches,target=/run/patches,ro \ + --mount=type=tmpfs,target=/tmp \ set -euo pipefail \ && wget -qO /tmp/sources.tar.gz https://github.com/containernetworking/plugins/archive/refs/tags/v$VERSION.tar.gz \ && { echo $HASH /tmp/sources.tar.gz | sha256sum -c -; } || { sha256sum /tmp/sources.tar.gz; exit 1; } \ - && tar xf /tmp/sources.tar.gz --strip-components=1 + && tar xf /tmp/sources.tar.gz --strip-components=1 \ + && for p in /run/patches/*; do \ + echo Applying "$p" >&2; \ + patch -p1 <"$p"; \ + done FROM sources AS bins diff --git a/files/patches/0001-bridge-always-pin-the-bridge-MAC-address.patch b/files/patches/0001-bridge-always-pin-the-bridge-MAC-address.patch new file mode 100644 index 0000000..d3a6b2e --- /dev/null +++ b/files/patches/0001-bridge-always-pin-the-bridge-MAC-address.patch @@ -0,0 +1,149 @@ +From 222058b490000246306210646c50a790f0be3a4c Mon Sep 17 00:00:00 2001 +From: Tom Wieczorek +Date: Wed, 17 Jun 2026 13:05:21 +0200 +Subject: [PATCH 1/2] bridge: always pin the bridge MAC address + +The ensureAddr function pins the bridge's MAC address so it doesn't +change as ports come and go. The pinning happens after the gateway +address is set, so if a run set the address but then failed before +pinning, subsequent runs never reattempted to pin the MAC, leaving +behind a bridge with a floating MAC. + +Always pin the MAC, even if the gateway address is already present, +regardless of the outcomes of previous attempts. + +Signed-off-by: Tom Wieczorek +--- + plugins/main/bridge/bridge.go | 12 ++++-- + plugins/main/bridge/bridge_test.go | 67 ++++++++++++++++++++++++++++++ + 2 files changed, 75 insertions(+), 4 deletions(-) + +diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go +index 476e6d5f..860ba8d1 100644 +--- a/plugins/main/bridge/bridge.go ++++ b/plugins/main/bridge/bridge.go +@@ -271,11 +271,13 @@ func ensureAddr(br netlink.Link, family int, ipn *net.IPNet, forceAddress bool) + } + + ipnStr := ipn.String() ++ addrFound := false + for _, a := range addrs { + + // string comp is actually easiest for doing IPNet comps + if a.IPNet.String() == ipnStr { +- return nil ++ addrFound = true ++ break + } + + // Multiple IPv6 addresses are allowed on the bridge if the +@@ -293,9 +295,11 @@ func ensureAddr(br netlink.Link, family int, ipn *net.IPNet, forceAddress bool) + } + } + +- addr := &netlink.Addr{IPNet: ipn, Label: ""} +- if err := netlink.AddrAdd(br, addr); err != nil && err != syscall.EEXIST { +- return fmt.Errorf("could not add IP address %s to %q: %v", ipnStr, br.Attrs().Name, err) ++ if !addrFound { ++ addr := &netlink.Addr{IPNet: ipn, Label: ""} ++ if err := netlink.AddrAdd(br, addr); err != nil && err != syscall.EEXIST { ++ return fmt.Errorf("could not add IP address %s to %q: %v", ipnStr, br.Attrs().Name, err) ++ } + } + + // Set the bridge's MAC to itself. Otherwise, the bridge will take the +diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go +index 3ed19022..cd3f562f 100644 +--- a/plugins/main/bridge/bridge_test.go ++++ b/plugins/main/bridge/bridge_test.go +@@ -21,6 +21,7 @@ import ( + "net" + "os" + "strings" ++ "sync" + + "github.com/coreos/go-iptables/iptables" + "github.com/networkplumbing/go-nft/nft" +@@ -407,6 +408,30 @@ func (tc testCase) expectedCIDRs() ([]*net.IPNet, []*net.IPNet) { + return cidrsV4, cidrsV6 + } + ++// Attaches a veth port with an optional fixed MAC address to a bridge in the ++// current network namespace. To be cleaned up via the returned function. ++func attachVethPort(br netlink.Link, mac string) (func() error, error) { ++ linkAttrs := netlink.NewLinkAttrs() ++ linkAttrs.Name = "testport0" ++ if mac != "" { ++ hwAddr, err := net.ParseMAC(mac) ++ if err != nil { ++ return nil, err ++ } ++ ++ linkAttrs.HardwareAddr = hwAddr ++ } ++ veth := &netlink.Veth{LinkAttrs: linkAttrs, PeerName: "testport1"} ++ if err := netlink.LinkAdd(veth); err != nil { ++ return nil, err ++ } ++ if err := netlink.LinkSetMaster(veth, br); err != nil { ++ return nil, err ++ } ++ ++ return sync.OnceValue(func() error { return netlink.LinkDel(veth) }), nil ++} ++ + // delBridgeAddrs() deletes addresses from the bridge + func delBridgeAddrs(testNS ns.NetNS) { + err := testNS.Do(func(ns.NetNS) error { +@@ -2377,6 +2402,48 @@ var _ = Describe("bridge Operations", func() { + }) + Expect(err).NotTo(HaveOccurred()) + }) ++ ++ It(fmt.Sprintf("[%s] (%d) keeps a stable MAC even if the gateway address already exists", ver, i), func() { ++ err := originalNS.Do(func(ns.NetNS) error { ++ defer GinkgoRecover() ++ ++ tc.cniVersion = ver ++ br, _, err := setupBridge(tc.netConf()) ++ Expect(err).NotTo(HaveOccurred()) ++ link, err := netlinksafe.LinkByName(BRNAME) ++ Expect(err).NotTo(HaveOccurred()) ++ originalMAC := link.Attrs().HardwareAddr ++ ++ // Pre-add the gateway address the plugin is going to ++ // configure, as left behind by an ADD that failed midway. ++ _, subnet, err := net.ParseCIDR(tc.subnet) ++ Expect(err).NotTo(HaveOccurred()) ++ gwIP := calcGatewayIP(subnet) ++ err = netlink.AddrAdd(br, &netlink.Addr{ ++ IPNet: &net.IPNet{IP: gwIP, Mask: subnet.Mask}, ++ }) ++ Expect(err).NotTo(HaveOccurred()) ++ ++ cmdAddDelTest(originalNS, targetNS, tc, dataDir) ++ ++ // The MAC must have been pinned nevertheless: it neither ++ // got zeroed when the container veth was removed, nor ++ // does it change with port churn. ++ link, err = netlinksafe.LinkByName(BRNAME) ++ Expect(err).NotTo(HaveOccurred()) ++ Expect(link.Attrs().HardwareAddr).To(Equal(originalMAC)) ++ ++ cleanupVethPort, err := attachVethPort(br, "02:00:00:00:00:01") ++ Expect(err).NotTo(HaveOccurred()) ++ defer func() { Expect(cleanupVethPort()).To(Succeed()) }() ++ ++ link, err = netlinksafe.LinkByName(BRNAME) ++ Expect(err).NotTo(HaveOccurred()) ++ Expect(link.Attrs().HardwareAddr).To(Equal(originalMAC)) ++ return nil ++ }) ++ Expect(err).NotTo(HaveOccurred()) ++ }) + } + + It(fmt.Sprintf("[%s] uses an explicit MAC addresses for the container iface (from CNI_ARGS)", ver), func() { +-- +2.54.0 + diff --git a/files/patches/0002-bridge-re-generate-a-bridge-MAC-when-the-kernel-has-.patch b/files/patches/0002-bridge-re-generate-a-bridge-MAC-when-the-kernel-has-.patch new file mode 100644 index 0000000..ae92821 --- /dev/null +++ b/files/patches/0002-bridge-re-generate-a-bridge-MAC-when-the-kernel-has-.patch @@ -0,0 +1,218 @@ +From e63b1f706facdf7683f5ab600f9335d91c654d65 Mon Sep 17 00:00:00 2001 +From: Tom Wieczorek +Date: Wed, 17 Jun 2026 13:24:48 +0200 +Subject: [PATCH 2/2] 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 +--- + plugins/main/bridge/bridge.go | 41 +++++++++++++- + plugins/main/bridge/bridge_test.go | 89 ++++++++++++++++++++++++++++++ + 2 files changed, 128 insertions(+), 2 deletions(-) + +diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go +index 860ba8d1..8d89c20d 100644 +--- a/plugins/main/bridge/bridge.go ++++ b/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 { ++ return fmt.Errorf("failed to generate random MAC address: %w", 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 ++} +diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go +index cd3f562f..5cd1b980 100644 +--- a/plugins/main/bridge/bridge_test.go ++++ b/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" +@@ -2446,6 +2449,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() +@@ -2824,3 +2891,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") ++} +-- +2.54.0 +