controller: FDB entries for localnet should not overwrite entries for…

… vifs

When localnet_learn_fdb is set, some localnet entries were overwriting
entries from vifs; as a result, packets were not delivered to vifs anymore.
This could have happened in the following config:
- Two hv (hv1 and hv2)
- vif1, vif2 and ln on ls
- vif1 on hv1 and vif2 on hv2
When vif1 (mac1) sends a packets to vif2 (mac2) through localnet
- mac1 is recorded in FDB when packet enters ls in hv1 (w/ in_port = vif1)
- mac1 is recorded in FDB when packet enters ls in hv2 (w/ in_port = ln)
- ...
Next time a packet is received by ln on hv1 w/ dst=mac1, the packet was dropped.

Flows have been added to prevent localnet entry to overwrite vif entry:
Lookup_fdb(port, mac) will succeed if
- port and mac are correct
- mac is correct and port is localnet

In addition, to handle potential race condition (packet received by hv1, mac1-vif1
written in fdb but flows not yet created in hv2 when packet is received in hv2),
slow path (pinctrl) also prevents localnet port to overwite vif port entry.

The extra flows (one per vif) are added whether or not localnet_learn_fdb is enabled.
They are only used if localnet_learn_fdb is enabled.
A future patch will prevent creating unncessary flows if localnet_learn_fdb is not
enabled.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2242830
Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
Reviewed-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>

controller/lflow.c

@@ -2065,11 +2065,16 @@ lflow_handle_changed_static_mac_bindings(
 static void
 consider_fdb_flows(const struct sbrec_fdb *fdb,
                    const struct hmap *local_datapaths,
-                   struct ovn_desired_flow_table *flow_table)
+                   struct ovn_desired_flow_table *flow_table,
+                   struct ovsdb_idl_index *sbrec_port_binding_by_key)
 {
-    if (!get_local_datapath(local_datapaths, fdb->dp_key)) {
+    struct local_datapath *ld = get_local_datapath(local_datapaths,
+                                                   fdb->dp_key);
+    if (!ld) {
         return;
     }
+    const struct sbrec_port_binding *pb = lport_lookup_by_key_with_dp(
+        sbrec_port_binding_by_key, ld->datapath, fdb->port_key);
 
     struct eth_addr mac;
     if (!eth_addr_from_string(fdb->mac, &mac)) {
@@ -2091,6 +2096,7 @@ consider_fdb_flows(const struct sbrec_fdb *fdb,
     ofpbuf_clear(&ofpacts);
 
     uint8_t value = 1;
+    uint8_t is_vif =  pb ? !strcmp(pb->type, "") : 0;
     put_load(&value, sizeof value, MFF_LOG_FLAGS,
              MLF_LOOKUP_FDB_BIT, 1, &ofpacts);
 
@@ -2101,6 +2107,18 @@ consider_fdb_flows(const struct sbrec_fdb *fdb,
     ofctrl_add_flow(flow_table, OFTABLE_LOOKUP_FDB, 100,
                     fdb->header_.uuid.parts[0], &lookup_match, &ofpacts,
                     &fdb->header_.uuid);
+
+    if (is_vif) {
+        struct match lookup_match_vif = MATCH_CATCHALL_INITIALIZER;
+        match_set_metadata(&lookup_match_vif, htonll(fdb->dp_key));
+        match_set_dl_src(&lookup_match_vif, mac);
+        match_set_reg_masked(&lookup_match_vif, MFF_LOG_FLAGS - MFF_REG0,
+                             MLF_LOCALNET, MLF_LOCALNET);
+
+        ofctrl_add_flow(flow_table, OFTABLE_LOOKUP_FDB, 100,
+                        fdb->header_.uuid.parts[0], &lookup_match_vif,
+                        &ofpacts, &fdb->header_.uuid);
+    }
     ofpbuf_uninit(&ofpacts);
 }
 
@@ -2109,11 +2127,13 @@ consider_fdb_flows(const struct sbrec_fdb *fdb,
 static void
 add_fdb_flows(const struct sbrec_fdb_table *fdb_table,
               const struct hmap *local_datapaths,
-              struct ovn_desired_flow_table *flow_table)
+              struct ovn_desired_flow_table *flow_table,
+              struct ovsdb_idl_index *sbrec_port_binding_by_key)
 {
     const struct sbrec_fdb *fdb;
     SBREC_FDB_TABLE_FOR_EACH (fdb, fdb_table) {
-        consider_fdb_flows(fdb, local_datapaths, flow_table);
+        consider_fdb_flows(fdb, local_datapaths, flow_table,
+                           sbrec_port_binding_by_key);
     }
 }
 
@@ -2136,7 +2156,8 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out)
                          l_ctx_in->lb_hairpin_use_ct_mark,
                          l_ctx_out->flow_table);
     add_fdb_flows(l_ctx_in->fdb_table, l_ctx_in->local_datapaths,
-                  l_ctx_out->flow_table);
+                  l_ctx_out->flow_table,
+                  l_ctx_in->sbrec_port_binding_by_key);
     add_port_sec_flows(l_ctx_in->binding_lports, l_ctx_in->chassis,
                        l_ctx_out->flow_table);
 }
