diff mbox series

[ovs-dev] bfd: Support overlay BFD

Message ID 1593032303-26749-1-git-send-email-pkusunyifeng@gmail.com
State New
Headers show
Series [ovs-dev] bfd: Support overlay BFD | expand

Commit Message

Yifeng Sun June 24, 2020, 8:58 p.m. UTC
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(-)

Comments

William Tu June 27, 2020, 10:06 p.m. UTC | #1
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
Yifeng Sun June 28, 2020, 3:59 a.m. UTC | #2
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 mbox series

Patch

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"/>