diff mbox series

[ovs-dev] ovn-northd.c: Fix ACL priority related to tcp_reset action.

Message ID 20200928000904.858685-1-hzhou@ovn.org
State Deferred
Headers show
Series [ovs-dev] ovn-northd.c: Fix ACL priority related to tcp_reset action. | expand

Commit Message

Han Zhou Sept. 28, 2020, 12:09 a.m. UTC 
When there "reject" is used as ACL action, there are logical flows generated
to handle TCP and non-TCP packets separately, so that tcp_reset is used to
reject TCP packets while ICMP is used to reject non-TCP packets.

The current implementation uses priority OVN_ACL_PRI_OFFSET + acl_priroity + 10
for handling TCP packets, while OVN_ACL_PRI_OFFSET + acl_priroity for
non-TCP packets. This can cause the ACL priorities defined by users incorrectly
handled. For example, a user creates two ACLs:

1. priority: 1001, match: tcp.dst == 443, action: allow
2. priority: 1000, match: any, action: reject

The generates lflows would be:
1. priority: 2010, match: any, action: tcp_reset
2. priority: 2001, match: tcp.dst == 443, action: allow
3. priority: 2000, match: any, action: icmp

Now if a TCP packet with dst port 443 comes, it will be rejected.

This patch fixes the problem by using OVN_ACL_PRI_OFFSET + acl_priority * 2 + 1
as flow priority for the tcp_reset flow, and OVN_ACL_PRI_OFFSET + acl_priority * 2
for other ACL related flows including the ICMP flows.

Fixes: 366ac0d89 ("OVN: add tcp_reset action to ovn acl reject support")
Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 NEWS                |  1 +
 northd/ovn-northd.c | 28 ++++++++++++++++++----------
 ovn-nb.ovsschema    |  6 +++---
 ovn-nb.xml          |  8 ++++----
 tests/ovn.at        | 37 +++++++++++++++++++++++++++++--------
 5 files changed, 55 insertions(+), 25 deletions(-)
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index ee5c2c393..90982b743 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@ 
 Post-v20.09.0
 ---------------------
+   - ACL priority range changed from 0 - 32767 to 0 - 30000.
 
 
 OVN v20.09.0 - xx xxx xxxx
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3324c9e81..3649c0614 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5362,6 +5362,14 @@  build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
     ds_put_cstr(actions, "); ");
 }
 
+static uint16_t
+acl_priority_base(uint16_t acl_priority)
+{
+    /* acl_priority range is 0 - 30000, so the resulted priority
+     * is between 1000 and 61000. */
+    return OVN_ACL_PRI_OFFSET + acl_priority * 2;
+}
+
 static void
 build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
                        enum ovn_stage stage, struct nbrec_acl *acl,
@@ -5384,7 +5392,7 @@  build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
                   ingress ? "next(pipeline=egress,table=5);"
                           : "next(pipeline=ingress,table=20);");
     ovn_lflow_add_with_hint(lflows, od, stage,
-                            acl->priority + OVN_ACL_PRI_OFFSET + 10,
+                            acl_priority_base(acl->priority) + 1,
                             ds_cstr(&match), ds_cstr(&actions), stage_hint);
     ds_clear(&match);
     ds_clear(&actions);
@@ -5399,7 +5407,7 @@  build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
                   ingress ? "next(pipeline=egress,table=5);"
                           : "next(pipeline=ingress,table=20);");
     ovn_lflow_add_with_hint(lflows, od, stage,
-                            acl->priority + OVN_ACL_PRI_OFFSET + 10,
+                            acl_priority_base(acl->priority) + 1,
                             ds_cstr(&match), ds_cstr(&actions), stage_hint);
 
     /* IP traffic */
@@ -5419,7 +5427,7 @@  build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
                   ingress ? "next(pipeline=egress,table=5);"
                           : "next(pipeline=ingress,table=20);");
     ovn_lflow_add_with_hint(lflows, od, stage,
-                            acl->priority + OVN_ACL_PRI_OFFSET,
+                            acl_priority_base(acl->priority),
                             ds_cstr(&match), ds_cstr(&actions), stage_hint);
     ds_clear(&match);
     ds_clear(&actions);
@@ -5437,7 +5445,7 @@  build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
                   ingress ? "next(pipeline=egress,table=5);"
                           : "next(pipeline=ingress,table=20);");
     ovn_lflow_add_with_hint(lflows, od, stage,
-                            acl->priority + OVN_ACL_PRI_OFFSET,
+                            acl_priority_base(acl->priority),
                             ds_cstr(&match), ds_cstr(&actions), stage_hint);
 
     ds_destroy(&match);
