Denote WithNetNS* functions with unsafe

Please see #2388 (comment) for more details.
weaveworks · Jul 4, 2016 · b688ac3 · b688ac3
1 parent e753413
commit b688ac3
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 14 deletions.
diff --git a/common/utils.go b/common/utils.go
@@ -48,7 +48,7 @@ func FindNetDevs(processID int, match func(link netlink.Link) bool) ([]NetDev, e
 	}
 	defer ns.Close()
 
-	err = weavenet.WithNetNS(ns, func() error {
+	err = weavenet.WithNetNSUnsafe(ns, func() error {
 		return forEachLink(func(link netlink.Link) error {
 			if match(link) {
 				netDev, err := linkToNetDev(link)
@@ -97,7 +97,7 @@ func linkToNetDev(link netlink.Link) (NetDev, error) {
 func ConnectedToBridgePredicate(bridgeName string) (func(link netlink.Link) bool, error) {
 	var br netlink.Link
 
-	err := weavenet.WithNetNSLinkByPid(1, bridgeName, func(link netlink.Link) error {
+	err := weavenet.WithNetNSLinkByPidUnsafe(1, bridgeName, func(link netlink.Link) error {
 		br = link
 		return nil
 	})
@@ -160,7 +160,7 @@ func GetWeaveNetDevs(processID int) ([]NetDev, error) {
 func GetBridgeNetDev(bridgeName string) (NetDev, error) {
 	var netdev NetDev
 
-	err := weavenet.WithNetNSLinkByPid(1, bridgeName, func(link netlink.Link) error {
+	err := weavenet.WithNetNSLinkByPidUnsafe(1, bridgeName, func(link netlink.Link) error {
 		var err error
 		netdev, err = linkToNetDev(link)
 		if err != nil {

diff --git a/net/netns.go b/net/netns.go
@@ -7,7 +7,19 @@ import (
 	"github.com/vishvananda/netns"
 )
 
-func WithNetNS(ns netns.NsHandle, work func() error) error {
+// NB: The following function is unsafe, because:
+//     - It changes a network namespace (netns) of an OS thread which runs
+//       the function. During execution, the Go runtime might clone a new OS thread
+//       for scheduling other go-routines, thus they might end up running in
+//       a "wrong" netns.
+//     - runtime.LockOSThread does not guarantee that a spawned go-routine on
+//       the locked thread will be run by it. Thus, the work function is
+//       not allowed to spawn any go-routine which is dependent on the given netns.
+
+//     Please see https://github.com/weaveworks/weave/issues/2388#issuecomment-228365069
+//     for more details and make sure that you understand the implications before
+//     using the function!
+func WithNetNSUnsafe(ns netns.NsHandle, work func() error) error {
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
@@ -26,8 +38,8 @@ func WithNetNS(ns netns.NsHandle, work func() error) error {
 	return err
 }
 
-func WithNetNSLink(ns netns.NsHandle, ifName string, work func(link netlink.Link) error) error {
-	return WithNetNS(ns, func() error {
+func WithNetNSLinkUnsafe(ns netns.NsHandle, ifName string, work func(link netlink.Link) error) error {
+	return WithNetNSUnsafe(ns, func() error {
 		link, err := netlink.LinkByName(ifName)
 		if err != nil {
 			return err
@@ -36,12 +48,12 @@ func WithNetNSLink(ns netns.NsHandle, ifName string, work func(link netlink.Link
 	})
 }
 
-func WithNetNSLinkByPid(pid int, ifName string, work func(link netlink.Link) error) error {
+func WithNetNSLinkByPidUnsafe(pid int, ifName string, work func(link netlink.Link) error) error {
 	ns, err := netns.GetFromPid(pid)
 	if err != nil {
 		return err
 	}
 	defer ns.Close()
 
-	return WithNetNSLink(ns, ifName, work)
+	return WithNetNSLinkUnsafe(ns, ifName, work)
 }
diff --git a/net/veth.go b/net/veth.go
@@ -104,7 +104,7 @@ const (
 )
 
 func interfaceExistsInNamespace(ns netns.NsHandle, ifName string) bool {
-	err := WithNetNS(ns, func() error {
+	err := WithNetNSUnsafe(ns, func() error {
 		_, err := netlink.LinkByName(ifName)
 		return err
 	})
@@ -122,7 +122,7 @@ func AttachContainer(ns netns.NsHandle, id, ifName, bridgeName string, mtu int,
 			if err := netlink.LinkSetNsFd(veth, int(ns)); err != nil {
 				return fmt.Errorf("failed to move veth to container netns: %s", err)
 			}
-			if err := WithNetNS(ns, func() error {
+			if err := WithNetNSUnsafe(ns, func() error {
 				if err := netlink.LinkSetName(veth, ifName); err != nil {
 					return err
 				}
@@ -140,7 +140,7 @@ func AttachContainer(ns netns.NsHandle, id, ifName, bridgeName string, mtu int,
 		}
 	}
 
-	if err := WithNetNSLink(ns, ifName, func(veth netlink.Link) error {
+	if err := WithNetNSLinkUnsafe(ns, ifName, func(veth netlink.Link) error {
 		newAddresses, err := AddAddresses(veth, cidrs)
 		if err != nil {
 			return err
@@ -177,7 +177,7 @@ func AttachContainer(ns netns.NsHandle, id, ifName, bridgeName string, mtu int,
 }
 
 func DetachContainer(ns netns.NsHandle, id, ifName string, cidrs []*net.IPNet) error {
-	return WithNetNSLink(ns, ifName, func(veth netlink.Link) error {
+	return WithNetNSLinkUnsafe(ns, ifName, func(veth netlink.Link) error {
 		existingAddrs, err := netlink.AddrList(veth, netlink.FAMILY_V4)
 		if err != nil {
 			return fmt.Errorf("failed to get IP address for %q: %v", veth.Attrs().Name, err)

diff --git a/plugin/net/cni.go b/plugin/net/cni.go
@@ -95,7 +95,7 @@ func (c *CNIPlugin) CmdAdd(args *skel.CmdArgs) error {
 	if err := weavenet.AttachContainer(ns, id, args.IfName, conf.BrName, conf.MTU, false, []*net.IPNet{&result.IP4.IP}, false); err != nil {
 		return err
 	}
-	if err := weavenet.WithNetNSLink(ns, args.IfName, func(link netlink.Link) error {
+	if err := weavenet.WithNetNSLinkUnsafe(ns, args.IfName, func(link netlink.Link) error {
 		return setupRoutes(link, args.IfName, result.IP4.IP, result.IP4.Gateway, result.IP4.Routes)
 	}); err != nil {
 		return fmt.Errorf("error setting up routes: %s", err)
@@ -158,7 +158,7 @@ func (c *CNIPlugin) CmdDel(args *skel.CmdArgs) error {
 		return err
 	}
 	defer ns.Close()
-	err = weavenet.WithNetNS(ns, func() error {
+	err = weavenet.WithNetNSUnsafe(ns, func() error {
 		link, err := netlink.LinkByName(args.IfName)
 		if err != nil {
 			return err