[ovs-dev,3/3] OVN: add acl reject support using icmp4 action

Message ID 2fc748339b8e35f6b698f821b04189a878ad7f12.1518532822.git.lorenzo.bianconi@redhat.com
State Changes Requested
Headers show
Series
  • add acl reject rule support introducing icmp4 action
Related show

Commit Message

Lorenzo Bianconi Feb. 13, 2018, 2:43 p.m.
Whenever the acl reject rule is hit send back an ICMPv4 destination
unreachable packet and do not handle reject rule as drop one.
Treat TCP connections as DROP for the moment since tcp_reset{} action
has not been implemented yet.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 ovn/northd/ovn-northd.c | 123 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 86 insertions(+), 37 deletions(-)

Comments

Ben Pfaff Feb. 14, 2018, 11:36 p.m. | #1
On Tue, Feb 13, 2018 at 03:43:52PM +0100, Lorenzo Bianconi wrote:
> Whenever the acl reject rule is hit send back an ICMPv4 destination
> unreachable packet and do not handle reject rule as drop one.
> Treat TCP connections as DROP for the moment since tcp_reset{} action
> has not been implemented yet.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Same as Mark, I'd prefer to see some tests.

Thanks,

Ben.

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 4d95a3d9d..90fcf9f9e 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2895,13 +2895,18 @@  build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
 
         /* Ingress and Egress Pre-ACL Table (Priority 110).
          *
-         * Not to do conntrack on ND packets. */
+         * Not to do conntrack on ND and ICMP destination
+         * unreachable packets. */
         ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, "nd", "next;");
         ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, "(nd_rs || nd_ra)",
                       "next;");
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110,
+                      "ip4 && icmp4.type == 3", "next;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, "nd", "next;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110,
                       "(nd_rs || nd_ra)", "next;");
