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

Ben Pfaff blp at nicira.com
Fri Aug 27 19:30:24 UTC 2010


On Thu, Aug 26, 2010 at 03:00:49PM -0700, Jesse Gross wrote:
> 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;
> }

OK, I made that change.




More information about the dev mailing list