[ovs-dev] [RFC] datapath/flow_netlink: Create right mask with disabled megaflows

Daniele Di Proietto ddiproietto at vmware.com
Wed Jul 9 22:30:36 UTC 2014


If megaflows are disabled, the userspace does not send the netlink attribute
OVS_FLOW_ATTR_MASK, and the kernel must create an exact match mask.

sw_flow_mask_set() sets every bytes (in 'range') of the mask to 0xff, even the
bytes that represent padding for struct sw_flow, or the bytes that represent
fields that may not be set during ovs_flow_extract().
This is a problem, because when we extract a flow from a packet,
we do not memset() anymore the struct sw_flow to 0 (since commit 9cef26ac6a71).

This commit gets rid of sw_flow_mask_set() and introduces mask_set_nlattr(),
which operates on the netlink attributes rather than on the mask key. Using
this approach we are sure that only the bytes that the user provided in the
flow are matched.

Also, if the parse_flow_mask_nlattrs() for the mask ENCAP attribute fails, we
now return with an error.

Reported-by: Alex Wang <alexw at nicira.com>
Suggested-by: Pravin B Shelar <pshelar at nicira.com>
Signed-off-by: Daniele Di Proietto <ddiproietto at vmware.com>
---
We chose this solution because it seemed generic and immune to changes in
struct sw_flow_key. It only needs knowledge of which OVS_KEY_ATTR_* are nested.

Given that supporting properly operations with disabled megaflows is only
useful for testing purposes, I would prefer discussing more rather that
including an improper fix.

What do you guys think?
---
 datapath/flow_netlink.c | 75 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 21 deletions(-)

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index f0cf2ed..4f0618d 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -106,11 +106,6 @@ static void update_range__(struct sw_flow_match *match,
 	SW_FLOW_KEY_MEMCPY_OFFSET(match, offsetof(struct sw_flow_key, field), \
 				  value_p, len, is_mask)
 
-static u16 range_n_bytes(const struct sw_flow_key_range *range)
-{
-	return range->end - range->start;
-}
-
 static bool match_validate(const struct sw_flow_match *match,
 			   u64 key_attrs, u64 mask_attrs)
 {
@@ -732,7 +727,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
 				mpls_key->mpls_lse, is_mask);
 
 		attrs &= ~(1ULL << OVS_KEY_ATTR_MPLS);
-	 }
+	}
 
 	if (attrs & (1ULL << OVS_KEY_ATTR_TCP)) {
 		const struct ovs_key_tcp *tcp_key;
@@ -814,13 +809,32 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
 	return 0;
 }
 
-static void sw_flow_mask_set(struct sw_flow_mask *mask,
-			     struct sw_flow_key_range *range, u8 val)
+/* We expect the nlattr stream to be already validated */
+static int nlattr_set(struct nlattr *attr, u8 val, bool parent)
 {
-	u8 *m = (u8 *)&mask->key + range->start;
+	struct nlattr *nla;
+	int rem;
+
+	nla_for_each_nested(nla, attr, rem) {
+		u16 type = nla_type(nla);
+
+		/* If we are parsing the parent attribute, we may encounter
+		 * nested attributes. To deal with them we call ourselves
+		 */
+		if (parent && (type == OVS_KEY_ATTR_ENCAP ||
+			       type == OVS_KEY_ATTR_TUNNEL)) {
+			nlattr_set(nla, val, false);
+		} else {
+			memset(nla_data(nla), val, nla_len(nla));
+		}
+	}
 
-	mask->range = *range;
-	memset(m, val, range_n_bytes(range));
+	return 0;
+}
+
+static int mask_set_nlattr(struct nlattr *attr, u8 val)
+{
+	return nlattr_set(attr, val, true);
 }
 
 /**
@@ -841,6 +855,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 {
 	const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
 	const struct nlattr *encap;
+	struct nlattr *newmask = NULL;
 	u64 key_attrs = 0;
 	u64 mask_attrs = 0;
 	bool encap_valid = false;
@@ -887,18 +902,35 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 	if (err)
 		return err;
 
+	if (match->mask && !mask) {
+		/* Create mask by cloning key attributes and setting them to
+		 * 0xff.
+		 */
+		newmask = kmemdup(key, nla_total_size(nla_len(key)),
+				  GFP_KERNEL);
+		if (!newmask)
+			return -ENOMEM;
+
+		err = mask_set_nlattr(newmask, 0xff);
+		if (err)
+			goto free_newmask;
+
+		mask = newmask;
+	}
+
 	if (mask) {
 		err = parse_flow_mask_nlattrs(mask, a, &mask_attrs);
 		if (err)
 			return err;
 
-		if (mask_attrs & 1ULL << OVS_KEY_ATTR_ENCAP)  {
+		if (mask_attrs & 1ULL << OVS_KEY_ATTR_ENCAP) {
 			__be16 eth_type = 0;
 			__be16 tci = 0;
 
 			if (!encap_valid) {
 				OVS_NLERR("Encap mask attribute is set for non-VLAN frame.\n");
-				return  -EINVAL;
+				err = -EINVAL;
+				goto free_newmask;
 			}
 
 			mask_attrs &= ~(1ULL << OVS_KEY_ATTR_ENCAP);
@@ -909,10 +941,12 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 				mask_attrs &= ~(1ULL << OVS_KEY_ATTR_ETHERTYPE);
 				encap = a[OVS_KEY_ATTR_ENCAP];
 				err = parse_flow_mask_nlattrs(encap, a, &mask_attrs);
+				goto free_newmask;
 			} else {
 				OVS_NLERR("VLAN frames must have an exact match on the TPID (mask=%x).\n",
 						ntohs(eth_type));
-				return -EINVAL;
+				err = -EINVAL;
+				goto free_newmask;
 			}
 
 			if (a[OVS_KEY_ATTR_VLAN])
@@ -920,23 +954,22 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 
 			if (!(tci & htons(VLAN_TAG_PRESENT))) {
 				OVS_NLERR("VLAN tag present bit must have an exact match (tci_mask=%x).\n", ntohs(tci));
-				return -EINVAL;
+				err = -EINVAL;
+				goto free_newmask;
 			}
 		}
 
 		err = ovs_key_from_nlattrs(match, mask_attrs, a, true);
 		if (err)
-			return err;
-	} else {
-		/* Populate exact match flow's key mask. */
-		if (match->mask)
-			sw_flow_mask_set(match->mask, &match->range, 0xff);
+			goto free_newmask;
 	}
 
 	if (!match_validate(match, key_attrs, mask_attrs))
 		return -EINVAL;
 
-	return 0;
+free_newmask:
+	kfree(newmask);
+	return err;
 }
 
 /**
-- 
2.0.0




More information about the dev mailing list