datapath: Use masked flow when validating actions.

It is important to validate flow actions to ensure that they do
not try to write off the end of a packet. The mechanism to do this
is to ensure that a flow is precise enough to describe valid vs.
invalid packets and only allowing actions on valid flows.

The introduction of megaflows broke this by using a narrow base
flow but a potentially wide match. This meant that while the
original flow was properly validated, later packets might not
conform to that flow and could be truncated. This switches to
using the masked flow instead, effectively requiring that all
possible matching packets be valid in order for a flow's actions
to be accepted.

This change only affects the flow setup path - executed packets
have always used the flow extracted from the packet and therefore
were properly validated.

Signed-off-by: Jesse Gross <jesse@nicira.com>

diff --git a/datapath/datapath.c b/datapath/datapath.c
index ba8da99..2b018c9 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1246,7 +1246,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
 {
        struct nlattr **a = info->attrs;
        struct ovs_header *ovs_header = info->userhdr;
-       struct sw_flow_key key;
+       struct sw_flow_key key, masked_key;
        struct sw_flow *flow = NULL;
        struct sw_flow_mask mask;
        struct sk_buff *reply;
@@ -1274,9 +1274,13 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
                if (IS_ERR(acts))
                        goto error;
 
-               error = validate_and_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &key,  0, &acts);
-               if (error)
+               ovs_flow_key_mask(&masked_key, &key, &mask);
+               error = validate_and_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
+                                                 &masked_key, 0, &acts);
+               if (error) {
+                       OVS_NLERR("Flow actions may not be safe on all matching packets.\n");
                        goto err_kfree;
+               }
        } else if (info->genlhdr->cmd == OVS_FLOW_CMD_NEW) {
                error = -EINVAL;
                goto error;
@@ -1319,6 +1323,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
                }
                clear_stats(flow);
 
+               flow->key = masked_key;
+               flow->unmasked_key = key;
+
                /* Make sure mask is unique in the system */
                mask_p = ovs_sw_flow_mask_find(table, &mask);
                if (!mask_p) {
@@ -1336,7 +1343,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
                rcu_assign_pointer(flow->sf_acts, acts);
 
                /* Put flow in bucket. */
-               ovs_flow_insert(table, flow, &key, match.range.end);
+               ovs_flow_insert(table, flow);
 
                reply = ovs_flow_cmd_build_info(flow, dp, info->snd_portid,
                                                info->snd_seq, OVS_FLOW_CMD_NEW);

diff --git a/datapath/flow.c b/datapath/flow.c
index ebc5c82..c766aa2 100644 (file)
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -349,9 +349,8 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
                                  sizeof(struct icmp6hdr));
 }
 
-static void flow_key_mask(struct sw_flow_key *dst,
-                         const struct sw_flow_key *src,
-                         const struct sw_flow_mask *mask)
+void ovs_flow_key_mask(struct sw_flow_key *dst, const struct sw_flow_key *src,
+                      const struct sw_flow_mask *mask)
 {
        u8 *m = (u8 *)&mask->key + mask->range.start;
        u8 *s = (u8 *)src + mask->range.start;
@@ -1043,7 +1042,7 @@ static struct sw_flow *ovs_masked_flow_lookup(struct flow_table *table,
        u32 hash;
        struct sw_flow_key masked_key;
 
-       flow_key_mask(&masked_key, flow_key, mask);
+       ovs_flow_key_mask(&masked_key, flow_key, mask);
        hash = ovs_flow_hash(&masked_key, key_start, key_len);
        head = find_bucket(table, hash);
        hlist_for_each_entry_rcu(flow, head, hash_node[table->node_ver]) {
@@ -1069,11 +1068,8 @@ struct sw_flow *ovs_flow_lookup(struct flow_table *tbl,
 }
 
 
-void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow,
-                        const struct sw_flow_key *key, int key_len)
+void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow)
 {
-       flow->unmasked_key = *key;
-       flow_key_mask(&flow->key, &flow->unmasked_key, ovsl_dereference(flow->mask));
        flow->hash = ovs_flow_hash(&flow->key,
                        ovsl_dereference(flow->mask)->range.start,
                        ovsl_dereference(flow->mask)->range.end);

diff --git a/datapath/flow.h b/datapath/flow.h
index 2366039..dc08d03 100644 (file)
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -219,9 +219,8 @@ void ovs_flow_tbl_destroy(struct flow_table *table, bool deferred);
 struct flow_table *ovs_flow_tbl_alloc(int new_size);
 struct flow_table *ovs_flow_tbl_expand(struct flow_table *table);
 struct flow_table *ovs_flow_tbl_rehash(struct flow_table *table);
-void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow,
-               const struct sw_flow_key *key, int key_len);
 
+void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow);
 void ovs_flow_remove(struct flow_table *table, struct sw_flow *flow);
 
 struct sw_flow *ovs_flow_dump_next(struct flow_table *table, u32 *bucket, u32 *idx);
@@ -261,4 +260,6 @@ void ovs_sw_flow_mask_del_ref(struct sw_flow_mask *, bool deferred);
 void ovs_sw_flow_mask_insert(struct flow_table *, struct sw_flow_mask *);
 struct sw_flow_mask *ovs_sw_flow_mask_find(const struct flow_table *,
                const struct sw_flow_mask *);
+void ovs_flow_key_mask(struct sw_flow_key *dst, const struct sw_flow_key *src,
+                      const struct sw_flow_mask *mask);
 #endif /* flow.h */