[ovs-dev] [encap 3/4] datapath: Allow flow key Netlink attributes to appear in any order.

Ben Pfaff blp at nicira.com
Sat Nov 12 20:35:33 UTC 2011


On Fri, Nov 11, 2011 at 05:10:06PM -0800, Jesse Gross wrote:
> On Fri, Nov 11, 2011 at 2:26 PM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index 9786c37..96dd716 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > ??int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??const struct nlattr *attr)
> [...]
> > + ?? ?? ?? ?? ?? ?? ?? if (type > OVS_KEY_ATTR_MAX || attrs & (1ULL << type) ||
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? nla_len(nla) < ovs_key_lens[type])
> 
> I don't think that we want to accept keys that are larger than our
> length.  Although it may be used for extensibility in other places,
> this is all exact match and therefore not permissive.

OK, fixed.

> > + ?? ?? ?? if (attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) {
> > + ?? ?? ?? ?? ?? ?? ?? swkey->eth.type = nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]);
> > + ?? ?? ?? ?? ?? ?? ?? if (ntohs(swkey->eth.type) < 1536)
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? return -EINVAL;
> > + ?? ?? ?? ?? ?? ?? ?? attrs &= ~(1 << OVS_KEY_ATTR_ETHERTYPE);
> > + ?? ?? ?? } else
> > + ?? ?? ?? ?? ?? ?? ?? swkey->eth.type = htons(ETH_P_802_2);
> 
> I think if one half of the if block uses curly braces then the entire
> thing is supposed to.

OK, fixed.  (I think we have one or two violations of that rule
elsewhere in datapath/.)

> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index a8da627..9830c12 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > +static void
> > +log_odp_key_attributes(struct vlog_rate_limit *rl, const char *title,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? uint32_t attrs,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? const struct nlattr *key, size_t key_len)
> > +{
> > + ?? ??struct ds s;
> > + ?? ??int i;
> > +
> > + ?? ??if (VLOG_DROP_WARN(rl)) {
> > + ?? ?? ?? ??return;
> > + ?? ??}
> > +
> > + ?? ??ds_init(&s);
> > + ?? ??for (i = 0; i < 32; i++) {
> > + ?? ?? ?? ??if (attrs & (1u << i)) {
> > + ?? ?? ?? ?? ?? ??ds_put_format(&s, " %s", ovs_key_attr_to_string(i));
> 
> Don't you want the space after the attribute name?

I intended for the attribute names to go directly after a colon, so that
the space would be correct, but I screwed up the formatting elsewhere,
so it still needed fixing.

I fixed it in the following patch (noticed during testing), forgetting
that I'd added this function in this patch.

I've moved the fix to this patch now.

> > ??static bool
> > ??odp_to_ovs_frag(uint8_t odp_frag, struct flow *flow)
> > ??{
> > + ?? ??static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > +
> > ?? ?? if (odp_frag > OVS_FRAG_TYPE_LATER) {
> > + ?? ?? ?? ??VLOG_ERR_RL(&rl, "invalid frag %"PRIu8, odp_frag);
> 
> Should we prepend some kind of description to these types of error
> message?  Otherwise, I'm not sure that "invalid frag" provides that
> much of a clue to most people.

OK, I've added "in flow key" to the log message here and elsewhere.
I'm not sure that it's too valuable to describe the problem in detail.
Most people will just file a bug report, and when that gets to the right
person he'll run grep.

> > +/* We don't use nl_policy_parse() so the .optional members don't matter. */
> > +static const struct nl_policy odp_key_policy[] = {
> > + ?? ??[OVS_KEY_ATTR_PRIORITY] = { .type = NL_A_U32 },
> > + ?? ??[OVS_KEY_ATTR_TUN_ID] = { .type = NL_A_BE64 },
> > + ?? ??[OVS_KEY_ATTR_IN_PORT] = { .type = NL_A_U32 },
> > + ?? ??[OVS_KEY_ATTR_ETHERNET] = { NL_POLICY_FOR(struct ovs_key_ethernet) },
> > + ?? ??[OVS_KEY_ATTR_8021Q] = { NL_POLICY_FOR(struct ovs_key_8021q) },
> > + ?? ??[OVS_KEY_ATTR_ETHERTYPE] = { .type = NL_A_BE16 },
> > + ?? ??[OVS_KEY_ATTR_IPV4] = { NL_POLICY_FOR(struct ovs_key_ipv4) },
> > + ?? ??[OVS_KEY_ATTR_IPV6] = { NL_POLICY_FOR(struct ovs_key_ipv6) },
> > + ?? ??[OVS_KEY_ATTR_TCP] = { NL_POLICY_FOR(struct ovs_key_tcp) },
> > + ?? ??[OVS_KEY_ATTR_UDP] = { NL_POLICY_FOR(struct ovs_key_udp) },
> > + ?? ??[OVS_KEY_ATTR_ICMP] = { NL_POLICY_FOR(struct ovs_key_icmp) },
> > + ?? ??[OVS_KEY_ATTR_ICMPV6] = { NL_POLICY_FOR(struct ovs_key_icmpv6) },
> > + ?? ??[OVS_KEY_ATTR_ARP] = { NL_POLICY_FOR(struct ovs_key_arp) },
> > + ?? ??[OVS_KEY_ATTR_ND] = { NL_POLICY_FOR(struct ovs_key_nd) },
> > +};
> 
> Is there an easy way to combine this with odp_flow_key_attr_len()?

Hmm, it's actually entirely redundant, good point.

I changed odp_flow_key_to_flow() to use odp_flow_key_attr_len() and
deleted the policy array.

> > ??int
> > ??odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??struct flow *flow)
> [...]
> 
> In the kernel we assign the 'none' type for ip_port and dl_type in the
> place where we find that there is no attribute for it instead of ahead
> of time, which seems a little nicer to me.

OK, changed.

I've been thinking that we could just use 0 instead of a special
constant for "none" in the ethernet type case, by the way.  There's no
particular reason to use 0x5ff as we do in userspace.

> > + ?? ??missing_attrs = present_attrs & ~expected_attrs;
> > + ?? ??if (missing_attrs) {
> > + ?? ?? ?? ??static struct vlog_rate_limit miss_rl = VLOG_RATE_LIMIT_INIT(10, 10);
> > + ?? ?? ?? ??log_odp_key_attributes(&miss_rl, "expected but not present",
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? missing_attrs, key, key_len);
> > + ?? ?? ?? ??return EINVAL;
> > + ?? ??}
> >
> > - ?? ??case __OVS_KEY_ATTR_MAX:
> > - ?? ??default:
> > - ?? ?? ?? ??NOT_REACHED();
> > + ?? ??extra_attrs = expected_attrs & ~present_attrs;
> > + ?? ??if (extra_attrs) {
> > + ?? ?? ?? ??static struct vlog_rate_limit extra_rl = VLOG_RATE_LIMIT_INIT(10, 10);
> > + ?? ?? ?? ??log_odp_key_attributes(&extra_rl, "present but not expected",
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? extra_attrs, key, key_len);
> > + ?? ?? ?? ??return EINVAL;
> 
> Aren't these two conditions reversed?

Yes, oops, fixed.

Here's an incremental, not tested beyond compiling:

--8<--------------------------cut here-------------------------->8--

diff --git a/datapath/flow.c b/datapath/flow.c
index 96dd716..6caca2a 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -993,7 +993,7 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
 		u16 type = nla_type(nla);
 
 		if (type > OVS_KEY_ATTR_MAX || attrs & (1ULL << type) ||
-		    nla_len(nla) < ovs_key_lens[type])
+		    nla_len(nla) != ovs_key_lens[type])
 			return -EINVAL;
 		attrs |= 1ULL << type;
 		a[type] = nla;
@@ -1012,8 +1012,9 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
 			return -EINVAL;
 		swkey->phy.in_port = in_port;
 		attrs &= ~(1 << OVS_KEY_ATTR_IN_PORT);
-	} else
+	} else {
 		swkey->phy.in_port = USHRT_MAX;
+	}
 
 	if (attrs & (1ULL << OVS_KEY_ATTR_TUN_ID)) {
 		swkey->phy.tun_id = nla_get_be64(a[OVS_KEY_ATTR_TUN_ID]);
@@ -1048,8 +1049,10 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
 		if (ntohs(swkey->eth.type) < 1536)
 			return -EINVAL;
 		attrs &= ~(1 << OVS_KEY_ATTR_ETHERTYPE);
-	} else
+	} else {
 		swkey->eth.type = htons(ETH_P_802_2);
+	}
+
 
 	if (swkey->eth.type == htons(ETH_P_IP)) {
 		const struct ovs_key_ipv4 *ipv4_key;
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 9830c12..c70ab11 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -998,6 +998,7 @@ log_odp_key_attributes(struct vlog_rate_limit *rl, const char *title,
     }
 
     ds_init(&s);
+    ds_put_format(&s, "%s:", title);
     for (i = 0; i < 32; i++) {
         if (attrs & (1u << i)) {
             ds_put_format(&s, " %s", ovs_key_attr_to_string(i));
@@ -1007,7 +1008,7 @@ log_odp_key_attributes(struct vlog_rate_limit *rl, const char *title,
     ds_put_cstr(&s, ": ");
     odp_flow_key_format(key, key_len, &s);
 
-    VLOG_WARN("%s: %s", title, ds_cstr(&s));
+    VLOG_WARN("%s", ds_cstr(&s));
     ds_destroy(&s);
 }
 
@@ -1017,7 +1018,8 @@ odp_to_ovs_frag(uint8_t odp_frag, struct flow *flow)
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 
     if (odp_frag > OVS_FRAG_TYPE_LATER) {
-        VLOG_ERR_RL(&rl, "invalid frag %"PRIu8, odp_frag);
+        VLOG_ERR_RL(&rl, "invalid frag %"PRIu8" in flow key",
+                    odp_frag);
         return false;
     }
 
@@ -1030,24 +1032,6 @@ odp_to_ovs_frag(uint8_t odp_frag, struct flow *flow)
     return true;
 }
 
-/* We don't use nl_policy_parse() so the .optional members don't matter. */
-static const struct nl_policy odp_key_policy[] = {
-    [OVS_KEY_ATTR_PRIORITY] = { .type = NL_A_U32 },
-    [OVS_KEY_ATTR_TUN_ID] = { .type = NL_A_BE64 },
-    [OVS_KEY_ATTR_IN_PORT] = { .type = NL_A_U32 },
-    [OVS_KEY_ATTR_ETHERNET] = { NL_POLICY_FOR(struct ovs_key_ethernet) },
-    [OVS_KEY_ATTR_8021Q] = { NL_POLICY_FOR(struct ovs_key_8021q) },
-    [OVS_KEY_ATTR_ETHERTYPE] = { .type = NL_A_BE16 },
-    [OVS_KEY_ATTR_IPV4] = { NL_POLICY_FOR(struct ovs_key_ipv4) },
-    [OVS_KEY_ATTR_IPV6] = { NL_POLICY_FOR(struct ovs_key_ipv6) },
-    [OVS_KEY_ATTR_TCP] = { NL_POLICY_FOR(struct ovs_key_tcp) },
-    [OVS_KEY_ATTR_UDP] = { NL_POLICY_FOR(struct ovs_key_udp) },
-    [OVS_KEY_ATTR_ICMP] = { NL_POLICY_FOR(struct ovs_key_icmp) },
-    [OVS_KEY_ATTR_ICMPV6] = { NL_POLICY_FOR(struct ovs_key_icmpv6) },
-    [OVS_KEY_ATTR_ARP] = { NL_POLICY_FOR(struct ovs_key_arp) },
-    [OVS_KEY_ATTR_ND] = { NL_POLICY_FOR(struct ovs_key_nd) },
-};
-
 /* Converts the 'key_len' bytes of OVS_KEY_ATTR_* attributes in 'key' to a flow
  * structure in 'flow'.  Returns 0 if successful, otherwise EINVAL. */
 int
@@ -1055,7 +1039,7 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
                      struct flow *flow)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-    const struct nlattr *attrs[ARRAY_SIZE(odp_key_policy)];
+    const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1];
     const struct nlattr *nla;
     uint64_t expected_attrs;
     uint64_t present_attrs;
@@ -1064,8 +1048,6 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
     size_t left;
 
     memset(flow, 0, sizeof *flow);
-    flow->in_port = OFPP_NONE;
-    flow->dl_type = htons(FLOW_DL_TYPE_NONE);
 
     memset(attrs, 0, sizeof attrs);
 
@@ -1073,19 +1055,23 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
     expected_attrs = 0;
     NL_ATTR_FOR_EACH (nla, left, key, key_len) {
         uint16_t type = nl_attr_type(nla);
-
-        if (type >= ARRAY_SIZE(odp_key_policy)
-            || odp_key_policy[type].type == NL_A_NO_ATTR) {
-            VLOG_ERR_RL(&rl, "unknown attribute %"PRIu16" in flow key", type);
+        size_t len = nl_attr_get_size(nla);
+        int expected_len = odp_flow_key_attr_len(type);
+
+        if (len != expected_len) {
+            if (expected_len == -1) {
+                VLOG_ERR_RL(&rl, "unknown attribute %"PRIu16" in flow key",
+                            type);
+            } else {
+                VLOG_ERR_RL(&rl, "attribute %s has length %zu but should have "
+                            "length %d", ovs_key_attr_to_string(type),
+                            len, expected_len);
+            }
             return EINVAL;
         } else if (present_attrs & (UINT64_C(1) << type)) {
             VLOG_ERR_RL(&rl, "duplicate %s attribute in flow key",
                         ovs_key_attr_to_string(type));
             return EINVAL;
-        } else if (!nl_attr_validate(nla, &odp_key_policy[type])) {
-            VLOG_ERR_RL(&rl, "%s attribute violates policy",
-                        ovs_key_attr_to_string(type));
-            return EINVAL;
         }
 
         present_attrs |= UINT64_C(1) << type;
@@ -1115,6 +1101,8 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
         }
         flow->in_port = odp_port_to_ofp_port(in_port);
         expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IN_PORT;
+    } else {
+        flow->in_port = OFPP_NONE;
     }
 
     if (attrs[OVS_KEY_ATTR_ETHERNET]) {
@@ -1124,7 +1112,7 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
         memcpy(flow->dl_src, eth_key->eth_src, ETH_ADDR_LEN);
         memcpy(flow->dl_dst, eth_key->eth_dst, ETH_ADDR_LEN);
     } else {
-        VLOG_ERR_RL(&rl, "missing Ethernet key");
+        VLOG_ERR_RL(&rl, "missing Ethernet attribute in flow key");
         return EINVAL;
     }
     expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET;
@@ -1134,12 +1122,12 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
 
         q_key = nl_attr_get(attrs[OVS_KEY_ATTR_8021Q]);
         if (q_key->q_tpid != htons(ETH_TYPE_VLAN)) {
-            VLOG_ERR_RL(&rl, "unsupported 802.1Q TPID %"PRIu16,
+            VLOG_ERR_RL(&rl, "unsupported 802.1Q TPID %"PRIu16" in flow key",
                         ntohs(q_key->q_tpid));
             return EINVAL;
         }
         if (q_key->q_tci & htons(VLAN_CFI)) {
-            VLOG_ERR_RL(&rl, "invalid 802.1Q TCI %"PRIu16,
+            VLOG_ERR_RL(&rl, "invalid 802.1Q TCI %"PRIu16" in flow key",
                         ntohs(q_key->q_tci));
             return EINVAL;
         }
@@ -1150,11 +1138,13 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
     if (attrs[OVS_KEY_ATTR_ETHERTYPE]) {
         flow->dl_type = nl_attr_get_be16(attrs[OVS_KEY_ATTR_ETHERTYPE]);
         if (ntohs(flow->dl_type) < 1536) {
-            VLOG_ERR_RL(&rl, "invalid Ethertype %"PRIu16,
+            VLOG_ERR_RL(&rl, "invalid Ethertype %"PRIu16" in flow key",
                         ntohs(flow->dl_type));
             return EINVAL;
         }
         expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE;
+    } else {
+        flow->dl_type = htons(FLOW_DL_TYPE_NONE);
     }
 
     if (flow->dl_type == htons(ETH_TYPE_IP)) {
@@ -1197,8 +1187,8 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
             flow->nw_src = arp_key->arp_sip;
             flow->nw_dst = arp_key->arp_tip;
             if (arp_key->arp_op & htons(0xff00)) {
-                VLOG_ERR_RL(&rl, "unsupported ARP opcode %"PRIu16,
-                            ntohs(arp_key->arp_op));
+                VLOG_ERR_RL(&rl, "unsupported ARP opcode %"PRIu16" in flow "
+                            "key", ntohs(arp_key->arp_op));
                 return EINVAL;
             }
             flow->nw_proto = ntohs(arp_key->arp_op);
@@ -1269,7 +1259,7 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
         }
     }
 
-    missing_attrs = present_attrs & ~expected_attrs;
+    missing_attrs = expected_attrs & ~present_attrs;
     if (missing_attrs) {
         static struct vlog_rate_limit miss_rl = VLOG_RATE_LIMIT_INIT(10, 10);
         log_odp_key_attributes(&miss_rl, "expected but not present",
@@ -1277,7 +1267,7 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
         return EINVAL;
     }
 
-    extra_attrs = expected_attrs & ~present_attrs;
+    extra_attrs = present_attrs & ~expected_attrs;
     if (extra_attrs) {
         static struct vlog_rate_limit extra_rl = VLOG_RATE_LIMIT_INIT(10, 10);
         log_odp_key_attributes(&extra_rl, "present but not expected",



More information about the dev mailing list