| Message ID | 202206181441546949224@chinatelecom.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] ofproto-dpif-xlate: Update tunnel neighbor when receive gratuitous arp. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Hello Han, "Han Ding" <handing@chinatelecom.cn> writes: > Commit ba07cf222a add the feature "Handle gratuitous ARP requests and > replies in tnl_arp_snoop()". But commit 83c2757bd1 just allow the ARP whitch > the destination address of the ARP is matched against the known xbridge addresses. > So the modification of commit ba07cf222a is not effective. When ovs receive the > gratuitous ARP from underlay gateway which the source address and destination > address are all gateway IP, tunnel neighbor will not be updated. > I think it would be clearer formatting the commits like below: $ git -P show -s --format="%h (\"%s\")" --abbrev=12 ba07cf222a ba07cf222a0c ("Handle gratuitous ARP requests and replies in tnl_arp_snoop()") $ git -P show -s --format="%h (\"%s\")" --abbrev=12 83c2757bd1 83c2757bd16e ("xlate: Move tnl_neigh_snoop() to terminate_native_tunnel()") I guess that the last commit deserves a Fixes tag as well. > Signed-off-by: Han Ding <handing@chinatelecom.cn> > --- > > Notes: > v3 > Correct the spell mistake. > > v2 > Change author name. > > ofproto/ofproto-dpif-xlate.c | 10 +++++++--- > tests/tunnel-push-pop.at | 20 ++++++++++++++++++++ > 2 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 8e5d030ac..6c69f981b 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4126,6 +4126,11 @@ xport_has_ip(const struct xport *xport) > return n_in6 ? true : false; > } > > +#define IS_VALID_NEIGHBOR_REPLY(flow, ctx) \ > +((flow->dl_type == htons(ETH_TYPE_ARP) || \ > + flow->nw_proto == IPPROTO_ICMPV6) && \ > + is_neighbor_reply_correct(ctx, flow)) > + Although terminate_native_tunnel() would be the only user, I guess a static function could be ok here, instead. > static bool > terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport, > struct flow *flow, struct flow_wildcards *wc, > @@ -4146,9 +4151,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport, > /* If no tunnel port was found and it's about an ARP or ICMPv6 packet, > * do tunnel neighbor snooping. */ > if (*tnl_port == ODPP_NONE && > - (flow->dl_type == htons(ETH_TYPE_ARP) || > - flow->nw_proto == IPPROTO_ICMPV6) && > - is_neighbor_reply_correct(ctx, flow)) { > + (IS_VALID_NEIGHBOR_REPLY(flow, ctx) || > + is_garp(flow, wc))) { AFAICT, this seems ok to me and the tests related to tunnel_push_pop succeed. There's probably some room for improvement in the code down to tnl_arp_snoop(), but I guess it's a bit out of scope of this patch. > tnl_neigh_snoop(flow, wc, ctx->xbridge->name, > ctx->xin->allow_side_effects); > } else if (*tnl_port != ODPP_NONE && > diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at > index c63344196..0bac362f4 100644 > --- a/tests/tunnel-push-pop.at > +++ b/tests/tunnel-push-pop.at > @@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl > 1.1.2.92 f8:bc:12:44:34:b6 br0 > ]) > > +dnl Receiving Gratuitous ARP request with correct VLAN id should alter tunnel neighbor cache > +AT_CHECK([ovs-appctl netdev-dummy/receive p0 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))']) > + > +ovs-appctl time/warp 1000 > +ovs-appctl time/warp 1000 > + > +AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl > +1.1.2.92 f8:bc:12:44:34:c8 br0 > +]) > + > +dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel neighbor cache > +AT_CHECK([ovs-appctl netdev-dummy/receive p0 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:34:b2,tha=f8:bc:12:44:34:b2))']) > + > +ovs-appctl time/warp 1000 > +ovs-appctl time/warp 1000 > + > +AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl > +1.1.2.92 f8:bc:12:44:34:b2 br0 > +]) > + > dnl Receive ARP reply without VLAN header > AT_CHECK([ovs-vsctl set port br0 tag=0]) > AT_CHECK([ovs-appctl tnl/neigh/flush], [0], [OK > -- > 2.27.0 > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Thanks for reviewing. I will change them in next version. -------------- Han Ding >Hello Han, > >"Han Ding" <handing@chinatelecom.cn> writes: > >> Commit ba07cf222a add the feature "Handle gratuitous ARP requests and >> replies in tnl_arp_snoop()". But commit 83c2757bd1 just allow the ARP whitch >> the destination address of the ARP is matched against the known xbridge addresses. >> So the modification of commit ba07cf222a is not effective. When ovs receive the >> gratuitous ARP from underlay gateway which the source address and destination >> address are all gateway IP, tunnel neighbor will not be updated. >> > >I think it would be clearer formatting the commits like below: > >$ git -P show -s --format="%h (\"%s\")" --abbrev=12 ba07cf222a >ba07cf222a0c ("Handle gratuitous ARP requests and replies in tnl_arp_snoop()") > >$ git -P show -s --format="%h (\"%s\")" --abbrev=12 83c2757bd1 >83c2757bd16e ("xlate: Move tnl_neigh_snoop() to terminate_native_tunnel()") > >I guess that the last commit deserves a Fixes tag as well. > >> Signed-off-by: Han Ding <handing@chinatelecom.cn> >> --- >> >> Notes: >> v3 >> Correct the spell mistake. >> >> v2 >> Change author name. >> >> ofproto/ofproto-dpif-xlate.c | 10 +++++++--- >> tests/tunnel-push-pop.at | 20 ++++++++++++++++++++ >> 2 files changed, 27 insertions(+), 3 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 8e5d030ac..6c69f981b 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -4126,6 +4126,11 @@ xport_has_ip(const struct xport *xport) >> return n_in6 ? true : false; >> } >> >> +#define IS_VALID_NEIGHBOR_REPLY(flow, ctx) \ >> +((flow->dl_type == htons(ETH_TYPE_ARP) || \ >> + flow->nw_proto == IPPROTO_ICMPV6) && \ >> + is_neighbor_reply_correct(ctx, flow)) >> + > >Although terminate_native_tunnel() would be the only user, I guess a >static function could be ok here, instead. > >> static bool >> terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport, >> struct flow *flow, struct flow_wildcards *wc, >> @@ -4146,9 +4151,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport, >> /* If no tunnel port was found and it's about an ARP or ICMPv6 packet, >> * do tunnel neighbor snooping. */ >> if (*tnl_port == ODPP_NONE && >> - (flow->dl_type == htons(ETH_TYPE_ARP) || >> - flow->nw_proto == IPPROTO_ICMPV6) && >> - is_neighbor_reply_correct(ctx, flow)) { >> + (IS_VALID_NEIGHBOR_REPLY(flow, ctx) || >> + is_garp(flow, wc))) { > >AFAICT, this seems ok to me and the tests related to tunnel_push_pop >succeed. There's probably some room for improvement in the code down to >tnl_arp_snoop(), but I guess it's a bit out of scope of this patch. > >> tnl_neigh_snoop(flow, wc, ctx->xbridge->name, >> ctx->xin->allow_side_effects); >> } else if (*tnl_port != ODPP_NONE && >> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at >> index c63344196..0bac362f4 100644 >> --- a/tests/tunnel-push-pop.at >> +++ b/tests/tunnel-push-pop.at >> @@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl >> 1.1.2.92 f8:bc:12:44:34:b6 br0 >> ]) >> >> +dnl Receiving Gratuitous ARP request with correct VLAN id should alter tunnel neighbor cache >> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))']) >> + >> +ovs-appctl time/warp 1000 >> +ovs-appctl time/warp 1000 >> + >> +AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl >> +1.1.2.92 f8:bc:12:44:34:c8 br0 >> +]) >> + >> +dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel neighbor cache >> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:34:b2,tha=f8:bc:12:44:34:b2))']) >> + >> +ovs-appctl time/warp 1000 >> +ovs-appctl time/warp 1000 >> + >> +AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl >> +1.1.2.92 f8:bc:12:44:34:b2 br0 >> +]) >> + >> dnl Receive ARP reply without VLAN header >> AT_CHECK([ovs-vsctl set port br0 tag=0]) >> AT_CHECK([ovs-appctl tnl/neigh/flush], [0], [OK >> -- >> 2.27.0 >> >> >> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 8e5d030ac..6c69f981b 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4126,6 +4126,11 @@ xport_has_ip(const struct xport *xport) return n_in6 ? true : false; } +#define IS_VALID_NEIGHBOR_REPLY(flow, ctx) \ +((flow->dl_type == htons(ETH_TYPE_ARP) || \ + flow->nw_proto == IPPROTO_ICMPV6) && \ + is_neighbor_reply_correct(ctx, flow)) + static bool terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport, struct flow *flow, struct flow_wildcards *wc, @@ -4146,9 +4151,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport, /* If no tunnel port was found and it's about an ARP or ICMPv6 packet, * do tunnel neighbor snooping. */ if (*tnl_port == ODPP_NONE && - (flow->dl_type == htons(ETH_TYPE_ARP) || - flow->nw_proto == IPPROTO_ICMPV6) && - is_neighbor_reply_correct(ctx, flow)) { + (IS_VALID_NEIGHBOR_REPLY(flow, ctx) || + is_garp(flow, wc))) { tnl_neigh_snoop(flow, wc, ctx->xbridge->name, ctx->xin->allow_side_effects); } else if (*tnl_port != ODPP_NONE && diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at index c63344196..0bac362f4 100644 --- a/tests/tunnel-push-pop.at +++ b/tests/tunnel-push-pop.at @@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl 1.1.2.92 f8:bc:12:44:34:b6 br0 ]) +dnl Receiving Gratuitous ARP request with correct VLAN id should alter tunnel neighbor cache +AT_CHECK([ovs-appctl netdev-dummy/receive p0 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))']) + +ovs-appctl time/warp 1000 +ovs-appctl time/warp 1000 + +AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl +1.1.2.92 f8:bc:12:44:34:c8 br0 +]) + +dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel neighbor cache +AT_CHECK([ovs-appctl netdev-dummy/receive p0 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:34:b2,tha=f8:bc:12:44:34:b2))']) + +ovs-appctl time/warp 1000 +ovs-appctl time/warp 1000 + +AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl +1.1.2.92 f8:bc:12:44:34:b2 br0 +]) + dnl Receive ARP reply without VLAN header AT_CHECK([ovs-vsctl set port br0 tag=0]) AT_CHECK([ovs-appctl tnl/neigh/flush], [0], [OK
Commit ba07cf222a add the feature "Handle gratuitous ARP requests and replies in tnl_arp_snoop()". But commit 83c2757bd1 just allow the ARP whitch the destination address of the ARP is matched against the known xbridge addresses. So the modification of commit ba07cf222a is not effective. When ovs receive the gratuitous ARP from underlay gateway which the source address and destination address are all gateway IP, tunnel neighbor will not be updated. Signed-off-by: Han Ding <handing@chinatelecom.cn> --- Notes: v3 Correct the spell mistake. v2 Change author name. ofproto/ofproto-dpif-xlate.c | 10 +++++++--- tests/tunnel-push-pop.at | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) -- 2.27.0