Use safe version of WithNetNS

net/netns.go

@@ -57,16 +57,6 @@ func WithNetNSLinkUnsafe(ns netns.NsHandle, ifName string, work func(link netlin
 	})
 }
 
-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)
-}
-
 // A safe version of WithNetNS* which creates a process executing
 // "nsenter --net=<ns-path> weaveutil <cmd> [args]".
 func WithNetNS(nsPath string, cmd string, args ...string) (string, error) {

net/veth.go

@@ -105,21 +105,26 @@ const (
 	vethPrefix = "v" + VethName // starts with "veth" to suppress UI notifications
 )
 
-func interfaceExistsInNamespace(ns netns.NsHandle, ifName string) bool {
-	err := WithNetNSUnsafe(ns, func() error {
-		_, err := netlink.LinkByName(ifName)
-		return err
-	})
+func interfaceExistsInNamespace(netNSPath string, ifName string) bool {
+	_, err := WithNetNS(netNSPath, "check-iface", ifName)
 	return err == nil
 }
 
-func AttachContainer(ns netns.NsHandle, id, ifName, bridgeName string, mtu int, withMulticastRoute bool, cidrs []*net.IPNet, keepTXOn bool) error {
+// NB: This function can be used only by a process that terminates immediately
+//     after calling the function as it changes netns via WithNetNSLinkUnsafe.
+func AttachContainer(netNSPath, id, ifName, bridgeName string, mtu int, withMulticastRoute bool, cidrs []*net.IPNet, keepTXOn bool) error {
+	ns, err := netns.GetFromPath(netNSPath)
+	if err != nil {
+		return err
+	}
+	defer ns.Close()
+
 	ipt, err := iptables.New()
 	if err != nil {
 		return err
 	}
 
-	if !interfaceExistsInNamespace(ns, ifName) {
+	if !interfaceExistsInNamespace(netNSPath, ifName) {
 		maxIDLen := IFNAMSIZ - 1 - len(vethPrefix+"pl")
 		if len(id) > maxIDLen {
 			id = id[:maxIDLen] // trim passed ID if too long
@@ -129,18 +134,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 := WithNetNSUnsafe(ns, func() error {
-				if err := netlink.LinkSetName(veth, ifName); err != nil {
-					return err
-				}
-				if err := ConfigureARPCache(ifName); err != nil {
-					return err
-				}
-				if err := ipt.Append("filter", "INPUT", "-i", ifName, "-d", "224.0.0.0/4", "-j", "DROP"); err != nil {
-					return err
-				}
-				return nil
-			}); err != nil {
+			if _, err := WithNetNS(netNSPath, "setup-iface", peerName, ifName); err != nil {
 				return fmt.Errorf("error setting up interface: %s", err)
 			}
 			return nil
@@ -201,7 +195,15 @@ func AttachContainer(ns netns.NsHandle, id, ifName, bridgeName string, mtu int,
 	return nil
 }
 
-func DetachContainer(ns netns.NsHandle, id, ifName string, cidrs []*net.IPNet) error {
+// NB: This function can be used only by a process that terminates immediately
+//     after calling the function as it changes netns via WithNetNSLinkUnsafe.
+func DetachContainer(netNSPath, id, ifName string, cidrs []*net.IPNet) error {
+	ns, err := netns.GetFromPath(netNSPath)
+	if err != nil {
+		return err
+	}
+	defer ns.Close()
+
 	ipt, err := iptables.New()
 	if err != nil {
 		return err

plugin/net/cni.go

@@ -107,7 +107,7 @@ func (c *CNIPlugin) CmdAdd(args *skel.CmdArgs) error {
 		id = fmt.Sprintf("%x", data)
 	}
 
-	if err := weavenet.AttachContainer(ns, id, args.IfName, conf.BrName, conf.MTU, false, []*net.IPNet{&result.IP4.IP}, false); err != nil {
+	if err := weavenet.AttachContainer(args.Netns, id, args.IfName, conf.BrName, conf.MTU, false, []*net.IPNet{&result.IP4.IP}, false); err != nil {
 		return err
 	}
 	if err := weavenet.WithNetNSLinkUnsafe(ns, args.IfName, func(link netlink.Link) error {
@@ -181,19 +181,7 @@ func (c *CNIPlugin) CmdDel(args *skel.CmdArgs) error {
 		return err
 	}
 
-	ns, err := netns.GetFromPath(args.Netns)
-	if err != nil {
-		return fmt.Errorf("error accessing namespace %q: %s", args.Netns, err)
-	}
-	defer ns.Close()
-	err = weavenet.WithNetNSUnsafe(ns, func() error {
-		link, err := netlink.LinkByName(args.IfName)
-		if err != nil {
-			return err
-		}
-		return netlink.LinkDel(link)
-	})
-	if err != nil {
+	if _, err = weavenet.WithNetNS(args.Netns, "del-iface", args.IfName); err != nil {
 		return fmt.Errorf("error removing interface %q: %s", args.IfName, err)
 	}
 

prog/weaveutil/attach.go

@@ -32,10 +32,15 @@ func attach(args []string) error {
 		}
 	}
 
-	pid, nsContainer, err := containerPidAndNs(args[0])
+	pid, err := containerPid(args[0])
 	if err != nil {
 		return err
 	}
+	nsContainer, err := netns.GetFromPid(pid)
+	if err != nil {
+		return fmt.Errorf("unable to open namespace for container %s: %s", args[0], err)
+	}
+
 	if nsHost, err := netns.GetFromPid(1); err != nil {
 		return fmt.Errorf("unable to open host namespace: %s", err)
 	} else if nsHost.Equal(nsContainer) {
@@ -50,7 +55,7 @@ func attach(args []string) error {
 		return err
 	}
 
-	err = weavenet.AttachContainer(nsContainer, fmt.Sprint(pid), weavenet.VethName, args[1], mtu, withMulticastRoute, cidrs, keepTXOn)
+	err = weavenet.AttachContainer(weavenet.NSPathByPid(pid), fmt.Sprint(pid), weavenet.VethName, args[1], mtu, withMulticastRoute, cidrs, keepTXOn)
 	// If we detected an error but the container has died, tell the user that instead.
 	if err != nil && !processExists(pid) {
 		err = fmt.Errorf("Container %s died", args[0])
@@ -58,23 +63,19 @@ func attach(args []string) error {
 	return err
 }
 
-func containerPidAndNs(containerID string) (int, netns.NsHandle, error) {
+func containerPid(containerID string) (int, error) {
 	c, err := docker.NewVersionedClientFromEnv("1.18")
 	if err != nil {
-		return 0, 0, fmt.Errorf("unable to connect to docker: %s", err)
+		return 0, fmt.Errorf("unable to connect to docker: %s", err)
 	}
 	container, err := c.InspectContainer(containerID)
 	if err != nil {
-		return 0, 0, fmt.Errorf("unable to inspect container %s: %s", containerID, err)
+		return 0, fmt.Errorf("unable to inspect container %s: %s", containerID, err)
 	}
 	if container.State.Pid == 0 {
-		return 0, 0, fmt.Errorf("container %s not running", containerID)
+		return 0, fmt.Errorf("container %s not running", containerID)
 	}
-	ns, err := netns.GetFromPid(container.State.Pid)
-	if err != nil {
-		return 0, 0, fmt.Errorf("unable to open namespace for container %s: %s", containerID, err)
-	}
-	return container.State.Pid, ns, nil
+	return container.State.Pid, nil
 }
 
 func processExists(pid int) bool {
@@ -99,7 +100,7 @@ func detach(args []string) error {
 		cmdUsage("detach-container", "<container-id> <cidr>...")
 	}
 
-	_, ns, err := containerPidAndNs(args[0])
+	pid, err := containerPid(args[0])
 	if err != nil {
 		return err
 	}
@@ -107,5 +108,5 @@ func detach(args []string) error {
 	if err != nil {
 		return err
 	}
-	return weavenet.DetachContainer(ns, args[0], weavenet.VethName, cidrs)
+	return weavenet.DetachContainer(weavenet.NSPathByPid(pid), args[0], weavenet.VethName, cidrs)
 }