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

venugopali at nvidia.com venugopali at nvidia.com
Thu Nov 14 17:55:12 UTC 2019


From: venu iyer <venugopali at nvidia.com>

If one creates a port group and a MAC address set, and create an
ACL that prevents packets being output to a port in the Port Group from
any MAC address in the 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 */)

However, with this, 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 complete the rules in the stateful case, i.e. instead
of looking for flows that are not established or not new, we should
first check if flows 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 staeful rule to make sure the behavior is the
same.

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

Signed-off-by: venu iyer <venugopali at nvidia.com>
---
 northd/ovn-northd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 6742bc002..1f5f97796 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4585,12 +4585,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);
@@ -4613,10 +4613,10 @@ 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
+            /* If the packet is not tracked or 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);
-- 
2.17.1



More information about the dev mailing list