On 5/7/21 4:35 PM, Numan Siddique wrote: > On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <[email protected]> wrote: >> >> On 5/6/21 3:52 PM, [email protected] wrote: >>> From: Numan Siddique <[email protected]> >>> >>> Some smart NICs can't offload datapath flows matching on conntrack >>> fields. If a deployment desires to make use of such smart NICs >>> then it cannot configure ACLs on the logical switches. If suppose >>> a logical switch S1 has no ACLs configured and a logical switch S2 >>> has ACLs configured, then the CMS would expect the datapath flows >>> belonging to S1 logical ports are offloaded since it has no ACLs. >>> But this is not working as expected (even if S1 and S2 are >>> not connected via a logical router). >>> >>> ovn-northd generates the below logical flows in ls_in_acl_hint >>> and ls_in_acl stages for S1 >>> >>> table=8 (ls_in_acl_hint ), priority=0 , match=(1), action=(next;) >>> table=9 (ls_in_acl ), priority=0 , match=(1), action=(next;) >>> >>> And the below for S2 >>> >>> table=8 (ls_in_acl_hint ), priority=7 , match=(ct.new && !ct.est), >>> action=(reg0[7] = 1; reg0[9] = 1; next;) >>> table=8 (ls_in_acl_hint ), priority=6 , match=(!ct.new && ct.est && >>> !ct.rpl && ct_label.blocked == 1), action=(reg0[7] = 1; reg0[9] = 1; next;) >>> table=8 (ls_in_acl_hint ), priority=5 , match=(!ct.trk), >>> action=(reg0[8] = 1; reg0[9] = 1; next;) >>> table=8 (ls_in_acl_hint ), priority=4 , match=(!ct.new && ct.est && >>> !ct.rpl && ct_label.blocked == 0), action=(reg0[8] = 1; reg0[10] = 1; next;) >>> table=8 (ls_in_acl_hint ), priority=3 , match=(!ct.est), >>> action=(reg0[9] = 1; next;) >>> table=8 (ls_in_acl_hint ), priority=2 , match=(ct.est && >>> ct_label.blocked == 1), action=(reg0[9] = 1; next;) >>> table=8 (ls_in_acl_hint ), priority=1 , match=(ct.est && >>> ct_label.blocked == 0), action=(reg0[10] = 1; next;) >>> table=8 (ls_in_acl_hint ), priority=0 , match=(1), action=(next;) >>> table=9 (ls_in_acl ), priority=65535, match=(!ct.est && ct.rel && >>> !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) >>> table=9 (ls_in_acl ), priority=65535, match=(ct.est && !ct.rel && >>> !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) >>> table=9 (ls_in_acl ), priority=65535, match=(ct.inv || (ct.est && >>> ct.rpl && ct_label.blocked == 1)), action=(drop;) >>> table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra || nd_rs >>> || mldv1 || mldv2), action=(next;) >>> table=9 (ls_in_acl ), priority=34000, match=(eth.dst == >>> $svc_monitor_mac), action=(next;) >>> table=9 (ls_in_acl ), priority=1 , match=(ip && (!ct.est || >>> (ct.est && ct_label.blocked == 1))), action=(reg0[1] = 1; next;) >>> table=9 (ls_in_acl ), priority=0 , match=(1), action=(next;) >>> >>> Because there are higher priority flows in 'ls_in_acl_hint' and >>> 'ls_in_acl' with the match on conntrack fields, >>> ovs-vswitchd will generate a datapath flow with the match on ct_state >>> fields as - >>> 'ct_state(-new-est-rel-rpl-inv-trk)' for the packet from S1, even though >>> the S1 pipeline doesn't have logical flows which match on conntrack >>> fields. [1] has more information. >>> >>> Modifying the below flows if a logical switch has no ACLs solves this >>> problem. >>> >>> table=8 (ls_in_acl_hint ), priority=65535 , match=(1), action=(next;) >>> table=9 (ls_in_acl ), priority=65535 , match=(1), action=(next;) >>> >>> With the above flows with higher priority, ovs-vswitchd will not >>> consider other flows in the same table during translation. >>> >>> This patch addresses this issue by using higher prioriy flows (for both >>> ls_in_acl* and ls_out_acl* stages). >>> >>> [1] - https://bugzilla.redhat.com/show_bug.cgi?id=1955191#c8 >>> >>> Signed-off-by: Numan Siddique <[email protected]> >>> --- >> >> Hi Numan, >> >> This needs a rebase after the recent commits to master. > > Thanks for the review. Sure I'll rebase and submit v2. > >> >>> northd/lswitch.dl | 12 ++++ >>> northd/ovn-northd.8.xml | 32 ++++++---- >>> northd/ovn-northd.c | 62 ++++++++++++++----- >>> northd/ovn_northd.dl | 62 +++++++++++-------- >>> tests/ovn-northd.at | 51 ++++++++++++---- >>> tests/system-ovn.at | 130 ++++++++++++++++++++++++++++++++++++++++ >>> 6 files changed, 284 insertions(+), 65 deletions(-) >>> >>> diff --git a/northd/lswitch.dl b/northd/lswitch.dl >>> index 47c497e0cf..9bcfe9c321 100644 >>> --- a/northd/lswitch.dl >>> +++ b/northd/lswitch.dl >>> @@ -125,6 +125,15 @@ LogicalSwitchHasStatefulACL(ls, false) :- >>> nb::Logical_Switch(._uuid = ls), >>> not LogicalSwitchStatefulACL(ls, _). >>> >>> +relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) >>> + >>> +LogicalSwitchHasACLs(ls, true) :- >>> + LogicalSwitchACL(ls, _). >>> + >>> +LogicalSwitchHasACLs(ls, false) :- >>> + nb::Logical_Switch(._uuid = ls), >>> + not LogicalSwitchACL(ls, _). >>> + >>> /* >>> * LogicalSwitchLocalnetPorts maps from each logical switch UUID >>> * to the logical switch's set of localnet ports. Each localnet >>> @@ -189,6 +198,7 @@ LogicalSwitchHasNonRouterPort(ls, false) :- >>> relation &Switch( >>> ls: nb::Logical_Switch, >>> has_stateful_acl: bool, >>> + has_acls: bool, >>> has_lb_vip: bool, >>> has_dns_records: bool, >>> has_unknown_ports: bool, >>> @@ -215,6 +225,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> >>> { >>> >>> &Switch(.ls = ls, >>> .has_stateful_acl = has_stateful_acl, >>> + .has_acls = has_acls, >>> .has_lb_vip = has_lb_vip, >>> .has_dns_records = has_dns_records, >>> .has_unknown_ports = has_unknown_ports, >>> @@ -226,6 +237,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> >>> { >>> .is_vlan_transparent = is_vlan_transparent) :- >>> nb::Logical_Switch[ls], >>> LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), >>> + LogicalSwitchHasACLs(ls._uuid, has_acls), >>> LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip), >>> LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records), >>> LogicalSwitchHasUnknownPorts(ls._uuid, has_unknown_ports), >>> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml >>> index 54e88d3fac..90a1f7d0b3 100644 >>> --- a/northd/ovn-northd.8.xml >>> +++ b/northd/ovn-northd.8.xml >>> @@ -545,6 +545,14 @@ >>> <p> >>> The table contains the following flows: >>> </p> >>> + <ul> >>> + <li> >>> + A priority-65535 flow to advance to the next table if the logical >>> + switch has <code>no</code> ACLs configured, otherwise a >>> + priority-0 flow to advance to the next table. >>> + </li> >>> + </ul> >>> + >>> <ul> >>> <li> >>> A priority-7 flow that matches on packets that initiate a new >>> session. >>> @@ -585,9 +593,6 @@ >>> This flow sets <code>reg0[10]</code> and then advances to the next >>> table. >>> </li> >>> - <li> >>> - A priority-0 flow to advance to the next table. >>> - </li> >>> </ul> >>> >>> <h3>Ingress table 9: <code>from-lport</code> ACLs</h3> >>> @@ -633,9 +638,14 @@ >>> </ul> >>> >>> <p> >>> - This table also contains a priority 0 flow with action >>> - <code>next;</code>, so that ACLs allow packets by default. If the >>> - logical datapath has a stateful ACL or a load balancer with VIP >>> + This table contains a priority-65535 flow to advance to the next >>> table >>> + if the logical switch has <code>no</code> ACLs configured, otherwise >>> a >>> + priority-0 flow to advance to the next table so that ACLs allow >>> + packets by default. >>> + </p> >>> + >>> + <p> >>> + If the logical datapath has a stateful ACL or a load balancer with >>> VIP >>> configured, the following flows will also be added: >>> </p> >>> >>> @@ -649,7 +659,7 @@ >>> </li> >>> >>> <li> >>> - A priority-65535 flow that allows any traffic in the reply >>> + A priority-65532 flow that allows any traffic in the reply >>> direction for a connection that has been committed to the >>> connection tracker (i.e., established flows), as long as >>> the committed flow does not have <code>ct_label.blocked</code> set. >>> @@ -662,19 +672,19 @@ >>> </li> >>> >>> <li> >>> - A priority-65535 flow that allows any traffic that is considered >>> + A priority-65532 flow that allows any traffic that is considered >>> related to a committed flow in the connection tracker (e.g., an >>> ICMP Port Unreachable from a non-listening UDP port), as long >>> as the committed flow does not have <code>ct_label.blocked</code> >>> set. >>> </li> >>> >>> <li> >>> - A priority-65535 flow that drops all traffic marked by the >>> + A priority-65532 flow that drops all traffic marked by the >>> connection tracker as invalid. >>> </li> >>> >>> <li> >>> - A priority-65535 flow that drops all traffic in the reply direction >>> + A priority-65532 flow that drops all traffic in the reply direction >>> with <code>ct_label.blocked</code> set meaning that the connection >>> should no longer be allowed due to a policy change. Packets >>> in the request direction are skipped here to let a newly created >>> @@ -682,7 +692,7 @@ >>> </li> >>> >>> <li> >>> - A priority-65535 flow that allows IPv6 Neighbor solicitation, >>> + A priority-65532 flow that allows IPv6 Neighbor solicitation, >>> Neighbor discover, Router solicitation, Router advertisement and >>> MLD >>> packets. >>> </li> >>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >>> index 94fae5648a..dfe4225bb3 100644 >>> --- a/northd/ovn-northd.c >>> +++ b/northd/ovn-northd.c >>> @@ -627,6 +627,7 @@ struct ovn_datapath { >>> bool has_stateful_acl; >>> bool has_lb_vip; >>> bool has_unknown; >>> + bool has_acls; >>> >>> /* IPAM data. */ >>> struct ipam_info ipam_info; >>> @@ -4783,6 +4784,23 @@ ls_has_stateful_acl(struct ovn_datapath *od) >>> return false; >>> } >>> >>> +static bool >>> +ls_has_acls(struct ovn_datapath *od) >>> +{ >>> + if (od->nbs->n_acls) { >>> + return true; >>> + } >>> + >>> + struct ovn_ls_port_group *ls_pg; >>> + HMAP_FOR_EACH (ls_pg, key_node, &od->nb_pgs) { >>> + if (ls_pg->nb_pg->n_acls) { >>> + return true; >>> + } >>> + } >>> + >>> + return false; >>> +} >> >> nit: ls_has_stateful_acl() and ls_has_acl() both walk the port groups >> associated to a logical switch. I wonder if it makes sense to combine >> the functions in one that sets both "has_acls" and "has_stateful_acl" in >> one go. >> > > Ack. Sounds good. > >>> + >>> /* Logical switch ingress table 0: Ingress port security - L2 >>> * (priority 50). >>> * Ingress table 1: Ingress port security - IP (priority 90 and 80) >>> @@ -5237,7 +5255,11 @@ build_acl_hints(struct ovn_datapath *od, struct hmap >>> *lflows) >>> enum ovn_stage stage = stages[i]; >>> >>> /* In any case, advance to the next stage. */ >>> - ovn_lflow_add(lflows, od, stage, 0, "1", "next;"); >>> + if (!od->has_acls && !od->has_lb_vip) { >>> + ovn_lflow_add(lflows, od, stage, UINT16_MAX, "1", "next;"); >>> + } else { >>> + ovn_lflow_add(lflows, od, stage, 0, "1", "next;"); >>> + } >>> >>> if (!od->has_stateful_acl && !od->has_lb_vip) { >>> continue; >> >> I didn't test this it out thoroughly but isn't it enough to change this >> whole block to: >> >> if (!od->has_stateful_acl && !od->has_lb_vip) { >> ovn_lflow_add(lflows, od, stage, UINT16_MAX, "1", "next;"); >> continue; >> } else { >> ovn_lflow_add(lflows, od, stage, 0, "1", "next;"); >> } > > Not really. > > If a logical switch has no ACLs or no LB VIPs we want to add > 65535-prio flow to advance the > packet to the next stage. But if a logical switch has any ACL (be it > allow, allow-related or drop) > we want to add a normal 0-priority flow to advance the packet to the next > stage. >
But if the switch has no stateful ACL or load balancer we might as well skip adding the flows that set CT hints, because they will not be used, I think. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
