Fix "weave ps"

common/utils.go

@@ -48,14 +48,14 @@ 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)
 				if err != nil {
 					return err
 				}
-				netDevs = append(netDevs, *netDev)
+				netDevs = append(netDevs, netDev)
 			}
 			return nil
 		})
@@ -77,21 +77,54 @@ func forEachLink(f func(netlink.Link) error) error {
 	return nil
 }
 
-func linkToNetDev(link netlink.Link) (*NetDev, error) {
+func linkToNetDev(link netlink.Link) (NetDev, error) {
 	addrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
 	if err != nil {
-		return nil, err
+		return NetDev{}, err
 	}
 
-	netDev := &NetDev{Name: link.Attrs().Name, MAC: link.Attrs().HardwareAddr}
+	netDev := NetDev{Name: link.Attrs().Name, MAC: link.Attrs().HardwareAddr}
 	for _, addr := range addrs {
 		netDev.CIDRs = append(netDev.CIDRs, addr.IPNet)
 	}
 	return netDev, nil
 }
 
-// Lookup the weave interface of a container
-func GetWeaveNetDevs(processID int) ([]NetDev, error) {
+// ConnectedToBridgePredicate returns a function which is used to query whether
+// a given link is a veth interface which one end is connected to a bridge.
+// The returned function should be called from a container network namespace which
+// the bridge does NOT belong to.
+func ConnectedToBridgePredicate(bridgeName string) (func(link netlink.Link) bool, error) {
+	indexes := make(map[int]struct{})
+
+	// Scan devices in root namespace to find those attached to weave bridge
+	err := weavenet.WithNetNSLinkByPidUnsafe(1, bridgeName,
+		func(br netlink.Link) error {
+			return forEachLink(func(link netlink.Link) error {
+				if link.Attrs().MasterIndex == br.Attrs().Index {
+					peerIndex := link.Attrs().ParentIndex
+					if peerIndex == 0 {
+						// perhaps running on an older kernel where ParentIndex doesn't work.
+						// as fall-back, assume the indexes are consecutive
+						peerIndex = link.Attrs().Index - 1
+					}
+					indexes[peerIndex] = struct{}{}
+				}
+				return nil
+			})
+		})
+	if err != nil {
+		return nil, err
+	}
+
+	return func(link netlink.Link) bool {
+		_, isveth := link.(*netlink.Veth)
+		_, found := indexes[link.Attrs().Index]
+		return isveth && found
+	}, nil
+}
+
+func GetNetDevsWithPredicate(processID int, predicate func(link netlink.Link) bool) ([]NetDev, error) {
 	// Bail out if this container is running in the root namespace
 	nsToplevel, err := netns.GetFromPid(1)
 	if err != nil {
@@ -105,39 +138,28 @@ func GetWeaveNetDevs(processID int) ([]NetDev, error) {
 		return nil, nil
 	}
 
-	weaveBridge, err := netlink.LinkByName("weave")
-	if err != nil {
-		return nil, fmt.Errorf("Cannot find weave bridge: %s", err)
-	}
-	// Scan devices in root namespace to find those attached to weave bridge
-	indexes := make(map[int]struct{})
-	err = forEachLink(func(link netlink.Link) error {
-		if link.Attrs().MasterIndex == weaveBridge.Attrs().Index {
-			peerIndex := link.Attrs().ParentIndex
-			if peerIndex == 0 {
-				// perhaps running on an older kernel where ParentIndex doesn't work.
-				// as fall-back, assume the indexes are consecutive
-				peerIndex = link.Attrs().Index - 1
-			}
-			indexes[peerIndex] = struct{}{}
-		}
-		return nil
-	})
+	return FindNetDevs(processID, predicate)
+}
+
+// Lookup the weave interface of a container
+func GetWeaveNetDevs(processID int) ([]NetDev, error) {
+	p, err := ConnectedToBridgePredicate("weave")
 	if err != nil {
 		return nil, err
 	}
-	return FindNetDevs(processID, func(link netlink.Link) bool {
-		_, isveth := link.(*netlink.Veth)
-		_, found := indexes[link.Attrs().Index]
-		return isveth && found
-	})
+
+	return GetNetDevsWithPredicate(processID, p)
 }
 
-// Get the weave bridge interface
-func GetBridgeNetDev(bridgeName string) ([]NetDev, error) {
-	return FindNetDevs(1, func(link netlink.Link) bool {
-		return link.Attrs().Name == bridgeName
-	})
+// Get the weave bridge interface.
+// NB: Should be called from the root network namespace.
+func GetBridgeNetDev(bridgeName string) (NetDev, error) {
+	link, err := netlink.LinkByName(bridgeName)
+	if err != nil {
+		return NetDev{}, err
+	}
+
+	return linkToNetDev(link)
 }
 
 // Do post-attach configuration of all veths we have created

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,12 +38,22 @@ 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
 		}
 		return work(link)
 	})
 }
