northd: Avoid matching on ct_state.dnat in logical flows.

Since commit f2e8130 ("northd: Explicitly handle SNAT for ICMP
need frag."), if LRP.gw_mtu is set, ovn-northd adds a priority 160
logical flow in the lr_in_larger_pkts router pipeline that matches
on "ct.trk && ct.rpl && ct.dnat" for egress traffic that's larger
than the configured MTU (if REGBIT_PKT_LARGER] == 1):

  table=23(lr_in_larger_pkts  ), priority=160,
  match=(inport == "lrp1" && outport == "lrp0" && ip4 &&
         reg9[1] && reg9[0] == 0 && ct.trk && ct.rpl && ct.dnat),
  action=(icmp4_error {...};)

The lr_in_larger_pkts logical pipeline stage (23) is translated by
ovn-controller to OpenFlow table 31.

 table=31, priority=160,
   ct_state=+rpl+trk+dnat,ip,reg9=0x2/0x3,reg14=0x2,reg15=0x1,metadata=0x3
   actions=controller(...)

Due to the way ovs-vswitchd translates OF rules to datapath flows,
all OpenFlow rules in table 31 that have a priority lower than 160
automatically get an implicit match on the inverse ct_state match
that's generated for the priority 160 OF rule, that is:

ct_state=-trk-rpl-dnat

On specific NICs, such matches (on dnat/snat ct_state) are not
offloadable to hardware.

However, in OVN, logical switches and logical routers pipelines both
get translated to the same set of OpenFlow tables. In theory that's
fine because the logical datapath distinction happens thanks to
additional metadata field matches (the metadata OF field is actually
logical datapath id).  But in this specific case it means that all
logical switch traffic that hits OpenFlow table 31 will also generate
a datapath flow that matches on ct_state=-trk-rpl-dnat, making it
non-offloadable.

E.g., in an OVN cluster with a logical switch that has no stateful
config (no stateful ACLs or LBs) traffic hitting the following
logical switch pipeline stage will also fail to be offloaded to
hardware if there's a logical router with gw_mtu configured:

  table=23(ls_in_dhcp_options ), priority=0, match=(1), action=(next;)

That is essentially all traffic forwarded on that logical switch.

Change the way we match on large packets that have been DNATed and
instead of directly matching on ct_state first save that into a
register (using the new ct_state_save() logical action).

That means that with the configuration mentioned above:
- all routed traffic that is less than MTU will now be forwarded by
  a datapath flow matching on ct_state=-trk
- all switched traffic will be forwarded by a datapath flow matching on
  ct_state=-trk

In order to not disrupt upgrades on stable branches generate the new
logical flows only if the ct_state_save() action is supported by all
ovn-controllers.

Reported-at: https://issues.redhat.com/browse/FDP-1271
Fixes: f2e8130 ("northd: Explicitly handle SNAT for ICMP need frag.")
Acked-by: Ales Musil <[email protected]>
Signed-off-by: Dumitru Ceara <[email protected]>
(cherry picked from commit 35a3092)

Author: dceara

northd/northd.c

@@ -205,6 +205,19 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2));
 #define REG_OBS_COLLECTOR_ID_NEW "reg8[0..7]"
 #define REG_OBS_COLLECTOR_ID_EST "reg8[8..15]"
 
