From 727c0d6cfde3a86ec99200993ff06db7630c55a2 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Tue, 7 May 2024 20:43:00 -0700
Subject: [PATCH] ipn/ipnserver: close a small race in ipnserver, ~simplify
 code

There was a small window in ipnserver after we assigned a LocalBackend
to the ipnserver's atomic but before we Start'ed it where our
initalization Start could conflict with API calls from the LocalAPI.

Simplify that a bit and lay out the rules in the docs.

Updates #12028

Change-Id: Ic5f5e4861e26340599184e20e308e709edec68b1
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 cmd/tailscaled/tailscaled.go | 10 ++++++++++
 ipn/ipnserver/server.go      | 25 ++-----------------------
 2 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/cmd/tailscaled/tailscaled.go b/cmd/tailscaled/tailscaled.go
index 17faeaab4..a1b9499d9 100644
--- a/cmd/tailscaled/tailscaled.go
+++ b/cmd/tailscaled/tailscaled.go
@@ -35,6 +35,7 @@ import (
 	"tailscale.com/control/controlclient"
 	"tailscale.com/drive/driveimpl"
 	"tailscale.com/envknob"
+	"tailscale.com/ipn"
 	"tailscale.com/ipn/conffile"
 	"tailscale.com/ipn/ipnlocal"
 	"tailscale.com/ipn/ipnserver"
@@ -480,6 +481,15 @@ func startIPNServer(ctx context.Context, logf logger.Logf, logID logid.PublicID,
 		lb, err := getLocalBackend(ctx, logf, logID, sys)
 		if err == nil {
 			logf("got LocalBackend in %v", time.Since(t0).Round(time.Millisecond))
+			if lb.Prefs().Valid() {
+				if err := lb.Start(ipn.Options{}); err != nil {
+					logf("LocalBackend.Start: %v", err)
+					lb.Shutdown()
+					lbErr.Store(err)
+					cancel()
+					return
+				}
+			}
 			srv.SetLocalBackend(lb)
 			close(wgEngineCreated)
 			return
diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go
index 817d4cabf..b0420773d 100644
--- a/ipn/ipnserver/server.go
+++ b/ipn/ipnserver/server.go
@@ -46,9 +46,6 @@ type Server struct {
 	// is true, the ForceDaemon pref can override this.
 	resetOnZero bool
 
-	startBackendOnce sync.Once
-	runCalled        atomic.Bool
-
 	// mu guards the fields that follow.
 	// lock order: mu, then LocalBackend.mu
 	mu            sync.Mutex
@@ -471,16 +468,15 @@ func New(logf logger.Logf, logID logid.PublicID, netMon *netmon.Monitor) *Server
 
 // SetLocalBackend sets the server's LocalBackend.
 //
-// If b.Run has already been called, then lb.Start will be called.
-// Otherwise Start will be called once Run is called.
+// It should only call be called after calling lb.Start.
 func (s *Server) SetLocalBackend(lb *ipnlocal.LocalBackend) {
 	if lb == nil {
 		panic("nil LocalBackend")
 	}
+
 	if !s.lb.CompareAndSwap(nil, lb) {
 		panic("already set")
 	}
-	s.startBackendIfNeeded()
 
 	s.mu.Lock()
 	s.backendWaiter.wakeAll()
@@ -490,21 +486,6 @@ func (s *Server) SetLocalBackend(lb *ipnlocal.LocalBackend) {
 	// https://github.com/tailscale/tailscale/issues/6522
 }
 
-func (b *Server) startBackendIfNeeded() {
-	if !b.runCalled.Load() {
-		return
-	}
-	lb := b.lb.Load()
-	if lb == nil {
-		return
-	}
-	if lb.Prefs().Valid() {
-		b.startBackendOnce.Do(func() {
-			lb.Start(ipn.Options{})
-		})
-	}
-}
-
 // connIdentityContextKey is the http.Request.Context's context.Value key for either an
 // *ipnauth.ConnIdentity or an error.
 type connIdentityContextKey struct{}
@@ -517,7 +498,6 @@ type connIdentityContextKey struct{}
 // If the Server's LocalBackend has already been set, Run starts it.
 // Otherwise, the next call to SetLocalBackend will start it.
 func (s *Server) Run(ctx context.Context, ln net.Listener) error {
-	s.runCalled.Store(true)
 	defer func() {
 		if lb := s.lb.Load(); lb != nil {
 			lb.Shutdown()
@@ -537,7 +517,6 @@ func (s *Server) Run(ctx context.Context, ln net.Listener) error {
 		ln.Close()
 	}()
 
-	s.startBackendIfNeeded()
 	systemd.Ready()
 
 	hs := &http.Server{