From 28657607c14d4b5135c48cb79fcb9f4fd0600b42 Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Fri, 16 Oct 2026 02:16:45 +0200 Subject: [PATCH] [client] Keep interface discovery out of the bind open path (#8226) * [client] Keep interface discovery out of the bind open path Since 0.81.0 a userspace-mode agent whose interface name is not covered by the interface blacklist deadlocks while bringing the device up, and the daemon, the GUI and the service stop all hang. wireguard-go holds the device net lock across bind.Open, our bind builds the UDP mux there, the mux enumerated interfaces in its constructor, and the interface filter probed every unlisted name through wgctrl. For the agent's own interface that probe connected to the daemon's own UAPI endpoint and waited, with no deadline, for a reply the handler could only give after taking the same lock. #7346 moved the first enumeration from the stdnet constructor, which ran before the device existed, to this call site. The mux no longer discovers interfaces when it is constructed. It records whether its socket is bound to an unspecified address and GetConn uses that instead of the length of the discovered list, while GetListenAddresses keeps refreshing on demand, which pion calls during candidate gathering outside the device locks. The WireGuard detector also answers for the agent's own interface without probing, so no filter asks the daemon about itself. * [client] Take the agent's own interface name in the detector constructor Every device implementation reports the configured name, so there is no later name to follow and the setter was state without a reader that needed it. The detector takes the name once and compares without a lock. * [client] Create the default mux Net in the constructor With discovery moved to GetListenAddresses, the nil Net fallback ran there as well, and the first concurrent callers raced on writing it. stdnet.NewNet does no discovery, so the constructor can create it before the mux is shared and the address refresh only reads it. * [client] Follow the created interface name in the detector Android ignores the configured interface name and keeps the TUN name returned by CreateUnmonitoredTUNFromFD, so the detector built from the configuration would still probe the agent's real interface. The engine updates the detector with wgInterface.Name() once the interface exists. * [client] Keep the detector's own interface name immutable On Android the kernel names the TUN and the name can change again on renew, while the UAPI socket cannot be opened there at all, so neither the probe nor a name set after creation can identify the agent's own interface reliably. Excluding that interface on Android belongs to the interface discovery the app provides. Everywhere else the configured name is the created name, so the constructor argument is enough and the detector needs no setter and no lock on the hot path. --- client/iface/bind/ice_bind_test.go | 88 ++++++++++++++++++++ client/iface/udpmux/mux.go | 20 ++--- client/iface/udpmux/mux_test.go | 99 +++++++++++++++++++++++ client/internal/engine.go | 2 +- client/internal/stdnet/wgdetector.go | 19 ++++- client/internal/stdnet/wgdetector_test.go | 36 +++++++++ 6 files changed, 250 insertions(+), 14 deletions(-) create mode 100644 client/iface/udpmux/mux_test.go diff --git a/client/iface/bind/ice_bind_test.go b/client/iface/bind/ice_bind_test.go index 0b8db7640..8dd82d7df 100644 --- a/client/iface/bind/ice_bind_test.go +++ b/client/iface/bind/ice_bind_test.go @@ -2,17 +2,22 @@ package bind import ( "fmt" + "io" "net" "net/netip" "sync" + "sync/atomic" "testing" "time" + "github.com/pion/transport/v3" "github.com/pion/transport/v3/stdnet" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/ipv4" "golang.org/x/net/ipv6" + "golang.zx2c4.com/wireguard/device" + "golang.zx2c4.com/wireguard/tun/netstack" "github.com/netbirdio/netbird/client/iface/wgaddr" ) @@ -326,3 +331,86 @@ func listenUDP(t *testing.T, network, addr string) *net.UDPConn { require.NoError(t, err) return conn } + +type countingNet struct { + transport.Net + calls atomic.Int64 + ifaces []*transport.Interface + before func() +} + +func (n *countingNet) Interfaces() ([]*transport.Interface, error) { + n.calls.Add(1) + if n.before != nil { + n.before() + } + return n.ifaces, nil +} + +func TestICEBind_OpenDoesNotDiscoverInterfaces(t *testing.T) { + n := newCountingNet() + iceBind := NewICEBind(n, testAddress(), 1280) + + for i := 0; i < 2; i++ { + _, port, err := iceBind.Open(0) + require.NoError(t, err) + require.NotZero(t, port, "the bind listens on a port") + assert.Equal(t, int64(0), n.calls.Load(), "opening the bind must not enumerate interfaces, attempt %d", i+1) + require.NoError(t, iceBind.Close()) + } + + _, _, err := iceBind.Open(0) + require.NoError(t, err) + t.Cleanup(func() { _ = iceBind.Close() }) + + mux, err := iceBind.GetICEMux() + require.NoError(t, err) + assert.NotEmpty(t, mux.SingleSocketUDPMux.GetListenAddresses(), "listen addresses come from the discovered interfaces") + assert.Equal(t, int64(1), n.calls.Load(), "interfaces are discovered when the listen addresses are asked for") +} + +func TestICEBind_DeviceUpDoesNotWaitOnInterfaceDiscovery(t *testing.T) { + tunDev, _, err := netstack.CreateNetTUN([]netip.Addr{netip.MustParseAddr("100.64.0.1")}, nil, 1280) + require.NoError(t, err) + + var dev *device.Device + n := newCountingNet() + n.before = func() { + _ = dev.IpcGetOperation(io.Discard) + } + iceBind := NewICEBind(n, testAddress(), 1280) + dev = device.NewDevice(tunDev, iceBind, device.NewLogger(device.LogLevelSilent, "")) + + upDone := make(chan error, 1) + go func() { + upDone <- dev.Up() + }() + + select { + case err := <-upDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("device Up did not return: interface discovery ran under the device lock and waited on the device itself") + } + t.Cleanup(dev.Close) + + assert.Equal(t, int64(0), n.calls.Load(), "bringing the device up must not enumerate interfaces") + + mux, err := iceBind.GetICEMux() + require.NoError(t, err) + assert.NotEmpty(t, mux.SingleSocketUDPMux.GetListenAddresses(), "discovery that talks to the device succeeds once the device is up") + assert.Equal(t, int64(1), n.calls.Load(), "interfaces are discovered outside the device lock") +} + +func newCountingNet() *countingNet { + ifc := transport.NewInterface(net.Interface{Index: 1, MTU: 1500, Name: "eth0", Flags: net.FlagUp | net.FlagMulticast}) + ifc.AddAddress(&net.IPNet{IP: net.IPv4(192, 168, 1, 10), Mask: net.CIDRMask(24, 32)}) + return &countingNet{ifaces: []*transport.Interface{ifc}} +} + +func testAddress() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("100.64.0.1"), + Network: netip.MustParsePrefix("100.64.0.0/10"), + } +} diff --git a/client/iface/udpmux/mux.go b/client/iface/udpmux/mux.go index 3aa0b4d88..2f7287b53 100644 --- a/client/iface/udpmux/mux.go +++ b/client/iface/udpmux/mux.go @@ -47,6 +47,7 @@ type SingleSocketUDPMux struct { // for UDP connection listen at unspecified address localAddrsForUnspecified []net.Addr + unspecified bool } const maxAddrSize = 512 @@ -157,6 +158,9 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { if params.Logger == nil { params.Logger = getLogger() } + if params.Net == nil { + params.Net = stdnet.NewNet(context.Background(), nil, nil) + } mux := &SingleSocketUDPMux{ addressMap: map[string][]*udpMuxedConn{}, @@ -165,6 +169,7 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { connsIPv6: make(map[string]*udpMuxedConn), candidateConnMap: make(map[string]*udpMuxedConn), closedChan: make(chan struct{}, 1), + unspecified: isUnspecified(params.UDPConn.LocalAddr()), pool: &sync.Pool{ New: func() interface{} { // big enough buffer to fit both packet and address @@ -173,7 +178,6 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { }, } - mux.updateLocalAddresses() return mux } @@ -199,10 +203,6 @@ func (m *SingleSocketUDPMux) updateLocalAddresses() { m.params.Logger.Errorf("LocalAddr expected IPV4 or IPV6, got %T", m.params.UDPConn.LocalAddr()) } if len(networks) > 0 { - if m.params.Net == nil { - m.params.Net = stdnet.NewNet(context.Background(), nil, nil) - } - ips, err := localInterfaces(m.params.Net, m.params.InterfaceFilter, nil, networks, true) if err == nil { for _, ip := range ips { @@ -241,10 +241,7 @@ func (m *SingleSocketUDPMux) GetListenAddresses() []net.Addr { // creates the connection if an existing one can't be found func (m *SingleSocketUDPMux) GetConn(ufrag string, addr net.Addr, candidateID string) (net.PacketConn, error) { // don't check addr for mux using unspecified address - m.mu.Lock() - lenLocalAddrs := len(m.localAddrsForUnspecified) - m.mu.Unlock() - if lenLocalAddrs == 0 && m.params.UDPConn.LocalAddr().String() != addr.String() { + if !m.unspecified && m.params.UDPConn.LocalAddr().String() != addr.String() { return nil, fmt.Errorf("invalid address %s", addr.String()) } @@ -508,3 +505,8 @@ func getLogger() logging.LeveledLogger { //fac.Writer = log.StandardLogger().Writer() return fac.NewLogger("ice") } + +func isUnspecified(addr net.Addr) bool { + udpAddr, ok := addr.(*net.UDPAddr) + return ok && udpAddr.IP.IsUnspecified() +} diff --git a/client/iface/udpmux/mux_test.go b/client/iface/udpmux/mux_test.go new file mode 100644 index 000000000..2e2770282 --- /dev/null +++ b/client/iface/udpmux/mux_test.go @@ -0,0 +1,99 @@ +package udpmux + +import ( + "fmt" + "net" + "sync" + "sync/atomic" + "testing" + + "github.com/pion/transport/v3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type countingNet struct { + transport.Net + calls atomic.Int64 + ifaces []*transport.Interface +} + +func (n *countingNet) Interfaces() ([]*transport.Interface, error) { + n.calls.Add(1) + return n.ifaces, nil +} + +func TestNewSingleSocketUDPMux_DoesNotDiscoverInterfaces(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + n := &countingNet{ifaces: []*transport.Interface{testInterface("eth0", net.IPv4(192, 168, 1, 10))}} + + mux := NewSingleSocketUDPMux(Params{UDPConn: conn, Net: n}) + t.Cleanup(func() { _ = mux.Close() }) + + assert.Equal(t, int64(0), n.calls.Load(), "constructing the mux must not enumerate interfaces") + + addrs := mux.GetListenAddresses() + assert.Equal(t, int64(1), n.calls.Load(), "listen addresses are discovered when they are asked for") + + port := conn.LocalAddr().(*net.UDPAddr).Port + require.Len(t, addrs, 1, "one address per discovered interface IP") + assert.Equal(t, fmt.Sprintf("192.168.1.10:%d", port), addrs[0].String(), "the discovered IP is paired with the socket port") +} + +func TestSingleSocketUDPMux_GetConnWithoutListenAddresses(t *testing.T) { + t.Run("unspecified socket accepts any address", func(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + n := &countingNet{} + mux := NewSingleSocketUDPMux(Params{UDPConn: conn, Net: n}) + t.Cleanup(func() { _ = mux.Close() }) + + port := conn.LocalAddr().(*net.UDPAddr).Port + pc, err := mux.GetConn("ufrag", &net.UDPAddr{IP: net.IPv4(192, 168, 1, 10), Port: port}, "candidate") + require.NoError(t, err) + assert.NotNil(t, pc, "a connection is created for a local address of an unspecified socket") + assert.Equal(t, int64(0), n.calls.Load(), "GetConn must not depend on a prior discovery") + }) + + t.Run("specific socket rejects a foreign address", func(t *testing.T) { + conn := listenUDP(t, net.IPv4(127, 0, 0, 1)) + mux := NewSingleSocketUDPMux(Params{UDPConn: conn}) + t.Cleanup(func() { _ = mux.Close() }) + + _, err := mux.GetConn("ufrag", &net.UDPAddr{IP: net.IPv4(192, 168, 1, 10), Port: 1234}, "candidate") + assert.Error(t, err, "an address the socket is not bound to must be rejected") + + pc, err := mux.GetConn("ufrag", conn.LocalAddr(), "candidate") + require.NoError(t, err) + assert.NotNil(t, pc, "the bound address is accepted") + }) +} + +func listenUDP(t *testing.T, ip net.IP) *net.UDPConn { + t.Helper() + conn, err := net.ListenUDP("udp4", &net.UDPAddr{IP: ip, Port: 0}) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + return conn +} + +func testInterface(name string, ip net.IP) *transport.Interface { + ifc := transport.NewInterface(net.Interface{Index: 1, MTU: 1500, Name: name, Flags: net.FlagUp | net.FlagMulticast}) + ifc.AddAddress(&net.IPNet{IP: ip, Mask: net.CIDRMask(24, 32)}) + return ifc +} + +func TestSingleSocketUDPMux_ConcurrentListenAddressesWithDefaultNet(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + mux := NewSingleSocketUDPMux(Params{UDPConn: conn}) + t.Cleanup(func() { _ = mux.Close() }) + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + assert.NotEmpty(t, mux.GetListenAddresses(), "every caller gets at least the socket address") + }() + } + wg.Wait() +} diff --git a/client/internal/engine.go b/client/internal/engine.go index fdf89ae06..c4bdf5121 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -387,7 +387,7 @@ func NewEngine( mgmClient: services.MgmClient, relayManager: services.RelayManager, peerStore: peerstore.NewConnStore(), - wgDetector: stdnet.NewWGDetector(), + wgDetector: stdnet.NewWGDetector(config.WgIfaceName), syncMsgMux: &sync.Mutex{}, config: config, mobileDep: mobileDep, diff --git a/client/internal/stdnet/wgdetector.go b/client/internal/stdnet/wgdetector.go index f34e469e1..8ed290190 100644 --- a/client/internal/stdnet/wgdetector.go +++ b/client/internal/stdnet/wgdetector.go @@ -34,6 +34,8 @@ type WGDetector struct { // probe is replaced in tests; it is the call this type exists to avoid repeating. probe func(string) bool + ownIface string + mu sync.RWMutex cache map[string]wgDetectorEntry @@ -41,11 +43,12 @@ type WGDetector struct { } // NewWGDetector returns a detector with the default time to live. -func NewWGDetector() *WGDetector { +func NewWGDetector(ownIface string) *WGDetector { return &WGDetector{ - ttl: wgDetectorTTL, - probe: probeWireGuard, - cache: make(map[string]wgDetectorEntry), + ttl: wgDetectorTTL, + probe: probeWireGuard, + ownIface: ownIface, + cache: make(map[string]wgDetectorEntry), } } @@ -57,6 +60,10 @@ func (d *WGDetector) IsWireGuard(iFace string) bool { return probeWireGuard(iFace) } + if d.isOwnInterface(iFace) { + return true + } + if isWireGuard, ok := d.cached(iFace); ok { return isWireGuard } @@ -104,6 +111,10 @@ func (d *WGDetector) store(iFace string, isWireGuard bool) { d.cache[iFace] = wgDetectorEntry{isWireGuard: isWireGuard, expireAt: now.Add(d.ttl)} } +func (d *WGDetector) isOwnInterface(iFace string) bool { + return d.ownIface != "" && iFace == d.ownIface +} + func probeWireGuard(iFace string) bool { wg, err := wgctrl.New() if err != nil { diff --git a/client/internal/stdnet/wgdetector_test.go b/client/internal/stdnet/wgdetector_test.go index 68260ae59..28cf85f6a 100644 --- a/client/internal/stdnet/wgdetector_test.go +++ b/client/internal/stdnet/wgdetector_test.go @@ -150,3 +150,39 @@ func TestWGDetectorDropsExpiredEntries(t *testing.T) { assert.Len(t, d.cache, 1, "expired entries of vanished interfaces must be dropped") assert.Contains(t, d.cache, "eth0", "the fresh answer must be kept") } + +func TestWGDetectorOwnInterfaceIsWireGuardWithoutProbing(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + d.ownIface = "NetBird" + + assert.True(t, d.IsWireGuard("NetBird"), "the agent's own interface is a WireGuard device") + assert.Equal(t, int64(0), calls.Load(), "the own interface must never reach the probe") + + assert.False(t, d.IsWireGuard("NetBird2"), "only the exact own name is answered without probing") + assert.Equal(t, int64(1), calls.Load(), "a different interface is probed") +} + +func TestWGDetectorWithoutOwnInterfaceProbesEmptyName(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + + assert.False(t, d.IsWireGuard(""), "an unnamed interface is not mistaken for the own one") + assert.Equal(t, int64(1), calls.Load(), "with no own interface set every name is probed") +} + +func TestInterfaceFilterRejectsOwnInterfaceRegardlessOfDisallowList(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + d.ownIface = "NetBird" + filter := InterfaceFilter([]string{"wt"}, d) + + assert.False(t, filter("NetBird"), "the own interface is rejected even when the disallow list does not cover it") + assert.Equal(t, int64(0), calls.Load(), "the own interface is not probed") + + assert.True(t, filter("eth0"), "an ordinary interface stays available") + assert.Equal(t, int64(1), calls.Load(), "an ordinary interface is still probed") +} + +func TestNewWGDetectorKeepsTheOwnInterface(t *testing.T) { + d := NewWGDetector("NetbirdWT") + + assert.True(t, d.IsWireGuard("NetbirdWT"), "the constructor records the own interface") +}