Flavio Leitner <f...@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 <pvale...@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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> --
> fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
