[ovs-dev] [PATCH ovn v3] ovn-northd: ls_*_acl behavior not consistent for untracked flows

venugopali at nvidia.com venugopali at nvidia.com
Thu Dec 19 18:45:40 UTC 2019


From: venu iyer <venugopali at nvidia.com>

If one creates a port group and a MAC address set, and an
ACL that prevents packets being output to a port in that Port Group from
any MAC address in that address set, the outcome is not consistent.

The outcome depends on whether there is a stateful rule on the switch or not.

Specifically:

Assuming 'l2pg' is a port group with a list of ports and 'macs' is an Address
Set with a list of MAC addresses and the intent is to drop all packets
with source MAC address in 'macs' to any port in 'l2pg' using:

ovn-nbctl acl-add <switch> to-lport 5000 \
	"outport == @l2pg && eth.src == $macs" drop

Without any stateful rule on the logical switch, the corresponding
logical flow looks like:
	table=4 (ls_out_acl        ), priority=6000,\
		match=(outport == @l2pg && eth.src == $macs), \
		action=(/* drop */)

Based on this rule, any packet destined to the ports in 'l2pg' with source
Address in 'macs' will be dropped - as is expected from the ACL above.

While with a Stateful rule on the switch (any stateful rule will do),
the same rule looks like:
	table=4 (ls_out_acl        ), priority=6000, \
		match=((!ct.est || (ct.est && ct_label.blocked == 1)) && \
		(outport == @l2pg && eth.src == $macs)), action=(/* drop */)

With this, however, only packets that are tracked will match the rule
and be dropped, e.g. IP packets will be dropped, but ARP etc., will go
through - this is not expected.

Based on whether there are stateful rules or not on the switch,
untracked packets will see different behavior.

The fix is to make the rule in the stateful case comprehensive, i.e.
instead of just looking for flows that are not established (or not new),
we should also look for flows that are not tracked.

The fix was tested in the above scenario. Additionally, the following
ACL was added to test the change in the "allow" case (i.e. to drop
all the packets based on the above ACL, but have a higher priority
rule that selectively allow ARP).

ovn-nbctl acl-add ls1 to-lport 6000
        "outport == @l2pg && eth.type == 0x806" allow

with and without the stateful rule to make sure the behavior is the
same.  The test suite has been enhanced to add the above test cases
(with different ethertype) for drop and allow.

OVN test cases were run with this fix and no failures were seen.

Signed-off-by: venu iyer <venugopali at nvidia.com>
---
 northd/ovn-northd.c |  13 +--
 tests/ovn.at        | 205 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 212 insertions(+), 6 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3a5cb7c91..d91a008b7 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4900,12 +4900,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
              * deletion.  There is no need to commit here, so we can just
              * proceed to the next table. We use this to ensure that this
              * connection is still allowed by the currently defined
-             * policy. */
+             * policy. Match untracked packets too. */
             ds_clear(&match);
             ds_clear(&actions);
             ds_put_format(&match,
-                          "!ct.new && ct.est && !ct.rpl"
-                          " && ct_label.blocked == 0 && (%s)",
+                          "(!ct.trk || (!ct.new && ct.est && !ct.rpl"
+                          " && ct_label.blocked == 0)) && (%s)",
                           acl->match);
 
             build_acl_log(&actions, acl);
