From b688ac3200535d694896556a6be9193d414b0e0d Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Fri, 1 Jul 2016 22:27:12 +0100 Subject: [PATCH] Denote WithNetNS* functions with unsafe Please see https://github.com/weaveworks/weave/issues/2388#issuecomment-228365069 for more details. --- common/utils.go | 6 +++--- net/netns.go | 22 +++++++++++++++++----- net/veth.go | 8 ++++---- plugin/net/cni.go | 4 ++-- 4 files changed, 26 insertions(+), 14 deletions(-) diff --git a/common/utils.go b/common/utils.go index f79ad9e053..a5a07f8244 100644 --- 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 index ac2cea4a50..b067690938 100644 --- 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 index fbca71c3a3..49c1f96a64 100644 --- 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 index 0382c5ec7a..00c3cf4a6e 100644 --- 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