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

Message ID cdbe5ce00e6bbd8766b15b1d17ec2f28ffb096dc.1515606160.git.lorenzo.bianconi@redhat.com
State RFC
Headers show
Series
  • add acl reject rule support introducing icmp4 action
Related show

Commit Message

Lorenzo Bianconi Jan. 10, 2018, 5:59 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

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

Comments

Ben Pfaff Jan. 23, 2018, 8:44 p.m. | #1
On Wed, Jan 10, 2018 at 06:59:01PM +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
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

It's nice to finally get this right!  Thank you.

I wonder about the treatment for TCP connections.  A connection attempt
to a TCP port that is not listening ordinarily yields a TCP RST
response.  I do not know whether an ICMP reply is acceptable.  Do you
have any thoughts on that?

I think that this should add an item to NEWS that describes the new
feature.

Thanks,

Ben.
Lorenzo Bianconi Feb. 9, 2018, 10:13 a.m. | #2
On Jan 23, Ben Pfaff wrote:
> On Wed, Jan 10, 2018 at 06:59:01PM +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
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> It's nice to finally get this right!  Thank you.
> 
> I wonder about the treatment for TCP connections.  A connection attempt
> to a TCP port that is not listening ordinarily yields a TCP RST
> response.  I do not know whether an ICMP reply is acceptable.  Do you
> have any thoughts on that?
> 

I agree, we need to add tcp feature, I was thinking to send a different patchset adding tcp stuff.
Do you prefer to squash tcp action to this patchset or repin it with your comments?

> I think that this should add an item to NEWS that describes the new
> feature.
> 
> Thanks,
> 
> Ben.

Regards,
Lorenzo
Ben Pfaff Feb. 9, 2018, 5:27 p.m. | #3
On Fri, Feb 09, 2018 at 11:13:09AM +0100, Lorenzo Bianconi wrote:
> On Jan 23, Ben Pfaff wrote:
> > On Wed, Jan 10, 2018 at 06:59:01PM +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
> > > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > 
> > It's nice to finally get this right!  Thank you.
> > 
> > I wonder about the treatment for TCP connections.  A connection attempt
> > to a TCP port that is not listening ordinarily yields a TCP RST
> > response.  I do not know whether an ICMP reply is acceptable.  Do you
> > have any thoughts on that?
> > 
> 
> I agree, we need to add tcp feature, I was thinking to send a different patchset adding tcp stuff.
> Do you prefer to squash tcp action to this patchset or repin it with your comments?

It's OK with me to do TCP in a different patch set.  It takes extra work
to write code to generate TCP RSTs.  I don't want to delay these patches
by requiring that extra work now.  I would like to see the TCP work
done, however.

For this patch set, do you think it is better to send ICMP for TCP or to
continue treating reject as drop for TCP?

Thanks,

Ben.
Lorenzo Bianconi Feb. 9, 2018, 8:58 p.m. | #4
> On Fri, Feb 09, 2018 at 11:13:09AM +0100, Lorenzo Bianconi wrote:
>> On Jan 23, Ben Pfaff wrote:
>> > On Wed, Jan 10, 2018 at 06:59:01PM +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
>> > >
>> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> >
>> > It's nice to finally get this right!  Thank you.
>> >
>> > I wonder about the treatment for TCP connections.  A connection attempt
>> > to a TCP port that is not listening ordinarily yields a TCP RST
>> > response.  I do not know whether an ICMP reply is acceptable.  Do you
>> > have any thoughts on that?
>> >
>>
>> I agree, we need to add tcp feature, I was thinking to send a different patchset adding tcp stuff.
>> Do you prefer to squash tcp action to this patchset or repin it with your comments?
>
> It's OK with me to do TCP in a different patch set.  It takes extra work
> to write code to generate TCP RSTs.  I don't want to delay these patches
> by requiring that extra work now.  I would like to see the TCP work
> done, however.

ack, I will send a new patchset soon

>
> For this patch set, do you think it is better to send ICMP for TCP or to
> continue treating reject as drop for TCP?
>

I guess we can maintain the standard 'drop' action for TCP connections
for the moment

> Thanks,
>
> Ben.

Thanks.
Regards,

Lorenzo

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 63ed97efb..9b7063f45 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2890,13 +2890,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).
          *
@@ -3256,28 +3261,30 @@  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. */
+                 * we can simply reject/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);
+                if (!strcmp(acl->action, "reject")) {
+                    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);");
+                } else {
+                    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
@@ -3288,7 +3295,8 @@  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,
@@ -3296,22 +3304,38 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
                               acl->match);
                 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")) {
+                    ds_put_format(&match, " && ip4");
+                    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);");
+                } else {
+                    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. */
+                 * so a "reject/drop" ACL is simply the "reject/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);
+                if (!strcmp(acl->action, "reject")) {
+                    ds_put_format(&match, "(%s) && ip4", acl->match);
+                    ds_put_format(&actions,
+                                  "eth.dst <-> eth.src; ip4.dst <-> ip4.src; "
+                                  "icmp4 { outport <-> inport; %s };",
+                                  ingress ? "output;"
+                                          : "next(pipeline=ingress,table=0);");
+                } else {
+                    ds_put_cstr(&match, 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));
             }
             ds_destroy(&match);
             ds_destroy(&actions);