[ovs-dev] [PATCH v2 3/7] tnl-neigh-cashe: tighten arp and nd snooping.

[Thadeu Lima de Souza Cascardo]

Fix subject: s/cashe/cache/.

On Tue, Mar 15, 2016 at 09:57:29AM -0700, Pravin B Shelar wrote:
> Currently arp and nd snooping is pretty loose. That causes
> unnecessary entries in neighbour cache. Following patch
> adds required checks.
> 
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
> ---
>  lib/tnl-neigh-cache.c    | 7 +++++--
>  tests/ofproto-dpif.at    | 2 +-
>  tests/tunnel-push-pop.at | 4 ++--
>  3 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
> index 0339b52..d95855a 100644
> --- a/lib/tnl-neigh-cache.c
> +++ b/lib/tnl-neigh-cache.c
> @@ -147,7 +147,9 @@ static int
>  tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc,
>                const char name[IFNAMSIZ])
>  {
> -    if (flow->dl_type != htons(ETH_TYPE_ARP)) {
> +    if (flow->dl_type != htons(ETH_TYPE_ARP) ||
> +        flow->nw_proto != ARP_OP_REPLY ||
> +        !memcmp(&flow->arp_sha, &eth_addr_zero, sizeof (flow->arp_sha))) {
>          return EINVAL;

There is eth_addr_is_zero.

>      }
>  
> @@ -167,7 +169,8 @@ tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc,
>      if (flow->dl_type != htons(ETH_TYPE_IPV6) ||
>          flow->nw_proto != IPPROTO_ICMPV6 ||
>          flow->tp_dst != htons(0) ||
> -        flow->tp_src != htons(ND_NEIGHBOR_ADVERT)) {
> +        flow->tp_src != htons(ND_NEIGHBOR_ADVERT) ||
> +        !memcmp(&flow->arp_tha, &eth_addr_zero, sizeof (flow->arp_tha))) {

Same here. In this particular case, I think it's important to note the cases
where an empty TLLs may occur, and why they don't matter for us.

In particular, here is what I wrote in my commit:

RFC4861 says Neighbor Advertisements sent in response to unicast Neighbor
Solicitations SHOULD include the Target link-layer address. However, Linux
doesn't. Responses to multicast NS MUST include it, which is what OVS sends.
So, the response to Solicitations sent by OVS will include the TLL address and
other Advertisements not including it can be ignored.

And I think that justifies for a separate check with a comment of its own,
mentioning this particular situation.

Thanks.
Cascardo.


>          return EINVAL;
>      }
>  
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 2d81711..72df90e 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5483,7 +5483,7 @@ AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/24 br0], [0], [OK
>  AT_CHECK([ovs-ofctl add-flow br0 action=normal])
>  
>  dnl Prime ARP Cache for 1.1.2.92
> -AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=1,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)'])
> +AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)'])
>  
>  dnl configure sflow on int-br only
>  ovs-vsctl \
> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
> index 583abb5..a6118a8 100644
> --- a/tests/tunnel-push-pop.at
> +++ b/tests/tunnel-push-pop.at
> @@ -37,8 +37,8 @@ AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/24 br0], [0], [OK
>  AT_CHECK([ovs-ofctl add-flow br0 action=normal])
>  
>  dnl Check ARP Snoop
> -AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=1,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)'])
> -AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b7,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.93,tip=1.1.2.88,op=1,sha=f8:bc:12:44:34:b7,tha=00:00:00:00:00:00)'])
> +AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)'])
> +AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b7,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.93,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b7,tha=00:00:00:00:00:00)'])
>  
>  AT_CHECK([ovs-appctl tnl/neigh/show], [0], [dnl
>  IP                                            MAC                 Bridge
> -- 
> 2.5.0
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev