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

Jesse Gross jesse at nicira.com
Tue Dec 28 07:13:47 UTC 2010


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.

> +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?

> +{
> +       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.

> 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
 */

> +
> +u32 flow_to_nlattrs(const struct sw_flow_key *, struct sk_buff *);

Is this used anywhere outside of flow_copy_to_user()?

> 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?

> 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?

> 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?

>  }
> 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.




More information about the dev mailing list