From 0ae13a2650582bfce2dfdff029cd03f56a5009f4 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Thu, 24 Nov 2016 18:24:16 +0000 Subject: [PATCH 1/4] Detect hairpin flows The introduced "isHairpinFlow" detects whether a flow has been created due to enabled hairpin mode. --- router/fastdp.go | 60 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 56 insertions(+), 4 deletions(-) diff --git a/router/fastdp.go b/router/fastdp.go index 6627cd4e..33e0cd41 100644 --- a/router/fastdp.go +++ b/router/fastdp.go @@ -50,7 +50,8 @@ type FastDatapath struct { seenMACs map[MAC]struct{} // vxlan vports associated with the given UDP ports - vxlanVportIDs map[int]odp.VportID + vxlanUDPPorts map[int]odp.VportID + vxlanVportIDs map[odp.VportID]struct{} mainVxlanVportID odp.VportID // A singleton pool for the occasions when we need to decode @@ -87,7 +88,8 @@ func NewFastDatapath(iface *net.Interface, port int) (*FastDatapath, error) { sendToPort: nil, sendToMAC: make(map[MAC]bridgeSender), seenMACs: make(map[MAC]struct{}), - vxlanVportIDs: make(map[int]odp.VportID), + vxlanUDPPorts: make(map[int]odp.VportID), + vxlanVportIDs: make(map[odp.VportID]struct{}), forwarders: make(map[mesh.PeerName]*fastDatapathForwarder), } @@ -433,7 +435,7 @@ func (fastdp *FastDatapath) getVxlanVportID(udpPort int) (odp.VportID, error) { fastdp.lock.Lock() defer fastdp.lock.Unlock() - if vxlanVportID, present := fastdp.vxlanVportIDs[udpPort]; present { + if vxlanVportID, present := fastdp.vxlanUDPPorts[udpPort]; present { return vxlanVportID, nil } @@ -459,7 +461,8 @@ func (fastdp *FastDatapath) getVxlanVportID(udpPort int) (odp.VportID, error) { } } - fastdp.vxlanVportIDs[udpPort] = vxlanVportID + fastdp.vxlanUDPPorts[udpPort] = vxlanVportID + fastdp.vxlanVportIDs[vxlanVportID] = struct{}{} fastdp.missHandlers[vxlanVportID] = func(fks odp.FlowKeys, lock *fastDatapathLock) FlowOp { log.Debug("ODP miss: ", fks, " on port ", vxlanVportID) tunnel := fks[odp.OVS_KEY_ATTR_TUNNEL].(odp.TunnelFlowKey) @@ -1134,6 +1137,11 @@ func (fastdp *FastDatapath) send(fops FlowOp, frame []byte, lock *fastDatapathLo } } + if fastdp.isHairpinFlow(&flow) { + log.Error("Vetoed installation of hairpin flow ", flow) + return + } + if dec != nil { // put the decoder back lock.relock() @@ -1171,6 +1179,50 @@ func (fastdp *FastDatapath) takeDecoder(lock *fastDatapathLock) *EthernetDecoder return dec } +// A isHairpinFlow checks whether the flow is created due to enabled hairpin +// mode on the weave bridge port which attaches the datapath. Such flow is +// identified by either: +// +// * in_port == out_port, where in_port is non-vxlan vport; +// * a packet is sent back to a vxlan tunnel it has been received from and +// the tunnel id is either the same or dstPeer and srcPeer are reversed. +func (fastdp *FastDatapath) isHairpinFlow(flow *odp.FlowSpec) bool { + var ( + vxlanKey odp.TunnelAttrs + inVport odp.VportID + inVxlan bool + ) + + for _, key := range flow.FlowKeys { + switch k := key.(type) { + case odp.InPortFlowKey: + inVport = k.VportID() + case odp.TunnelFlowKey: + inVxlan = true + vxlanKey = k.Key() + } + } + + for _, action := range flow.Actions { + switch a := action.(type) { + case odp.SetTunnelAction: + if inVxlan && a.TunnelAttrs.TunnelId == vxlanKey.TunnelId && + a.TunnelAttrs.Ipv4Src == vxlanKey.Ipv4Dst && + a.TunnelAttrs.Ipv4Dst == vxlanKey.Ipv4Src { + return true + } + case odp.OutputAction: + if a.VportID() == inVport { + if _, ok := fastdp.vxlanVportIDs[a.VportID()]; !ok { + return true + } + } + } + } + + return false +} + type odpActionsFlowOp struct { NonDiscardingFlowOp fastdp *FastDatapath From ab1b99a4d9ac712a625ac2296ab5ee99d1befcd5 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Fri, 25 Nov 2016 17:32:38 +0000 Subject: [PATCH 2/4] Update vishvananda/netlink vendored dependency The updated version includes parsing of IFLA_PROTINFO which is needed to get info about hairpin mode when subscribing to link updates. --- vendor/github.com/vishvananda/netlink | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vendor/github.com/vishvananda/netlink b/vendor/github.com/vishvananda/netlink index a632d6dc..266f02d3 160000 --- a/vendor/github.com/vishvananda/netlink +++ b/vendor/github.com/vishvananda/netlink @@ -1 +1 @@ -Subproject commit a632d6dc2806fa19d2f7693017d3fb79d3d8fa03 +Subproject commit 266f02d3a80fb75c33302a2c91e7632ec33c1f32 From baa8f229f7ac6c5ae5e389083d9bf4757f9aef8d Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Fri, 25 Nov 2016 18:05:38 +0000 Subject: [PATCH 3/4] Detect enabling of hairpin mode on vethwe-bridge Enabled hairpin mode on vethwe-bridge was one of the root causes for installing looping flows in the fastdp mode. Also, it confused the weave custom L2 bridge, as the bridge treats a bounced back packet, e.g. ARP brodcast request, as a local one (the case for "Captured a packet associated with remote host"). --- net/bridge.go | 35 +++++++++++++++++++++++++++++++++ prog/weaver/main.go | 6 ++++++ test/705_detect_hairpin_test.sh | 12 +++++++++++ 3 files changed, 53 insertions(+) create mode 100755 test/705_detect_hairpin_test.sh diff --git a/net/bridge.go b/net/bridge.go index 07d1f6cb..da0bd40a 100644 --- a/net/bridge.go +++ b/net/bridge.go @@ -1,6 +1,9 @@ package net import ( + "fmt" + + "github.com/Sirupsen/logrus" "github.com/vishvananda/netlink" ) @@ -51,3 +54,35 @@ func isDatapath(link netlink.Link) bool { return false } } + +func DetectHairpin(portIfName string, log *logrus.Logger) error { + link, err := netlink.LinkByName(portIfName) + if err != nil { + return fmt.Errorf("Unable to find link %q: %s", portIfName, err) + } + + ch := make(chan netlink.LinkUpdate) + // See EnsureInterface for why done channel is not passed + if err := netlink.LinkSubscribe(ch, nil); err != nil { + return fmt.Errorf("Unable to subscribe to netlink updates: %s", err) + } + + pi, err := netlink.LinkGetProtinfo(link) + if err != nil { + return fmt.Errorf("Unable to get link protinfo %q: %s", portIfName, err) + } + if pi.Hairpin { + return fmt.Errorf("Hairpin mode enabled on %q", portIfName) + } + + go func() { + for up := range ch { + if up.Attrs().Name == portIfName && up.Attrs().Protinfo != nil && + up.Attrs().Protinfo.Hairpin { + log.Errorf("Hairpin mode enabled on %q", portIfName) + } + } + }() + + return nil +} diff --git a/prog/weaver/main.go b/prog/weaver/main.go index 92cb5bba..2050f49e 100644 --- a/prog/weaver/main.go +++ b/prog/weaver/main.go @@ -226,6 +226,12 @@ func main() { overlay, bridge := createOverlay(datapathName, ifaceName, isAWSVPC, config.Host, config.Port, bufSzMB) networkConfig.Bridge = bridge + if bridge != nil { + if err := weavenet.DetectHairpin("vethwe-bridge", Log); err != nil { + Log.Errorf("DetectHairpin failed: %s", err) + } + } + name := peerName(routerName, bridge.Interface()) if nickName == "" { diff --git a/test/705_detect_hairpin_test.sh b/test/705_detect_hairpin_test.sh new file mode 100755 index 00000000..4ba915ed --- /dev/null +++ b/test/705_detect_hairpin_test.sh @@ -0,0 +1,12 @@ +#! /bin/bash + +. ./config.sh + +start_suite "Detect hairpin mode" + +weave_on $HOST1 launch + +assert_raises "run_on $HOST1 sudo ip link set vethwe-bridge type bridge_slave hairpin on" +assert_raises "docker_on $HOST1 logs weave 2>&1 | grep -q 'Hairpin mode enabled on \"vethwe-bridge\"'" + +end_suite From aa9c03217674692ca44878bdb311c0e8dcd99ce8 Mon Sep 17 00:00:00 2001 From: Martynas Pumputis Date: Fri, 25 Nov 2016 18:32:43 +0000 Subject: [PATCH 4/4] Add sanity check to detect when receiving own packets --- router/network_router.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/router/network_router.go b/router/network_router.go index e9a85e16..a7c83dba 100644 --- a/router/network_router.go +++ b/router/network_router.go @@ -124,6 +124,13 @@ func (router *NetworkRouter) handleCapturedPacket(key PacketKey) FlowOp { } func (router *NetworkRouter) handleForwardedPacket(key ForwardPacketKey) FlowOp { + srcMac := net.HardwareAddr(key.SrcMAC[:]) + dstMac := net.HardwareAddr(key.DstMAC[:]) + + if key.SrcPeer == router.Ourself.Peer { + log.Warn("Received own packet to peer ", key.DstPeer, " from MAC (", srcMac, ") to (", dstMac, ")") + } + if key.DstPeer != router.Ourself.Peer { // it's not for us, we're just relaying it router.PacketLogging.LogForwardPacket("Relaying", key) @@ -134,9 +141,6 @@ func (router *NetworkRouter) handleForwardedPacket(key ForwardPacketKey) FlowOp // (because the DstPeer on a forwarded broadcast packet is // always set to the peer being forwarded to) - srcMac := net.HardwareAddr(key.SrcMAC[:]) - dstMac := net.HardwareAddr(key.DstMAC[:]) - switch newSrcMac, conflictPeer := router.Macs.AddForced(srcMac, key.SrcPeer); { case newSrcMac: log.Print("Discovered remote MAC ", srcMac, " at ", key.SrcPeer)