[ovs-dev] [PATCH RFC 3/5] Tunnel: Snoop ingress packets and update neigh cache if needed.

[Paolo Valerio]

Flavio Leitner <fbl at sysclose.org> writes:

> On Thu, Oct 07, 2021 at 02:35:28PM +0200, Paolo Valerio wrote:
>> In case of native tunnel with bfd enabled, if the MAC address of the
>> remote end's interface changes (e.g. because it got rebooted, and the
>> MAC address is allocated dinamically), the BFD session will never be
>> re-established.
>> 
>> This happens because the local tunnel neigh entry doesn't get updated,
>> and the local end keeps sending BFD packets with the old destination
>> MAC address. This was not an issue until
>> b23ddcc57d41 ("tnl-neigh-cache: tighten arp and nd snooping.")
>> because ARP requests were snooped as well avoiding the problem.
>> 
>> Fix this by snooping the incoming packets, and updating the neigh
>> cache accordingly.
>
>
> Can we add a mention that this only affects slow path?
> Otherwise it may suggests a performance impact.
>

Sure.

>
>> Signed-off-by: Paolo Valerio <pvalerio at redhat.com>
>> Fixes: b23ddcc57d41 ("tnl-neigh-cache: tighten arp and nd snooping.")
>> ---
>>  lib/tnl-neigh-cache.c        |   12 ++++++------
>>  lib/tnl-neigh-cache.h        |    3 +++
>>  ofproto/ofproto-dpif-xlate.c |   14 ++++++++++++++
>>  3 files changed, 23 insertions(+), 6 deletions(-)
>> 
>> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
>> index c8a7b60cd..9d3f00ad9 100644
>> --- a/lib/tnl-neigh-cache.c
>> +++ b/lib/tnl-neigh-cache.c
>> @@ -135,9 +135,9 @@ tnl_neigh_delete(struct tnl_neigh_entry *neigh)
>>      ovsrcu_postpone(neigh_entry_free, neigh);
>>  }
>>  
>> -static void
>> -tnl_neigh_set__(const char name[IFNAMSIZ], const struct in6_addr *dst,
>> -                const struct eth_addr mac)
>> +void
>> +tnl_neigh_set(const char name[IFNAMSIZ], const struct in6_addr *dst,
>> +              const struct eth_addr mac)
>>  {
>>      ovs_mutex_lock(&mutex);
>>      struct tnl_neigh_entry *neigh = tnl_neigh_lookup__(name, dst);
>> @@ -168,7 +168,7 @@ tnl_arp_set(const char name[IFNAMSIZ], ovs_be32 dst,
>>              const struct eth_addr mac)
>>  {
>>      struct in6_addr dst6 = in6_addr_mapped_ipv4(dst);
>> -    tnl_neigh_set__(name, &dst6, mac);
>> +    tnl_neigh_set(name, &dst6, mac);
>>  }
>>  
>>  static int
>> @@ -208,7 +208,7 @@ 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);
>> +    tnl_neigh_set(name, &flow->nd_target, flow->arp_tha);
>>      return 0;
>>  }
>>  
>> @@ -355,7 +355,7 @@ tnl_neigh_cache_add(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>          return;
>>      }
>>  
>> -    tnl_neigh_set__(br_name, &ip6, mac);
>> +    tnl_neigh_set(br_name, &ip6, mac);
>>      unixctl_command_reply(conn, "OK");
>>  }
>>  
>> diff --git a/lib/tnl-neigh-cache.h b/lib/tnl-neigh-cache.h
>> index e4b42b059..92fdf5a93 100644
>> --- a/lib/tnl-neigh-cache.h
>> +++ b/lib/tnl-neigh-cache.h
>> @@ -33,6 +33,9 @@
>>  
>>  int tnl_neigh_snoop(const struct flow *flow, struct flow_wildcards *wc,
>>                      const char dev_name[IFNAMSIZ]);
>> +void
>> +tnl_neigh_set(const char name[IFNAMSIZ], const struct in6_addr *dst,
>> +              const struct eth_addr mac);
>>  int tnl_neigh_lookup(const char dev_name[IFNAMSIZ], const struct in6_addr *dst,
>>                       struct eth_addr *mac);
>>  void tnl_neigh_cache_init(void);
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 8723cb4e8..4430ac073 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -4098,6 +4098,20 @@ terminate_native_tunnel(struct xlate_ctx *ctx, struct flow *flow,
>>               flow->nw_proto == IPPROTO_ICMPV6) &&
>>               is_neighbor_reply_correct(ctx, flow)) {
>>              tnl_neigh_snoop(flow, wc, ctx->xbridge->name);
>> +        } else if (*tnl_port != ODPP_NONE &&
>> +                   ctx->xin->allow_side_effects &&
>> +                   (flow->dl_type == htons(ETH_TYPE_IP) ||
>> +                    flow->dl_type == htons(ETH_TYPE_IPV6))) {
>> +            struct eth_addr mac = flow->dl_src;
>> +            struct in6_addr s_ip6;
>> +
>> +            if (flow->nw_src) {
>
> I don't think we will have zeros as valid source IP addr at this
> point, but this looks odd. Why not repeating the same check?
>                if (flow->dl_type == htons(ETH_TYPE_IP)) {
>

ACK. Will do.

>
>> +                in6_addr_set_mapped_ipv4(&s_ip6, flow->nw_src);
>> +            } else {
>> +                s_ip6 = flow->ipv6_src;
>> +            }
>> +
>> +            tnl_neigh_set(ctx->xbridge->name, &s_ip6, mac);
>>          }
>>      }
>>  
>> 
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> -- 
> fbl