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

[Justin Pettit]

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.)

>> +               if (!pskb_may_pull(skb, skb_transport_offset(skb) + icmp_len))
>> +                       return -ENOMEM;
>> +
>> +               nd = (struct nd_msg *)skb_transport_header(skb);
>> +               memcpy(&key->nd_target, &nd->target, sizeof(key->nd_target));
> 
> key->nd_target is an array, so no address of.

Changed.

>> +
>> +               icmp_len -= sizeof (*nd);
> 
> There's an extra space after sizeof.

Cleaned up.

>> +               offset = 0;
>> +               while (icmp_len >= 8) {
>> +                       struct nd_opt_hdr *nd_opt = (struct nd_opt_hdr *)(nd->opt + offset);
>> +                       int opt_len = nd_opt->nd_opt_len * 8;
>> +
>> +                       if (!opt_len || (opt_len > icmp_len))
>> +                               goto invalid;
>> +
>> +                       if (nd_opt->nd_opt_type == ND_OPT_SOURCE_LL_ADDR
>> +                                       && opt_len == 8) {
>> +                               memcpy(&key->nd_sll, &nd->opt[offset+2], ETH_ALEN);
> 
> The two bytes that you are skipping are the option header?  If so,
> then what about sizeof(struct nd_opt_hdr)?

Yes.  It caused the line to wrap.  That's a pretty lame reason, so I did "sizeof(*nd_opt)" and broke it across two lines.

>> +                               break;
>> +                       } else if (nd_opt->nd_opt_type == ND_OPT_TARGET_LL_ADDR
>> +                                       && opt_len == 8) {
>> +                               memcpy(&key->nd_tll, &nd->opt[offset+2], ETH_ALEN);
>> +                               break;
> 
> Shouldn't have address of for these arrays either.

Done.

>> +                       }
>> +
>> +                       icmp_len -= opt_len;
>> +                       offset += opt_len;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +
>> +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".

>> +       memset(&key->nd_target, '\0', sizeof(key->nd_target));
>> +       memset(&key->nd_sll, '\0', sizeof(key->nd_sll));
>> +       memset(&key->nd_tll, '\0', sizeof(key->nd_tll));
> 
> Just using 0 instead of '\0' would be more canonical.

Yeah, that's a bit weird looking.  :-)

>> @@ -514,12 +577,12 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct odp_flow_key *key,
>>                        }
>>                } else if (key->nw_proto == NEXTHDR_ICMP) {
>>                        if (icmp6hdr_ok(skb)) {
>> -                               struct icmp6hdr *icmp = icmp6_hdr(skb);
>> -                               /* The ICMPv6 type and code fields use the 16-bit
>> -                                * transport port fields, so we need to store them
>> -                                * in 16-bit network byte order. */
>> -                               key->tp_src = htons(icmp->icmp6_type);
>> -                               key->tp_dst = htons(icmp->icmp6_code);
>> +                               struct ipv6hdr *nh = ipv6_hdr(skb);
>> +                               int icmp_len = ntohs(nh->payload_len) + sizeof *nh - nh_len;
>> +                               int error = parse_icmpv6(skb, key, icmp_len);
> 
> Why don't we do the length calculation in parse_icmpv6?  It would be
> more self-contained.

That's cleaner.  Changed.

> It looks like this is the only place that we use nh_len.  Isn't it the
> same as skb_transport_offset() - skb_network_offset()?  That would be
> cleaner than passing it around.
> 
> sizeof needs parentheses.

Fixed.

>> diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
>> index afc3956..343c3f1 100644
>> --- a/include/openvswitch/datapath-protocol.h
>> +++ b/include/openvswitch/datapath-protocol.h
>> @@ -237,6 +237,9 @@ struct odp_flow_key {
>>     uint8_t  arp_tha[6];        /* ARP target hardware address. */
>>     uint32_t ipv6_src[4];       /* IPv6 source address. */
>>     uint32_t ipv6_dst[4];       /* IPv6 destination address. */
>> +    uint8_t  nd_target[16];     /* IPv6 ND target address */
>> +    uint8_t  nd_sll[6];         /* IPv6 ND source link layer address */
>> +    uint8_t  nd_tll[6];         /* IPv6 ND target link layer address */
> 
> All of the IPv6 addresses should be stored the same way, either arrays
> of uint8_t or uint32_t.

Yeah, they were originally all uint8_t, but Ben suggested I switch them to uint32_t.  I forgot to hit this commit with the change, too.

> Why are the ND hardware addresses broken out as separate fields here?
> In userspace they are aliased on the ARP fields.

As I mentioned in the previous review, I had anticipated that the netlink changes would precede these ones.  Since each message type gets its own structure in netlink, I left them separate when writing the code.  This should only be this way for a few days until netlink goes in.

>> diff --git a/lib/flow.c b/lib/flow.c
>> index d990673..3fd7cb6 100644
>> --- a/lib/flow.c
>> +++ b/lib/flow.c
>> @@ -228,11 +228,83 @@ parse_ipv6(struct ofpbuf *packet, struct flow *flow)
>>         return -EINVAL;
>>     }
>> 
>> -    ofpbuf_pull(packet, nh_len);
> 
> Looks like this should be squashed into the previous patch.

Yep.  Thanks.

>> +static int
>> +parse_icmpv6(struct ofpbuf *b, struct flow *flow, int icmp_len)
>> +{
>> +    const struct icmp6_hdr *icmp = pull_icmpv6(b);
>> +    if (!icmp) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    flow->icmp_type = htons(icmp->icmp6_type);
>> +    flow->icmp_code = htons(icmp->icmp6_code);
>> +
>> +    if ((icmp->icmp6_type == ND_NEIGHBOR_SOLICIT)
>> +        || (icmp->icmp6_type == ND_NEIGHBOR_ADVERT)) {
>> +        struct nd_neighbor_solicit *nd_ns;  /* Identical to ND advert */
>> +
> 
> Do we need to check that icmp_len is at least long enough for a ND
> header (if the length in the IP header does not match the packet
> length)?  Regardless, it seems like there are differences in icmp_len
> validation from the kernel implementation.
> 
> Otherwise some of the comments from the kernel apply here as well.  It
> would be nice if the logic were more similar between the two (how
> option lengths are stored, etc.).

Okay, they should be in more sync now.

I want to look at the changes with fresher eyes in the morning.  I'll send out these last two patches for review after that.

Thanks for the reviews.

--Justin