[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