[ovs-dev] [PATCH v6 1/2] dpif-netdev: Properly create exact match masks.

Jarno Rajahalme jrajahalme at nicira.com
Tue Dec 10 02:35:57 UTC 2013


Normally OVS userspace supplies a mask along with a flow key for each
new data path flow that should be created.  OVS also provides an
option to disable the kernel wildcarding, in which case the flows are
created without a mask.  When kernel wildcarding is disabled, the
datapath should use exact match, i.e. not wildcard any bits in the
flow key.  Currently, what happens with the userspace datapath instead
is that a datapath flow with mostly empty mask is created (i.e., most
fields are wildcarded), as the current code does not examine the given
mask key length to find out that the mask key is actually empty.  This
results in the same datapath flow matching on packets of multiple
different flows, wrong actions being processed, and stats being
incorrect.

This patch refactors userspace datapath code to explicitly initialize
a suitable exact match mask when a flow put without a mask is
executed.

Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>

v6: - Unwildcard all fields whose prerequisities are met, add test case
      to prevent future regressions.

v5: - Split dpif_netdev_flow_mask_from_nlattrs() to
      dpif_netdev_flow_from_nlattrs() and dpif_netdev_mask_from_nlattrs()
      and make the latter to require non-zero mask_key_len.
    - Make dp_netdev_flow_add() to initialize an exact match mask if no
      wildcards are given.  This reflects the kernel datapath behavior.
---
 lib/dpif-netdev.c     |   92 ++++++++++++++++++++++++++++++++++---------------
 tests/ofproto-dpif.at |   22 ++++++++++++
 2 files changed, 86 insertions(+), 28 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 911cb5d..20f4a9e 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -40,6 +40,7 @@
 #include "flow.h"
 #include "hmap.h"
 #include "list.h"
+#include "meta-flow.h"
 #include "netdev.h"
 #include "netdev-vport.h"
 #include "netlink.h"
@@ -788,29 +789,72 @@ get_dpif_flow_stats(struct dp_netdev_flow *netdev_flow,
 }
 
 static int
