[ovs-dev] [PATCH] datapath: Avoid NULL mask check while building mask

Daniele Di Proietto ddiproietto at vmware.com
Fri Aug 8 16:25:37 UTC 2014


Hi Pravin,

couple of comments:

- I would remove also the check for match->mask in update_range__().
- I think braces are not necessary for if/else statements in the macros.

Of course I would wait for review by Jesse or Andy, but other than that,
LGTM.

Acked-by: Daniele Di Proietto <ddiproietto at vmware.com>


On 8/7/14, 9:26 PM, "Pravin B Shelar" <pshelar at nicira.com> wrote:

>OVS does mask validation even if it does not need to convert
>netlink mask attributes to mask structure.  ovs_nla_get_match()
>caller can pass NULL mask structure pointer if the caller does
>not need mask.  Therefore NULL check is required in SW_FLOW_KEY*
>macros.  Following patch does not convert mask netlink attributes
>if mask pointer is NULL, so we do not need these checks in
>SW_FLOW_KEY* macro.
>
>Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>---
> datapath/flow_netlink.c | 74
>++++++++++++++++++++++++++-----------------------
> 1 file changed, 39 insertions(+), 35 deletions(-)
>
>diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>index 75172de..63ab7a6 100644
>--- a/datapath/flow_netlink.c
>+++ b/datapath/flow_netlink.c
>@@ -84,8 +84,7 @@ static void update_range__(struct sw_flow_match *match,
> 		update_range__(match, offsetof(struct sw_flow_key, field),  \
> 				     sizeof((match)->key->field), is_mask); \
> 		if (is_mask) {						    \
>-			if ((match)->mask)				    \
>-				(match)->mask->key.field = value;	    \
>+			(match)->mask->key.field = value;		    \
> 		} else {                                                    \
> 			(match)->key->field = value;		            \
> 		}                                                           \
>@@ -95,10 +94,9 @@ static void update_range__(struct sw_flow_match *match,
> 	do { \
> 		update_range__(match, offset, len, is_mask);                \
> 		if (is_mask) {						    \
>-			if ((match)->mask)				    \
>-				memcpy((u8 *)&(match)->mask->key + offset, value_p, len);\
>+			memcpy((u8 *)&(match)->mask->key + offset, value_p, len);\
> 		} else {                                                    \
>-			memcpy((u8 *)(match)->key + offset, value_p, len);         \
>+			memcpy((u8 *)(match)->key + offset, value_p, len);  \
> 		}                                                           \
> 	} while (0)
> 
>@@ -111,9 +109,8 @@ static void update_range__(struct sw_flow_match
>*match,
> 		update_range__(match, offsetof(struct sw_flow_key, field),  \
> 				     sizeof((match)->key->field), is_mask); \
> 		if (is_mask) {						    \
>-			if ((match)->mask)				    \
>-				memset((u8 *)&(match)->mask->key.field, value,\
>-				       sizeof((match)->mask->key.field));   \
>+			memset((u8 *)&(match)->mask->key.field, value,\
>+			       sizeof((match)->mask->key.field));   \
> 		} else {                                                    \
> 			memset((u8 *)&(match)->key->field, value,           \
> 			       sizeof((match)->key->field));                \
>@@ -633,8 +630,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
>*match, u64 attrs,
> 
> 		SW_FLOW_KEY_PUT(match, eth.tci, tci, is_mask);
> 		attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
>-	} else if (!is_mask)
>-		SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
>+	}
> 
> 	if (attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) {
> 		__be16 eth_type;
>@@ -859,8 +855,8 @@ static void mask_set_nlattr(struct nlattr *attr, u8
>val)
>  * attribute specifies the mask field of the wildcarded flow.
>  */
> int ovs_nla_get_match(struct sw_flow_match *match,
>-		      const struct nlattr *key,
>-		      const struct nlattr *mask)
>+		      const struct nlattr *nla_key,
>+		      const struct nlattr *nla_mask)
> {
> 	const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
> 	const struct nlattr *encap;
>@@ -870,7 +866,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
> 	bool encap_valid = false;
> 	int err;
> 
>-	err = parse_flow_nlattrs(key, a, &key_attrs);
>+	err = parse_flow_nlattrs(nla_key, a, &key_attrs);
> 	if (err)
> 		return err;
> 
>@@ -911,35 +907,43 @@ int ovs_nla_get_match(struct sw_flow_match *match,
> 	if (err)
> 		return err;
> 
>-	if (match->mask && !mask) {
>-		/* Create an exact match mask. We need to set to 0xff all the
>-		 * 'match->mask' fields that have been touched in 'match->key'.
>-		 * We cannot simply memset 'match->mask', because padding bytes
>-		 * and fields not specified in 'match->key' should be left to 0.
>-		 * Instead, we use a stream of netlink attributes, copied from
>-		 * 'key' and set to 0xff: ovs_key_from_nlattrs() will take care
>-		 * of filling 'match->mask' appropriately.
>-		 */
>-		newmask = kmemdup(key, nla_total_size(nla_len(key)),
>-				  GFP_KERNEL);
>-		if (!newmask)
>-			return -ENOMEM;
>+	if (match->mask) {
>+
>+		if (!nla_mask) {
>+			/* Create an exact match mask. We need to set to 0xff
>+			 * all the 'match->mask' fields that have been touched
>+			 * in 'match->key'. We cannot simply memset
>+			 * 'match->mask', because padding bytes and fields not
>+			 * specified in 'match->key' should be left to 0.
>+			 * Instead, we use a stream of netlink attributes,
>+			 * copied from 'key' and set to 0xff: ovs_key_from_nlattrs()
>+			 * will take care of filling 'match->mask' appropriately.
>+			 */
>+			newmask = kmemdup(nla_key,
>+					  nla_total_size(nla_len(nla_key)),
>+					  GFP_KERNEL);
>+			if (!newmask)
>+				return -ENOMEM;
> 
>-		mask_set_nlattr(newmask, 0xff);
>+			mask_set_nlattr(newmask, 0xff);
> 
>-		/* The userspace does not send tunnel attributes that are 0,
>-		 * but we should not wildcard them nonetheless. */
>-		if (match->key->tun_key.ipv4_dst)
>-			SW_FLOW_KEY_MEMSET_FIELD(match, tun_key, 0xff, true);
>+			/* The userspace does not send tunnel attributes that
>+			 * are 0, but we should not wildcard them nonetheless.
>+			 */
>+			if (match->key->tun_key.ipv4_dst)
>+				SW_FLOW_KEY_MEMSET_FIELD(match, tun_key,
>+							 0xff, true);
> 
>-		mask = newmask;
>-	}
>+			nla_mask = newmask;
>+		}
> 
>-	if (mask) {
>-		err = parse_flow_mask_nlattrs(mask, a, &mask_attrs);
>+		err = parse_flow_mask_nlattrs(nla_mask, a, &mask_attrs);
> 		if (err)
> 			goto free_newmask;
> 
>+		/* Always match on tci. */
>+		SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
>+
> 		if (mask_attrs & 1ULL << OVS_KEY_ATTR_ENCAP) {
> 			__be16 eth_type = 0;
> 			__be16 tci = 0;
>-- 
>1.9.3
>
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/
>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5z%2BU
>6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=9WggZ4ZovrrQ5GRI9bcKsMv%2BY8JSaoZB1XECseA
>iUS4%3D%0A&s=06b570a825ecaa3421698357c4647851c197cace2bf3cf1b28d6900c0e5c0
>cc1




More information about the dev mailing list