wgengine/magicsock: remove locking sync requirements on conn disco keys

The lazy initialization of the disco key is not necessary, and
contributes to unnecessary locking and state checking.

Updates #cleanup

Signed-off-by: James Tucker <james@tailscale.com>
pull/7836/head
James Tucker 2 years ago committed by James Tucker
parent e1b71c83ac
commit e09c434e5d

@ -370,6 +370,15 @@ type Conn struct {
// captureHook, if non-nil, is the pcap logging callback when capturing. // captureHook, if non-nil, is the pcap logging callback when capturing.
captureHook syncs.AtomicValue[capture.Callback] captureHook syncs.AtomicValue[capture.Callback]
// discoPrivate is the private naclbox key used for active
// discovery traffic. It is always present, and immutable.
discoPrivate key.DiscoPrivate
// public of discoPrivate. It is always present and immutable.
discoPublic key.DiscoPublic
// ShortString of discoPublic (to save logging work later). It is always
// present and immutable.
discoShort string
// ============================================================ // ============================================================
// mu guards all following fields; see userspaceEngine lock // mu guards all following fields; see userspaceEngine lock
// ordering rules against the engine. For derphttp, mu must // ordering rules against the engine. For derphttp, mu must
@ -421,12 +430,6 @@ type Conn struct {
// in other maps below that are keyed by peer public key. // in other maps below that are keyed by peer public key.
peerSet map[key.NodePublic]struct{} peerSet map[key.NodePublic]struct{}
// discoPrivate is the private naclbox key used for active
// discovery traffic. It's created once near (but not during)
// construction.
discoPrivate key.DiscoPrivate
discoPublic key.DiscoPublic // public of discoPrivate
discoShort string // ShortString of discoPublic (to save logging work later)
// nodeOfDisco tracks the networkmap Node entity for each peer // nodeOfDisco tracks the networkmap Node entity for each peer
// discovery key. // discovery key.
peerMap peerMap peerMap peerMap
@ -599,13 +602,17 @@ func (o *Options) derpActiveFunc() func() {
// newConn is the error-free, network-listening-side-effect-free based // newConn is the error-free, network-listening-side-effect-free based
// of NewConn. Mostly for tests. // of NewConn. Mostly for tests.
func newConn() *Conn { func newConn() *Conn {
discoPrivate := key.NewDisco()
c := &Conn{ c := &Conn{
derpRecvCh: make(chan derpReadResult, 1), // must be buffered, see issue 3736 derpRecvCh: make(chan derpReadResult, 1), // must be buffered, see issue 3736
derpStarted: make(chan struct{}), derpStarted: make(chan struct{}),
peerLastDerp: make(map[key.NodePublic]int), peerLastDerp: make(map[key.NodePublic]int),
peerMap: newPeerMap(), peerMap: newPeerMap(),
discoInfo: make(map[key.DiscoPublic]*discoInfo), discoInfo: make(map[key.DiscoPublic]*discoInfo),
discoPrivate: discoPrivate,
discoPublic: discoPrivate.Public(),
} }
c.discoShort = c.discoPublic.ShortString()
c.bind = &connBind{Conn: c, closed: true} c.bind = &connBind{Conn: c, closed: true}
c.receiveBatchPool = sync.Pool{New: func() any { c.receiveBatchPool = sync.Pool{New: func() any {
msgs := make([]ipv6.Message, c.bind.BatchSize()) msgs := make([]ipv6.Message, c.bind.BatchSize())
@ -670,6 +677,7 @@ func NewConn(opts Options) (*Conn, error) {
c.logf("[v1] couldn't create raw v6 disco listener, using regular listener instead: %v", err) c.logf("[v1] couldn't create raw v6 disco listener, using regular listener instead: %v", err)
} }
c.logf("magicsock: disco key = %v", c.discoShort)
return c, nil return c, nil
} }
@ -1056,15 +1064,6 @@ func (c *Conn) derpRegionCodeLocked(regionID int) string {
// DiscoPublicKey returns the discovery public key. // DiscoPublicKey returns the discovery public key.
func (c *Conn) DiscoPublicKey() key.DiscoPublic { func (c *Conn) DiscoPublicKey() key.DiscoPublic {
c.mu.Lock()
defer c.mu.Unlock()
if c.discoPrivate.IsZero() {
priv := key.NewDisco()
c.discoPrivate = priv
c.discoPublic = priv.Public()
c.discoShort = c.discoPublic.ShortString()
c.logf("magicsock: disco key = %v", c.discoShort)
}
return c.discoPublic return c.discoPublic
} }
@ -2186,12 +2185,6 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netip.AddrPort, derpNodeSrc ke
// Still return true, to not pass it down to wireguard. // Still return true, to not pass it down to wireguard.
return return
} }
if c.discoPrivate.IsZero() {
if debugDisco() {
c.logf("magicsock: disco: ignoring disco-looking frame, no local key")
}
return
}
if !c.peerMap.anyEndpointForDiscoKey(sender) { if !c.peerMap.anyEndpointForDiscoKey(sender) {
metricRecvDiscoBadPeer.Add(1) metricRecvDiscoBadPeer.Add(1)

Loading…
Cancel
Save