[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