dpif-netdev: Fix dpif_netdev_flow_put.

OVS allows overlapping megaflows, as long as the actions of these megaflows are equal. However, the current implementation of action modification relies on flow_lookup instead of ufid, this could result in looking up a wrong megaflow and make the ukeys and megaflows inconsistent Just like the test case in the patch, at first we have a rule with the prefix: 10.1.2.0/24 and we will get a megaflow with prefixes 10.1.2.2/24 when a packet with IP 10.1.2.2 is received. Then suppose we change the rule into 10.1.0.0/16. OVS prefers to keep the 10.1.2.2/24 megaflow and just changes its action instead of extending the prefix into 10.1.2.2/16. then suppose we have a 10.1.0.2 packet, since it misses the megaflow, this time, we will have an overlapping megaflow with the right prefix: 10.1.0.2/16 now we have two megaflows: 10.1.2.2/24 10.1.0.2/16 last, suppose we have changed the ruleset again. The revalidator this time still decides to change the actions of both megaflows instead of deleting them. The dpif_netdev_flow_put will search the megaflow to modify with unmasked keys, however it might lookup the wrong megaflow as the key 10.1.2.2 matches both 10.1.2.2/24 and 10.1.0.2/16! This patch changes the megaflow lookup code in modification path into relying the ufid to find the correct megaflow instead of key lookup. Signed-off-by: Peng He <[email protected]> Signed-off-by: Ilya Maximets <[email protected]>
igsilya · Aug 11, 2023 · 14acf7f · 14acf7f
1 parent 65f0f78
commit 14acf7f
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 15 deletions.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
@@ -4206,24 +4206,43 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
                 const struct dpif_flow_put *put,
                 struct dpif_flow_stats *stats)
 {
-    struct dp_netdev_flow *netdev_flow;
+    struct dp_netdev_flow *netdev_flow = NULL;
     int error = 0;
 
     if (stats) {
         memset(stats, 0, sizeof *stats);
     }
 
     ovs_mutex_lock(&pmd->flow_mutex);
-    netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
-    if (!netdev_flow) {
-        if (put->flags & DPIF_FP_CREATE) {
-            dp_netdev_flow_add(pmd, match, ufid, put->actions,
-                               put->actions_len, ODPP_NONE);
+    if (put->ufid) {
+        netdev_flow = dp_netdev_pmd_find_flow(pmd, put->ufid,
+                                              put->key, put->key_len);
+    } else {
+        /* Use key instead of the locally generated ufid
+         * to search netdev_flow. */
+        netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
+    }
+
+    if (put->flags & DPIF_FP_CREATE) {
+        if (!netdev_flow) {
+            dp_netdev_flow_add(pmd, match, ufid,
+                               put->actions, put->actions_len, ODPP_NONE);
         } else {
-            error = ENOENT;
+            error = EEXIST;
         }
-    } else {
-        if (put->flags & DPIF_FP_MODIFY) {
+        goto exit;
+    }
+
+    if (put->flags & DPIF_FP_MODIFY) {
+        if (!netdev_flow) {
+            error = ENOENT;
+        } else {
+            if (!put->ufid && !flow_equal(&match->flow, &netdev_flow->flow)) {
+                /* Overlapping flow. */
+                error = EINVAL;
+                goto exit;
+            }
+
             struct dp_netdev_actions *new_actions;
             struct dp_netdev_actions *old_actions;
 
@@ -4254,15 +4273,11 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
                  *   counter, and subtracting it before outputting the stats */
                 error = EOPNOTSUPP;
             }
-
             ovsrcu_postpone(dp_netdev_actions_free, old_actions);
-        } else if (put->flags & DPIF_FP_CREATE) {
-            error = EEXIST;
-        } else {
-            /* Overlapping flow. */
-            error = EINVAL;
         }
     }
+
+exit:
     ovs_mutex_unlock(&pmd->flow_mutex);
     return error;
 }

diff --git a/tests/pmd.at b/tests/pmd.at
@@ -1331,3 +1331,50 @@ Default max sleep:  499 us
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([PMD - revalidator wrongly modify userspace megaflows])
+
+OVS_VSWITCHD_START(
+[add-port br0 p1 \
+   -- set bridge br0 datapath-type=dummy \
+   -- set interface p1 type=dummy-pmd \
+   -- add-port br0 p2 \
+   -- set interface p2 type=dummy-pmd
+])
+
+dnl Add one openflow rule and generate a megaflow.
+AT_CHECK([ovs-ofctl add-flow br0 'table=0,in_port=p1,ip,nw_dst=10.1.2.0/24,actions=p2'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)'])
+
+OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/dump-flows | sed 's/.*core: [[0-9]]*//'], [
+recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:never, actions:2])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)'])
+dnl Replace openflow rules, trigger the revalidation.
+AT_CHECK([echo 'table=0,in_port=p1,ip,nw_dst=10.1.0.0/16 actions=ct(commit)' | dnl
+          ovs-ofctl --bundle replace-flows br0 -])
+AT_CHECK([ovs-appctl revalidator/wait])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ipv4(src=10.0.0.1,dst=10.1.0.2),tcp(src=1,dst=2)'])
+OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/dump-flows | sed 's/.*core: [[0-9]]*//' | strip_xout_keep_actions], [
+recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=10.1.0.2/255.255.0.0,frag=no), packets:0, bytes:0, used:never, actions:ct(commit)
+recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:0.0s, actions:ct(commit)])
+
+dnl Hold the prefix 10.1.2.2/24 by another 10s.
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)'])
+dnl Send more 10.1.0.2 to make 10.1.0.0/16 tuple preprend 10.1.2.0/24 tuple in the pvector of subtables.
+for i in $(seq 0 256); do
+    AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ipv4(src=10.0.0.1,dst=10.1.0.2),tcp(src=1,dst=2)'])
+done
+
+AT_CHECK([echo 'table=0,in_port=p1,ip,nw_dst=10.1.0.0/16 actions=p2' | dnl
+          ovs-ofctl --bundle replace-flows br0 -])
+
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: [[0-9]]*//' | strip_xout_keep_actions], [0], [
+recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=10.1.0.2/255.255.0.0,frag=no), packets:0, bytes:0, used:0.0s, actions:2
+recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:0.0s, actions:2
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP