[ovs-dev] [PATCH 2/2] datapath: Hash only the part of struct sw_flow_key populated by flow_extract().
Andrew Evans
aevans at nicira.com
Sat Apr 30 21:29:09 UTC 2011
On Fri, 2011-04-29 at 19:58 -0700, Jesse Gross wrote:
> 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.
Ah, now your comments about setting key_len to the end of the ipv4/ipv6
structs makes sense to me. I'll change the code to always set the key
length to the end of the IPv4/IPv6 structs if any of the respective
fields are set.
More information about the dev
mailing list