@@ -2226,7 +2247,8 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
     SBREC_FDB_FOR_EACH_EQUAL (fdb_row, fdb_index_row,
                               l_ctx_in->sbrec_fdb_by_dp_key) {
         consider_fdb_flows(fdb_row, l_ctx_in->local_datapaths,
-                           l_ctx_out->flow_table);
+                           l_ctx_out->flow_table,
+                           l_ctx_in->sbrec_port_binding_by_key);
     }
     sbrec_fdb_index_destroy_row(fdb_index_row);
 
@@ -2419,7 +2441,8 @@ lflow_handle_changed_fdbs(struct lflow_ctx_in *l_ctx_in,
         VLOG_DBG("Add fdb flows for fdb "UUID_FMT,
                  UUID_ARGS(&fdb->header_.uuid));
         consider_fdb_flows(fdb, l_ctx_in->local_datapaths,
-                           l_ctx_out->flow_table);
+                           l_ctx_out->flow_table,
+                           l_ctx_in->sbrec_port_binding_by_key);
     }
 
     return true;

controller/lflow.h

@@ -100,6 +100,7 @@ struct lflow_ctx_in {
     struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
     struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group;
     struct ovsdb_idl_index *sbrec_port_binding_by_name;
+    struct ovsdb_idl_index *sbrec_port_binding_by_key;
     struct ovsdb_idl_index *sbrec_fdb_by_dp_key;
     struct ovsdb_idl_index *sbrec_mac_binding_by_datapath;
     struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath;

controller/lport.c

@@ -44,13 +44,10 @@ lport_lookup_by_name(struct ovsdb_idl_index *sbrec_port_binding_by_name,
 }
 
 const struct sbrec_port_binding *
-lport_lookup_by_key(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
-                    struct ovsdb_idl_index *sbrec_port_binding_by_key,
-                    uint64_t dp_key, uint64_t port_key)
+lport_lookup_by_key_with_dp(struct ovsdb_idl_index *sbrec_port_binding_by_key,
+                            const struct sbrec_datapath_binding *db,
+                            uint64_t port_key)
 {
-    /* Lookup datapath corresponding to dp_key. */
-    const struct sbrec_datapath_binding *db = datapath_lookup_by_key(
-        sbrec_datapath_binding_by_key, dp_key);
     if (!db) {
         return NULL;
     }
@@ -69,6 +66,19 @@ lport_lookup_by_key(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
     return retval;
 }
 
+const struct sbrec_port_binding *
+lport_lookup_by_key(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
+                    struct ovsdb_idl_index *sbrec_port_binding_by_key,
+                    uint64_t dp_key, uint64_t port_key)
+{
+    /* Lookup datapath corresponding to dp_key. */
+    const struct sbrec_datapath_binding *db = datapath_lookup_by_key(
+        sbrec_datapath_binding_by_key, dp_key);
+
+    return lport_lookup_by_key_with_dp(sbrec_port_binding_by_key, db,
+                                       port_key);
+}
+
 bool
 lport_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
                           const struct sbrec_chassis *chassis,

controller/lport.h

@@ -43,6 +43,10 @@ const struct sbrec_port_binding *lport_lookup_by_key(
     struct ovsdb_idl_index *sbrec_port_binding_by_key,
     uint64_t dp_key, uint64_t port_key);
 
+const struct sbrec_port_binding *lport_lookup_by_key_with_dp(
+    struct ovsdb_idl_index *sbrec_port_binding_by_key,
+    const struct sbrec_datapath_binding *dp, uint64_t port_key);
+
 enum can_bind {
     CANNOT_BIND = 0,
     CAN_BIND_AS_MAIN,

controller/ovn-controller.c

@@ -3792,6 +3792,11 @@ init_lflow_ctx(struct engine_node *node,
                 engine_get_input("SB_port_binding", node),
                 "name");
 
+    struct ovsdb_idl_index *sbrec_port_binding_by_key =
+        engine_ovsdb_node_get_index(
+                engine_get_input("SB_port_binding", node),
+                "key");
+
     struct ovsdb_idl_index *sbrec_logical_flow_by_dp =
         engine_ovsdb_node_get_index(
                 engine_get_input("SB_logical_flow", node),
@@ -3891,6 +3896,7 @@ init_lflow_ctx(struct engine_node *node,
     l_ctx_in->sbrec_logical_flow_by_logical_dp_group =
         sbrec_logical_flow_by_dp_group;
     l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
+    l_ctx_in->sbrec_port_binding_by_key = sbrec_port_binding_by_key;
     l_ctx_in->sbrec_fdb_by_dp_key = sbrec_fdb_by_dp_key;
     l_ctx_in->sbrec_mac_binding_by_datapath = sbrec_mac_binding_by_datapath;
     l_ctx_in->sbrec_static_mac_binding_by_datapath =

controller/pinctrl.c

@@ -378,9 +378,13 @@ static const struct sbrec_fdb *fdb_lookup(
     uint32_t dp_key, const char *mac);
 static void run_put_fdb(struct ovsdb_idl_txn *ovnsb_idl_txn,
                         struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac,
+            struct ovsdb_idl_index *sbrec_port_binding_by_key,
+            struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                         const struct fdb_entry *fdb_e)
                         OVS_REQUIRES(pinctrl_mutex);
 static void run_put_fdbs(struct ovsdb_idl_txn *ovnsb_idl_txn,
+            struct ovsdb_idl_index *sbrec_port_binding_by_key,
+            struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                         struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac)
                         OVS_REQUIRES(pinctrl_mutex);
 static void wait_put_fdbs(struct ovsdb_idl_txn *ovnsb_idl_txn);
@@ -3627,7 +3631,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
                       chassis);
     bfd_monitor_run(ovnsb_idl_txn, bfd_table, sbrec_port_binding_by_name,
                     chassis, active_tunnels);
-    run_put_fdbs(ovnsb_idl_txn, sbrec_fdb_by_dp_key_mac);
+    run_put_fdbs(ovnsb_idl_txn, sbrec_port_binding_by_key,
+                 sbrec_datapath_binding_by_key, sbrec_fdb_by_dp_key_mac);
     run_activated_ports(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
                         sbrec_port_binding_by_key, chassis);
     ovs_mutex_unlock(&pinctrl_mutex);
@@ -8299,6 +8304,8 @@ fdb_lookup(struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac, uint32_t dp_key,
 static void
 run_put_fdb(struct ovsdb_idl_txn *ovnsb_idl_txn,
             struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac,
+            struct ovsdb_idl_index *sbrec_port_binding_by_key,
+            struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
             const struct fdb_entry *fdb_e)
 {
     /* Convert ethernet argument to string form for database. */
@@ -8307,14 +8314,28 @@ run_put_fdb(struct ovsdb_idl_txn *ovnsb_idl_txn,
              ETH_ADDR_FMT, ETH_ADDR_ARGS(fdb_e->mac));
 
     /* Update or add an FDB entry. */
+    const struct sbrec_port_binding *sb_entry_pb = NULL;
+    const struct sbrec_port_binding *new_entry_pb = NULL;
     const struct sbrec_fdb *sb_fdb =
         fdb_lookup(sbrec_fdb_by_dp_key_mac, fdb_e->dp_key, mac_string);
     if (!sb_fdb) {
         sb_fdb = sbrec_fdb_insert(ovnsb_idl_txn);
         sbrec_fdb_set_dp_key(sb_fdb, fdb_e->dp_key);
         sbrec_fdb_set_mac(sb_fdb, mac_string);
+    } else {
+        /* check whether sb_fdb->port_key is vif or localnet type */
+        sb_entry_pb = lport_lookup_by_key(
+            sbrec_datapath_binding_by_key, sbrec_port_binding_by_key,
+            sb_fdb->dp_key, sb_fdb->port_key);
+        new_entry_pb = lport_lookup_by_key(
+            sbrec_datapath_binding_by_key, sbrec_port_binding_by_key,
+            fdb_e->dp_key, fdb_e->port_key);
+    }
+    /* Do not have localnet overwrite a previous vif entry */
+    if (!sb_entry_pb || !new_entry_pb || strcmp(sb_entry_pb->type, "") ||
+        strcmp(new_entry_pb->type, "localnet")) {
+        sbrec_fdb_set_port_key(sb_fdb, fdb_e->port_key);
     }
-    sbrec_fdb_set_port_key(sb_fdb, fdb_e->port_key);
 
     /* For backward compatibility check if timestamp column is available
      * in SB DB. */
@@ -8325,6 +8346,8 @@ run_put_fdb(struct ovsdb_idl_txn *ovnsb_idl_txn,
 
 static void
 run_put_fdbs(struct ovsdb_idl_txn *ovnsb_idl_txn,
+             struct ovsdb_idl_index *sbrec_port_binding_by_key,
+             struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
              struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac)
              OVS_REQUIRES(pinctrl_mutex)
 {
@@ -8334,7 +8357,9 @@ run_put_fdbs(struct ovsdb_idl_txn *ovnsb_idl_txn,
 
     const struct fdb_entry *fdb_e;
     HMAP_FOR_EACH (fdb_e, hmap_node, &put_fdbs) {
-        run_put_fdb(ovnsb_idl_txn, sbrec_fdb_by_dp_key_mac, fdb_e);
+        run_put_fdb(ovnsb_idl_txn, sbrec_fdb_by_dp_key_mac,
+                    sbrec_port_binding_by_key,
+                    sbrec_datapath_binding_by_key, fdb_e);
     }
     ovn_fdbs_flush(&put_fdbs);
 }

include/ovn/logical-fields.h

@@ -77,6 +77,7 @@ enum mff_log_flags_bits {
     MLF_CHECK_PORT_SEC_BIT = 12,
     MLF_LOOKUP_COMMIT_ECMP_NH_BIT = 13,
     MLF_USE_LB_AFF_SESSION_BIT = 14,
+    MLF_LOCALNET_BIT = 15,
 };
 
 /* MFF_LOG_FLAGS_REG flag assignments */
@@ -124,6 +125,10 @@ enum mff_log_flags {
     MLF_LOOKUP_COMMIT_ECMP_NH = (1 << MLF_LOOKUP_COMMIT_ECMP_NH_BIT),
 
     MLF_USE_LB_AFF_SESSION = (1 << MLF_USE_LB_AFF_SESSION_BIT),
+
+    /* Indicate that the port is localnet. */
+    MLF_LOCALNET = (1 << MLF_LOCALNET_BIT),
+
 };
 
 /* OVN logical fields

lib/logical-fields.c

@@ -129,6 +129,10 @@ ovn_init_symtab(struct shash *symtab)
              MLF_USE_SNAT_ZONE);
     expr_symtab_add_subfield(symtab, "flags.use_snat_zone", NULL,
                              flags_str);
+    snprintf(flags_str, sizeof flags_str, "flags[%d]",
+             MLF_LOCALNET_BIT);
+    expr_symtab_add_subfield(symtab, "flags.localnet", NULL,
+                             flags_str);
 
     /* Connection tracking state. */
     expr_symtab_add_field_scoped(symtab, "ct_mark", MFF_CT_MARK, NULL, false,

northd/northd.c

@@ -7045,6 +7045,9 @@ build_lswitch_learn_fdb_op(
         ds_clear(match);
         ds_clear(actions);
         ds_put_format(match, "inport == %s", op->json_key);
+        if (lsp_is_localnet(op->nbsp)) {
+            ds_put_cstr(actions, "flags.localnet = 1; ");
+        }
         ds_put_format(actions, REGBIT_LKUP_FDB
                       " = lookup_fdb(inport, eth.src); next;");
         ovn_lflow_add_with_lport_and_hint(lflows, op->od,

northd/ovn-northd.8.xml

@@ -399,14 +399,16 @@
     <p>
       This table looks up the MAC learning table of the logical switch
       datapath to check if the <code>port-mac</code> pair is present
-      or not. MAC is learnt only for logical switch VIF ports whose
-      port security is disabled and 'unknown' address set.
+      or not. MAC is learnt for logical switch VIF ports whose
+      port security is disabled and 'unknown' address setn as well as
+      for localnet ports with option localnet_learn_fdb. A localnet
+      port entry does not overwrite a VIF port entry.
     </p>
 
     <ul>
       <li>
         <p>
-          For each such logical port <var>p</var> whose port security
+          For each such VIF logical port <var>p</var> whose port security
           is disabled and 'unknown' address set following flow
           is added.
         </p>
@@ -420,6 +422,22 @@
         </ul>
       </li>
 
+      <li>
+        <p>
+          For each such localnet logical port <var>p</var> following flow
+          is added.
+        </p>
+
+        <ul>
+          <li>
+            Priority 100 flow with the match
+            <code>inport == <var>p</var></code> and action
+            <code>flags.localnet = 1;</code>
+            <code>reg0[11] = lookup_fdb(inport, eth.src); next;</code>
+          </li>
+        </ul>
+      </li>
+
       <li>
         One priority-0 fallback flow that matches all packets and advances to
         the next table.
@@ -429,17 +447,20 @@
     <h3>Ingress Table 3: Learn MAC of 'unknown' ports.</h3>
 
     <p>
-      This table learns the MAC addresses seen on the logical ports
-      whose port security is disabled and 'unknown' address set
+      This table learns the MAC addresses seen on the VIF logical ports
+      whose port security is disabled and 'unknown' address set as well
+      as on localnet ports with localnet_learn_fdb option set
       if the <code>lookup_fdb</code> action returned false in the
-      previous table.
+      previous table. For localnet ports (with flags.localnet = 1),
+      lookup_fdb returns true if (port, mac) is found or if a mac
+      is found for a port of type vif.
     </p>
 
     <ul>
       <li>
         <p>
-          For each such logical port <var>p</var> whose port security
-          is disabled and 'unknown' address set following flow
+          For each such VIF logical port <var>p</var> whose port security is
+          disabled and 'unknown' address set and localnet port following flow
           is added.
         </p>
 

ovn-sb.xml

@@ -1721,9 +1721,12 @@
 
           <p>
             Looks up <var>A</var> in fdb table. If an entry is found
-            and the the logical port key is <var>P</var>, <code>P</code>,
+            and the logical port key is <var>P</var>, <code>P</code>,
             stores <code>1</code> in the 1-bit subfield
-            <var>R</var>, else 0.
+            <var>R</var>, else 0. If <code>flags.localnet</code> is set
+            then <code>1</code> is stored if an entry is found and the
+            logical port key is <var>P</var> or if an entry is found and
+            the entry port type is VIF.
           </p>
 
           <p>

tests/ovn-northd.at

@@ -8644,7 +8644,7 @@ AT_CHECK([ovn-sbctl dump-flows ls0 | grep -e 'ls_in_\(put\|lookup\)_fdb' | sort
 AT_CHECK([ovn-nbctl --wait=sb lsp-set-options ln_port localnet_learn_fdb=true])
 AT_CHECK([ovn-sbctl dump-flows ls0 | grep -e 'ls_in_\(put\|lookup\)_fdb' | sort | sed 's/table=./table=?/'], [0], [dnl
   table=? (ls_in_lookup_fdb   ), priority=0    , match=(1), action=(next;)
-  table=? (ls_in_lookup_fdb   ), priority=100  , match=(inport == "ln_port"), action=(reg0[[11]] = lookup_fdb(inport, eth.src); next;)
+  table=? (ls_in_lookup_fdb   ), priority=100  , match=(inport == "ln_port"), action=(flags.localnet = 1; reg0[[11]] = lookup_fdb(inport, eth.src); next;)
   table=? (ls_in_put_fdb      ), priority=0    , match=(1), action=(next;)
   table=? (ls_in_put_fdb      ), priority=100  , match=(inport == "ln_port" && reg0[[11]] == 0), action=(put_fdb(inport, eth.src); next;)
 ])