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 ? Thanks Numan > > > > Thanks > > Numan > > > > Thanks, > Dumitru > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
