Please discard my previous email, I misunderstood your question. The packet above is dnl outer IP: Source: 172.16.180.105 Destination: 172.16.180.106 dnl inner IP: Source: 192.168.10.104 Destination: 192.168.10.105 So the bfd_should_process_flow returns false.
Yes, you are correct and this patch returns false in this case. For the above packet, outer IP is extracted as tunnel info, flow->nw_dst is 192.168.10.105. So bfd_should_process_flow returns false. Thanks, Yifeng On Wed, Jul 22, 2020 at 11:02 AM Yifeng Sun <pkusunyif...@gmail.com> wrote: > Thanks for reviewing. > > For these two packets: > > dnl outer IP: Source: 172.16.180.105 Destination: 172.16.180.106 > This one is normal BFD packet, bfd_should_process_flow should return > true, as used to. > > dnl inner IP: Source: 192.168.10.104 Destination: 192.168.10.105 > This one is overlay BFD packet, bfd_should_process_flow should return > false so this packet won't be intercepted by OVS's BFD engine. > > Thanks, > Yifeng > > On Wed, Jul 22, 2020 at 10:54 AM William Tu <u9012...@gmail.com> wrote: > >> On Wed, Jul 22, 2020 at 01:59:04AM -0700, Yifeng Sun wrote: >> > Current OVS intercepts and processes all BFD packets, thus VM-2-VM >> > BFD packets get lost and the recipient VM never sees them. >> > >> > This patch fixes it by only intercepting and processing BFD packets >> > destined to a configured BFD instance, and other BFD packets are made >> > available to the OVS flow table for forwarding. >> > >> > This patch adds new test to validate BFD overlay. >> > >> > This patch keeps BFD's backward compatibility. >> > >> > Signed-off-by: Yifeng Sun <pkusunyif...@gmail.com> >> > --- >> > v1->v2: Add test by William's suggestion. >> > >> > lib/bfd.c | 16 +++++++++++++--- >> > tests/system-traffic.at | 42 >> ++++++++++++++++++++++++++++++++++++++++++ >> > vswitchd/vswitch.xml | 7 +++++++ >> > 3 files changed, 62 insertions(+), 3 deletions(-) >> > >> > diff --git a/lib/bfd.c b/lib/bfd.c >> > index cc8c6857afa4..3c965699ace3 100644 >> > --- a/lib/bfd.c >> > +++ b/lib/bfd.c >> > @@ -149,6 +149,9 @@ BUILD_ASSERT_DECL(BFD_PACKET_LEN == sizeof(struct >> msg)); >> > #define FLAGS_MASK 0x3f >> > #define DEFAULT_MULT 3 >> > >> > +#define BFD_DEFAULT_SRC_IP 0xA9FE0101 /* 169.254.1.1 */ >> > +#define BFD_DEFAULT_DST_IP 0xA9FE0100 /* 169.254.1.0 */ >> > + >> > struct bfd { >> > struct hmap_node node; /* In 'all_bfds'. */ >> > uint32_t disc; /* bfd.LocalDiscr. Key in 'all_bfds' >> hmap. */ >> > @@ -457,9 +460,9 @@ bfd_configure(struct bfd *bfd, const char *name, >> const struct smap *cfg, >> > &bfd->rmt_eth_dst); >> > >> > bfd_lookup_ip(smap_get_def(cfg, "bfd_src_ip", ""), >> > - htonl(0xA9FE0101) /* 169.254.1.1 */, &bfd->ip_src); >> > + htonl(BFD_DEFAULT_SRC_IP), &bfd->ip_src); >> > bfd_lookup_ip(smap_get_def(cfg, "bfd_dst_ip", ""), >> > - htonl(0xA9FE0100) /* 169.254.1.0 */, &bfd->ip_dst); >> > + htonl(BFD_DEFAULT_DST_IP), &bfd->ip_dst); >> > >> > forwarding_if_rx = smap_get_bool(cfg, "forwarding_if_rx", false); >> > if (bfd->forwarding_if_rx != forwarding_if_rx) { >> > @@ -674,7 +677,14 @@ bfd_should_process_flow(const struct bfd *bfd_, >> const struct flow *flow, >> > memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); >> > if (flow->nw_proto == IPPROTO_UDP >> > && !(flow->nw_frag & FLOW_NW_FRAG_LATER) >> > - && tp_dst_equals(flow, BFD_DEST_PORT, wc)) { >> > + && tp_dst_equals(flow, BFD_DEST_PORT, wc) >> > + && (bfd->ip_src == htonl(BFD_DEFAULT_SRC_IP) >> > + || bfd->ip_src == flow->nw_dst)) { >> > + >> > + if (bfd->ip_src == flow->nw_dst) { >> > + memset(&wc->masks.nw_dst, 0xffffffff, sizeof >> wc->masks.nw_dst); >> > + } >> > + >> > bool check_tnl_key; >> > >> > atomic_read_relaxed(&bfd->check_tnl_key, &check_tnl_key); >> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> > index 2a0fbadff4a1..80b58996d530 100644 >> > --- a/tests/system-traffic.at >> > +++ b/tests/system-traffic.at >> > @@ -6289,3 +6289,45 @@ OVS_WAIT_UNTIL([cat p2.pcap | egrep "0x0050: >> *0000 *0000 *5002 *2000 *b85e *0000 >> > >> > OVS_TRAFFIC_VSWITCHD_STOP >> > AT_CLEANUP >> > + >> > +AT_SETUP([bfd - BFD overlay]) >> > +OVS_CHECK_GENEVE() >> > + >> > +OVS_TRAFFIC_VSWITCHD_START() >> > + >> > +AT_CHECK([ovs-vsctl -- set bridge br0 >> other-config:hwaddr=\"f2:ff:00:00:00:01\"]) >> > +ADD_BR([br-underlay], [set bridge br-underlay >> other-config:hwaddr=\"ee:09:e0:4d:bf:31\"]) >> > + >> > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) >> > +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"]) >> > + >> > +ADD_NAMESPACES(at_ns0) >> > + >> > +dnl Set up underlay link from host into the namespace using veth pair. >> > +ADD_VETH(p0, at_ns0, br-underlay, "172.16.180.105/24", >> 4e:12:5d:6c:74:3d) >> > +AT_CHECK([ip addr add dev br-underlay "172.16.180.106/24"]) >> > +AT_CHECK([ip link set dev br-underlay up]) >> > + >> > +dnl Set up tunnel endpoints on OVS outside the namespace. >> > +ADD_OVS_TUNNEL([geneve], [br0], [at_gnv0], [172.16.180.105], [ >> 192.168.10.100/24], >> > + [options:packet_type=ptap]) >> > + >> > +dnl Certain Linux distributions, like CentOS, have default iptable >> rules >> > +dnl to reject input traffic from br-underlay. Here we add a rule to >> walk >> > +dnl around it. >> > +iptables -I INPUT 1 -i br-underlay -j ACCEPT >> > +on_exit 'iptables -D INPUT 1' >> > + >> > +dnl Firstly, test normal BFD packet. >> > +ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 >> packet=ee09e04dbf314e125d6c743d080045c0006624d94000401153f9ac10b469ac10b46a739f17c1005247f900806558000000000023200000016a0f5a9ed376080045c0003400000000ff11fa03ac10b469ac10b46ac0070ec80020000021c003187b3c96ebbc96b962000186a0000f424000000000 >> actions=NORMAL" >> The packet above is >> dnl outer IP: Source: 172.16.180.105 Destination: 172.16.180.106 >> dnl inner IP: Source: 172.16.180.105 Destination: 172.16.180.106 >> >> And why does this trigger ovs to process it ex: bfd_should_process_flow() >> return true? >> In your patch, you're adding extra check >> bfd->ip_src == flow->nw_dst >> and here >> bfd->ip_src is default 169.254.1.1 >> flow->nw_dst is 172.16.180.106 >> >> >> > +dnl Next we test overlay BFD packet. >> > +ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 >> packet=ee09e04dbf314e125d6c743d08004500005b2558400040115445ac10b469ac10b46a6b1017c1004722c1024065580000000001048001001803005254009d0b6d5254000c89840800450000210e22400040119688c0a80a68c0a80a6995cd0ec8000dd342746573740a >> actions=NORMAL" >> >> The packet above is >> dnl outer IP: Source: 172.16.180.105 Destination: 172.16.180.106 >> dnl inner IP: Source: 192.168.10.104 Destination: 192.168.10.105 >> So the bfd_should_process_flow returns false. >> >> > + >> > +ovs-dpctl dump-flows > datapath-flows.txt >> > +dnl BFD packet was handled by BFD flow. >> > +AT_FAIL_IF([cat datapath-flows.txt | egrep >> "tunnel\(.*,src=172.16.180.105,dst=172.16.180.106,.*,eth\(\),eth_type\(0x0800\),ipv4\(proto=17,.*\),udp\(dst=3784\), >> .*, actions:userspace\(pid=.*,slow_path\(bfd\)\)" 2>&1 1>/dev/null]) >> > +dnl Overlay BFD packet was handled by non-BFD flows. >> > +AT_FAIL_IF([cat datapath-flows.txt | egrep >> "tunnel\(.*,src=172.16.180.105,dst=172.16.180.106,.*,eth\(.*\),eth_type\(0x0800\),ipv4\(src=192.168.10.104,dst=192.168.10.105,proto=17,.*\),udp\(.*,dst=3784\), >> .*, actions:drop" 2>&1 1>/dev/null]) >> > + >> > +OVS_TRAFFIC_VSWITCHD_STOP >> > +AT_CLEANUP >> >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev