wgengine/monitor: ignore duplicate RTM_NEWADDRs

Ignoring the events at this layer is the simpler path for right now, a
broader change should follow to suppress irrelevant change events in a
higher layer so as to avoid related problems with other monitoring paths
on other platforms.  This approach may also carry a small risk that it
applies an at-most-once invariant low in the chain that could be assumed
otherwise higher in the code.

I adjusted the newAddrMessage type to include interface index rather
than a label, as labels are not always supplied, and in particular on my
test hosts they were consistently missing for ipv6 address messages.

I adjusted the newAddrMessage.Addr field to be populated from
Attributes.Address rather than Attributes.Local, as again for ipv6
.Local was always empty, and with ipv4 the .Address and .Local contained
the same contents in each of my test environments.

Update #4282

Signed-off-by: James Tucker <[email protected]>

wgengine/monitor/monitor_linux.go

@@ -38,6 +38,12 @@ type nlConn struct {
 	logf     logger.Logf
 	conn     *netlink.Conn
 	buffered []netlink.Message
+
+	// addrCache maps interface indices to a set of addresses, and is
+	// used to suppress duplicate RTM_NEWADDR messages. It is populated
+	// by RTM_NEWADDR messages and de-populated by RTM_DELADDR. See
+	// issue #4282.
+	addrCache map[uint32]map[netaddr.IP]bool
 }
 
 func newOSMon(logf logger.Logf, m *Mon) (osMon, error) {
@@ -55,7 +61,7 @@ func newOSMon(logf logger.Logf, m *Mon) (osMon, error) {
 		logf("monitor_linux: AF_NETLINK RTMGRP failed, falling back to polling")
 		return newPollingMon(logf, m)
 	}
-	return &nlConn{logf: logf, conn: conn}, nil
+	return &nlConn{logf: logf, conn: conn, addrCache: make(map[uint32]map[netaddr.IP]bool)}, nil
 }
 
 func (c *nlConn) Close() error { return c.conn.Close() }
@@ -86,10 +92,58 @@ func (c *nlConn) Receive() (message, error) {
 			return unspecifiedMessage{}, nil
 		}
 