+/* Register used for storing the ct_state in the router pipeline and
+ * ct_saved_state helpers, matching the ct_state bits definitions from
+ * ovs-fields(7). */
+#define REG_CT_STATE "reg4[0..7]"
+
+static const char *reg_ct_state[] = {
+#define CS_STATE(ENUM, INDEX, NAME) \
+    [CS_##ENUM] = "reg4[" #INDEX "]",
+
+    CS_STATES
+#undef CS_STATE
+};
+
 /* Register used for temporarily store ECMP eth.src to avoid masked ct_label
  * access. It doesn't really occupy registers because the content of the
  * register is saved to stack and then restored in the same flow.
@@ -268,10 +281,12 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2));
  * |     |                           | 1 |                 |   |                                    |
  * +-----+---------------------------+---+-----------------+---+------------------------------------+
  * | R4  |  REG_LB_AFF_BACKEND_IP4   | X |                 |   |                                    |
- * |     |                           | R |                 |   |                                    |
- * +-----+---------------------------+ E |     UNUSED      | X |                                    |
- * | R5  |        UNUSED             | G |                 | X |                                    |
- * |     |                           | 2 |                 | R |        LB_L3_AFF_BACKEND_IP6       |
+ * |     |        REG_CT_STATE       | R |                 |   |                                    |
+ * |     |  (>= IN_CHK_PKT_LEN &&    | R |                 |   |                                    |
+ * |     |   <= IN_LARGER_PKTS)      | E |                 |   |                                    |
+ * +-----+---------------------------+ G |     UNUSED      | X |                                    |
+ * | R5  |        UNUSED             | 2 |                 | X |                                    |
+ * |     |                           |   |                 | R |        LB_L3_AFF_BACKEND_IP6       |
  * +-----+---------------------------+---+-----------------+ E |           (<= IN_DNAT)             |
  * | R6  |        UNUSED             | X |                 | G |                                    |
  * |     |                           | R |                 | 1 |                                    |
@@ -14576,6 +14591,7 @@ build_icmperr_pkt_big_flows(struct ovn_port *op, int mtu,
                             const struct shash *meter_groups, struct ds *match,
                             struct ds *actions, enum ovn_stage stage,
                             struct ovn_port *outport,
+                            const char *ct_state_match,
                             struct lflow_ref *lflow_ref)
 {
     const char *ipv4_meter = copp_meter_get(COPP_ICMP4_ERR, op->od->nbr->copp,
@@ -14587,15 +14603,17 @@ build_icmperr_pkt_big_flows(struct ovn_port *op, int mtu,
     ds_put_format(match, "inport == %s", op->json_key);
 
     if (outport) {
+        ovs_assert(ct_state_match);
+
         ds_put_format(match, " && outport == %s", outport->json_key);
 
         create_icmp_need_frag_lflow(op, mtu, actions, match, ipv4_meter,
                                     lflows, lflow_ref, stage, 160, false,
-                                    "ct.trk && ct.rpl && ct.dnat",
+                                    ct_state_match,
                                     "flags.icmp_snat = 1; ");
         create_icmp_need_frag_lflow(op, mtu, actions, match, ipv6_meter,
                                     lflows, lflow_ref, stage, 160, true,
-                                    "ct.trk && ct.rpl && ct.dnat",
+                                    ct_state_match,
                                     "flags.icmp_snat = 1; ");
     }
 
@@ -14621,15 +14639,25 @@ build_check_pkt_len_flows_for_lrp(struct ovn_port *op,
 
     ds_clear(match);
     ds_put_format(match, "outport == %s", op->json_key);
+    const char *mtu_flow_action = features->ct_state_save
+                                  ? REG_CT_STATE " = ct_state_save(); next;"
+                                  : "next;";
     build_gateway_mtu_flow(lflows, op, S_ROUTER_IN_CHK_PKT_LEN, 50, 55,
-                           match, actions, &op->nbrp->header_,
-                           lflow_ref, "next;");
+                           match, actions, &op->nbrp->header_, lflow_ref,
+                           "%s", mtu_flow_action);
 
     /* ingress traffic */
     build_icmperr_pkt_big_flows(op, gw_mtu, lflows, meter_groups,
                                 match, actions, S_ROUTER_IN_IP_INPUT,
-                                NULL, lflow_ref);
-
+                                NULL, NULL, lflow_ref);
+
+    /* Additional match at egress on tracked and reply and dnat-ed traffic. */
+    char *ct_match = features->ct_state_save
+                     ? xasprintf("%s && %s && %s",
+                                 reg_ct_state[CS_TRACKED],
+                                 reg_ct_state[CS_REPLY_DIR],
+                                 reg_ct_state[CS_DST_NAT])
+                     : xstrdup("ct.trk && ct.rpl && ct.dnat");
     for (size_t i = 0; i < op->od->nbr->n_ports; i++) {
         struct ovn_port *rp = ovn_port_find(lr_ports,
                                             op->od->nbr->ports[i]->name);
@@ -14640,8 +14668,9 @@ build_check_pkt_len_flows_for_lrp(struct ovn_port *op,
         /* egress traffic */
         build_icmperr_pkt_big_flows(rp, gw_mtu, lflows, meter_groups,
                                     match, actions, S_ROUTER_IN_LARGER_PKTS,
-                                    op, lflow_ref);
+                                    op, ct_match, lflow_ref);
     }
+    free(ct_match);
 
     if (features->ct_commit_nat_v2) {
         ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_OUT_POST_SNAT, 100,

northd/ovn-northd.8.xml

@@ -4835,12 +4835,14 @@ output;
       integer value, this table adds a priority-50 logical flow with
       the match <code>outport == <var>GW_PORT</var></code> where
       <var>GW_PORT</var> is the gateway router port and applies the
-      action <code>check_pkt_larger</code> and advances the packet to
-      the next table.
+      actions <code>check_pkt_larger</code> and <code>ct_state_save</code>
+      and then advances the packet to the next table.
     </p>
 
     <pre>
-REGBIT_PKT_LARGER = check_pkt_larger(<var>L</var>); next;
+REGBIT_PKT_LARGER = check_pkt_larger(<var>L</var>);
+REG_CT_STATE = ct_state_save();
+next;
     </pre>
 
     <p>