[ovs-dev,RFC,ovn] ovn-northd: ls_*_acl behavior not consistent for untracked flows

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>
---
 northd/ovn-northd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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
>