[ovs-dev] [PATCH ovn] northd: Support flow offloading for logical switches with no ACLs.

Numan Siddique numans at ovn.org
Fri May 7 14:44:09 UTC 2021


On Fri, May 7, 2021 at 10:35 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <dceara at redhat.com> wrote:
> >
> > On 5/6/21 3:52 PM, numans at ovn.org wrote:
> > > From: Numan Siddique <numans at ovn.org>
> > >
> > > 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 <numans at ovn.org>
> > > ---
> >
> > Hi Numan,
> >
> > This needs a rebase after the recent commits to master.
>
> Thanks for the review.  Sure I'll rebase and submit v2.

v2 submitted - https://patchwork.ozlabs.org/project/ovn/patch/20210507144258.510878-1-numans@ovn.org/
Request to take a look.

Thanks
Numan

>
> >
> > >  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.
>
> Thanks
> Numan
>
> >
> > Regards,
> > Dumitru
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list