[PATCH net v2] openvswitch: Zero flows on allocation.

[PATCH net v2] openvswitch: Zero flows on allocation.

[Jesse Gross]

When support for megaflows was introduced, OVS needed to start
installing flows with a mask applied to them. Since masking is an
expensive operation, OVS also had an optimization that would only
take the parts of the flow keys that were covered by a non-zero
mask. The values stored in the remaining pieces should not matter
because they are masked out.

While this works fine for the purposes of matching (which must always
look at the mask), serialization to netlink can be problematic. Since
the flow and the mask are serialized separately, the uninitialized
portions of the flow can be encoded with whatever values happen to be
present.

In terms of functionality, this has little effect since these fields
will be masked out by definition. However, it leaks kernel memory to
userspace, which is a potential security vulnerability. It is also
possible that other code paths could look at the masked key and get
uninitialized data, although this does not currently appear to be an
issue in practice.

This removes the mask optimization for flows that are being installed.
This was always intended to be the case as the mask optimizations were
really targetting per-packet flow operations.

Fixes: 03f0d916 ("openvswitch: Mega flow implementation")
Signed-off-by: Jesse Gross <jesse@nicira.com>
---
v2: This uses a different strategy from v1 since I realized that there
are other code paths that could theoretically hit the same problem.
Depending on the situation, this might also be faster too since it zeros
less memory.
---
 net/openvswitch/datapath.c   |  4 ++--
 net/openvswitch/flow_table.c | 23 ++++++++++++-----------
 net/openvswitch/flow_table.h |  2 +-
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 6fbd2de..b816ff8 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -952,7 +952,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
 	if (error)
 		goto err_kfree_flow;
 
-	ovs_flow_mask_key(&new_flow->key, &key, &mask);
+	ovs_flow_mask_key(&new_flow->key, &key, true, &mask);
 
 	/* Extract flow identifier. */
 	error = ovs_nla_get_identifier(&new_flow->id, a[OVS_FLOW_ATTR_UFID],
@@ -1080,7 +1080,7 @@ static struct sw_flow_actions *get_flow_actions(struct net *net,
 	struct sw_flow_key masked_key;
 	int error;
 
-	ovs_flow_mask_key(&masked_key, key, mask);
+	ovs_flow_mask_key(&masked_key, key, true, mask);
 	error = ovs_nla_copy_actions(net, a, &masked_key, &acts, log);
 	if (error) {
 		OVS_NLERR(log,
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index d22d8e9..f2ea83b 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -57,20 +57,21 @@ static u16 range_n_bytes(const struct sw_flow_key_range *range)
 }
 
 void ovs_flow_mask_key(struct sw_flow_key *dst, const struct sw_flow_key *src,
-		       const struct sw_flow_mask *mask)
+		       bool full, const struct sw_flow_mask *mask)
 {
-	const long *m = (const long *)((const u8 *)&mask->key +
-				mask->range.start);
-	const long *s = (const long *)((const u8 *)src +
-				mask->range.start);
-	long *d = (long *)((u8 *)dst + mask->range.start);
+	int start = full ? 0 : mask->range.start;
+	int len = full ? sizeof *dst : range_n_bytes(&mask->range);
+	const long *m = (const long *)((const u8 *)&mask->key + start);
+	const long *s = (const long *)((const u8 *)src + start);
+	long *d = (long *)((u8 *)dst + start);
 	int i;
 
-	/* The memory outside of the 'mask->range' are not set since
-	 * further operations on 'dst' only uses contents within
-	 * 'mask->range'.
+	/* If 'full' is true then all of 'dst' is fully initialized. Otherwise,
+	 * if 'full' is false the memory outside of the 'mask->range' is left
+	 * uninitialized. This can be used as an optimization when further
+	 * operations on 'dst' only use contents within 'mask->range'.
 	 */
-	for (i = 0; i < range_n_bytes(&mask->range); i += sizeof(long))
+	for (i = 0; i < len; i += sizeof(long))
 		*d++ = *s++ & *m++;
 }
 
@@ -475,7 +476,7 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
 	u32 hash;
 	struct sw_flow_key masked_key;
 
-	ovs_flow_mask_key(&masked_key, unmasked, mask);
+	ovs_flow_mask_key(&masked_key, unmasked, false, mask);
 	hash = flow_hash(&masked_key, &mask->range);
 	head = find_bucket(ti, hash);
 	hlist_for_each_entry_rcu(flow, head, flow_table.node[ti->node_ver]) {
diff --git a/net/openvswitch/flow_table.h b/net/openvswitch/flow_table.h
index 616eda1..2dd9900 100644
--- a/net/openvswitch/flow_table.h
+++ b/net/openvswitch/flow_table.h
@@ -86,5 +86,5 @@ struct sw_flow *ovs_flow_tbl_lookup_ufid(struct flow_table *,
 bool ovs_flow_cmp(const struct sw_flow *, const struct sw_flow_match *);
 
 void ovs_flow_mask_key(struct sw_flow_key *dst, const struct sw_flow_key *src,
-		       const struct sw_flow_mask *mask);
+		       bool full, const struct sw_flow_mask *mask);
 #endif /* flow_table.h */
-- 
2.1.4

Re: [PATCH net v2] openvswitch: Zero flows on allocation.

[Pravin Shelar]

On Mon, Sep 21, 2015 at 8:21 PM, Jesse Gross <jesse@nicira.com> wrote:
> When support for megaflows was introduced, OVS needed to start
> installing flows with a mask applied to them. Since masking is an
> expensive operation, OVS also had an optimization that would only
> take the parts of the flow keys that were covered by a non-zero
> mask. The values stored in the remaining pieces should not matter
> because they are masked out.
>
> While this works fine for the purposes of matching (which must always
> look at the mask), serialization to netlink can be problematic. Since
> the flow and the mask are serialized separately, the uninitialized
> portions of the flow can be encoded with whatever values happen to be
> present.
>
> In terms of functionality, this has little effect since these fields
> will be masked out by definition. However, it leaks kernel memory to
> userspace, which is a potential security vulnerability. It is also
> possible that other code paths could look at the masked key and get
> uninitialized data, although this does not currently appear to be an
> issue in practice.
>
> This removes the mask optimization for flows that are being installed.
> This was always intended to be the case as the mask optimizations were
> really targetting per-packet flow operations.
>
> Fixes: 03f0d916 ("openvswitch: Mega flow implementation")
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCH net v2] openvswitch: Zero flows on allocation.

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Mon, 21 Sep 2015 20:21:20 -0700

> When support for megaflows was introduced, OVS needed to start
> installing flows with a mask applied to them. Since masking is an
> expensive operation, OVS also had an optimization that would only
> take the parts of the flow keys that were covered by a non-zero
> mask. The values stored in the remaining pieces should not matter
> because they are masked out.
> 
> While this works fine for the purposes of matching (which must always
> look at the mask), serialization to netlink can be problematic. Since
> the flow and the mask are serialized separately, the uninitialized
> portions of the flow can be encoded with whatever values happen to be
> present.
> 
> In terms of functionality, this has little effect since these fields
> will be masked out by definition. However, it leaks kernel memory to
> userspace, which is a potential security vulnerability. It is also
> possible that other code paths could look at the masked key and get
> uninitialized data, although this does not currently appear to be an
> issue in practice.
> 
> This removes the mask optimization for flows that are being installed.
> This was always intended to be the case as the mask optimizations were
> really targetting per-packet flow operations.
> 
> Fixes: 03f0d916 ("openvswitch: Mega flow implementation")
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Applied and queued up for -stable, thanks.