+		nip := netaddrIP(rmsg.Attributes.Address)
+
+		if debugNetlinkMessages {
+			typ := "RTM_NEWADDR"
+			if msg.Header.Type == unix.RTM_DELADDR {
+				typ = "RTM_DELADDR"
+			}
+
+			// label attributes are seemingly only populated for ipv4 addresses in the wild.
+			label := rmsg.Attributes.Label
+			if label == "" {
+				itf, err := net.InterfaceByIndex(int(rmsg.Index))
+				if err == nil {
+					label = itf.Name
+				}
+			}
+
+			c.logf("%s: %s(%d) %s / %s", typ, label, rmsg.Index, rmsg.Attributes.Address, rmsg.Attributes.Local)
+		}
+
+		addrs := c.addrCache[rmsg.Index]
+
+		// Ignore duplicate RTM_NEWADDR messages using c.addrCache to
+		// detect them. See nlConn.addrcache and issue #4282.
+		if msg.Header.Type == unix.RTM_NEWADDR {
+			if addrs == nil {
+				addrs = make(map[netaddr.IP]bool)
+				c.addrCache[rmsg.Index] = addrs
+			}
+
+			if addrs[nip] {
+				if debugNetlinkMessages {
+					c.logf("ignored duplicate RTM_NEWADDR for %s", nip)
+				}
+				return ignoreMessage{}, nil
+			}
+
+			addrs[nip] = true
+		} else { // msg.Header.Type == unix.RTM_DELADDR
+			if addrs != nil {
+				delete(addrs, nip)
+			}
+
+			if len(addrs) == 0 {
+				delete(c.addrCache, rmsg.Index)
+			}
+		}
+
 		nam := &newAddrMessage{
-			Label:  rmsg.Attributes.Label,
-			Addr:   netaddrIP(rmsg.Attributes.Local),
-			Delete: msg.Header.Type == unix.RTM_DELADDR,
+			IfIndex: rmsg.Index,
+			Addr:    nip,
+			Delete:  msg.Header.Type == unix.RTM_DELADDR,
 		}
 		if debugNetlinkMessages {
 			c.logf("%+v", nam)
@@ -218,9 +272,9 @@ func (m *newRouteMessage) ignore() bool {
 
 // newAddrMessage is a message for a new address being added.
 type newAddrMessage struct {
-	Delete bool
-	Addr   netaddr.IP
-	Label  string // netlink Label attribute (e.g. "tailscale0")
+	Delete  bool
+	Addr    netaddr.IP
+	IfIndex uint32 // interface index
 }
 
 func (m *newAddrMessage) ignore() bool {

wgengine/monitor/monitor_linux_test.go

@@ -0,0 +1,100 @@
+// Copyright (c) 2022 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package monitor
+
+import (
+	"net"
+	"testing"
+
+	"github.com/jsimonetti/rtnetlink"
+	"github.com/mdlayher/netlink"
+	"golang.org/x/sys/unix"
+	"inet.af/netaddr"
+)
+
+func newAddrMsg(iface uint32, addr string, typ netlink.HeaderType) netlink.Message {
+	ip := net.ParseIP(addr)
+	if ip == nil {
+		panic("newAddrMsg: invalid addr: " + addr)
+	}
+
+	addrMsg := rtnetlink.AddressMessage{
+		Index: iface,
+		Attributes: &rtnetlink.AddressAttributes{
+			Address: ip,
+		},
+	}
+
+	b, err := addrMsg.MarshalBinary()
+	if err != nil {
+		panic(err)
+	}
+
+	return netlink.Message{
+		Header: netlink.Header{Type: typ},
+		Data:   b,
+	}
+}
+
+// See issue #4282 and nlConn.addrCache.
+func TestIgnoreDuplicateNEWADDR(t *testing.T) {
+	mustReceive := func(c *nlConn) message {
+		msg, err := c.Receive()
+		if err != nil {
+			t.Fatalf("mustReceive: unwanted error: %s", err)
+		}
+		return msg
+	}
+
+	t.Run("suppress duplicate NEWADDRs", func(t *testing.T) {
+		c := nlConn{
+			buffered: []netlink.Message{
+				newAddrMsg(1, "192.168.0.5", unix.RTM_NEWADDR),
+				newAddrMsg(1, "192.168.0.5", unix.RTM_NEWADDR),
+			},
+			addrCache: make(map[uint32]map[netaddr.IP]bool),
+		}
+
+		msg := mustReceive(&c)
+		if _, ok := msg.(*newAddrMessage); !ok {
+			t.Fatalf("want newAddrMessage, got %T %v", msg, msg)
+		}
+
+		msg = mustReceive(&c)
+		if _, ok := msg.(ignoreMessage); !ok {
+			t.Fatalf("want ignoreMessage, got %T %v", msg, msg)
+		}
+	})
+
+	t.Run("do not suppress after DELADDR", func(t *testing.T) {
+		c := nlConn{
+			buffered: []netlink.Message{
+				newAddrMsg(1, "192.168.0.5", unix.RTM_NEWADDR),
+				newAddrMsg(1, "192.168.0.5", unix.RTM_DELADDR),
+				newAddrMsg(1, "192.168.0.5", unix.RTM_NEWADDR),
+			},
+			addrCache: make(map[uint32]map[netaddr.IP]bool),
+		}
+
+		msg := mustReceive(&c)
+		if _, ok := msg.(*newAddrMessage); !ok {
+			t.Fatalf("want newAddrMessage, got %T %v", msg, msg)
+		}
+
+		msg = mustReceive(&c)
+		if m, ok := msg.(*newAddrMessage); !ok {
+			t.Fatalf("want newAddrMessage, got %T %v", msg, msg)
+		} else {
+			if !m.Delete {
+				t.Fatalf("want delete, got %#v", m)
+			}
+		}
+
+		msg = mustReceive(&c)
+		if _, ok := msg.(*newAddrMessage); !ok {
+			t.Fatalf("want newAddrMessage, got %T %v", msg, msg)
+		}
+	})
+}