-dpif_netdev_flow_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len,
-                                   const struct nlattr *mask_key,
-                                   uint32_t mask_key_len, struct flow *flow,
-                                   struct flow *mask)
+dpif_netdev_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len,
+                              const struct nlattr *mask_key,
+                              uint32_t mask_key_len, const struct flow *flow,
+                              struct flow *mask)
+{
+    if (mask_key_len) {
+        if (odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow)) {
+            /* This should not happen: it indicates that
+             * odp_flow_key_from_mask() and odp_flow_key_to_mask()
+             * disagree on the acceptable form of a mask.  Log the problem
+             * as an error, with enough details to enable debugging. */
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+
+            if (!VLOG_DROP_ERR(&rl)) {
+                struct ds s;
+
+                ds_init(&s);
+                odp_flow_format(key, key_len, mask_key, mask_key_len, NULL, &s,
+                                true);
+                VLOG_ERR("internal error parsing flow mask %s", ds_cstr(&s));
+                ds_destroy(&s);
+            }
+
+            return EINVAL;
+        }
+        /* Force unwildcard the in_port. */
+        mask->in_port.odp_port = u32_to_odp(UINT32_MAX);
+    } else {
+        enum mf_field_id id;
+        /* No mask key, unwildcard everything except fields whose
+         * prerequisities are not met. */
+        memset(mask, 0x0, sizeof *mask);
+
+        for (id = 0; id < MFF_N_IDS; ++id) {
+            /* Skip registers and metadata. */
+            if (!(id >= MFF_REG0 && id < MFF_REG0 + FLOW_N_REGS)
+                && id != MFF_METADATA) {
+                const struct mf_field *mf = mf_from_id(id);
+                if (mf_are_prereqs_ok(mf, flow)) {
+                    mf_mask_field(mf, mask);
+                }
+            }
+        }
+    }
+
+    return 0;
+}
+
+static int
+dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
+                              struct flow *flow)
 {
     odp_port_t in_port;
 
-    if (odp_flow_key_to_flow(key, key_len, flow)
-        || (mask_key
-            && odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow))) {
+    if (odp_flow_key_to_flow(key, key_len, flow)) {
         /* This should not happen: it indicates that odp_flow_key_from_flow()
-         * and odp_flow_key_to_flow() disagree on the acceptable form of a flow
-         * or odp_flow_key_from_mask() and odp_flow_key_to_mask() disagree on
-         * the acceptable form of a mask.  Log the problem as an error, with
-         * enough details to enable debugging. */
+         * and odp_flow_key_to_flow() disagree on the acceptable form of a
+         * flow.  Log the problem as an error, with enough details to enable
+         * debugging. */
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 
         if (!VLOG_DROP_ERR(&rl)) {
             struct ds s;
 
             ds_init(&s);
-            odp_flow_format(key, key_len, mask_key, mask_key_len, NULL, &s,
-                            true);
+            odp_flow_format(key, key_len, NULL, 0, NULL, &s, true);
             VLOG_ERR("internal error parsing flow key %s", ds_cstr(&s));
             ds_destroy(&s);
         }
@@ -818,11 +862,6 @@ dpif_netdev_flow_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len,
         return EINVAL;
     }
 
-    if (mask_key) {
-        /* Force unwildcard the in_port. */
-        mask->in_port.odp_port = u32_to_odp(UINT32_MAX);
-    }
-
     in_port = flow->in_port.odp_port;
     if (!is_valid_port_number(in_port) && in_port != ODPP_NONE) {
         return EINVAL;
@@ -832,14 +871,6 @@ dpif_netdev_flow_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len,
 }
 
 static int
-dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
-                              struct flow *flow)
-{
-    return dpif_netdev_flow_mask_from_nlattrs(key, key_len, NULL, 0, flow,
-                                              NULL);
-}
-
-static int
 dpif_netdev_flow_get(const struct dpif *dpif,
                      const struct nlattr *nl_key, size_t nl_key_len,
                      struct ofpbuf **actionsp, struct dpif_flow_stats *stats)
@@ -934,8 +965,13 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put)
     struct flow_wildcards wc;
     int error;
 
-    error = dpif_netdev_flow_mask_from_nlattrs(put->key, put->key_len,
-                put->mask, put->mask_len, &flow, &wc.masks);
+    error = dpif_netdev_flow_from_nlattrs(put->key, put->key_len, &flow);
+    if (error) {
+        return error;
+    }
+    error = dpif_netdev_mask_from_nlattrs(put->key, put->key_len,
+                                          put->mask, put->mask_len,
+                                          &flow, &wc.masks);
     if (error) {
         return error;
     }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 4d8d460..f382571 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2953,6 +2953,28 @@ skb_priority(0),in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto-dpif megaflow - disabled])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [2])
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1,ip,nw_dst=10.0.0.1 actions=output(2)
+table=0 in_port=1,ip,nw_dst=10.0.0.3 actions=drop
+])
+AT_CHECK([ovs-appctl dpif/disable-megaflows], [0], [megaflows disabled
+], [])
+AT_CHECK([ovs-appctl vlog/set dpif_netdev:dbg], [0], [], [])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+for i in 1 2 3 4; do
+    AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
+    AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
+done
+AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_USED], [0], [dnl
+skb_priority(0),skb_mark(0),in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0), packets:3, bytes:180, used:0.0s, actions:2
+skb_priority(0),skb_mark(0),in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0), packets:3, bytes:180, used:0.0s, actions:drop
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto-dpif - datapath port number change])
 OVS_VSWITCHD_START([set Bridge br0 fail-mode=standalone])
 ADD_OF_PORTS([br0], 1)
-- 
1.7.10.4




More information about the dev mailing list