On 5/10/21 4:44 PM, Numan Siddique wrote: > On Mon, May 10, 2021 at 9:29 AM Dumitru Ceara <[email protected]> wrote: >> >> On 5/10/21 11:58 AM, Numan Siddique wrote: >>> On Fri, May 7, 2021 at 11:26 AM Dumitru Ceara <[email protected]> wrote: >>>> >>>> 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. >>> >>> Yes. I agree. We do the same with the current master and this patch >>> too doesn't change >>> that behavior. >>> >>> This patch adds a 65535 flow to advance the pkt to the next stage in >>> ls_in_acl_hint if there are >>> no ACLs at all. If there is at least one ACL (be it allow, drop or >>> allow-related) associated with the logical switch, >>> priority-0 flow will be added to advance the pkt to the next stage. >>> >>> So you want a 65535 flow to advance the pkt to next stage if there are >>> no stateful ACLs ? >> >> Yes, that was what I was thinking. >> >>> >>> Please note that in the stage - "ls_in_acl" we cannot add >>> 65535-priority flow to advance the pkt to next stage >>> if there are stateless ACLs associated with logical switch (be it >>> allow or drop). >> >> I see now. >> >>> >>> So I thought to be consistent for both the stages - ls_in_acl_hint and >>> ls_in_acl. >>> >>> i.e If there are NO acls associated with a logical switch, then the >>> priority of the flow to advance the pkt to next stage >>> will be 65535, else it will be 0. >>> >>> Please let me know if I've missed anything or if I misunderstood you ? >> >> I'm worried of the case when: >> >> LS1-ACLs: >> - <match1>, prio X, then "allow-related" >> >> LS2-ACLs: >> - <match2>, prio Y, then "allow" >> >> Even though the two logical switches are independent and the ACLs >> defined on LS2 are "stateless" we'll still not be able to offload the >> flows, right? >> >> What if reduce the maximum number of allowed ACL priorities? It's >> currently (UINT16_MAX - 1000). Would it make sense to split it in half >> and when a logical switch has only "allow" ACLs use the top half, >> otherwise use the bottom half? >> >> That would ensure that flows for ACLs defined on LS2 always have higher >> priority than flows defined for ACLs on LS1 (which match on ct_state). >> > > This is an interesting idea. Thanks for the suggestion. Until now all > the flows would match on ct_state even if one binding in the chassis > belongs to a logical switch configured with ACLs. > > I think we can probably divide this in 2 stages. > 1. What this patch does. > 2. What you suggested. > > If that sounds good, I will work on your suggestion as a follow up patch ? > > I think having (1) would still help. I don't see too much of work for > (2), but I might > take some time to address it. > > Let me know if it works for you ?
Sure, sounds good to me. I'll have a look at the v2 of this patch soon. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
