| Message ID | 1593032303-26749-1-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] bfd: Support overlay BFD | expand |
Thanks for the patch. On Wed, Jun 24, 2020 at 1:58 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > Current OVS intercepts and processes all BFD packets, thus VM-2-VM > BFD packets get lost and the recipient VM never sees them. Can you explain in more detail the issue? How about this below, if I understand correctly: " Currently we treat any packets coming into a BFD-enabled OVS interface as BFD packets for OVS, if they are IP, UDP, No_frag, and having BFD DEST_PORT. However, when consider overlay network, customers might deployment their own BFD protocol stack, and as a result, the BFD packets should be forwarded to the customer instead of being processed at OVS. " > > This patch fixes it by only intercepting and processing BFD packets maybe not "intercepting", but here we are adding additional check. > destined to a configured BFD instance, and other BFD packets are made > available to the OVS flow table for forwarding. > > This patch keeps BFD's backward compatibility. > Is there a VMware-BZ ID? > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > --- > lib/bfd.c | 16 +++++++++++++--- > vswitchd/vswitch.xml | 7 +++++++ > 2 files changed, 20 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); Why doing the above? btw, is it possible to add a userspace test (make check)? Thanks, William
Thanks for the review. Please check my inline comments. Will fix them in v2. Yifeng On Sat, Jun 27, 2020 at 3:07 PM William Tu <u9012063@gmail.com> wrote: > Thanks for the patch. > > On Wed, Jun 24, 2020 at 1:58 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > > > Current OVS intercepts and processes all BFD packets, thus VM-2-VM > > BFD packets get lost and the recipient VM never sees them. > > Can you explain in more detail the issue? > How about this below, if I understand correctly: > " > Currently we treat any packets coming into a BFD-enabled OVS interface > as BFD packets for OVS, > if they are IP, UDP, No_frag, and having BFD DEST_PORT. However, when > consider overlay network, > customers might deployment their own BFD protocol stack, and as a > result, the BFD packets should be > forwarded to the customer instead of being processed at OVS. > " > > Yes, correct. > > > > This patch fixes it by only intercepting and processing BFD packets > > maybe not "intercepting", but here we are adding additional check. > > Yes, your rephrase is more accurate. > > destined to a configured BFD instance, and other BFD packets are made > > available to the OVS flow table for forwarding. > > > > This patch keeps BFD's backward compatibility. > > > Is there a VMware-BZ ID? > Yes, there is, will add it in v2. > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > --- > > lib/bfd.c | 16 +++++++++++++--- > > vswitchd/vswitch.xml | 7 +++++++ > > 2 files changed, 20 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); > Why doing the above? > This is because, besides BFD port, we additionally match BFD packet's destination address with OVS's BFD interface. Therefore, OVS won't treat BFD packets targeted at VM as local BFD packets. > > btw, is it possible to add a userspace test (make check)? > > Good idea, will do in v2. > Thanks, > William >
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/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index b6acb34ca1b8..e1f895134ff2 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -3662,6 +3662,13 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ optional Authentication or ``Echo Mode'' features. </p> + <p> + OVS 2.13 and earlier intercepted and processed all BFD packets. + OVS 2.14 and later only intercept and process BFD packets destined + to a configured BFD instance, and other BFD packets are made available + to the OVS flow table for forwarding. + </p> + <group title="BFD Configuration"> <p> A controller sets up key-value pairs in the <ref column="bfd"/>
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 keeps BFD's backward compatibility. Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> --- lib/bfd.c | 16 +++++++++++++--- vswitchd/vswitch.xml | 7 +++++++ 2 files changed, 20 insertions(+), 3 deletions(-)