@@ -4928,10 +4928,11 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
          * depending on whether the connection was previously committed
          * to the connection tracker with ct_commit. */
         if (has_stateful) {
-            /* If the packet is not part of an established connection, then
-             * we can simply reject/drop it. */
+            /* If the packet is not tracked or not part of an established
+             * connection, then we can simply reject/drop it. */
             ds_put_cstr(&match,
-                        "(!ct.est || (ct.est && ct_label.blocked == 1))");
+                        "(!ct.trk || !ct.est"
+                        " || (ct.est && ct_label.blocked == 1))");
             if (!strcmp(acl->action, "reject")) {
                 build_reject_acl_rules(od, lflows, stage, acl, &match,
                                        &actions);
diff --git a/tests/ovn.at b/tests/ovn.at
index 1d5369341..213a1b1f9 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -12357,6 +12357,211 @@ AT_CHECK([cat 2.packets], [0], [])
 
 AT_CLEANUP
 
+# 3 hypervisors, one logical switch, 3 logical ports per hypervisor
+AT_SETUP([ovn -- L2 Drop and Allow ACL w/ Stateful ACL])
+ovn_start
+
+# Create hypervisors hv[123].
+# Add vif11 to hv1, vif21 to hv2, vif31 to hv3.
+# Add all of the vifs to a single logical switch lsw0.
+# Create Port Group with vif11 and vif21
+# Create Address Set with vif11 and vif21's MAC addresses
+# Test 1:
+#  Create Drop ACL to drop all packets with ethertype 1234 between 11 and 21
+#  No ACL for 31
+#  Validate the drop ACL with and without any stateful rule on lsw0.
+# Test 2:
+#  Create Drop ACL to drop all packets between 11 and 21
+#  Create higher priority ACL to allow packet with ethertype 1234 between 11 and 21
+#  No ACL for 31
+#  Validate the allow ACL with and without any stateful rule on lsw0.
+#
+ovn-nbctl ls-add lsw0
+net_add n1
+for i in 1 2 3; do
+    sim_add hv$i
+    as hv$i
+    ovs-vsctl add-br br-phys
+    ovn_attach n1 br-phys 192.168.0.$i
+
+    ovs-vsctl add-port br-int vif${i}1 -- set Interface vif${i}1 external-ids:iface-id=lp${i}1 options:tx_pcap=hv$i/vif${i}1-tx.pcap options:rxq_pcap=hv$i/vif${i}1-rx.pcap ofport-request=${i}1
+    ovn-nbctl lsp-add lsw0 lp${i}1
+    ovn-nbctl lsp-set-addresses lp${i}1 "f0:00:00:00:00:${i}1 192.168.0.${i}1" unknown
+done
+#
+
+get_lsp_uuid () {
+    ovn-nbctl lsp-list lsw0 | grep $1 | awk '{ print $1 }'
+}
+
+# Create Port Group and corresponding Address set.
+ovn-nbctl create Port_Group name=pg1 ports=`get_lsp_uuid lp11`,`get_lsp_uuid lp21`
+ovn-nbctl create Address_Set name=set1 addresses=\"f0:00:00:00:00:11\",\"f0:00:00:00:00:21\"
+
+# Pre-populate the hypervisors' ARP tables so that we don't lose any
+# packets for ARP resolution (native tunneling doesn't queue packets
+# for ARP resolution).
+OVN_POPULATE_ARP
+
+# Allow some time for ovn-northd and ovn-controller to catch up.
+# XXX This should be more systematic.
+sleep 1
+
+# Make sure there is no attempt to adding duplicated flows by ovn-controller
+AT_FAIL_IF([test -n "`grep duplicate hv1/ovn-controller.log`"])
+AT_FAIL_IF([test -n "`grep duplicate hv2/ovn-controller.log`"])
+AT_FAIL_IF([test -n "`grep duplicate hv3/ovn-controller.log`"])
+
+# Given the name of a logical port, prints the name of the hypervisor
+# on which it is located.
+vif_to_hv() {
+    echo hv${1%?}
+}
+
+# test_packet INPORT DST SRC ETHTYPE OUTPORT...
+#
+# This shell function causes a packet to be received on INPORT.  The packet's
+# content has Ethernet destination DST and source SRC (each exactly 12 hex
+# digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
+# more) list the VIFs on which the packet should be received.  INPORT and the
+# OUTPORTs are specified as logical switch port numbers, e.g. 11 for vif11.
+for i in 1 2 3; do
+    : > ${i}1.expected
+done
+test_packet() {
+    local inport=$1 packet=$2$3$4; shift; shift; shift; shift
+    hv=`vif_to_hv $inport`
+    vif=vif$inport
+    as $hv ovs-appctl netdev-dummy/receive $vif $packet
+    for outport; do
+        echo $packet >> $outport.expected
+    done
+}
+
+# Test drop rule
+# --------------
+ovn-nbctl acl-del lsw0
+ovn-nbctl --log --severity=info --name=drop-acl acl-add lsw0 to-lport 5000 'outport == @pg1 && eth.src == $set1 && eth.type == 0x1234' drop
+for sf in 0 1; do
+    if test ${sf} = 1; then
+        # Add a stateful rule and re-run the check to make sure the
+        # drop rule is still effective..
+        ovn-nbctl acl-add lsw0 from-lport 2000  "inport == lp31 && ip" allow-related
+    fi
+    for is in 1 2 3; do
+        s=${is}1
+        for id in 1 2 3; do
+            d=${id}1
+
+            if test $d != $s;
+            then
+                if test ${is} = 3 || test ${id} = 3; then
+                    test_packet $s f000000000$d f000000000$s 1234 $d # Allow to/from 31
+                else
+                    test_packet $s f000000000$d f000000000$s 1234    # Drop between 11 and 21
+                fi
+            fi
+        done
+
+        # Broadcast and multicast.
+        if test ${is} = 3; then
+            bcast="11 21" # Allow from 3
+        else
+            bcast="31"   # Allow only to 31 from 11 or 21.
+        fi
+        test_packet $s ffffffffffff f000000000$s 1234 $bcast
+        test_packet $s 010000000000 f000000000$s 1234 $bcast
+    done
+done
+
+# Test allow rule
+#----------------
+ovn-nbctl acl-del lsw0
+# drop all packets to 11 and 21.
+ovn-nbctl acl-add lsw0 to-lport 5000 'outport == @pg1 && eth.src == $set1' drop
+# allow 0x1234 between 11 and 21
+ovn-nbctl --log --severity=info --name=allow-acl acl-add lsw0 to-lport 6000 'outport == @pg1 && eth.src == $set1 && eth.type == 0x1234' allow
+for sf in 0 1; do
+    if test ${sf} = 1; then
+        # Add a stateful rule and re-run the check to make sure the
+        # allow rule is still effective..
+        ovn-nbctl acl-add lsw0 from-lport 2000  "inport == lp31 && ip" allow-related
+    fi
+    for is in 1 2 3; do
+        s=${is}1
+        for id in 1 2 3; do
+            d=${id}1
+
+            if test $d != $s;
+            then
+                test_packet $s f000000000$d f000000000$s 1234 $d # allow 1234 to 11, 21, and 31
+            fi
+        done
+
+        # Broadcast and multicast. Allow from one to the other 2.
+        if test ${is} = 1; then
+            bcast="21 31"
+        elif test ${is} = 2; then
+            bcast="11 31"
+        else
+            bcast="11 21"
+        fi
+        test_packet $s ffffffffffff f000000000$s 1234 $bcast
+        test_packet $s 010000000000 f000000000$s 1234 $bcast
+    done
+done
+
+# Clean up the ACL
+ovn-nbctl acl-del lsw0
+
+# dump information and flows with counters
+ovn-sbctl dump-flows -- list multicast_group
+
+echo "------ hv1 dump ------"
+as hv1 ovs-vsctl show
+as hv1 ovs-ofctl -O OpenFlow13 dump-flows br-int
+
+echo "------ hv2 dump ------"
+as hv2 ovs-vsctl show
+as hv2 ovs-ofctl -O OpenFlow13 dump-flows br-int
+
+echo "------ hv3 dump ------"
+as hv3 ovs-vsctl show
+as hv3 ovs-ofctl -O OpenFlow13 dump-flows br-int
+
+# Now check the packets actually received against the ones expected.
+for i in 1 2 3; do
+    OVN_CHECK_PACKETS([hv$i/vif${i}1-tx.pcap], [${i}1.expected])
+done
+
+# need to verify the log for ACL hit as well, since in the allow case
+# (unlike the drop case) it is tricky to pass just with the expected;
+# since with the stateful rule the packet will still get by (default
+# rule) even if it doesn't hit the allow rule.
+# The hit count for the ACL is 6 (1 unicast + 2 non-unicast) * 2
+# (with/without stateful rule) for hv1 and hv2, each.
+
+hv1_drop_acl_hit=`grep acl_log hv1/ovn-controller.log | grep "|INFO|name=\"drop-acl\"" | wc -l`
+hv2_drop_acl_hit=`grep acl_log hv2/ovn-controller.log | grep "|INFO|name=\"drop-acl\"" | wc -l`
+
+hv1_allow_acl_hit=`grep acl_log hv1/ovn-controller.log | grep "|INFO|name=\"allow-acl\"" | wc -l`
+hv2_allow_acl_hit=`grep acl_log hv2/ovn-controller.log | grep "|INFO|name=\"allow-acl\"" | wc -l`
+
+echo "hv1_drop hit $hv1_drop_acl_hit"
+echo "hv2_drop hit $hv2_drop_acl_hit"
+echo "hv1_allow hit $hv1_allow_acl_hit"
+echo "hv2_allow hit $hv2_allow_acl_hit"
+
+AT_CHECK([test $hv1_drop_acl_hit = 6])
+AT_CHECK([test $hv2_drop_acl_hit = 6])
+
+AT_CHECK([test $hv1_allow_acl_hit = 6])
+AT_CHECK([test $hv2_allow_acl_hit = 6])
+
+OVN_CLEANUP([hv1],[hv2],[hv3])
+
+AT_CLEANUP
+
 AT_SETUP([ovn -- TTL exceeded])
 AT_KEYWORDS([ttl-exceeded])
 ovn_start
-- 
2.17.1



More information about the dev mailing list