@@ -5463,7 +5471,7 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
             build_acl_log(&actions, acl);
             ds_put_cstr(&actions, "next;");
             ovn_lflow_add_with_hint(lflows, od, stage,
-                                    acl->priority + OVN_ACL_PRI_OFFSET,
+                                    acl_priority_base(acl->priority),
                                     acl->match, ds_cstr(&actions),
                                     &acl->header_);
             ds_destroy(&actions);
@@ -5489,7 +5497,7 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
             build_acl_log(&actions, acl);
             ds_put_cstr(&actions, "next;");
             ovn_lflow_add_with_hint(lflows, od, stage,
-                                    acl->priority + OVN_ACL_PRI_OFFSET,
+                                    acl_priority_base(acl->priority),
                                     ds_cstr(&match),
                                     ds_cstr(&actions),
                                     &acl->header_);
@@ -5508,7 +5516,7 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
             build_acl_log(&actions, acl);
             ds_put_cstr(&actions, "next;");
             ovn_lflow_add_with_hint(lflows, od, stage,
-                                    acl->priority + OVN_ACL_PRI_OFFSET,
+                                    acl_priority_base(acl->priority),
                                     ds_cstr(&match), ds_cstr(&actions),
                                     &acl->header_);
 
@@ -5536,7 +5544,7 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
                 build_acl_log(&actions, acl);
                 ds_put_cstr(&actions, "/* drop */");
                 ovn_lflow_add_with_hint(lflows, od, stage,
-                                        acl->priority + OVN_ACL_PRI_OFFSET,
+                                        acl_priority_base(acl->priority),
                                         ds_cstr(&match), ds_cstr(&actions),
                                         &acl->header_);
             }
@@ -5563,7 +5571,7 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
                 build_acl_log(&actions, acl);
                 ds_put_cstr(&actions, "/* drop */");
                 ovn_lflow_add_with_hint(lflows, od, stage,
-                                        acl->priority + OVN_ACL_PRI_OFFSET,
+                                        acl_priority_base(acl->priority),
                                         ds_cstr(&match), ds_cstr(&actions),
                                         &acl->header_);
             }
@@ -5578,7 +5586,7 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
                 build_acl_log(&actions, acl);
                 ds_put_cstr(&actions, "/* drop */");
                 ovn_lflow_add_with_hint(lflows, od, stage,
-                                        acl->priority + OVN_ACL_PRI_OFFSET,
+                                        acl_priority_base(acl->priority),
                                         acl->match, ds_cstr(&actions),
                                         &acl->header_);
             }
diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
index 092322ab2..82d0933e6 100644
--- a/ovn-nb.ovsschema
+++ b/ovn-nb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Northbound",
-    "version": "5.27.0",
-    "cksum": "3507518247 26773",
+    "version": "5.28.0",
+    "cksum": "3562758453 26773",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -211,7 +211,7 @@ 
                                           "min": 0, "max": 1}},
                 "priority": {"type": {"key": {"type": "integer",
                                               "minInteger": 0,
-                                              "maxInteger": 32767}}},
+                                              "maxInteger": 30000}}},
                 "direction": {"type": {"key": {"type": "string",
                                             "enum": ["set", ["from-lport", "to-lport"]]}}},
                 "match": {"type": "string"},
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 86195af34..07854de94 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -1646,10 +1646,10 @@ 
 
     <column name="priority">
       <p>
-        The ACL rule's priority.  Rules with numerically higher priority
-        take precedence over those with lower.  If two ACL rules with
-        the same priority both match, then the one actually applied to a
-        packet is undefined.
+        The ACL rule's priority, an integer between 0 and 30000.  Rules with
+        numerically higher priority take precedence over those with lower.  If
+        two ACL rules with the same priority both match, then the one actually
+        applied to a packet is undefined.
       </p>
 
       <p>
diff --git a/tests/ovn.at b/tests/ovn.at
index b6c8622ba..3ae1bbc85 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -12817,13 +12817,19 @@  test_ipv6_packet() {
     as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
 }
 
-# test_tcp_syn_packet INPORT HV ETH_SRC ETH_DST IPV4_SRC IPV4_DST IP_CHKSUM TCP_SPORT TCP_DPORT TCP_CHKSUM EXP_IP_CHKSUM EXP_TCP_RST_CHKSUM
+# test_tcp_syn_packet INPORT HV ETH_SRC ETH_DST IPV4_SRC IPV4_DST IP_CHKSUM
+# TCP_SPORT TCP_DPORT TCP_CHKSUM EXP_IP_CHKSUM EXP_TCP_RST_CHKSUM [OUTPORT]
 #
