[ovs-dev] [IPv6 Redux 6/6] nicira-ext: Support matching IPv6 Neighbor Discovery messages.

[Jesse Gross]

On Sun, Jan 23, 2011 at 3:33 AM, Justin Pettit <jpettit at nicira.com> wrote:
> On Jan 22, 2011, at 7:30 PM, Jesse Gross wrote:
>
>> On Sat, Jan 22, 2011 at 3:11 AM, Justin Pettit <jpettit at nicira.com> wrote:
>>>
>>> +               /* In order to process neighbor discovery options, we need the
>>> +                * entire packet. */
>>> +               if (icmp_len < sizeof(*nd)
>>> +                               || skb->len < skb_transport_offset(skb) + icmp_len)
>>> +                       goto invalid;
>>
>> We could just check that skb->len is at least as long as the length in
>> the IP header.  If not, then we can just abort IPv6 processing in
>> parse_ipv6hdr() - that's what we do for IPv4.
>
> Good idea.  That cleans up a lot of places.  Can you do a sanity check on lengths after I make that change to make sure I didn't introduce anything stupid?  (Not that I really have to ask.)

Don't worry...

>>> +invalid:
>>> +       key->tp_src = htons(0);
>>> +       key->tp_dst = htons(0);
>>
>> Should we really zero out the type and code if we don't have a valid ND packet?
>
> My concern was that if someone did something we disliked (e.g., setting two SLL options in a Neighbor Solictation message), then a rule writer would have a hard time distinguishing that from a (valid) NS message with no options.  I suppose they could drop based on "nd_target" being 0, but that seems really confusing.  I believe we were already going to suggest to people wishing to enforce ND policies that they drop all ICMPv6 traffic with "icmp_code=0".

I'm not sure I understand why you think that dropping based on
nd_target is confusing.  If you think of ND as being essentially
another protocol layer, then it is consistent with how we handle other
protocols.  It's also consistent with how you implemented it: before
this patch an invalid ND message would have a type and code but after
the patch those are zeroed out.  Therefore, this (and any future
parsing that we implement along these lines) changes preexisting
behavior even if you aren't using them.

Do you mean drop traffic with icmp_type that is zero?  It seems like
these messages all use code of zero.  Also, should we be checking the
code when parsing?