+
+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 WithNetNSLinkUnsafe(ns, ifName, work)
+}

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)

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)
@@ -131,23 +131,20 @@ func setupRoutes(link netlink.Link, name string, ipnet net.IPNet, gw net.IP, rou
 }
 
 func findBridgeIP(bridgeName string, subnet net.IPNet) (net.IP, error) {
-	netdevs, err := common.GetBridgeNetDev(bridgeName)
+	netdev, err := common.GetBridgeNetDev(bridgeName)
 	if err != nil {
 		return nil, fmt.Errorf("Failed to get netdev for %q bridge: %s", bridgeName, err)
 	}
-	if len(netdevs) == 0 {
-		return nil, fmt.Errorf("Could not find %q bridge", bridgeName)
-	}
-	if len(netdevs[0].CIDRs) == 0 {
+	if len(netdev.CIDRs) == 0 {
 		return nil, fmt.Errorf("Bridge %q has no IP addresses; did you forget to run 'weave expose'?", bridgeName)
 	}
-	for _, cidr := range netdevs[0].CIDRs {
+	for _, cidr := range netdev.CIDRs {
 		if subnet.Contains(cidr.IP) {
 			return cidr.IP, nil
 		}
 	}
 	// None in the required subnet; just return the first one
-	return netdevs[0].CIDRs[0].IP, nil
+	return netdev.CIDRs[0].IP, nil
 }
 
 func (c *CNIPlugin) CmdDel(args *skel.CmdArgs) error {
@@ -161,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

prog/weaveutil/addrs.go

@@ -2,7 +2,9 @@ package main
 
 import (
 	"fmt"
+
 	"github.com/fsouza/go-dockerclient"
+	"github.com/vishvananda/netlink"
 	"github.com/weaveworks/weave/common"
 )
 
@@ -12,46 +14,66 @@ func containerAddrs(args []string) error {
 	}
 	bridgeName := args[0]
 
-	c, err := docker.NewVersionedClientFromEnv("1.18")
+	client, err := docker.NewVersionedClientFromEnv("1.18")
 	if err != nil {
 		return err
 	}
 
-	for _, containerID := range args[1:] {
-		netDevs, err := getNetDevs(bridgeName, c, containerID)
-		if err != nil {
+	pred, err := common.ConnectedToBridgePredicate(bridgeName)
+	if err != nil {
+		return err
+	}
+
+	var containerIDs []string
+	containers := make(map[string]*docker.Container)
+
+	for _, cid := range args[1:] {
+		if cid == "weave:expose" {
+			netDev, err := common.GetBridgeNetDev(bridgeName)
+			if err != nil {
+				return err
+			}
+			printNetDevs(cid, []common.NetDev{netDev})
+			continue
+		}
+		if containers[cid], err = client.InspectContainer(cid); err != nil {
 			if _, ok := err.(*docker.NoSuchContainer); ok {
 				continue
 			}
 			return err
 		}
+		// To output in the right order, we keep the list of container IDs
+		containerIDs = append(containerIDs, cid)
+	}
 
-		for _, netDev := range netDevs {
-			fmt.Printf("%12s %s %s", containerID, netDev.Name, netDev.MAC.String())
-			for _, cidr := range netDev.CIDRs {
-				prefixLength, _ := cidr.Mask.Size()
-				fmt.Printf(" %v/%v", cidr.IP, prefixLength)
-			}
-			fmt.Println()
+	// NB: Because network namespaces (netns) are changed many times inside the loop,
+	//     it's NOT safe to exec any code depending on the root netns without
+	//     wrapping with WithNetNS*.
+	for _, cid := range containerIDs {
+		netDevs, err := getNetDevs(client, containers[cid], pred)
+		if err != nil {
+			return err
 		}
+		printNetDevs(cid, netDevs)
 	}
 
 	return nil
 }
 
-func getNetDevs(bridgeName string, c *docker.Client, containerID string) ([]common.NetDev, error) {
-	if containerID == "weave:expose" {
-		return common.GetBridgeNetDev(bridgeName)
-	}
-
-	container, err := c.InspectContainer(containerID)
-	if err != nil {
-		return nil, err
-	}
-
+func getNetDevs(c *docker.Client, container *docker.Container, pred func(netlink.Link) bool) ([]common.NetDev, error) {
 	if container.State.Pid == 0 {
 		return nil, nil
 	}
+	return common.GetNetDevsWithPredicate(container.State.Pid, pred)
+}
 
-	return common.GetWeaveNetDevs(container.State.Pid)
+func printNetDevs(cid string, netDevs []common.NetDev) {
+	for _, netDev := range netDevs {
+		fmt.Printf("%12s %s %s", cid, netDev.Name, netDev.MAC.String())
+		for _, cidr := range netDev.CIDRs {
+			prefixLength, _ := cidr.Mask.Size()
+			fmt.Printf(" %v/%v", cidr.IP, prefixLength)
+		}
+		fmt.Println()
+	}
 }

proxy/proxy.go

@@ -149,14 +149,14 @@ func NewProxy(c Config) (*Proxy, error) {
 	p.client = client.Client
 
 	if !p.WithoutDNS {
-		netDevs, err := common.GetBridgeNetDev(c.DockerBridge)
+		netDev, err := common.GetBridgeNetDev(c.DockerBridge)
 		if err != nil {
 			return nil, err
 		}
-		if len(netDevs) != 1 || len(netDevs[0].CIDRs) != 1 {
+		if len(netDev.CIDRs) != 1 {
 			return nil, fmt.Errorf("Could not obtain address of %s", c.DockerBridge)
 		}
-		p.dockerBridgeIP = netDevs[0].CIDRs[0].IP.String()
+		p.dockerBridgeIP = netDev.CIDRs[0].IP.String()
 	}
 
 	p.hostnameMatchRegexp, err = regexp.Compile(c.HostnameMatch)