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

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@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(-)

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@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev

On Tue, Mar 15, 2016 at 10:10 AM, Thadeu Lima de Souza Cascardo
<cascardo@redhat.com> wrote:
> 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@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.
>
ok, I did not realized that. I was just reading ovs flow-extract code
and noticed that it is possible to have zero ethernet address in
ICMPV6 flow in case of error. I will update the patch incorporating
both cases.

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@nicira.com>

I think you should get Thadeu's ack for this patch instead of mine.