diff mbox series

[ovs-dev,RFC,4/5] Native tunnel: Do not refresh the entry while revalidating.

Message ID 163361013437.2049658.1927530737014999774.stgit@fed.void
State Superseded
Headers show
Series Native tunnel: Update neigh entries in tnl termination. | expand

Commit Message

Paolo Valerio Oct. 7, 2021, 12:35 p.m. UTC 
This is a minor issue but visible e.g. when you try to flush the neigh
cache while the ARP flow is still present in the datapath, triggering
the revalidation of the datapath flows which subsequntly
refreshes/adds the entry in the cache.

Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
 lib/tnl-neigh-cache.c        |   20 +++++++++++++-------
 lib/tnl-neigh-cache.h        |    2 +-
 ofproto/ofproto-dpif-xlate.c |    3 ++-
 3 files changed, 16 insertions(+), 9 deletions(-)

Comments

Flavio Leitner Oct. 26, 2021, 1:06 a.m. UTC | #1 
Hi Paolo,

On Thu, Oct 07, 2021 at 02:35:34PM +0200, Paolo Valerio wrote:
> This is a minor issue but visible e.g. when you try to flush the neigh
> cache while the ARP flow is still present in the datapath, triggering
> the revalidation of the datapath flows which subsequntly

Typo

> refreshes/adds the entry in the cache.

Otherwise it looks ok.
fbl


> 
> Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
> ---
>  lib/tnl-neigh-cache.c        |   20 +++++++++++++-------
>  lib/tnl-neigh-cache.h        |    2 +-
>  ofproto/ofproto-dpif-xlate.c |    3 ++-
>  3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
> index 9d3f00ad9..df8de48eb 100644
> --- a/lib/tnl-neigh-cache.c
> +++ b/lib/tnl-neigh-cache.c
> @@ -173,7 +173,7 @@ tnl_arp_set(const char name[IFNAMSIZ], ovs_be32 dst,
>  
>  static int
>  tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc,
> -              const char name[IFNAMSIZ])
> +              const char name[IFNAMSIZ], bool update)
>  {
>      /* Snoop normal ARP replies and gratuitous ARP requests/replies only */
>      if (!is_arp(flow)
> @@ -183,13 +183,17 @@ tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc,
>          return EINVAL;
>      }
>  
> -    tnl_arp_set(name, FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src), flow->arp_sha);
> +    memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
> +
> +    if (update) {
> +        tnl_arp_set(name, flow->nw_src, flow->arp_sha);
> +    }
>      return 0;
>  }
>  
>  static int
>  tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc,
> -             const char name[IFNAMSIZ])
> +             const char name[IFNAMSIZ], bool update)
>  {
>      if (!is_nd(flow, wc) || flow->tp_src != htons(ND_NEIGHBOR_ADVERT)) {
>          return EINVAL;
> @@ -208,20 +212,22 @@ tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc,
>      memset(&wc->masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst);
>      memset(&wc->masks.nd_target, 0xff, sizeof wc->masks.nd_target);
>  
> -    tnl_neigh_set(name, &flow->nd_target, flow->arp_tha);
> +    if (update) {
> +        tnl_neigh_set(name, &flow->nd_target, flow->arp_tha);
> +    }
>      return 0;
>  }
>  
>  int
>  tnl_neigh_snoop(const struct flow *flow, struct flow_wildcards *wc,
> -                const char name[IFNAMSIZ])
> +                const char name[IFNAMSIZ], bool update)
>  {
>      int res;
> -    res = tnl_arp_snoop(flow, wc, name);
> +    res = tnl_arp_snoop(flow, wc, name, update);
>      if (res != EINVAL) {
>          return res;
>      }
> -    return tnl_nd_snoop(flow, wc, name);
> +    return tnl_nd_snoop(flow, wc, name, update);
>  }
>  
>  void
> diff --git a/lib/tnl-neigh-cache.h b/lib/tnl-neigh-cache.h
> index 92fdf5a93..a2fd9f4ae 100644
> --- a/lib/tnl-neigh-cache.h
> +++ b/lib/tnl-neigh-cache.h
> @@ -32,7 +32,7 @@
>  #include "util.h"
>  
>  int tnl_neigh_snoop(const struct flow *flow, struct flow_wildcards *wc,
> -                    const char dev_name[IFNAMSIZ]);
> +                    const char dev_name[IFNAMSIZ], bool update);
>  void
>  tnl_neigh_set(const char name[IFNAMSIZ], const struct in6_addr *dst,
>                const struct eth_addr mac);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 4430ac073..44a49dae8 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4097,7 +4097,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, struct flow *flow,
>              (flow->dl_type == htons(ETH_TYPE_ARP) ||
>               flow->nw_proto == IPPROTO_ICMPV6) &&
>               is_neighbor_reply_correct(ctx, flow)) {
> -            tnl_neigh_snoop(flow, wc, ctx->xbridge->name);
> +            tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
> +                            ctx->xin->allow_side_effects);
>          } else if (*tnl_port != ODPP_NONE &&
>                     ctx->xin->allow_side_effects &&
>                     (flow->dl_type == htons(ETH_TYPE_IP) ||
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
index 9d3f00ad9..df8de48eb 100644
--- a/lib/tnl-neigh-cache.c
+++ b/lib/tnl-neigh-cache.c
@@ -173,7 +173,7 @@  tnl_arp_set(const char name[IFNAMSIZ], ovs_be32 dst,
 
 static int
 tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc,
-              const char name[IFNAMSIZ])
+              const char name[IFNAMSIZ], bool update)
 {
     /* Snoop normal ARP replies and gratuitous ARP requests/replies only */
     if (!is_arp(flow)
@@ -183,13 +183,17 @@  tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc,
         return EINVAL;
     }
 
-    tnl_arp_set(name, FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src), flow->arp_sha);
+    memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
+
+    if (update) {
+        tnl_arp_set(name, flow->nw_src, flow->arp_sha);
+    }
     return 0;
 }
 
 static int
 tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc,
-             const char name[IFNAMSIZ])
+             const char name[IFNAMSIZ], bool update)
 {
     if (!is_nd(flow, wc) || flow->tp_src != htons(ND_NEIGHBOR_ADVERT)) {
         return EINVAL;
@@ -208,20 +212,22 @@  tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc,
     memset(&wc->masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst);
     memset(&wc->masks.nd_target, 0xff, sizeof wc->masks.nd_target);
 
-    tnl_neigh_set(name, &flow->nd_target, flow->arp_tha);
+    if (update) {
+        tnl_neigh_set(name, &flow->nd_target, flow->arp_tha);
+    }
     return 0;
 }
 
 int
 tnl_neigh_snoop(const struct flow *flow, struct flow_wildcards *wc,
-                const char name[IFNAMSIZ])
+                const char name[IFNAMSIZ], bool update)
 {
     int res;
-    res = tnl_arp_snoop(flow, wc, name);
+    res = tnl_arp_snoop(flow, wc, name, update);
     if (res != EINVAL) {
         return res;
     }
-    return tnl_nd_snoop(flow, wc, name);
+    return tnl_nd_snoop(flow, wc, name, update);
 }
 
 void
diff --git a/lib/tnl-neigh-cache.h b/lib/tnl-neigh-cache.h
index 92fdf5a93..a2fd9f4ae 100644
--- a/lib/tnl-neigh-cache.h
+++ b/lib/tnl-neigh-cache.h
@@ -32,7 +32,7 @@ 
 #include "util.h"
 
 int tnl_neigh_snoop(const struct flow *flow, struct flow_wildcards *wc,
-                    const char dev_name[IFNAMSIZ]);
+                    const char dev_name[IFNAMSIZ], bool update);
 void
 tnl_neigh_set(const char name[IFNAMSIZ], const struct in6_addr *dst,
               const struct eth_addr mac);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 4430ac073..44a49dae8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4097,7 +4097,8 @@  terminate_native_tunnel(struct xlate_ctx *ctx, struct flow *flow,
             (flow->dl_type == htons(ETH_TYPE_ARP) ||
              flow->nw_proto == IPPROTO_ICMPV6) &&
              is_neighbor_reply_correct(ctx, flow)) {
-            tnl_neigh_snoop(flow, wc, ctx->xbridge->name);
+            tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
+                            ctx->xin->allow_side_effects);
         } else if (*tnl_port != ODPP_NONE &&
                    ctx->xin->allow_side_effects &&
                    (flow->dl_type == htons(ETH_TYPE_IP) ||