[ovs-dev] [runt-flows 5/8] datapath: Report memory allocation errors in flow_extract().

Jesse Gross jesse at nicira.com
Thu Aug 26 22:00:49 UTC 2010


I forgot that I wasn't typing in an editor and hit tab or something
and the message was sent.  Trying again...

On Thu, Aug 26, 2010 at 2:56 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Fri, Aug 13, 2010 at 10:55 AM, Ben Pfaff <blp at nicira.com> wrote:
>> Until now flow_extract() has simply returned a bogus flow when memory
>> allocation errors occurred.  This fixes the problem by propagating the
>> error to the caller.
>>
>> Signed-off-by: Ben Pfaff <blp at nicira.com>
>
> Hmm, that's pretty nasty behavior that we had before.
>
> One general comment about this is that the behavior seems a little
> asymmetrical for the different protocol types: why switch to this new
> style for IP but not for ARP?  Why are the L4 protocols different from
> L3?  Why can't we just do all of this at the beginning?  The answer to
> all of these questions is (I assume) "IP options".  However, it took a
> little more thought to convince myself that it is OK than seems
> necessary.

>> -static inline bool iphdr_ok(struct sk_buff *skb)
>> +static inline int check_iphdr(struct sk_buff *skb)
>>  {
>> -       int nh_ofs = skb_network_offset(skb);
>> -       if (skb->len >= nh_ofs + sizeof(struct iphdr)) {
>> -               int ip_len = ip_hdrlen(skb);
>> -               return (ip_len >= sizeof(struct iphdr)
>> -                       && pskb_may_pull(skb, nh_ofs + ip_len));
>> -       }
>> -       return false;
>> +       unsigned int nh_ofs = skb_network_offset(skb);
>> +       unsigned int ip_len;
>> +
>> +       if (skb->len < nh_ofs + sizeof(struct iphdr))
>> +               return -EINVAL;
>> +
>> +       ip_len = ip_hdrlen(skb);
>> +       if (ip_len < sizeof(struct iphdr) || skb->len < nh_ofs + ip_len)
>> +               return -EINVAL;
>> +
>> +       /*
>> +        * Pull enough header bytes to account for the IP header plus the
>> +        * longest transport header that we parse, currently 20 bytes for TCP.
>> +        */
>> +       if (!pskb_may_pull(skb, min(nh_ofs + ip_len + 20, skb->len)))
>> +               return -ENOMEM;
>> +
>> +       skb_set_transport_header(skb, nh_ofs + ip_len);
>> +       return 0;
>>  }
>
> My usual complaint - can you add unlikely() to these branches?
>
>> +       if (key->dl_type == htons(ETH_P_IP)) {
>> +               struct iphdr *nh;
>> +               int error;
>> +
>> +               error = check_iphdr(skb);
>> +               if (unlikely(error == -ENOMEM))
>> +                       return -ENOMEM;
>> +               else if (unlikely(error == -EINVAL)) {
>> +                       skb->transport_header = skb->network_header;
>> +                       return 0;
>> +               }
>
> It makes me nervous to only check for two specific error types, even
> if we currently only return those two.  Why not do something like
> this:

if (unlikely(error)) {
    if (error == -EINVAL) {
        skb->transport_header = skb->network_header;
        return 0;
    } else
        return error;
}




More information about the dev mailing list