From 334b605e726db70fdba2740ba2b48fd72490cc44 Mon Sep 17 00:00:00 2001 From: Jordan Whited Date: Thu, 23 Mar 2023 16:57:21 -0700 Subject: [PATCH] conn: fix StdNetEndpoint data race by dynamically allocating endpoints In 9e2f386 ("conn, device, tun: implement vectorized I/O on Linux"), the Linux-specific Bind implementation was collapsed into StdNetBind. This introduced a race on StdNetEndpoint from getSrcFromControl() and setSrcControl(). Remove the sync.Pool involved in the race, and simplify StdNetBind's receive path to allocate StdNetEndpoint on the heap instead, with the intent for it to be cleaned up by the GC, later. This essentially reverts ef5c587 ("conn: remove the final alloc per packet receive"), adding back that allocation, unfortunately. This does slightly increase resident memory usage in higher throughput scenarios. StdNetBind is the only Bind implementation that was using this Endpoint recycling technique prior to this commit. This is considered a stop-gap solution, and there are plans to replace the allocation with a better mechanism. Reported-by: lsc Link: https://lore.kernel.org/wireguard/ac87f86f-6837-4e0e-ec34-1df35f52540e@lv6.tw/ Fixes: 9e2f386 ("conn, device, tun: implement vectorized I/O on Linux") Cc: Josh Bleecher Snyder Reviewed-by: James Tucker Signed-off-by: Jordan Whited Signed-off-by: Jason A. Donenfeld --- conn/bind_std.go | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/conn/bind_std.go b/conn/bind_std.go index e8cf7b8..491e571 100644 --- a/conn/bind_std.go +++ b/conn/bind_std.go @@ -93,7 +93,12 @@ var ( func (*StdNetBind) ParseEndpoint(s string) (Endpoint, error) { e, err := netip.ParseAddrPort(s) - return asEndpoint(e), err + if err != nil { + return nil, err + } + return &StdNetEndpoint{ + AddrPort: e, + }, nil } func (e *StdNetEndpoint) ClearSrc() { @@ -228,7 +233,7 @@ func (s *StdNetBind) makeReceiveIPv4(pc *ipv4.PacketConn, conn *net.UDPConn) Rec msg := &(*msgs)[i] sizes[i] = msg.N addrPort := msg.Addr.(*net.UDPAddr).AddrPort() - ep := asEndpoint(addrPort) + ep := &StdNetEndpoint{AddrPort: addrPort} // TODO: remove allocation getSrcFromControl(msg.OOB[:msg.NN], ep) eps[i] = ep } @@ -261,7 +266,7 @@ func (s *StdNetBind) makeReceiveIPv6(pc *ipv6.PacketConn, conn *net.UDPConn) Rec msg := &(*msgs)[i] sizes[i] = msg.N addrPort := msg.Addr.(*net.UDPAddr).AddrPort() - ep := asEndpoint(addrPort) + ep := &StdNetEndpoint{AddrPort: addrPort} // TODO: remove allocation getSrcFromControl(msg.OOB[:msg.NN], ep) eps[i] = ep } @@ -408,24 +413,3 @@ func (s *StdNetBind) send6(conn *net.UDPConn, pc *ipv6.PacketConn, ep Endpoint, s.ipv6MsgsPool.Put(msgs) return err } - -// endpointPool contains a re-usable set of mapping from netip.AddrPort to Endpoint. -// This exists to reduce allocations: Putting a netip.AddrPort in an Endpoint allocates, -// but Endpoints are immutable, so we can re-use them. -var endpointPool = sync.Pool{ - New: func() any { - return make(map[netip.AddrPort]*StdNetEndpoint) - }, -} - -// asEndpoint returns an Endpoint containing ap. -func asEndpoint(ap netip.AddrPort) *StdNetEndpoint { - m := endpointPool.Get().(map[netip.AddrPort]*StdNetEndpoint) - defer endpointPool.Put(m) - e, ok := m[ap] - if !ok { - e = &StdNetEndpoint{AddrPort: ap} - m[ap] = e - } - return e -}