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

[pravin shelar]

On Tue, Mar 15, 2016 at 10:10 AM, Thadeu Lima de Souza Cascardo
<cascardo at 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 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.
>
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.