[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