-# Causes a packet to be received on INPORT of the hypervisor HV. The packet is an TCP syn segment with
-# ETH_SRC, ETH_DST, IPV4_SRC, IPV4_DST, IP_CHKSUM, TCP_SPORT, TCP_DPORT, TCP_CHKSUM  as specified.
-# EXP_IP_CHKSUM and EXP_TCP_RST_CHKSUM are the ip and tcp checksums of the tcp reset segment generated from ACL rule hit
+# Causes a TCP packet to be sent from the INPORT.  If the OUTPORT is specified,
+# the original packet is expected to be received at OUTPORT. Otherwise, a TCP
+# reset packet is expected to be received at INPORT.
 #
-# INPORT is an lport number, e.g. 11 for vif11.
+# The packet is an TCP syn segment with ETH_SRC, ETH_DST, IPV4_SRC, IPV4_DST,
+# IP_CHKSUM, TCP_SPORT, TCP_DPORT, TCP_CHKSUM  as specified.  EXP_IP_CHKSUM and
+# EXP_TCP_RST_CHKSUM are the ip and tcp checksums of the tcp reset segment
+# generated from ACL rule hit.
+#
+# INPORT and OUTPORT are lport numbers, e.g. 11 for vif11.
 # HV is an hypervisor number
 # ETH_SRC and ETH_DST are each 12 hex digits.
 # IPV4_SRC and IPV4_DST are each 8 hex digits.
@@ -12834,13 +12840,20 @@  test_tcp_syn_packet() {
     local tcp_sport=$8 tcp_dport=$9 tcp_chksum=${10}
     local exp_ip_chksum=${11} exp_tcp_rst_chksum=${12}
     shift 12
+    local outport=$1
 
     local ip_ttl=ff
     local packet=${eth_dst}${eth_src}08004500002800004000${ip_ttl}06${ip_chksum}${ipv4_src}${ipv4_dst}${tcp_sport}${tcp_dport}000000010000000050027210${tcp_chksum}0000
 
-    local tcp_rst_ttl=3f
-    local reply=${eth_src}${eth_dst}08004500002800004000${tcp_rst_ttl}06${exp_ip_chksum}${ipv4_dst}${ipv4_src}${tcp_dport}${tcp_sport}000000000000000250140000${exp_tcp_rst_chksum}0000
-    echo $reply >> vif$inport.expected
+    if test x$outport == x; then
+        echo TCPRESET
+        local tcp_rst_ttl=3f
+        local reply=${eth_src}${eth_dst}08004500002800004000${tcp_rst_ttl}06${exp_ip_chksum}${ipv4_dst}${ipv4_src}${tcp_dport}${tcp_sport}000000000000000250140000${exp_tcp_rst_chksum}0000
+        echo $reply >> vif$inport.expected
+    else
+        echo ORIGINAL
+        echo $packet >> vif$outport.expected
+    fi
 
     as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
 }
@@ -12886,6 +12899,10 @@  ovn-nbctl --log acl-add sw0 to-lport 1000 "outport == \"sw0-p12\"" reject
 ovn-nbctl --log acl-add sw0 from-lport 1000 "inport == \"sw0-p11\"" reject
 ovn-nbctl --log acl-add sw0 from-lport 1000 "inport == \"sw0-p21\"" reject
 
+# A higher priority rule that allows all traffic sent from p21 to dst IP of p31
+ovn-nbctl --log acl-add sw0 from-lport 1001 "inport == \"sw0-p21\"
+    && ip4.dst == 192.168.1.31" allow
+
 # Allow some time for ovn-northd and ovn-controller to catch up.
 ovn-nbctl --timeout=3 --wait=hv sync
 
@@ -12899,6 +12916,10 @@  test_tcp_syn_packet 11 1 000000000011 000000000021 $(ip_to_hex 192 168 1 11) $(i
 test_tcp_syn_packet 21 2 000000000021 000000000011 $(ip_to_hex 192 168 1 21) $(ip_to_hex 192 168 1 11) 0000 8b40 3039 0000 b85f 70e4
 test_tcp_syn_packet 31 3 000000000031 000000000012 $(ip_to_hex 192 168 1 31) $(ip_to_hex 192 168 1 12) 0000 8b40 3039 0000 b854 70d9
 
+# From p21 to p31, the packet should be received in p31, because of the last
+# rule.
+test_tcp_syn_packet 21 2 000000000021 000000000031 $(ip_to_hex 192 168 1 21) $(ip_to_hex 192 168 1 31) 0000 8b40 3039 0000 b854 70d9 31
+
 for i in 1 2 3; do
     OVN_CHECK_PACKETS([hv$i/vif${i}1-tx.pcap], [vif${i}1.expected])
 done