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

[Numan Siddique]

On Thu, Nov 14, 2019 at 11:25 PM <venugopali at nvidia.com> wrote:
>
> From: venu iyer <venugopali at 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 at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>