[ovs-dev] [PATCH 2/2] datapath: Hash only the part of struct sw_flow_key populated by flow_extract().

[Jesse Gross]

On Fri, Apr 29, 2011 at 3:04 PM, Andrew Evans <aevans at nicira.com> wrote:
>> >>>  static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
>> >>> -                       int nh_len)
>> >>> +                       int *key_len, int nh_len)
>> >>>  {
>> >>>        struct icmp6hdr *icmp = icmp6_hdr(skb);
>> >>> +       int saved_key_len = *key_len;
>> >>>
>> >>>        /* 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->ipv6.tp.src = htons(icmp->icmp6_type);
>> >>>        key->ipv6.tp.dst = htons(icmp->icmp6_code);
>> >>> +       *key_len = SW_FLOW_KEY_OFFSET(ipv6.tp.dst);
>> >>
>> >> Don't we need to store the key_len at this point, not before?  If we
>> >> jump to the invalid block then we don't get rid of type and code.
>>
>> Were you intending to fix this in the diff below?  I don't see how it
>> rolls back the length for invalid packets.
>>
> Oops. I meant to consolidate all the function return points to one. I think this patch now
> ensures that *key_lenp will reflect the portion of the flow key containing valid data when
> control returns to the caller.

I was thinking about this a bit more and realized that we have to be
more careful about how we handle invalid packets, not just here but in
other places as well.  In some places if the packet is invalid then we
don't set length to include the invalid headers.  At first I thought
this was correct but it's actually a significant problem.  Take IPv6
for example: if the length of the packet is shorter than the IPv6
header then we won't include the IPv6 fields.  However, this means
that the extracted flow will actually match any IPv6 flow, which is
clearly not right.  Instead we need to include the length of the IPv6
fields so that we will compare them and know that we are looking at an
invalid packet.