libnetwork/osl: clean up Linux InvokeFunc()

Aside from unconditionally unlocking the OS thread even if restoring the
thread's network namespace fails, func (*networkNamespace).InvokeFunc()
correctly implements invoking a function inside a network namespace.
This is far from obvious, however. func InitOSContext() does much of the
heavy lifting but in a bizarre fashion: it restores the initial network
namespace before it is changed in the first place, and the cleanup
function it returns does not restore the network namespace at all! The
InvokeFunc() implementation has to restore the network namespace
explicitly by deferring a call to ns.SetNamespace().

func InitOSContext() is a leaky abstraction taped to a footgun. On the
one hand, it defensively resets the current thread's network namespace,
which has the potential to fix up the thread state if other buggy code
had failed to maintain the invariant that an OS thread must be locked to
a goroutine unless it is interchangeable with a "clean" thread as
spawned by the Go runtime. On the other hand, it _facilitates_ writing
buggy code which fails to maintain the aforementioned invariant because
the cleanup function it returns unlocks the thread from the goroutine
unconditionally while neglecting to restore the thread's network
namespace! It is quite scary to need a function which fixes up threads'
network namespaces after the fact as an arbitrary number of goroutines
could have been scheduled onto a "dirty" thread and run non-libnetwork
code before the thread's namespace is fixed up. Any number of
(not-so-)subtle misbehaviours could result if an unfortunate goroutine
is scheduled onto a "dirty" thread. The whole repository has been
audited to ensure that the aforementioned invariant is never violated,
making after-the-fact fixing up of thread network namespaces redundant.
Make InitOSContext() a no-op on Linux and inline the thread-locking into
the function (singular) which previously relied on it to do so.

func ns.SetNamespace() is of similarly dubious utility. It intermixes
capturing the initial network namespace and restoring the thread's
network namespace, which could result in threads getting put into the
wrong network namespace if the wrong thread is the first to call it.
Delete it entirely; functions which need to manipulate a thread's
network namespace are better served by being explicit about capturing
and restoring the thread's namespace.

Rewrite InvokeFunc() to invoke the closure inside a goroutine to enable
a graceful and safe recovery if the thread's network namespace could not
be restored. Avoid any potential race conditions due to changing the
main thread's network namespace by preventing the aforementioned
goroutines from being eligible to be scheduled onto the main thread.

Signed-off-by: Cory Snider <csnider@mirantis.com>

libnetwork/ns/init_linux.go

@@ -2,7 +2,6 @@ package ns
 
 import (
 	"fmt"
-	"os"
 	"os/exec"
 	"strings"
 	"sync"
@@ -39,19 +38,6 @@ func Init() {
 	}
 }
 
-// SetNamespace sets the initial namespace handler
-func SetNamespace() error {
-	initOnce.Do(Init)
-	if err := netns.Set(initNs); err != nil {
-		linkInfo, linkErr := getLink()
-		if linkErr != nil {
-			linkInfo = linkErr.Error()
-		}
-		return fmt.Errorf("failed to set to initial namespace, %v, initns fd %d: %v", linkInfo, initNs, err)
-	}
-	return nil
-}
-
 // ParseHandlerInt transforms the namespace handler into an integer
 func ParseHandlerInt() int {
 	return int(getHandler())
@@ -63,10 +49,6 @@ func getHandler() netns.NsHandle {
 	return initNs
 }
 
-func getLink() (string, error) {
-	return os.Readlink(fmt.Sprintf("/proc/%d/task/%d/ns/net", os.Getpid(), syscall.Gettid()))
-}
-
 // NlHandle returns the netlink handler
 func NlHandle() *netlink.Handle {
 	initOnce.Do(Init)

libnetwork/osl/namespace_linux.go

@@ -28,6 +28,14 @@ const defaultPrefix = "/var/run/docker"
 
 func init() {
 	reexec.Register("set-ipv6", reexecSetIPv6)
+
+	// Lock main() to the initial thread to exclude the goroutines spawned
+	// by func (*networkNamespace) InvokeFunc() from being scheduled onto
+	// that thread. Changes to the network namespace of the initial thread
+	// alter /proc/self/ns/net, which would break any code which
+	// (incorrectly) assumes that that file is a handle to the network
+	// namespace for the thread it is currently executing on.
+	runtime.LockOSThread()
 }
 
 var (
@@ -411,44 +419,47 @@ func (n *networkNamespace) DisableARPForVIP(srcName string) (Err error) {
 	return
 }
 
-func (n *networkNamespace) InvokeFunc(f func()) error {
-	return nsInvoke(n.nsPath(), func(nsFD int) error { return nil }, func(callerFD int) error {
-		f()
-		return nil
-	})
-}
-
 // InitOSContext initializes OS context while configuring network resources
 func InitOSContext() func() {
-	runtime.LockOSThread()
-	if err := ns.SetNamespace(); err != nil {
-		logrus.Error(err)
-	}
-	return runtime.UnlockOSThread
+	return func() {}
 }
 
-func nsInvoke(path string, prefunc func(nsFD int) error, postfunc func(callerFD int) error) error {
-	defer InitOSContext()()
-
-	newNs, err := netns.GetFromPath(path)
+func (n *networkNamespace) InvokeFunc(f func()) error {
+	origNS, err := netns.Get()
 	if err != nil {
-		return fmt.Errorf("failed get network namespace %q: %v", path, err)
+		return fmt.Errorf("failed to get original network namespace: %w", err)
 	}
-	defer newNs.Close()
+	defer origNS.Close()
 
-	// Invoked before the namespace switch happens but after the namespace file
-	// handle is obtained.
-	if err := prefunc(int(newNs)); err != nil {
-		return fmt.Errorf("failed in prefunc: %v", err)
+	path := n.nsPath()
+	newNS, err := netns.GetFromPath(path)
+	if err != nil {
+		return fmt.Errorf("failed get network namespace %q: %w", path, err)
 	}
+	defer newNS.Close()
 
-	if err = netns.Set(newNs); err != nil {
-		return err
-	}
-	defer ns.SetNamespace()
-
-	// Invoked after the namespace switch.
-	return postfunc(ns.ParseHandlerInt())
+	done := make(chan error, 1)
+	go func() {
+		runtime.LockOSThread()
+		if err := netns.Set(newNS); err != nil {
+			runtime.UnlockOSThread()
+			done <- err
+			return
+		}
+		defer func() {
+			close(done)
+			if err := netns.Set(origNS); err != nil {
+				logrus.WithError(err).Warn("failed to restore thread's network namespace")
+				// Recover from the error by leaving this goroutine locked to
+				// the thread. The runtime will terminate the thread and replace
+				// it with a clean one when this goroutine returns.
+			} else {
+				runtime.UnlockOSThread()
+			}
+		}()
+		f()
+	}()
+	return <-done
 }
 
 func (n *networkNamespace) nsPath() string {