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

2018-02-14 Thread Ben Pfaff
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 

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

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


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

2018-02-13 Thread Lorenzo Bianconi
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 
---
 ovn/northd/ovn-northd.c | 123 +---
 1 file changed, 86 insertions(+), 37 deletions(-)

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(, acl);
+if (extra_match->length > 0) {
+ds_put_format(, "(%s) && ", extra_match->string);
+}
+ds_put_format(, "ip && tcp && (%s)", acl->match);
+ds_put_cstr(, "/* drop */");
+ovn_lflow_add(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET,
+  ds_cstr(), ds_cstr());
+
+/* IPv4 traffic */
+ds_clear();
+ds_clear();
+build_acl_log(, acl);
+if (extra_match->length > 0) {
+ds_put_format(, "(%s) && ", extra_match->string);
+}
+ds_put_format(, "ip4 && (%s)", acl->match);
+if (extra_actions->length > 0) {
+ds_put_format(, "%s ", extra_actions->string);
+}
+ds_put_format(, "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(), ds_cstr());
+ds_destroy();
+ds_destroy();
+}
+
 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(,
-  "(!ct.est || (ct.est && ct_label.blocked == 1)) "
-  "&& (%s)",
-  acl->match);
-build_acl_log(, acl);
-ds_put_cstr(, "/* drop */");
-ovn_lflow_add_with_hint(lflows, od, stage,
-acl->priority + OVN_ACL_PRI_OFFSET,
-ds_cstr(), ds_cstr(),
-stage_hint);
-
+ * we can simply reject/drop it. */
+ds_put_cstr(,
+"(!ct.est || (ct.est && ct_label.blocked == 1))");
+if (!strcmp(acl->action, "reject")) {
+build_reject_acl_rules(od, lflows, stage, acl, ,
+