Message ID | 20191114175512.10286-1-venugopali@nvidia.com |
---|---|
State | RFC |
Headers | show |
Series | [ovs-dev,RFC,ovn] ovn-northd: ls_*_acl behavior not consistent for untracked flows | expand |
On Thu, Nov 14, 2019 at 11:25 PM <venugopali@nvidia.com> wrote: > > From: venu iyer <venugopali@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@nvidia.com> Makes sense to me. Can you please submit a formal patch ? Also it would be great if you could consider adding few test cases so that we don't regress it later. Thanks Numan > --- > 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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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);