From 057ff989fe129441ed067563a7d2b8bb3548b63e Mon Sep 17 00:00:00 2001 From: David Crawshaw Date: Fri, 29 May 2020 15:38:26 +1000 Subject: [PATCH] wgengine: ensure pingers are gone before returning from Close We canceled the pingers in Close, but didn't wait around for their goroutines to be cleaned up. This caused the ipn/e2e_test to catch pingers in its resource leak check. This commit introduces an object, but also simplifies the semantics around the pinger's cancel functions. They no longer need to be called while holding the mutex. Signed-off-by: David Crawshaw --- wgengine/userspace.go | 114 ++++++++++++++++++++++++++++-------------- 1 file changed, 76 insertions(+), 38 deletions(-) diff --git a/wgengine/userspace.go b/wgengine/userspace.go index c47fb0fa7..a19ca15e5 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -66,7 +66,7 @@ type userspaceEngine struct { statusCallback StatusCallback peerSequence []wgcfg.Key endpoints []string - pingers map[wgcfg.Key]context.CancelFunc // mu must be held to call CancelFunc + pingers map[wgcfg.Key]*pinger linkState *interfaces.State // Lock ordering: wgLock, then mu. @@ -128,7 +128,7 @@ func newUserspaceEngineAdvanced(logf logger.Logf, tundev *tstun.TUN, routerGen R reqCh: make(chan struct{}, 1), waitCh: make(chan struct{}), tundev: tundev, - pingers: make(map[wgcfg.Key]context.CancelFunc), + pingers: make(map[wgcfg.Key]*pinger), } e.linkState, _ = getLinkState() @@ -259,28 +259,29 @@ func newUserspaceEngineAdvanced(logf logger.Logf, tundev *tstun.TUN, routerGen R // // These generated packets are used to ensure we trigger the spray logic in // the magicsock package for NAT traversal. -func (e *userspaceEngine) pinger(peerKey wgcfg.Key, ips []wgcfg.IP) { - e.logf("generating initial ping traffic to %s (%v)", peerKey.ShortString(), ips) - var srcIP packet.IP +type pinger struct { + e *userspaceEngine + done chan struct{} // closed after shutdown (not the ctx.Done() chan) + cancel context.CancelFunc +} - e.wgLock.Lock() - if len(e.lastCfg.Addresses) > 0 { - srcIP = packet.NewIP(e.lastCfg.Addresses[0].IP.IP()) - } - e.wgLock.Unlock() +// close cleans up pinger and removes it from the userspaceEngine.pingers map. +// It cannot be called while p.e.mu is held. +func (p *pinger) close() { + p.cancel() + <-p.done +} - if srcIP == 0 { - e.logf("generating initial ping traffic: no source IP") - return - } +func (p *pinger) run(ctx context.Context, peerKey wgcfg.Key, ips []wgcfg.IP, srcIP packet.IP) { + defer func() { + p.e.mu.Lock() + if p.e.pingers[peerKey] == p { + delete(p.e.pingers, peerKey) + } + p.e.mu.Unlock() - e.mu.Lock() - if cancel := e.pingers[peerKey]; cancel != nil { - cancel() - } - ctx, cancel := context.WithCancel(context.Background()) - e.pingers[peerKey] = cancel - e.mu.Unlock() + close(p.done) + }() // sendFreq is slightly longer than sprayFreq in magicsock to ensure // that if these ping packets are the only source of early packets @@ -296,19 +297,6 @@ func (e *userspaceEngine) pinger(peerKey wgcfg.Key, ips []wgcfg.IP) { payload := []byte("magicsock_spray") // no meaning - defer func() { - e.mu.Lock() - defer e.mu.Unlock() - select { - case <-ctx.Done(): - return - default: - } - // If the pinger context is not done, then the - // CancelFunc is still in the pingers map. - delete(e.pingers, peerKey) - }() - ipid := uint16(1) t := time.NewTicker(sendFreq) defer t.Stop() @@ -323,10 +311,52 @@ func (e *userspaceEngine) pinger(peerKey wgcfg.Key, ips []wgcfg.IP) { } for _, dstIP := range dstIPs { b := packet.GenICMP(srcIP, dstIP, ipid, packet.ICMPEchoRequest, 0, payload) - e.tundev.InjectOutbound(b) + p.e.tundev.InjectOutbound(b) } ipid++ } + +} + +// pinger sends ping packets for a few seconds. +// +// These generated packets are used to ensure we trigger the spray logic in +// the magicsock package for NAT traversal. +func (e *userspaceEngine) pinger(peerKey wgcfg.Key, ips []wgcfg.IP) { + e.logf("generating initial ping traffic to %s (%v)", peerKey.ShortString(), ips) + var srcIP packet.IP + + e.wgLock.Lock() + if len(e.lastCfg.Addresses) > 0 { + srcIP = packet.NewIP(e.lastCfg.Addresses[0].IP.IP()) + } + e.wgLock.Unlock() + + if srcIP == 0 { + e.logf("generating initial ping traffic: no source IP") + return + } + + ctx, cancel := context.WithCancel(context.Background()) + p := &pinger{ + e: e, + done: make(chan struct{}), + cancel: cancel, + } + + e.mu.Lock() + if e.closing { + e.mu.Unlock() + return + } + oldPinger := e.pingers[peerKey] + e.pingers[peerKey] = p + e.mu.Unlock() + + if oldPinger != nil { + oldPinger.close() + } + p.run(ctx, peerKey, ips, srcIP) } func configSignature(cfg *wgcfg.Config, routerCfg *router.Config) (string, error) { @@ -565,15 +595,16 @@ func (e *userspaceEngine) RequestStatus() { } func (e *userspaceEngine) Close() { + var pingers []*pinger + e.mu.Lock() if e.closing { e.mu.Unlock() return } e.closing = true - for key, cancel := range e.pingers { - delete(e.pingers, key) - cancel() + for _, pinger := range e.pingers { + pingers = append(pingers, pinger) } e.mu.Unlock() @@ -583,6 +614,13 @@ func (e *userspaceEngine) Close() { e.linkMon.Close() e.router.Close() e.magicConn.Close() + + // Shut down pingers after tundev is closed (by e.wgdev.Close) so the + // synchronous close does not get stuck on InjectOutbound. + for _, pinger := range pingers { + pinger.close() + } + close(e.waitCh) }