Skip to content

Commit 334b605

Browse files
jwhitedzx2c4
authored andcommitted
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 <[email protected]> Link: https://lore.kernel.org/wireguard/[email protected]/ Fixes: 9e2f386 ("conn, device, tun: implement vectorized I/O on Linux") Cc: Josh Bleecher Snyder <[email protected]> Reviewed-by: James Tucker <[email protected]> Signed-off-by: Jordan Whited <[email protected]> Signed-off-by: Jason A. Donenfeld <[email protected]>
1 parent 3a9e753 commit 334b605

File tree

1 file changed

+8
-24
lines changed

1 file changed

+8
-24
lines changed

conn/bind_std.go

Lines changed: 8 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,12 @@ var (
9393

9494
func (*StdNetBind) ParseEndpoint(s string) (Endpoint, error) {
9595
e, err := netip.ParseAddrPort(s)
96-
return asEndpoint(e), err
96+
if err != nil {
97+
return nil, err
98+
}
99+
return &StdNetEndpoint{
100+
AddrPort: e,
101+
}, nil
97102
}
98103

99104
func (e *StdNetEndpoint) ClearSrc() {
@@ -228,7 +233,7 @@ func (s *StdNetBind) makeReceiveIPv4(pc *ipv4.PacketConn, conn *net.UDPConn) Rec
228233
msg := &(*msgs)[i]
229234
sizes[i] = msg.N
230235
addrPort := msg.Addr.(*net.UDPAddr).AddrPort()
231-
ep := asEndpoint(addrPort)
236+
ep := &StdNetEndpoint{AddrPort: addrPort} // TODO: remove allocation
232237
getSrcFromControl(msg.OOB[:msg.NN], ep)
233238
eps[i] = ep
234239
}
@@ -261,7 +266,7 @@ func (s *StdNetBind) makeReceiveIPv6(pc *ipv6.PacketConn, conn *net.UDPConn) Rec
261266
msg := &(*msgs)[i]
262267
sizes[i] = msg.N
263268
addrPort := msg.Addr.(*net.UDPAddr).AddrPort()
264-
ep := asEndpoint(addrPort)
269+
ep := &StdNetEndpoint{AddrPort: addrPort} // TODO: remove allocation
265270
getSrcFromControl(msg.OOB[:msg.NN], ep)
266271
eps[i] = ep
267272
}
@@ -408,24 +413,3 @@ func (s *StdNetBind) send6(conn *net.UDPConn, pc *ipv6.PacketConn, ep Endpoint,
408413
s.ipv6MsgsPool.Put(msgs)
409414
return err
410415
}
411-
412-
// endpointPool contains a re-usable set of mapping from netip.AddrPort to Endpoint.
413-
// This exists to reduce allocations: Putting a netip.AddrPort in an Endpoint allocates,
414-
// but Endpoints are immutable, so we can re-use them.
415-
var endpointPool = sync.Pool{
416-
New: func() any {
417-
return make(map[netip.AddrPort]*StdNetEndpoint)
418-
},
419-
}
420-
421-
// asEndpoint returns an Endpoint containing ap.
422-
func asEndpoint(ap netip.AddrPort) *StdNetEndpoint {
423-
m := endpointPool.Get().(map[netip.AddrPort]*StdNetEndpoint)
424-
defer endpointPool.Put(m)
425-
e, ok := m[ap]
426-
if !ok {
427-
e = &StdNetEndpoint{AddrPort: ap}
428-
m[ap] = e
429-
}
430-
return e
431-
}

0 commit comments

Comments
 (0)