Hello Han,
"Han Ding" <[email protected]> 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 <[email protected]>
> ---
>
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
