[ovs-dev] [PATCH 6/7] datapath: Convert odp_flow_key to use Netlink attributes instead.

Ben Pfaff blp at nicira.com
Tue Dec 28 19:06:48 UTC 2010


On Tue, Dec 28, 2010 at 02:13:47AM -0500, Jesse Gross wrote:
> On Thu, Dec 23, 2010 at 8:15 PM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index 7b55698..9096c44 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -814,14 +815,19 @@ static int do_put_flow(struct datapath *dp, struct odp_flow_put *uf,
> >                       struct odp_flow_stats *stats)
> >  {
> >        struct tbl_node *flow_node;
> > +       struct sw_flow_key key;
> >        struct sw_flow *flow;
> >        struct tbl *table;
> >        int error;
> >        u32 hash;
> >
> > -       hash = flow_hash(&uf->flow.key);
> > +       error = flow_copy_from_user(&key, uf->flow.key, uf->flow.key_len);
> 
> This causes sparse to warn because the key pointer in struct odp_flow
> is not marked as __user.  We either need to add a cast here or start
> annotating datapath-protocol.h.  Of course this problem will go away
> when we move to Netlink proper.  This patch adds 4 new sparse
> warnings, all on things like this, so it might be easier if you just
> take a pass through it yourself.

OK, I made a pass and fixed these.

> > +int flow_copy_to_user(const struct sw_flow_key *swkey, struct nlattr __user *ukey, u32 ukey_len)
> 
> copy_to_user (and most other copy operations) use the convention dst,
> src, len.  Should we use that here as well?

OK, done.

> > +{
> > +       struct sk_buff *skb;
> > +       int retval;
> > +
> > +       skb = alloc_skb(FLOW_BUFSIZE, GFP_KERNEL);
> > +       if (!skb)
> > +               return -ENOMEM;
> > +
> > +       retval = flow_to_nlattrs(swkey, skb);
> > +       if (copy_to_user(ukey, skb->data, min(skb->len, ukey_len)))
> > +               retval = -EFAULT;
> > +       kfree_skb(skb);
> 
> If the copy fails, this will leak the skb.

I don't see how.

> > diff --git a/datapath/flow.h b/datapath/flow.h
> > index 0764482..f5a6bfe 100644
> > --- a/datapath/flow.h
> > +++ b/datapath/flow.h
> > +/* By my calculations currently the longest valid nlattr-formatted flow key is
> > + * 80 bytes long, so this leaves some safety margin. */
> > +#define FLOW_BUFSIZE 96
> 
> Multiline comments should be of the form:
> 
> /* XXX
>  * YYY
>  */

Fixed.

> > +
> > +u32 flow_to_nlattrs(const struct sw_flow_key *, struct sk_buff *);
> 
> Is this used anywhere outside of flow_copy_to_user()?

Not until the next patch.  I've moved this to the next patch.

> > diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
> > index 2dc3f83..e0ec9b2 100644
> > --- a/include/openvswitch/datapath-protocol.h
> > +++ b/include/openvswitch/datapath-protocol.h
> > +struct odp_key_ip {
> > +       ovs_be32 ip_src;
> > +       ovs_be32 ip_dst;
> > +       uint8_t  ip_proto;
> > +       uint8_t  ip_tos;
> > +};
> 
> Maybe this (and other places that refer to IP) should be IPv4?

OK, done, except for a few references to 'tos' since that field is
defined the same way (with different names) by the same RFC for both.

> > diff --git a/lib/flow.h b/lib/flow.h
> > index ee460e3..bbf35c7 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -35,6 +35,10 @@ struct ofpbuf;
> >  #define FLOW_N_REGS 4
> >  BUILD_ASSERT_DECL(FLOW_N_REGS <= NXM_NX_MAX_REGS);
> >
> > +/* Used for struct flow's dl_type member for frames that have no Ethernet
> > + * type, that is, pure 802.2 frames. */
> > +#define FLOW_DL_TYPE_NONE 0
> 
> OpenFlow uses 0x05ff to represent this.  Does it get translated anywhere?

Yes, odp_flow_key_{to,from}_flow do the translation correctly.

> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 09f8b83..7a56292 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > +/* Converts the 'key_len' bytes of ODPKT_* attributes in 'key' to a flow
> > + * structure in 'flow'.  Returns 0 if successful, otherwise EINVAL. */
> > +int
> > +odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
> > +                     struct flow *flow)
> >  {
> 
> [...]
> 
> > +    switch (prev_type) {
> > +    case ODPKT_UNSPEC:
> > +        return EINVAL;
> > +
> > +    case ODPKT_TUN_ID:
> > +    case ODPKT_IN_PORT:
> > +        return EINVAL;
> > +
> > +    case ODPKT_ETHERNET:
> > +    case ODPKT_8021Q:
> > +        return 0;
> > +
> > +    case ODPKT_ETHERTYPE:
> > +        if (flow->dl_type == htons(ETH_TYPE_IP)
> > +            || flow->dl_type == htons(ETH_TYPE_ARP)) {
> > +            return EINVAL;
> > +        }
> > +        return 0;
> > +
> > +    case ODPKT_IP:
> > +        if (flow->nw_proto == htons(IP_TYPE_TCP)
> > +            || flow->nw_proto == htons(IP_TYPE_UDP)
> > +            || flow->nw_proto == htons(IP_TYPE_ICMP)) {
> > +            return EINVAL;
> > +        }
> > +        return 0;
> > +
> > +    case ODPKT_TCP:
> > +    case ODPKT_UDP:
> > +    case ODPKT_ICMP:
> > +    case ODPKT_ARP:
> > +        return 0;
> > +
> > +    case __ODPKT_MAX:
> > +    default:
> > +        NOT_REACHED();
> > +    }
> 
> Is there a reason why we don't have this check in the kernel?

There's no reason why not; I've added it there now.

> > diff --git a/lib/odp-util.h b/lib/odp-util.h
> > index 6051c52..9dfe6db 100644
> > --- a/lib/odp-util.h
> > +++ b/lib/odp-util.h
> > -void odp_flow_key_from_flow(struct odp_flow_key *, const struct flow *);
> > -void odp_flow_key_to_flow(const struct odp_flow_key *, struct flow *);
> > +/* By my calculations currently the longest valid nlattr-formatted flow key is
> > + * 80 bytes long, so this leaves some safety margin.
> > + *
> > + * We allocate temporary on-stack buffers for flow keys as arrays of uint32_t
> > + * to ensure proper 32-bit alignment for Netlink attributes.  (An array of
> > + * "struct nlattr" might not, in theory, be sufficiently aligned because it
> > + * only contains 16-bit types.) */
> > +#define ODPUTIL_FLOW_KEY_BYTES 128
> > +#define ODPUTIL_FLOW_KEY_U32S DIV_ROUND_UP(ODPUTIL_FLOW_KEY_BYTES, 4)
> 
> It seems a little odd that this is different from the kernel flow buffer size.

I think I changed one without changing the other at some point.

I've set both of them to 96 bytes now.

I'll send out the revised version with the whole series later today.




More information about the dev mailing list