From f4aad61e676e19e2f1bb14ac3773cb1eb7059920 Mon Sep 17 00:00:00 2001 From: James Tucker Date: Fri, 8 Apr 2022 13:06:40 -0700 Subject: [PATCH] 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 --- wgengine/monitor/monitor_linux.go | 68 +++++++++++++++-- wgengine/monitor/monitor_linux_test.go | 100 +++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 wgengine/monitor/monitor_linux_test.go diff --git a/wgengine/monitor/monitor_linux.go b/wgengine/monitor/monitor_linux.go index c3ca126fe..92b5fe54b 100644 --- a/wgengine/monitor/monitor_linux.go +++ b/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 { diff --git a/wgengine/monitor/monitor_linux_test.go b/wgengine/monitor/monitor_linux_test.go new file mode 100644 index 000000000..25bad943f --- /dev/null +++ b/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) + } + }) +}