| Message ID | 163361012812.2049658.7617456030065856447.stgit@fed.void |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Native tunnel: Update neigh entries in tnl termination. | expand |
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. > Signed-off-by: Paolo Valerio <pvalerio@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)) { > + 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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Flavio Leitner <fbl@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@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@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- > fbl
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) { + 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); } }
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. Signed-off-by: Paolo Valerio <pvalerio@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(-)