+        ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110,
+                      "ip4 && icmp4.type == 3", "next;");
 
         /* Ingress and Egress Pre-ACL Table (Priority 100).
          *
@@ -3082,6 +3087,46 @@  build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
     ds_put_cstr(actions, "); ");
 }
 
+static void
+build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
+                       enum ovn_stage stage, struct nbrec_acl *acl,
+                       struct ds *extra_match, struct ds *extra_actions)
+{
+    struct ds match = DS_EMPTY_INITIALIZER;
+    struct ds actions = DS_EMPTY_INITIALIZER;
+    bool ingress = (stage == S_SWITCH_IN_ACL);
+
+    /* XXX: Treat TCP connections as "drop;" for now */
+    build_acl_log(&actions, acl);
+    if (extra_match->length > 0) {
+        ds_put_format(&match, "(%s) && ", extra_match->string);
+    }
+    ds_put_format(&match, "ip && tcp && (%s)", acl->match);
+    ds_put_cstr(&actions, "/* drop */");
+    ovn_lflow_add(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET,
+                  ds_cstr(&match), ds_cstr(&actions));
+
+    /* IPv4 traffic */
+    ds_clear(&match);
+    ds_clear(&actions);
+    build_acl_log(&actions, acl);
+    if (extra_match->length > 0) {
+        ds_put_format(&match, "(%s) && ", extra_match->string);
+    }
+    ds_put_format(&match, "ip4 && (%s)", acl->match);
+    if (extra_actions->length > 0) {
+        ds_put_format(&actions, "%s ", extra_actions->string);
+    }
+    ds_put_format(&actions, "reg0 = 0; "
+                  "eth.dst <-> eth.src; ip4.dst <-> ip4.src; "
+                  "icmp4 { outport <-> inport; %s };",
+                  ingress ? "output;" : "next(pipeline=ingress,table=0);");
+    ovn_lflow_add(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET,
+                  ds_cstr(&match), ds_cstr(&actions));
+    ds_destroy(&match);
+    ds_destroy(&actions);
+}
+
 static void
 build_acls(struct ovn_datapath *od, struct hmap *lflows)
 {
@@ -3261,29 +3306,26 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
             struct ds match = DS_EMPTY_INITIALIZER;
             struct ds actions = DS_EMPTY_INITIALIZER;
 
-            /* XXX Need to support "reject", treat it as "drop;" for now. */
-            if (!strcmp(acl->action, "reject")) {
-                VLOG_INFO("reject is not a supported action");
-            }
-
             /* The implementation of "drop" differs if stateful ACLs are in
              * use for this datapath.  In that case, the actions differ
              * 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 drop it. */
-                ds_put_format(&match,
-                              "(!ct.est || (ct.est && ct_label.blocked == 1)) "
-                              "&& (%s)",
-                              acl->match);
-                build_acl_log(&actions, acl);
-                ds_put_cstr(&actions, "/* drop */");
-                ovn_lflow_add_with_hint(lflows, od, stage,
-                                        acl->priority + OVN_ACL_PRI_OFFSET,
-                                        ds_cstr(&match), ds_cstr(&actions),
-                                        stage_hint);
-
+                 * we can simply reject/drop it. */
+                ds_put_cstr(&match,
+                            "(!ct.est || (ct.est && ct_label.blocked == 1))");
+                if (!strcmp(acl->action, "reject")) {
+                    build_reject_acl_rules(od, lflows, stage, acl, &match,
+                                           &actions);
+                } else {
+                    ds_put_format(&match, " && (%s)", acl->match);
+                    build_acl_log(&actions, acl);
+                    ds_put_cstr(&actions, "/* drop */");
+                    ovn_lflow_add(lflows, od, stage,
+                                  acl->priority + OVN_ACL_PRI_OFFSET,
+                                  ds_cstr(&match), ds_cstr(&actions));
+                }
                 /* For an existing connection without ct_label set, we've
                  * encountered a policy change. ACLs previously allowed
                  * this connection and we committed the connection tracking
@@ -3293,30 +3335,37 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
                  * specifying "next;", we implicitly drop the packet after
                  * updating conntrack state.  We would normally defer
                  * ct_commit() to the "stateful" stage, but since we're
-                 * dropping the packet, we go ahead and do it here. */
+                 * rejecting/dropping the packet, we go ahead and do it here.
+                 */
                 ds_clear(&match);
                 ds_clear(&actions);
-                ds_put_format(&match,
-                              "ct.est && ct_label.blocked == 0 && (%s)",
-                              acl->match);
+                ds_put_cstr(&match, "ct.est && ct_label.blocked == 0");
                 ds_put_cstr(&actions, "ct_commit(ct_label=1/1); ");
-                build_acl_log(&actions, acl);
-                ds_put_cstr(&actions, "/* drop */");
-                ovn_lflow_add_with_hint(lflows, od, stage,
-                                        acl->priority + OVN_ACL_PRI_OFFSET,
-                                        ds_cstr(&match), ds_cstr(&actions),
-                                        stage_hint);
-
+                if (!strcmp(acl->action, "reject")) {
+                    build_reject_acl_rules(od, lflows, stage, acl, &match,
+                                           &actions);
+                } else {
+                    ds_put_format(&match, " && (%s)", acl->match);
+                    build_acl_log(&actions, acl);
+                    ds_put_cstr(&actions, "/* drop */");
+                    ovn_lflow_add(lflows, od, stage,
+                                  acl->priority + OVN_ACL_PRI_OFFSET,
+                                  ds_cstr(&match), ds_cstr(&actions));
+                }
             } else {
                 /* There are no stateful ACLs in use on this datapath,
-                 * so a "drop" ACL is simply the "drop" logical flow action
-                 * in all cases. */
-                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->match, ds_cstr(&actions),
-                                        stage_hint);
+                 * so a "reject/drop" ACL is simply the "reject/drop"
+                 * logical flow action in all cases. */
+                if (!strcmp(acl->action, "reject")) {
+                    build_reject_acl_rules(od, lflows, stage, acl, &match,
+                                           &actions);
+                } else {
+                    build_acl_log(&actions, acl);
+                    ds_put_cstr(&actions, "/* drop */");
+                    ovn_lflow_add(lflows, od, stage,
+                                  acl->priority + OVN_ACL_PRI_OFFSET,
+                                  acl->match, ds_cstr(&actions));
+                }
             }
             ds_destroy(&match);
             ds_destroy(&actions);