[ovs-dev,2/4] mac-cache: Properly handle deletion of SB mac_bindings.

SB.MAC_Binding doesn't store a strong reference to SB.Port_Binding.
Instead it stores the port binding 'logical_port' name.  As the
mac-binding cache in ovn-controller stored entries indexed by
[datapath-key, port-key, ip] it meant that when processing
SB.MAC_Binding changes (including deletion) it had to first lookup the
corresponding SB.Port_Binding.

If the logical port associated with the MAC binding is deleted in the
same transaction then looking it up in the IDL index fails (deleted IDL
records are not indexed).  This leads to stale entries staying in the
mac cache and in consequence to potential use after free (of the SB
MAC_Binding record) or to crashes if the statctrl module tries to update
timestamps of MAC_Bindings that happened to have been removed in the
current iteration (but still linger in the cache).

In practice we don't need the port-key we can simply use the port
binding UUID as unique index.  However, the same struct mac_binding type
is used by pinctrl to temporarily efficiently store buffered packets for
yet to be resolved MAC bindings.  In those cases there's no SB
MAC_Binding record yet so the logical ingress port tunnel_key is used as
index.

The two ways of using the MAC binding maps are orthogonal, that is, we
will never use a single map to store both types of bindings.

In order to avoid issues we now include the SB MAC_Binding UUID into the
key of the struct mac_binding_data.

Reported-at: https://issues.redhat.com/browse/FDP-906
Fixes: b57f60a617b4 ("controller: Add MAC cache I-P node")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 controller/mac-cache.c      |  77 ++++++++++++++++++-------
 controller/mac-cache.h      |  30 +++++++---
 controller/ovn-controller.c |  67 ++++++++++------------
 controller/pinctrl.c        |  24 +++++---
 tests/ovn.at                | 110 ++++++++++++++++++++++++++++++++++++
 5 files changed, 236 insertions(+), 72 deletions(-)

Acked-by: Mark Michelson <mmichels@redhat.com>

On 11/1/24 09:29, Dumitru Ceara wrote:
> SB.MAC_Binding doesn't store a strong reference to SB.Port_Binding.
> Instead it stores the port binding 'logical_port' name.  As the
> mac-binding cache in ovn-controller stored entries indexed by
> [datapath-key, port-key, ip] it meant that when processing
> SB.MAC_Binding changes (including deletion) it had to first lookup the
> corresponding SB.Port_Binding.
> 
> If the logical port associated with the MAC binding is deleted in the
> same transaction then looking it up in the IDL index fails (deleted IDL
> records are not indexed).  This leads to stale entries staying in the
> mac cache and in consequence to potential use after free (of the SB
> MAC_Binding record) or to crashes if the statctrl module tries to update
> timestamps of MAC_Bindings that happened to have been removed in the
> current iteration (but still linger in the cache).
> 
> In practice we don't need the port-key we can simply use the port
> binding UUID as unique index.  However, the same struct mac_binding type
> is used by pinctrl to temporarily efficiently store buffered packets for
> yet to be resolved MAC bindings.  In those cases there's no SB
> MAC_Binding record yet so the logical ingress port tunnel_key is used as
> index. >
> The two ways of using the MAC binding maps are orthogonal, that is, we
> will never use a single map to store both types of bindings.
> 
> In order to avoid issues we now include the SB MAC_Binding UUID into the
> key of the struct mac_binding_data.
> 
> Reported-at: https://issues.redhat.com/browse/FDP-906
> Fixes: b57f60a617b4 ("controller: Add MAC cache I-P node")
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
>   controller/mac-cache.c      |  77 ++++++++++++++++++-------
>   controller/mac-cache.h      |  30 +++++++---
>   controller/ovn-controller.c |  67 ++++++++++------------
>   controller/pinctrl.c        |  24 +++++---
>   tests/ovn.at                | 110 ++++++++++++++++++++++++++++++++++++
>   5 files changed, 236 insertions(+), 72 deletions(-)
> 
> diff --git a/controller/mac-cache.c b/controller/mac-cache.c
> index de45d7a6a5..d6ba9e20ee 100644
> --- a/controller/mac-cache.c
> +++ b/controller/mac-cache.c
> @@ -142,21 +142,18 @@ mac_cache_thresholds_clear(struct mac_cache_data *data)
>   }
>   
>   /* MAC binding. */
> -struct mac_binding *
> +void
>   mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
>                   long long timestamp) {
>   
>       struct mac_binding *mb = mac_binding_find(map, &mb_data);
>       if (!mb) {
>           mb = xmalloc(sizeof *mb);
> -        mb->sbrec_mb = NULL;
>           hmap_insert(map, &mb->hmap_node, mac_binding_data_hash(&mb_data));
>       }
>   
>       mb->data = mb_data;
>       mb->timestamp = timestamp;
> -
> -    return mb;
>   }
>   
>   void
> @@ -181,24 +178,37 @@ mac_binding_find(const struct hmap *map,
>   }
>   
>   bool
> -mac_binding_data_from_sbrec(struct mac_binding_data *data,
> -                            const struct sbrec_mac_binding *mb,
> -                            struct ovsdb_idl_index *sbrec_pb_by_name)
> +mac_binding_data_parse(struct mac_binding_data *data,
> +                       uint32_t dp_key, uint32_t port_key,
> +                       const char *ip_str, const char *mac_str)
>   {
> -    const struct sbrec_port_binding *pb =
> -            lport_lookup_by_name(sbrec_pb_by_name, mb->logical_port);
> +    struct eth_addr mac;
> +    struct in6_addr ip;
>   
> -    if (!pb || !pb->datapath || !ip46_parse(mb->ip, &data->ip) ||
> -        !eth_addr_from_string(mb->mac, &data->mac)) {
> +    if (!ip46_parse(ip_str, &ip) || !eth_addr_from_string(mac_str, &mac)) {
>           static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> -        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s, "
> -                     "logical_port=%s", mb->ip, mb->mac, mb->logical_port);
> +        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s",
> +                     ip_str, mac_str);
>           return false;
>       }
>   
> -    data->dp_key = mb->datapath->tunnel_key;
> -    data->port_key = pb->tunnel_key;
> +    mac_binding_data_init(data, dp_key, port_key, ip, mac);
> +    return true;
> +}
> +
> +bool
> +mac_binding_data_from_sbrec(struct mac_binding_data *data,
> +                            const struct sbrec_mac_binding *mb)
> +{
> +    /* This explicitly sets the port_key to 0 as port_binding tunnel_keys
> +     * can change.  Instead use add the SB.MAC_Binding UUID as key; this
> +     * makes the mac_binding_data key unique. */
> +    if (!mac_binding_data_parse(data, mb->datapath->tunnel_key, 0,
> +                                mb->ip, mb->mac)) {
> +        return false;
> +    }
>   
> +    data->sbrec = mb;
>       return true;
>   }
>   
> @@ -211,6 +221,30 @@ mac_bindings_clear(struct hmap *map)
>       }
>   }
>   
> +void
> +mac_bindings_to_string(const struct hmap *map, struct ds *out_data)
> +{
> +    struct mac_binding *mb;
> +    HMAP_FOR_EACH (mb, hmap_node, map) {
> +        char ip[INET6_ADDRSTRLEN];
> +
> +        if (!ipv6_string_mapped(ip, &mb->data.ip)) {
> +            continue;
> +        }
> +        if (mb->data.sbrec) {
> +            ds_put_format(out_data, "SB UUID: " UUID_FMT ", ",
> +                          UUID_ARGS(&mb->data.sbrec->header_.uuid));
> +        } else {
> +            ds_put_cstr(out_data, "SB UUID: <none>, ");
> +        }
> +        ds_put_format(out_data, "datapath-key: %"PRIu32", "
> +                                "port-key: %"PRIu32", "
> +                                "ip: %s, mac: " ETH_ADDR_FMT "\n",
> +                      mb->data.dp_key, mb->data.port_key,
> +                      ip, ETH_ADDR_ARGS(mb->data.mac));
> +    }
> +}
> +
>   bool
>   sb_mac_binding_updated(const struct sbrec_mac_binding *mb)
>   {
> @@ -382,9 +416,9 @@ mac_binding_stats_run(struct ovs_list *stats_list, uint64_t *req_delay,
>            * used on this chassis. Also make sure that we don't update the
>            * timestamp more than once during the dump period. */
>           if (stats->idle_age_ms < threshold->value &&
> -                (timewall_now - mb->sbrec_mb->timestamp) >=
> +                (timewall_now - mb->data.sbrec->timestamp) >=
>               threshold->dump_period) {
> -            sbrec_mac_binding_set_timestamp(mb->sbrec_mb, timewall_now);
> +            sbrec_mac_binding_set_timestamp(mb->data.sbrec, timewall_now);
>           }
>   
>           free(stats);
> @@ -608,7 +642,9 @@ mac_cache_stats_destroy(struct ovs_list *stats_list)
>   static uint32_t
>   mac_binding_data_hash(const struct mac_binding_data *mb_data)
>   {
> -    uint32_t hash = 0;
> +    uint32_t hash = mb_data->sbrec
> +                    ? uuid_hash(&mb_data->sbrec->header_.uuid)
> +                    : 0;
>   
>       hash = hash_add(hash, mb_data->port_key);
>       hash = hash_add(hash, mb_data->dp_key);
> @@ -621,9 +657,10 @@ static inline bool
>   mac_binding_data_equals(const struct mac_binding_data *a,
>                           const struct mac_binding_data *b)
>   {
> -    return a->port_key == b->port_key &&
> +    return a->sbrec == b->sbrec &&
> +           a->port_key == b->port_key &&
>              a->dp_key == b->dp_key &&
> -            ipv6_addr_equals(&a->ip, &b->ip);
> +           ipv6_addr_equals(&a->ip, &b->ip);
>   }
>   
>   static uint32_t
> diff --git a/controller/mac-cache.h b/controller/mac-cache.h
> index b94e7a1cf2..df84ef072c 100644
> --- a/controller/mac-cache.h
> +++ b/controller/mac-cache.h
> @@ -52,6 +52,7 @@ struct mac_cache_threshold {
>   
>   struct mac_binding_data {
>       /* Keys. */
> +    const struct sbrec_mac_binding * sbrec; /* Only the UUID is used as key.*/
>       uint32_t port_key;
>       uint32_t dp_key;
>       struct in6_addr ip;
> @@ -63,8 +64,6 @@ struct mac_binding {
>       struct hmap_node hmap_node;
>       /* Common data to identify MAC binding. */
>       struct mac_binding_data data;
> -    /* Reference to the SB MAC binding record (Might be NULL). */
> -    const struct sbrec_mac_binding *sbrec_mb;
>       /* User specified timestamp (in ms) */
>       long long timestamp;
>   };
> @@ -128,20 +127,37 @@ void mac_cache_thresholds_sync(struct mac_cache_data *data,
>   void mac_cache_thresholds_clear(struct mac_cache_data *data);
>   
>   /* MAC binding. */
> -struct mac_binding *mac_binding_add(struct hmap *map,
> -                                    struct mac_binding_data mb_data,
> -                                    long long timestamp);
> +
> +static inline void
> +mac_binding_data_init(struct mac_binding_data *data,
> +                      uint32_t dp_key, uint32_t port_key,
> +                      struct in6_addr ip, struct eth_addr mac)
> +{
> +    *data = (struct mac_binding_data) {
> +        .sbrec = NULL,
> +        .dp_key = (dp_key),
> +        .port_key = (port_key),
> +        .ip = (ip),
> +        .mac = (mac),
> +    };
> +}
> +
> +void mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
> +                     long long timestamp);
>   
>   void mac_binding_remove(struct hmap *map, struct mac_binding *mb);
>   
>   struct mac_binding *mac_binding_find(const struct hmap *map,
>                                        const struct mac_binding_data *mb_data);
>   
> +bool mac_binding_data_parse(struct mac_binding_data *data,
> +                            uint32_t dp_key, uint32_t port_key,
> +                            const char *ip_str, const char *mac_str);
>   bool mac_binding_data_from_sbrec(struct mac_binding_data *data,
> -                                 const struct sbrec_mac_binding *mb,
> -                                 struct ovsdb_idl_index *sbrec_pb_by_name);
> +                                 const struct sbrec_mac_binding *mb);
>   
>   void mac_bindings_clear(struct hmap *map);
> +void mac_bindings_to_string(const struct hmap *map, struct ds *out_data);
>   
>   bool sb_mac_binding_updated(const struct sbrec_mac_binding *mb);
>   
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index e87274121c..ab95938502 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -101,6 +101,7 @@ static unixctl_cb_func debug_status_execution;
>   static unixctl_cb_func debug_dump_local_bindings;
>   static unixctl_cb_func debug_dump_related_lports;
>   static unixctl_cb_func debug_dump_local_template_vars;
> +static unixctl_cb_func debug_dump_local_mac_bindings;
>   static unixctl_cb_func debug_dump_lflow_conj_ids;
>   static unixctl_cb_func lflow_cache_flush_cmd;
>   static unixctl_cb_func lflow_cache_show_stats_cmd;
> @@ -2932,26 +2933,22 @@ en_lb_data_cleanup(void *data)
>   
>   static void
>   mac_binding_add_sb(struct mac_cache_data *data,
> -                   const struct sbrec_mac_binding *smb,
> -                   struct ovsdb_idl_index *sbrec_pb_by_name)
> +                   const struct sbrec_mac_binding *smb)
>   {
>       struct mac_binding_data mb_data;
> -    if (!mac_binding_data_from_sbrec(&mb_data, smb, sbrec_pb_by_name)) {
> +    if (!mac_binding_data_from_sbrec(&mb_data, smb)) {
>           return;
>       }
>   
> -    struct mac_binding *mb = mac_binding_add(&data->mac_bindings, mb_data, 0);
> -
> -    mb->sbrec_mb = smb;
> +    mac_binding_add(&data->mac_bindings, mb_data, 0);
>   }
>   
>   static void
>   mac_binding_remove_sb(struct mac_cache_data *data,
> -                      const struct sbrec_mac_binding *smb,
> -                      struct ovsdb_idl_index *sbrec_pb_by_name)
> +                      const struct sbrec_mac_binding *smb)
>   {
>       struct mac_binding_data mb_data;
> -    if (!mac_binding_data_from_sbrec(&mb_data, smb, sbrec_pb_by_name)) {
> +    if (!mac_binding_data_from_sbrec(&mb_data, smb)) {
>           return;
>       }
>   
> @@ -2995,8 +2992,7 @@ fdb_remove_sb(struct mac_cache_data *data, const struct sbrec_fdb *sfdb)
>   static void
>   mac_cache_mb_handle_for_datapath(struct mac_cache_data *data,
>                                    const struct sbrec_datapath_binding *dp,
> -                                 struct ovsdb_idl_index *sbrec_mb_by_dp,
> -                                 struct ovsdb_idl_index *sbrec_pb_by_name)
> +                                 struct ovsdb_idl_index *sbrec_mb_by_dp)
>   {
>       bool has_threshold = mac_cache_threshold_find(data, dp->tunnel_key);
>   
> @@ -3007,9 +3003,9 @@ mac_cache_mb_handle_for_datapath(struct mac_cache_data *data,
>       const struct sbrec_mac_binding *mb;
>       SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row, sbrec_mb_by_dp) {
>           if (has_threshold) {
> -            mac_binding_add_sb(data, mb, sbrec_pb_by_name);
> +            mac_binding_add_sb(data, mb);
>           } else {
> -            mac_binding_remove_sb(data, mb, sbrec_pb_by_name);
> +            mac_binding_remove_sb(data, mb);
>           }
>       }
>   
> @@ -3064,10 +3060,6 @@ en_mac_cache_run(struct engine_node *node, void *data)
>               engine_ovsdb_node_get_index(
>                       engine_get_input("SB_mac_binding", node),
>                       "datapath");
> -    struct ovsdb_idl_index *sbrec_pb_by_name =
> -            engine_ovsdb_node_get_index(
> -                    engine_get_input("SB_port_binding", node),
> -                    "name");
>       struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>               engine_ovsdb_node_get_index(
>                       engine_get_input("SB_fdb", node),
> @@ -3086,7 +3078,7 @@ en_mac_cache_run(struct engine_node *node, void *data)
>   
>           mac_cache_threshold_add(cache_data, sbrec_dp);
>           mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
> -                                         sbrec_mb_by_dp, sbrec_pb_by_name);
> +                                         sbrec_mb_by_dp);
>           mac_cache_fdb_handle_for_datapath(cache_data, sbrec_dp,
>                                             sbrec_fdb_by_dp_key);
>       }
> @@ -3102,11 +3094,6 @@ mac_cache_sb_mac_binding_handler(struct engine_node *node, void *data)
>               engine_get_input_data("runtime_data", node);
>       const struct sbrec_mac_binding_table *mb_table =
>               EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
> -    struct ovsdb_idl_index *sbrec_pb_by_name =
> -            engine_ovsdb_node_get_index(
> -                    engine_get_input("SB_port_binding", node),
> -                    "name");
> -
>       size_t previous_size = hmap_count(&cache_data->mac_bindings);
>   
>       const struct sbrec_mac_binding *sbrec_mb;
> @@ -3116,8 +3103,7 @@ mac_cache_sb_mac_binding_handler(struct engine_node *node, void *data)
>           }
>   
>           if (!sbrec_mac_binding_is_new(sbrec_mb)) {
> -            mac_binding_remove_sb(cache_data, sbrec_mb,
> -                                  sbrec_pb_by_name);
> +            mac_binding_remove_sb(cache_data, sbrec_mb);
>           }
>   
>           if (sbrec_mac_binding_is_deleted(sbrec_mb) ||
> @@ -3128,7 +3114,7 @@ mac_cache_sb_mac_binding_handler(struct engine_node *node, void *data)
>   
>           if (mac_cache_threshold_find(cache_data,
>                                        sbrec_mb->datapath->tunnel_key)) {
> -            mac_binding_add_sb(cache_data, sbrec_mb, sbrec_pb_by_name);
> +            mac_binding_add_sb(cache_data, sbrec_mb);
>           }
>       }
>   
> @@ -3189,10 +3175,6 @@ mac_cache_runtime_data_handler(struct engine_node *node, void *data OVS_UNUSED)
>               engine_ovsdb_node_get_index(
>                       engine_get_input("SB_mac_binding", node),
>                       "datapath");
> -    struct ovsdb_idl_index *sbrec_pb_by_name =
> -            engine_ovsdb_node_get_index(
> -                    engine_get_input("SB_port_binding", node),
> -                    "name");
>       struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>               engine_ovsdb_node_get_index(
>                       engine_get_input("SB_fdb", node),
> @@ -3217,7 +3199,7 @@ mac_cache_runtime_data_handler(struct engine_node *node, void *data OVS_UNUSED)
>   
>       HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
>           mac_cache_mb_handle_for_datapath(cache_data, tdp->dp,
> -                                         sbrec_mb_by_dp, sbrec_pb_by_name);
> +                                         sbrec_mb_by_dp);
>   
>           mac_cache_fdb_handle_for_datapath(cache_data, tdp->dp,
>                                             sbrec_fdb_by_dp_key);
> @@ -3243,10 +3225,6 @@ mac_cache_sb_datapath_binding_handler(struct engine_node *node, void *data)
>               engine_ovsdb_node_get_index(
>                       engine_get_input("SB_mac_binding", node),
>                       "datapath");
> -    struct ovsdb_idl_index *sbrec_pb_by_name =
> -            engine_ovsdb_node_get_index(
> -                    engine_get_input("SB_port_binding", node),
> -                    "name");
>       struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>               engine_ovsdb_node_get_index(
>                       engine_get_input("SB_fdb", node),
> @@ -3274,7 +3252,7 @@ mac_cache_sb_datapath_binding_handler(struct engine_node *node, void *data)
>   
>       SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_dp, dp_table) {
>           mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
> -                                         sbrec_mb_by_dp, sbrec_pb_by_name);
> +                                         sbrec_mb_by_dp);
>   
>           mac_cache_fdb_handle_for_datapath(cache_data, sbrec_dp,
>                                             sbrec_fdb_by_dp_key);
> @@ -5399,6 +5377,10 @@ main(int argc, char *argv[])
>                                debug_dump_local_template_vars,
>                                &template_vars_data->local_templates);
>   
> +    unixctl_command_register("debug/dump-mac-bindings", "", 0, 0,
> +                             debug_dump_local_mac_bindings,
> +                             &mac_cache_data->mac_bindings);
> +
>       unixctl_command_register("debug/ignore-startup-delay", "", 0, 0,
>                                debug_ignore_startup_delay, NULL);
>   
> @@ -6356,6 +6338,19 @@ debug_dump_local_template_vars(struct unixctl_conn *conn, int argc OVS_UNUSED,
>       ds_destroy(&tv_str);
>   }
>   
> +static void
> +debug_dump_local_mac_bindings(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +                               const char *argv[] OVS_UNUSED,
> +                               void *mac_bindings)
> +{
> +    struct ds mb_str = DS_EMPTY_INITIALIZER;
> +
> +    ds_put_cstr(&mb_str, "Local MAC bindings:\n");
> +    mac_bindings_to_string(mac_bindings, &mb_str);
> +    unixctl_command_reply(conn, ds_cstr(&mb_str));
> +    ds_destroy(&mb_str);
> +}
> +
>   static void
>   debug_ignore_startup_delay(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                              const char *argv[] OVS_UNUSED, void *arg OVS_UNUSED)
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index dfb3130aa2..ca880a1da6 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1539,19 +1539,20 @@ pinctrl_handle_buffered_packets(const struct ofputil_packet_in *pin,
>   OVS_REQUIRES(pinctrl_mutex)
>   {
>       const struct match *md = &pin->flow_metadata;
> -    struct mac_binding_data mb_data = (struct mac_binding_data) {
> -            .dp_key =  ntohll(md->flow.metadata),
> -            .port_key =  md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0],
> -            .mac = eth_addr_zero,
> -    };
> +    struct mac_binding_data mb_data;
> +    struct in6_addr ip;
>   
>       if (is_arp) {
> -        mb_data.ip = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
> +        ip = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
>       } else {
>           ovs_be128 ip6 = hton128(flow_get_xxreg(&md->flow, 0));
> -        memcpy(&mb_data.ip, &ip6, sizeof mb_data.ip);
> +        memcpy(&ip, &ip6, sizeof ip);
>       }
>   
> +    mac_binding_data_init(&mb_data, ntohll(md->flow.metadata),
> +                          md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0],
> +                          ip, eth_addr_zero);
> +
>       struct buffered_packets *bp = buffered_packets_add(&buffered_packets_ctx,
>                                                          mb_data);
>       if (!bp) {
> @@ -4959,9 +4960,14 @@ run_buffered_binding(const struct sbrec_mac_binding_table *mac_binding_table,
>           const struct sbrec_port_binding *pb = lport_lookup_by_name(
>               sbrec_port_binding_by_name, smb->logical_port);
>   
> +        if (!pb || !pb->datapath) {
> +            continue;
> +        }
> +
>           struct mac_binding_data mb_data;
> -        if (!mac_binding_data_from_sbrec(&mb_data, smb,
> -                                         sbrec_port_binding_by_name)) {
> +
> +        if (!mac_binding_data_parse(&mb_data, smb->datapath->tunnel_key,
> +                                    pb->tunnel_key, smb->ip, smb->mac)) {
>               continue;
>           }
>   
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 10cd7a79b9..44d58ca160 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -35168,6 +35168,116 @@ OVN_CLEANUP([hv1], [hv2])
>   AT_CLEANUP
>   ])
>   
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([MAC binding aging - port deletion])
> +AT_SKIP_IF([test $HAVE_SCAPY = no])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +check ovn-nbctl                                                     \
> +    -- ls-add ls1                                                   \
> +    -- ls-add ls2                                                   \
> +    -- lr-add lr                                                    \
> +    -- set logical_router lr options:mac_binding_age_threshold=3600 \
> +    -- lrp-add lr lr-ls1 00:00:00:00:10:00 192.168.10.1/24          \
> +    -- lrp-add lr lr-ls2 00:00:00:00:20:00 192.168.20.1/24          \
> +    -- lsp-add ls1 ls1-lr                                           \
> +    -- lsp-set-type ls1-lr router                                   \
> +    -- lsp-set-addresses ls1-lr router                              \
> +    -- lsp-set-options ls1-lr router-port=lr-ls1                    \
> +    -- lsp-add ls1 vif1                                             \
> +    -- lsp-set-addresses vif1 "00:00:00:00:10:10 192.168.10.10"     \
> +    -- lsp-add ls2 ls2-lr                                           \
> +    -- lsp-set-type ls2-lr router                                   \
> +    -- lsp-set-addresses ls2-lr router                              \
> +    -- lsp-set-options ls2-lr router-port=lr-ls2                    \
> +    -- lsp-add ls2 vif2                                             \
> +    -- lsp-set-addresses vif2 "00:00:00:00:20:10 192.168.20.10"
> +
> +check ovs-vsctl                                      \
> +    -- add-port br-int vif1                          \
> +    -- set interface vif1 external-ids:iface-id=vif1 \
> +    -- add-port br-int vif2                          \
> +    -- set interface vif2 external-ids:iface-id=vif2
> +
> +OVN_POPULATE_ARP
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +dnl Wait for pinctrl thread to be connected.
> +OVS_WAIT_UNTIL([grep pinctrl hv1/ovn-controller.log | grep -q connected])
> +
> +send_garp() {
> +    hv=$1
> +    dev=$2
> +    mac=$3
> +    ip=$4
> +
> +    packet=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${mac}')/ \
> +                      ARP(op=2, hwsrc='${mac}', hwdst='${mac}',     \
> +                      psrc='${ip}', pdst='${ip}')")
> +    as $hv ovs-appctl netdev-dummy/receive $dev $packet
> +}
> +
> +AS_BOX([Remove LRP with learnt MAC_Binding])
> +dnl Populate MAC Binding entry.
> +send_garp hv1 vif1 00:00:00:00:10:10 192.168.10.10
> +wait_row_count mac_binding 1 ip="192.168.10.10" logical_port="lr-ls1"
> +
> +dnl Check local mac binding cache.
> +lr_key=$(fetch_column datapath_binding tunnel_key external_ids:name=lr)
> +mb_uuid=$(fetch_column mac_binding _uuid logical_port=lr-ls1)
> +OVS_WAIT_FOR_OUTPUT_UNQUOTED([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings], [0], [dnl
> +Local MAC bindings:
> +SB UUID: $mb_uuid, datapath-key: $lr_key, port-key: 0, ip: 192.168.10.10, mac: 00:00:00:00:10:10
> +])
> +
> +dnl Remove router port.  This deletes the mac binding in the same transaction.
> +check ovn-nbctl --wait=hv lrp-del lr-ls1
> +
> +dnl Check local mac binding cache - it should be empty now.
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings], [0], [dnl
> +Local MAC bindings:
> +])
> +
> +AS_BOX([Change LRP tunnel-key with learnt MAC_Binding and remove LRP])
> +dnl Re-add port.
> +check ovn-nbctl \
> +    -- set logical_router_port lr-ls2 options:requested-tnl-key=42
> +check ovn-nbctl --wait=hv sync
> +check_column 42 Port_Binding tunnel_key logical_port=lr-ls2
> +
> +dnl Populate MAC Binding entry.
> +send_garp hv1 vif2 00:00:00:00:20:10 192.168.20.10
> +wait_row_count mac_binding 1 ip="192.168.20.10" logical_port="lr-ls2"
> +
> +dnl Check local mac binding cache.
> +mb_uuid=$(fetch_column mac_binding _uuid logical_port=lr-ls2)
> +OVS_WAIT_FOR_OUTPUT_UNQUOTED([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings], [0], [dnl
> +Local MAC bindings:
> +SB UUID: $mb_uuid, datapath-key: $lr_key, port-key: 0, ip: 192.168.20.10, mac: 00:00:00:00:20:10
> +])
> +
> +dnl Change LRP tunnel key.
> +check ovn-nbctl --wait=hv set logical_router_port lr-ls2 options:requested-tnl-key=43
> +check_column 43 Port_Binding tunnel_key logical_port=lr-ls2
> +
> +dnl Remove router port.  This deletes the mac binding in the same transaction.
> +check ovn-nbctl --wait=hv lrp-del lr-ls2
> +dnl Check local mac binding cache - it should be empty now.
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings], [0], [dnl
> +Local MAC bindings:
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> +
>   OVN_FOR_EACH_NORTHD([
>   AT_SETUP([router port type update and then remove])
>   ovn_start

On Fri, Nov 1, 2024 at 2:29 PM Dumitru Ceara <dceara@redhat.com> wrote:

> SB.MAC_Binding doesn't store a strong reference to SB.Port_Binding.
> Instead it stores the port binding 'logical_port' name.  As the
> mac-binding cache in ovn-controller stored entries indexed by
> [datapath-key, port-key, ip] it meant that when processing
> SB.MAC_Binding changes (including deletion) it had to first lookup the
> corresponding SB.Port_Binding.
>
> If the logical port associated with the MAC binding is deleted in the
> same transaction then looking it up in the IDL index fails (deleted IDL
> records are not indexed).  This leads to stale entries staying in the
> mac cache and in consequence to potential use after free (of the SB
> MAC_Binding record) or to crashes if the statctrl module tries to update
> timestamps of MAC_Bindings that happened to have been removed in the
> current iteration (but still linger in the cache).
>
> In practice we don't need the port-key we can simply use the port
> binding UUID as unique index.  However, the same struct mac_binding type
> is used by pinctrl to temporarily efficiently store buffered packets for
> yet to be resolved MAC bindings.  In those cases there's no SB
> MAC_Binding record yet so the logical ingress port tunnel_key is used as
> index.
>
> The two ways of using the MAC binding maps are orthogonal, that is, we
> will never use a single map to store both types of bindings.
>
> In order to avoid issues we now include the SB MAC_Binding UUID into the
> key of the struct mac_binding_data.
>
> Reported-at: https://issues.redhat.com/browse/FDP-906
> Fixes: b57f60a617b4 ("controller: Add MAC cache I-P node")
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
>

Hi Dumitru,

thank you for the fix. There is just one small typo down below.
We should probably think about a way of decoupling pinctrl and
ovn-controller mac-cache in the future as it unfortunately doesn't
work well together.

 controller/mac-cache.c      |  77 ++++++++++++++++++-------
>  controller/mac-cache.h      |  30 +++++++---
>  controller/ovn-controller.c |  67 ++++++++++------------
>  controller/pinctrl.c        |  24 +++++---
>  tests/ovn.at                | 110 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 236 insertions(+), 72 deletions(-)
>
> diff --git a/controller/mac-cache.c b/controller/mac-cache.c
> index de45d7a6a5..d6ba9e20ee 100644
> --- a/controller/mac-cache.c
> +++ b/controller/mac-cache.c
> @@ -142,21 +142,18 @@ mac_cache_thresholds_clear(struct mac_cache_data
> *data)
>  }
>
>  /* MAC binding. */
> -struct mac_binding *
> +void
>  mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
>                  long long timestamp) {
>
>      struct mac_binding *mb = mac_binding_find(map, &mb_data);
>      if (!mb) {
>          mb = xmalloc(sizeof *mb);
> -        mb->sbrec_mb = NULL;
>          hmap_insert(map, &mb->hmap_node, mac_binding_data_hash(&mb_data));
>      }
>
>      mb->data = mb_data;
>      mb->timestamp = timestamp;
> -
> -    return mb;
>  }
>
>  void
> @@ -181,24 +178,37 @@ mac_binding_find(const struct hmap *map,
>  }
>
>  bool
> -mac_binding_data_from_sbrec(struct mac_binding_data *data,
> -                            const struct sbrec_mac_binding *mb,
> -                            struct ovsdb_idl_index *sbrec_pb_by_name)
> +mac_binding_data_parse(struct mac_binding_data *data,
> +                       uint32_t dp_key, uint32_t port_key,
> +                       const char *ip_str, const char *mac_str)
>  {
> -    const struct sbrec_port_binding *pb =
> -            lport_lookup_by_name(sbrec_pb_by_name, mb->logical_port);
> +    struct eth_addr mac;
> +    struct in6_addr ip;
>
> -    if (!pb || !pb->datapath || !ip46_parse(mb->ip, &data->ip) ||
> -        !eth_addr_from_string(mb->mac, &data->mac)) {
> +    if (!ip46_parse(ip_str, &ip) || !eth_addr_from_string(mac_str, &mac))
> {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> -        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s, "
> -                     "logical_port=%s", mb->ip, mb->mac,
> mb->logical_port);
> +        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s",
> +                     ip_str, mac_str);
>          return false;
>      }
>
> -    data->dp_key = mb->datapath->tunnel_key;
> -    data->port_key = pb->tunnel_key;
> +    mac_binding_data_init(data, dp_key, port_key, ip, mac);
> +    return true;
> +}
> +
> +bool
> +mac_binding_data_from_sbrec(struct mac_binding_data *data,
> +                            const struct sbrec_mac_binding *mb)
> +{
> +    /* This explicitly sets the port_key to 0 as port_binding tunnel_keys
> +     * can change.  Instead use add the SB.MAC_Binding UUID as key; this
> +     * makes the mac_binding_data key unique. */
> +    if (!mac_binding_data_parse(data, mb->datapath->tunnel_key, 0,
> +                                mb->ip, mb->mac)) {
> +        return false;
> +    }
>
> +    data->sbrec = mb;
>      return true;
>  }
>
> @@ -211,6 +221,30 @@ mac_bindings_clear(struct hmap *map)
>      }
>  }
>
> +void
> +mac_bindings_to_string(const struct hmap *map, struct ds *out_data)
> +{
> +    struct mac_binding *mb;
> +    HMAP_FOR_EACH (mb, hmap_node, map) {
> +        char ip[INET6_ADDRSTRLEN];
> +
> +        if (!ipv6_string_mapped(ip, &mb->data.ip)) {
> +            continue;
> +        }
> +        if (mb->data.sbrec) {
> +            ds_put_format(out_data, "SB UUID: " UUID_FMT ", ",
> +                          UUID_ARGS(&mb->data.sbrec->header_.uuid));
> +        } else {
> +            ds_put_cstr(out_data, "SB UUID: <none>, ");
> +        }
> +        ds_put_format(out_data, "datapath-key: %"PRIu32", "
> +                                "port-key: %"PRIu32", "
> +                                "ip: %s, mac: " ETH_ADDR_FMT "\n",
> +                      mb->data.dp_key, mb->data.port_key,
> +                      ip, ETH_ADDR_ARGS(mb->data.mac));
> +    }
> +}
> +
>  bool
>  sb_mac_binding_updated(const struct sbrec_mac_binding *mb)
>  {
> @@ -382,9 +416,9 @@ mac_binding_stats_run(struct ovs_list *stats_list,
> uint64_t *req_delay,
>           * used on this chassis. Also make sure that we don't update the
>           * timestamp more than once during the dump period. */
>          if (stats->idle_age_ms < threshold->value &&
> -                (timewall_now - mb->sbrec_mb->timestamp) >=
> +                (timewall_now - mb->data.sbrec->timestamp) >=
>              threshold->dump_period) {
> -            sbrec_mac_binding_set_timestamp(mb->sbrec_mb, timewall_now);
> +            sbrec_mac_binding_set_timestamp(mb->data.sbrec, timewall_now);
>          }
>
>          free(stats);
> @@ -608,7 +642,9 @@ mac_cache_stats_destroy(struct ovs_list *stats_list)
>  static uint32_t
>  mac_binding_data_hash(const struct mac_binding_data *mb_data)
>  {
> -    uint32_t hash = 0;
> +    uint32_t hash = mb_data->sbrec
> +                    ? uuid_hash(&mb_data->sbrec->header_.uuid)
> +                    : 0;
>
>      hash = hash_add(hash, mb_data->port_key);
>      hash = hash_add(hash, mb_data->dp_key);
> @@ -621,9 +657,10 @@ static inline bool
>  mac_binding_data_equals(const struct mac_binding_data *a,
>                          const struct mac_binding_data *b)
>  {
> -    return a->port_key == b->port_key &&
> +    return a->sbrec == b->sbrec &&
> +           a->port_key == b->port_key &&
>             a->dp_key == b->dp_key &&
> -            ipv6_addr_equals(&a->ip, &b->ip);
> +           ipv6_addr_equals(&a->ip, &b->ip);
>  }
>
>  static uint32_t
> diff --git a/controller/mac-cache.h b/controller/mac-cache.h
> index b94e7a1cf2..df84ef072c 100644
> --- a/controller/mac-cache.h
> +++ b/controller/mac-cache.h
> @@ -52,6 +52,7 @@ struct mac_cache_threshold {
>
>  struct mac_binding_data {
>      /* Keys. */
> +    const struct sbrec_mac_binding * sbrec; /* Only the UUID is used as
> key.*/
>

nit: Space after *.


>      uint32_t port_key;
>      uint32_t dp_key;
>      struct in6_addr ip;
> @@ -63,8 +64,6 @@ struct mac_binding {
>      struct hmap_node hmap_node;
>      /* Common data to identify MAC binding. */
>      struct mac_binding_data data;
> -    /* Reference to the SB MAC binding record (Might be NULL). */
> -    const struct sbrec_mac_binding *sbrec_mb;
>      /* User specified timestamp (in ms) */
>      long long timestamp;
>  };
> @@ -128,20 +127,37 @@ void mac_cache_thresholds_sync(struct mac_cache_data
> *data,
>  void mac_cache_thresholds_clear(struct mac_cache_data *data);
>
>  /* MAC binding. */
> -struct mac_binding *mac_binding_add(struct hmap *map,
> -                                    struct mac_binding_data mb_data,
> -                                    long long timestamp);
> +
> +static inline void
> +mac_binding_data_init(struct mac_binding_data *data,
> +                      uint32_t dp_key, uint32_t port_key,
> +                      struct in6_addr ip, struct eth_addr mac)
> +{
> +    *data = (struct mac_binding_data) {
> +        .sbrec = NULL,
> +        .dp_key = (dp_key),
> +        .port_key = (port_key),
> +        .ip = (ip),
> +        .mac = (mac),
> +    };
> +}
> +
> +void mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
> +                     long long timestamp);
>
>  void mac_binding_remove(struct hmap *map, struct mac_binding *mb);
>
>  struct mac_binding *mac_binding_find(const struct hmap *map,
>                                       const struct mac_binding_data
> *mb_data);
>
> +bool mac_binding_data_parse(struct mac_binding_data *data,
> +                            uint32_t dp_key, uint32_t port_key,
> +                            const char *ip_str, const char *mac_str);
>  bool mac_binding_data_from_sbrec(struct mac_binding_data *data,
> -                                 const struct sbrec_mac_binding *mb,
> -                                 struct ovsdb_idl_index
> *sbrec_pb_by_name);
> +                                 const struct sbrec_mac_binding *mb);
>
>  void mac_bindings_clear(struct hmap *map);
> +void mac_bindings_to_string(const struct hmap *map, struct ds *out_data);
>
>  bool sb_mac_binding_updated(const struct sbrec_mac_binding *mb);
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index e87274121c..ab95938502 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -101,6 +101,7 @@ static unixctl_cb_func debug_status_execution;
>  static unixctl_cb_func debug_dump_local_bindings;
>  static unixctl_cb_func debug_dump_related_lports;
>  static unixctl_cb_func debug_dump_local_template_vars;
> +static unixctl_cb_func debug_dump_local_mac_bindings;
>  static unixctl_cb_func debug_dump_lflow_conj_ids;
>  static unixctl_cb_func lflow_cache_flush_cmd;
>  static unixctl_cb_func lflow_cache_show_stats_cmd;
> @@ -2932,26 +2933,22 @@ en_lb_data_cleanup(void *data)
>
>  static void
>  mac_binding_add_sb(struct mac_cache_data *data,
> -                   const struct sbrec_mac_binding *smb,
> -                   struct ovsdb_idl_index *sbrec_pb_by_name)
> +                   const struct sbrec_mac_binding *smb)
>  {
>      struct mac_binding_data mb_data;
> -    if (!mac_binding_data_from_sbrec(&mb_data, smb, sbrec_pb_by_name)) {
> +    if (!mac_binding_data_from_sbrec(&mb_data, smb)) {
>          return;
>      }
>
> -    struct mac_binding *mb = mac_binding_add(&data->mac_bindings,
> mb_data, 0);
> -
> -    mb->sbrec_mb = smb;
> +    mac_binding_add(&data->mac_bindings, mb_data, 0);
>  }
>
>  static void
>  mac_binding_remove_sb(struct mac_cache_data *data,
> -                      const struct sbrec_mac_binding *smb,
> -                      struct ovsdb_idl_index *sbrec_pb_by_name)
> +                      const struct sbrec_mac_binding *smb)
>  {
>      struct mac_binding_data mb_data;
> -    if (!mac_binding_data_from_sbrec(&mb_data, smb, sbrec_pb_by_name)) {
> +    if (!mac_binding_data_from_sbrec(&mb_data, smb)) {
>          return;
>      }
>
> @@ -2995,8 +2992,7 @@ fdb_remove_sb(struct mac_cache_data *data, const
> struct sbrec_fdb *sfdb)
>  static void
>  mac_cache_mb_handle_for_datapath(struct mac_cache_data *data,
>                                   const struct sbrec_datapath_binding *dp,
> -                                 struct ovsdb_idl_index *sbrec_mb_by_dp,
> -                                 struct ovsdb_idl_index *sbrec_pb_by_name)
> +                                 struct ovsdb_idl_index *sbrec_mb_by_dp)
>  {
>      bool has_threshold = mac_cache_threshold_find(data, dp->tunnel_key);
>
> @@ -3007,9 +3003,9 @@ mac_cache_mb_handle_for_datapath(struct
> mac_cache_data *data,
>      const struct sbrec_mac_binding *mb;
>      SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row, sbrec_mb_by_dp) {
>          if (has_threshold) {
> -            mac_binding_add_sb(data, mb, sbrec_pb_by_name);
> +            mac_binding_add_sb(data, mb);
>          } else {
> -            mac_binding_remove_sb(data, mb, sbrec_pb_by_name);
> +            mac_binding_remove_sb(data, mb);
>          }
>      }
>
> @@ -3064,10 +3060,6 @@ en_mac_cache_run(struct engine_node *node, void
> *data)
>              engine_ovsdb_node_get_index(
>                      engine_get_input("SB_mac_binding", node),
>                      "datapath");
> -    struct ovsdb_idl_index *sbrec_pb_by_name =
> -            engine_ovsdb_node_get_index(
> -                    engine_get_input("SB_port_binding", node),
> -                    "name");
>      struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>              engine_ovsdb_node_get_index(
>                      engine_get_input("SB_fdb", node),
> @@ -3086,7 +3078,7 @@ en_mac_cache_run(struct engine_node *node, void
> *data)
>
>          mac_cache_threshold_add(cache_data, sbrec_dp);
>          mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
> -                                         sbrec_mb_by_dp,
> sbrec_pb_by_name);
> +                                         sbrec_mb_by_dp);
>          mac_cache_fdb_handle_for_datapath(cache_data, sbrec_dp,
>                                            sbrec_fdb_by_dp_key);
>      }
> @@ -3102,11 +3094,6 @@ mac_cache_sb_mac_binding_handler(struct engine_node
> *node, void *data)
>              engine_get_input_data("runtime_data", node);
>      const struct sbrec_mac_binding_table *mb_table =
>              EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
> -    struct ovsdb_idl_index *sbrec_pb_by_name =
> -            engine_ovsdb_node_get_index(
> -                    engine_get_input("SB_port_binding", node),
> -                    "name");
> -
>      size_t previous_size = hmap_count(&cache_data->mac_bindings);
>
>      const struct sbrec_mac_binding *sbrec_mb;
> @@ -3116,8 +3103,7 @@ mac_cache_sb_mac_binding_handler(struct engine_node
> *node, void *data)
>          }
>
>          if (!sbrec_mac_binding_is_new(sbrec_mb)) {
> -            mac_binding_remove_sb(cache_data, sbrec_mb,
> -                                  sbrec_pb_by_name);
> +            mac_binding_remove_sb(cache_data, sbrec_mb);
>          }
>
>          if (sbrec_mac_binding_is_deleted(sbrec_mb) ||
> @@ -3128,7 +3114,7 @@ mac_cache_sb_mac_binding_handler(struct engine_node
> *node, void *data)
>
>          if (mac_cache_threshold_find(cache_data,
>                                       sbrec_mb->datapath->tunnel_key)) {
> -            mac_binding_add_sb(cache_data, sbrec_mb, sbrec_pb_by_name);
> +            mac_binding_add_sb(cache_data, sbrec_mb);
>          }
>      }
>
> @@ -3189,10 +3175,6 @@ mac_cache_runtime_data_handler(struct engine_node
> *node, void *data OVS_UNUSED)
>              engine_ovsdb_node_get_index(
>                      engine_get_input("SB_mac_binding", node),
>                      "datapath");
> -    struct ovsdb_idl_index *sbrec_pb_by_name =
> -            engine_ovsdb_node_get_index(
> -                    engine_get_input("SB_port_binding", node),
> -                    "name");
>      struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>              engine_ovsdb_node_get_index(
>                      engine_get_input("SB_fdb", node),
> @@ -3217,7 +3199,7 @@ mac_cache_runtime_data_handler(struct engine_node
> *node, void *data OVS_UNUSED)
>
>      HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
>          mac_cache_mb_handle_for_datapath(cache_data, tdp->dp,
> -                                         sbrec_mb_by_dp,
> sbrec_pb_by_name);
> +                                         sbrec_mb_by_dp);
>
>          mac_cache_fdb_handle_for_datapath(cache_data, tdp->dp,
>                                            sbrec_fdb_by_dp_key);
> @@ -3243,10 +3225,6 @@ mac_cache_sb_datapath_binding_handler(struct
> engine_node *node, void *data)
>              engine_ovsdb_node_get_index(
>                      engine_get_input("SB_mac_binding", node),
>                      "datapath");
> -    struct ovsdb_idl_index *sbrec_pb_by_name =
> -            engine_ovsdb_node_get_index(
> -                    engine_get_input("SB_port_binding", node),
> -                    "name");
>      struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>              engine_ovsdb_node_get_index(
>                      engine_get_input("SB_fdb", node),
> @@ -3274,7 +3252,7 @@ mac_cache_sb_datapath_binding_handler(struct
> engine_node *node, void *data)
>
>      SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_dp, dp_table) {
>          mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
> -                                         sbrec_mb_by_dp,
> sbrec_pb_by_name);
> +                                         sbrec_mb_by_dp);
>
>          mac_cache_fdb_handle_for_datapath(cache_data, sbrec_dp,
>                                            sbrec_fdb_by_dp_key);
> @@ -5399,6 +5377,10 @@ main(int argc, char *argv[])
>                               debug_dump_local_template_vars,
>                               &template_vars_data->local_templates);
>
> +    unixctl_command_register("debug/dump-mac-bindings", "", 0, 0,
> +                             debug_dump_local_mac_bindings,
> +                             &mac_cache_data->mac_bindings);
> +
>      unixctl_command_register("debug/ignore-startup-delay", "", 0, 0,
>                               debug_ignore_startup_delay, NULL);
>
> @@ -6356,6 +6338,19 @@ debug_dump_local_template_vars(struct unixctl_conn
> *conn, int argc OVS_UNUSED,
>      ds_destroy(&tv_str);
>  }
>
> +static void
> +debug_dump_local_mac_bindings(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> +                               const char *argv[] OVS_UNUSED,
> +                               void *mac_bindings)
> +{
> +    struct ds mb_str = DS_EMPTY_INITIALIZER;
> +
> +    ds_put_cstr(&mb_str, "Local MAC bindings:\n");
> +    mac_bindings_to_string(mac_bindings, &mb_str);
> +    unixctl_command_reply(conn, ds_cstr(&mb_str));
> +    ds_destroy(&mb_str);
> +}
> +
>  static void
>  debug_ignore_startup_delay(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                             const char *argv[] OVS_UNUSED, void *arg
> OVS_UNUSED)
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index dfb3130aa2..ca880a1da6 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1539,19 +1539,20 @@ pinctrl_handle_buffered_packets(const struct
> ofputil_packet_in *pin,
>  OVS_REQUIRES(pinctrl_mutex)
>  {
>      const struct match *md = &pin->flow_metadata;
> -    struct mac_binding_data mb_data = (struct mac_binding_data) {
> -            .dp_key =  ntohll(md->flow.metadata),
> -            .port_key =  md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0],
> -            .mac = eth_addr_zero,
> -    };
> +    struct mac_binding_data mb_data;
> +    struct in6_addr ip;
>
>      if (is_arp) {
> -        mb_data.ip = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
> +        ip = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
>      } else {
>          ovs_be128 ip6 = hton128(flow_get_xxreg(&md->flow, 0));
> -        memcpy(&mb_data.ip, &ip6, sizeof mb_data.ip);
> +        memcpy(&ip, &ip6, sizeof ip);
>      }
>
> +    mac_binding_data_init(&mb_data, ntohll(md->flow.metadata),
> +                          md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0],
> +                          ip, eth_addr_zero);
> +
>      struct buffered_packets *bp =
> buffered_packets_add(&buffered_packets_ctx,
>                                                         mb_data);
>      if (!bp) {
> @@ -4959,9 +4960,14 @@ run_buffered_binding(const struct
> sbrec_mac_binding_table *mac_binding_table,
>          const struct sbrec_port_binding *pb = lport_lookup_by_name(
>              sbrec_port_binding_by_name, smb->logical_port);
>
> +        if (!pb || !pb->datapath) {
> +            continue;
> +        }
> +
>          struct mac_binding_data mb_data;
> -        if (!mac_binding_data_from_sbrec(&mb_data, smb,
> -                                         sbrec_port_binding_by_name)) {
> +
> +        if (!mac_binding_data_parse(&mb_data, smb->datapath->tunnel_key,
> +                                    pb->tunnel_key, smb->ip, smb->mac)) {
>              continue;
>          }
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 10cd7a79b9..44d58ca160 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -35168,6 +35168,116 @@ OVN_CLEANUP([hv1], [hv2])
>  AT_CLEANUP
>  ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([MAC binding aging - port deletion])
> +AT_SKIP_IF([test $HAVE_SCAPY = no])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +check ovn-nbctl                                                     \
> +    -- ls-add ls1                                                   \
> +    -- ls-add ls2                                                   \
> +    -- lr-add lr                                                    \
> +    -- set logical_router lr options:mac_binding_age_threshold=3600 \
> +    -- lrp-add lr lr-ls1 00:00:00:00:10:00 192.168.10.1/24          \
> +    -- lrp-add lr lr-ls2 00:00:00:00:20:00 192.168.20.1/24          \
> +    -- lsp-add ls1 ls1-lr                                           \
> +    -- lsp-set-type ls1-lr router                                   \
> +    -- lsp-set-addresses ls1-lr router                              \
> +    -- lsp-set-options ls1-lr router-port=lr-ls1                    \
> +    -- lsp-add ls1 vif1                                             \
> +    -- lsp-set-addresses vif1 "00:00:00:00:10:10 192.168.10.10"     \
> +    -- lsp-add ls2 ls2-lr                                           \
> +    -- lsp-set-type ls2-lr router                                   \
> +    -- lsp-set-addresses ls2-lr router                              \
> +    -- lsp-set-options ls2-lr router-port=lr-ls2                    \
> +    -- lsp-add ls2 vif2                                             \
> +    -- lsp-set-addresses vif2 "00:00:00:00:20:10 192.168.20.10"
> +
> +check ovs-vsctl                                      \
> +    -- add-port br-int vif1                          \
> +    -- set interface vif1 external-ids:iface-id=vif1 \
> +    -- add-port br-int vif2                          \
> +    -- set interface vif2 external-ids:iface-id=vif2
> +
> +OVN_POPULATE_ARP
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +dnl Wait for pinctrl thread to be connected.
> +OVS_WAIT_UNTIL([grep pinctrl hv1/ovn-controller.log | grep -q connected])
> +
> +send_garp() {
> +    hv=$1
> +    dev=$2
> +    mac=$3
> +    ip=$4
> +
> +    packet=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${mac}')/ \
> +                      ARP(op=2, hwsrc='${mac}', hwdst='${mac}',     \
> +                      psrc='${ip}', pdst='${ip}')")
> +    as $hv ovs-appctl netdev-dummy/receive $dev $packet
> +}
> +
> +AS_BOX([Remove LRP with learnt MAC_Binding])
> +dnl Populate MAC Binding entry.
> +send_garp hv1 vif1 00:00:00:00:10:10 192.168.10.10
> +wait_row_count mac_binding 1 ip="192.168.10.10" logical_port="lr-ls1"
> +
> +dnl Check local mac binding cache.
> +lr_key=$(fetch_column datapath_binding tunnel_key external_ids:name=lr)
> +mb_uuid=$(fetch_column mac_binding _uuid logical_port=lr-ls1)
> +OVS_WAIT_FOR_OUTPUT_UNQUOTED([as hv1 ovn-appctl -t ovn-controller
> debug/dump-mac-bindings], [0], [dnl
> +Local MAC bindings:
> +SB UUID: $mb_uuid, datapath-key: $lr_key, port-key: 0, ip: 192.168.10.10,
> mac: 00:00:00:00:10:10
> +])
> +
> +dnl Remove router port.  This deletes the mac binding in the same
> transaction.
> +check ovn-nbctl --wait=hv lrp-del lr-ls1
> +
> +dnl Check local mac binding cache - it should be empty now.
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings],
> [0], [dnl
> +Local MAC bindings:
> +])
> +
> +AS_BOX([Change LRP tunnel-key with learnt MAC_Binding and remove LRP])
> +dnl Re-add port.
> +check ovn-nbctl \
> +    -- set logical_router_port lr-ls2 options:requested-tnl-key=42
> +check ovn-nbctl --wait=hv sync
> +check_column 42 Port_Binding tunnel_key logical_port=lr-ls2
> +
> +dnl Populate MAC Binding entry.
> +send_garp hv1 vif2 00:00:00:00:20:10 192.168.20.10
> +wait_row_count mac_binding 1 ip="192.168.20.10" logical_port="lr-ls2"
> +
> +dnl Check local mac binding cache.
> +mb_uuid=$(fetch_column mac_binding _uuid logical_port=lr-ls2)
> +OVS_WAIT_FOR_OUTPUT_UNQUOTED([as hv1 ovn-appctl -t ovn-controller
> debug/dump-mac-bindings], [0], [dnl
> +Local MAC bindings:
> +SB UUID: $mb_uuid, datapath-key: $lr_key, port-key: 0, ip: 192.168.20.10,
> mac: 00:00:00:00:20:10
> +])
> +
> +dnl Change LRP tunnel key.
> +check ovn-nbctl --wait=hv set logical_router_port lr-ls2
> options:requested-tnl-key=43
> +check_column 43 Port_Binding tunnel_key logical_port=lr-ls2
> +
> +dnl Remove router port.  This deletes the mac binding in the same
> transaction.
> +check ovn-nbctl --wait=hv lrp-del lr-ls2
> +dnl Check local mac binding cache - it should be empty now.
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings],
> [0], [dnl
> +Local MAC bindings:
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> +
>  OVN_FOR_EACH_NORTHD([
>  AT_SETUP([router port type update and then remove])
>  ovn_start
> --
> 2.46.2
>
>
Other than that the fix looks good.

Acked-by: Ales Musil <amusil@redhat.com>

Thanks,
Ales

On 11/4/24 10:44, Ales Musil wrote:
> On Fri, Nov 1, 2024 at 2:29 PM Dumitru Ceara <dceara@redhat.com> wrote:
> 
>> SB.MAC_Binding doesn't store a strong reference to SB.Port_Binding.
>> Instead it stores the port binding 'logical_port' name.  As the
>> mac-binding cache in ovn-controller stored entries indexed by
>> [datapath-key, port-key, ip] it meant that when processing
>> SB.MAC_Binding changes (including deletion) it had to first lookup the
>> corresponding SB.Port_Binding.
>>
>> If the logical port associated with the MAC binding is deleted in the
>> same transaction then looking it up in the IDL index fails (deleted IDL
>> records are not indexed).  This leads to stale entries staying in the
>> mac cache and in consequence to potential use after free (of the SB
>> MAC_Binding record) or to crashes if the statctrl module tries to update
>> timestamps of MAC_Bindings that happened to have been removed in the
>> current iteration (but still linger in the cache).
>>
>> In practice we don't need the port-key we can simply use the port
>> binding UUID as unique index.  However, the same struct mac_binding type
>> is used by pinctrl to temporarily efficiently store buffered packets for
>> yet to be resolved MAC bindings.  In those cases there's no SB
>> MAC_Binding record yet so the logical ingress port tunnel_key is used as
>> index.
>>
>> The two ways of using the MAC binding maps are orthogonal, that is, we
>> will never use a single map to store both types of bindings.
>>
>> In order to avoid issues we now include the SB MAC_Binding UUID into the
>> key of the struct mac_binding_data.
>>
>> Reported-at: https://issues.redhat.com/browse/FDP-906
>> Fixes: b57f60a617b4 ("controller: Add MAC cache I-P node")
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>> ---
>>
> 
> Hi Dumitru,
> 

Hi Ales,

> thank you for the fix. There is just one small typo down below.
> We should probably think about a way of decoupling pinctrl and
> ovn-controller mac-cache in the future as it unfortunately doesn't
> work well together.
> 

I agree, it would be nice to follow up and split the two mac-caches.

>  controller/mac-cache.c      |  77 ++++++++++++++++++-------
>>  controller/mac-cache.h      |  30 +++++++---
>>  controller/ovn-controller.c |  67 ++++++++++------------
>>  controller/pinctrl.c        |  24 +++++---
>>  tests/ovn.at                | 110 ++++++++++++++++++++++++++++++++++++
>>  5 files changed, 236 insertions(+), 72 deletions(-)
>>
>> diff --git a/controller/mac-cache.c b/controller/mac-cache.c
>> index de45d7a6a5..d6ba9e20ee 100644
>> --- a/controller/mac-cache.c
>> +++ b/controller/mac-cache.c
>> @@ -142,21 +142,18 @@ mac_cache_thresholds_clear(struct mac_cache_data
>> *data)
>>  }
>>
>>  /* MAC binding. */
>> -struct mac_binding *
>> +void
>>  mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
>>                  long long timestamp) {
>>
>>      struct mac_binding *mb = mac_binding_find(map, &mb_data);
>>      if (!mb) {
>>          mb = xmalloc(sizeof *mb);
>> -        mb->sbrec_mb = NULL;
>>          hmap_insert(map, &mb->hmap_node, mac_binding_data_hash(&mb_data));
>>      }
>>
>>      mb->data = mb_data;
>>      mb->timestamp = timestamp;
>> -
>> -    return mb;
>>  }
>>
>>  void
>> @@ -181,24 +178,37 @@ mac_binding_find(const struct hmap *map,
>>  }
>>
>>  bool
>> -mac_binding_data_from_sbrec(struct mac_binding_data *data,
>> -                            const struct sbrec_mac_binding *mb,
>> -                            struct ovsdb_idl_index *sbrec_pb_by_name)
>> +mac_binding_data_parse(struct mac_binding_data *data,
>> +                       uint32_t dp_key, uint32_t port_key,
>> +                       const char *ip_str, const char *mac_str)
>>  {
>> -    const struct sbrec_port_binding *pb =
>> -            lport_lookup_by_name(sbrec_pb_by_name, mb->logical_port);
>> +    struct eth_addr mac;
>> +    struct in6_addr ip;
>>
>> -    if (!pb || !pb->datapath || !ip46_parse(mb->ip, &data->ip) ||
>> -        !eth_addr_from_string(mb->mac, &data->mac)) {
>> +    if (!ip46_parse(ip_str, &ip) || !eth_addr_from_string(mac_str, &mac))
>> {
>>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> -        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s, "
>> -                     "logical_port=%s", mb->ip, mb->mac,
>> mb->logical_port);
>> +        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s",
>> +                     ip_str, mac_str);
>>          return false;
>>      }
>>
>> -    data->dp_key = mb->datapath->tunnel_key;
>> -    data->port_key = pb->tunnel_key;
>> +    mac_binding_data_init(data, dp_key, port_key, ip, mac);
>> +    return true;
>> +}
>> +
>> +bool
>> +mac_binding_data_from_sbrec(struct mac_binding_data *data,
>> +                            const struct sbrec_mac_binding *mb)
>> +{
>> +    /* This explicitly sets the port_key to 0 as port_binding tunnel_keys
>> +     * can change.  Instead use add the SB.MAC_Binding UUID as key; this
>> +     * makes the mac_binding_data key unique. */
>> +    if (!mac_binding_data_parse(data, mb->datapath->tunnel_key, 0,
>> +                                mb->ip, mb->mac)) {
>> +        return false;
>> +    }
>>
>> +    data->sbrec = mb;
>>      return true;
>>  }
>>
>> @@ -211,6 +221,30 @@ mac_bindings_clear(struct hmap *map)
>>      }
>>  }
>>
>> +void
>> +mac_bindings_to_string(const struct hmap *map, struct ds *out_data)
>> +{
>> +    struct mac_binding *mb;
>> +    HMAP_FOR_EACH (mb, hmap_node, map) {
>> +        char ip[INET6_ADDRSTRLEN];
>> +
>> +        if (!ipv6_string_mapped(ip, &mb->data.ip)) {
>> +            continue;
>> +        }
>> +        if (mb->data.sbrec) {
>> +            ds_put_format(out_data, "SB UUID: " UUID_FMT ", ",
>> +                          UUID_ARGS(&mb->data.sbrec->header_.uuid));
>> +        } else {
>> +            ds_put_cstr(out_data, "SB UUID: <none>, ");
>> +        }
>> +        ds_put_format(out_data, "datapath-key: %"PRIu32", "
>> +                                "port-key: %"PRIu32", "
>> +                                "ip: %s, mac: " ETH_ADDR_FMT "\n",
>> +                      mb->data.dp_key, mb->data.port_key,
>> +                      ip, ETH_ADDR_ARGS(mb->data.mac));
>> +    }
>> +}
>> +
>>  bool
>>  sb_mac_binding_updated(const struct sbrec_mac_binding *mb)
>>  {
>> @@ -382,9 +416,9 @@ mac_binding_stats_run(struct ovs_list *stats_list,
>> uint64_t *req_delay,
>>           * used on this chassis. Also make sure that we don't update the
>>           * timestamp more than once during the dump period. */
>>          if (stats->idle_age_ms < threshold->value &&
>> -                (timewall_now - mb->sbrec_mb->timestamp) >=
>> +                (timewall_now - mb->data.sbrec->timestamp) >=
>>              threshold->dump_period) {
>> -            sbrec_mac_binding_set_timestamp(mb->sbrec_mb, timewall_now);
>> +            sbrec_mac_binding_set_timestamp(mb->data.sbrec, timewall_now);
>>          }
>>
>>          free(stats);
>> @@ -608,7 +642,9 @@ mac_cache_stats_destroy(struct ovs_list *stats_list)
>>  static uint32_t
>>  mac_binding_data_hash(const struct mac_binding_data *mb_data)
>>  {
>> -    uint32_t hash = 0;
>> +    uint32_t hash = mb_data->sbrec
>> +                    ? uuid_hash(&mb_data->sbrec->header_.uuid)
>> +                    : 0;
>>
>>      hash = hash_add(hash, mb_data->port_key);
>>      hash = hash_add(hash, mb_data->dp_key);
>> @@ -621,9 +657,10 @@ static inline bool
>>  mac_binding_data_equals(const struct mac_binding_data *a,
>>                          const struct mac_binding_data *b)
>>  {
>> -    return a->port_key == b->port_key &&
>> +    return a->sbrec == b->sbrec &&
>> +           a->port_key == b->port_key &&
>>             a->dp_key == b->dp_key &&
>> -            ipv6_addr_equals(&a->ip, &b->ip);
>> +           ipv6_addr_equals(&a->ip, &b->ip);
>>  }
>>
>>  static uint32_t
>> diff --git a/controller/mac-cache.h b/controller/mac-cache.h
>> index b94e7a1cf2..df84ef072c 100644
>> --- a/controller/mac-cache.h
>> +++ b/controller/mac-cache.h
>> @@ -52,6 +52,7 @@ struct mac_cache_threshold {
>>
>>  struct mac_binding_data {
>>      /* Keys. */
>> +    const struct sbrec_mac_binding * sbrec; /* Only the UUID is used as
>> key.*/
>>
> 
> nit: Space after *.
> 

Ah, true, I fixed it before applying the patch.

> 
>>      uint32_t port_key;
>>      uint32_t dp_key;
>>      struct in6_addr ip;
>> @@ -63,8 +64,6 @@ struct mac_binding {
>>      struct hmap_node hmap_node;
>>      /* Common data to identify MAC binding. */
>>      struct mac_binding_data data;
>> -    /* Reference to the SB MAC binding record (Might be NULL). */
>> -    const struct sbrec_mac_binding *sbrec_mb;
>>      /* User specified timestamp (in ms) */
>>      long long timestamp;
>>  };
>> @@ -128,20 +127,37 @@ void mac_cache_thresholds_sync(struct mac_cache_data
>> *data,
>>  void mac_cache_thresholds_clear(struct mac_cache_data *data);
>>
>>  /* MAC binding. */
>> -struct mac_binding *mac_binding_add(struct hmap *map,
>> -                                    struct mac_binding_data mb_data,
>> -                                    long long timestamp);
>> +
>> +static inline void
>> +mac_binding_data_init(struct mac_binding_data *data,
>> +                      uint32_t dp_key, uint32_t port_key,
>> +                      struct in6_addr ip, struct eth_addr mac)
>> +{
>> +    *data = (struct mac_binding_data) {
>> +        .sbrec = NULL,
>> +        .dp_key = (dp_key),
>> +        .port_key = (port_key),
>> +        .ip = (ip),
>> +        .mac = (mac),
>> +    };
>> +}
>> +
>> +void mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
>> +                     long long timestamp);
>>
>>  void mac_binding_remove(struct hmap *map, struct mac_binding *mb);
>>
>>  struct mac_binding *mac_binding_find(const struct hmap *map,
>>                                       const struct mac_binding_data
>> *mb_data);
>>
>> +bool mac_binding_data_parse(struct mac_binding_data *data,
>> +                            uint32_t dp_key, uint32_t port_key,
>> +                            const char *ip_str, const char *mac_str);
>>  bool mac_binding_data_from_sbrec(struct mac_binding_data *data,
>> -                                 const struct sbrec_mac_binding *mb,
>> -                                 struct ovsdb_idl_index
>> *sbrec_pb_by_name);
>> +                                 const struct sbrec_mac_binding *mb);
>>
>>  void mac_bindings_clear(struct hmap *map);
>> +void mac_bindings_to_string(const struct hmap *map, struct ds *out_data);
>>
>>  bool sb_mac_binding_updated(const struct sbrec_mac_binding *mb);
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index e87274121c..ab95938502 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -101,6 +101,7 @@ static unixctl_cb_func debug_status_execution;
>>  static unixctl_cb_func debug_dump_local_bindings;
>>  static unixctl_cb_func debug_dump_related_lports;
>>  static unixctl_cb_func debug_dump_local_template_vars;
>> +static unixctl_cb_func debug_dump_local_mac_bindings;
>>  static unixctl_cb_func debug_dump_lflow_conj_ids;
>>  static unixctl_cb_func lflow_cache_flush_cmd;
>>  static unixctl_cb_func lflow_cache_show_stats_cmd;
>> @@ -2932,26 +2933,22 @@ en_lb_data_cleanup(void *data)
>>
>>  static void
>>  mac_binding_add_sb(struct mac_cache_data *data,
>> -                   const struct sbrec_mac_binding *smb,
>> -                   struct ovsdb_idl_index *sbrec_pb_by_name)
>> +                   const struct sbrec_mac_binding *smb)
>>  {
>>      struct mac_binding_data mb_data;
>> -    if (!mac_binding_data_from_sbrec(&mb_data, smb, sbrec_pb_by_name)) {
>> +    if (!mac_binding_data_from_sbrec(&mb_data, smb)) {
>>          return;
>>      }
>>
>> -    struct mac_binding *mb = mac_binding_add(&data->mac_bindings,
>> mb_data, 0);
>> -
>> -    mb->sbrec_mb = smb;
>> +    mac_binding_add(&data->mac_bindings, mb_data, 0);
>>  }
>>
>>  static void
>>  mac_binding_remove_sb(struct mac_cache_data *data,
>> -                      const struct sbrec_mac_binding *smb,
>> -                      struct ovsdb_idl_index *sbrec_pb_by_name)
>> +                      const struct sbrec_mac_binding *smb)
>>  {
>>      struct mac_binding_data mb_data;
>> -    if (!mac_binding_data_from_sbrec(&mb_data, smb, sbrec_pb_by_name)) {
>> +    if (!mac_binding_data_from_sbrec(&mb_data, smb)) {
>>          return;
>>      }
>>
>> @@ -2995,8 +2992,7 @@ fdb_remove_sb(struct mac_cache_data *data, const
>> struct sbrec_fdb *sfdb)
>>  static void
>>  mac_cache_mb_handle_for_datapath(struct mac_cache_data *data,
>>                                   const struct sbrec_datapath_binding *dp,
>> -                                 struct ovsdb_idl_index *sbrec_mb_by_dp,
>> -                                 struct ovsdb_idl_index *sbrec_pb_by_name)
>> +                                 struct ovsdb_idl_index *sbrec_mb_by_dp)
>>  {
>>      bool has_threshold = mac_cache_threshold_find(data, dp->tunnel_key);
>>
>> @@ -3007,9 +3003,9 @@ mac_cache_mb_handle_for_datapath(struct
>> mac_cache_data *data,
>>      const struct sbrec_mac_binding *mb;
>>      SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row, sbrec_mb_by_dp) {
>>          if (has_threshold) {
>> -            mac_binding_add_sb(data, mb, sbrec_pb_by_name);
>> +            mac_binding_add_sb(data, mb);
>>          } else {
>> -            mac_binding_remove_sb(data, mb, sbrec_pb_by_name);
>> +            mac_binding_remove_sb(data, mb);
>>          }
>>      }
>>
>> @@ -3064,10 +3060,6 @@ en_mac_cache_run(struct engine_node *node, void
>> *data)
>>              engine_ovsdb_node_get_index(
>>                      engine_get_input("SB_mac_binding", node),
>>                      "datapath");
>> -    struct ovsdb_idl_index *sbrec_pb_by_name =
>> -            engine_ovsdb_node_get_index(
>> -                    engine_get_input("SB_port_binding", node),
>> -                    "name");
>>      struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>>              engine_ovsdb_node_get_index(
>>                      engine_get_input("SB_fdb", node),
>> @@ -3086,7 +3078,7 @@ en_mac_cache_run(struct engine_node *node, void
>> *data)
>>
>>          mac_cache_threshold_add(cache_data, sbrec_dp);
>>          mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
>> -                                         sbrec_mb_by_dp,
>> sbrec_pb_by_name);
>> +                                         sbrec_mb_by_dp);
>>          mac_cache_fdb_handle_for_datapath(cache_data, sbrec_dp,
>>                                            sbrec_fdb_by_dp_key);
>>      }
>> @@ -3102,11 +3094,6 @@ mac_cache_sb_mac_binding_handler(struct engine_node
>> *node, void *data)
>>              engine_get_input_data("runtime_data", node);
>>      const struct sbrec_mac_binding_table *mb_table =
>>              EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
>> -    struct ovsdb_idl_index *sbrec_pb_by_name =
>> -            engine_ovsdb_node_get_index(
>> -                    engine_get_input("SB_port_binding", node),
>> -                    "name");
>> -
>>      size_t previous_size = hmap_count(&cache_data->mac_bindings);
>>
>>      const struct sbrec_mac_binding *sbrec_mb;
>> @@ -3116,8 +3103,7 @@ mac_cache_sb_mac_binding_handler(struct engine_node
>> *node, void *data)
>>          }
>>
>>          if (!sbrec_mac_binding_is_new(sbrec_mb)) {
>> -            mac_binding_remove_sb(cache_data, sbrec_mb,
>> -                                  sbrec_pb_by_name);
>> +            mac_binding_remove_sb(cache_data, sbrec_mb);
>>          }
>>
>>          if (sbrec_mac_binding_is_deleted(sbrec_mb) ||
>> @@ -3128,7 +3114,7 @@ mac_cache_sb_mac_binding_handler(struct engine_node
>> *node, void *data)
>>
>>          if (mac_cache_threshold_find(cache_data,
>>                                       sbrec_mb->datapath->tunnel_key)) {
>> -            mac_binding_add_sb(cache_data, sbrec_mb, sbrec_pb_by_name);
>> +            mac_binding_add_sb(cache_data, sbrec_mb);
>>          }
>>      }
>>
>> @@ -3189,10 +3175,6 @@ mac_cache_runtime_data_handler(struct engine_node
>> *node, void *data OVS_UNUSED)
>>              engine_ovsdb_node_get_index(
>>                      engine_get_input("SB_mac_binding", node),
>>                      "datapath");
>> -    struct ovsdb_idl_index *sbrec_pb_by_name =
>> -            engine_ovsdb_node_get_index(
>> -                    engine_get_input("SB_port_binding", node),
>> -                    "name");
>>      struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>>              engine_ovsdb_node_get_index(
>>                      engine_get_input("SB_fdb", node),
>> @@ -3217,7 +3199,7 @@ mac_cache_runtime_data_handler(struct engine_node
>> *node, void *data OVS_UNUSED)
>>
>>      HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
>>          mac_cache_mb_handle_for_datapath(cache_data, tdp->dp,
>> -                                         sbrec_mb_by_dp,
>> sbrec_pb_by_name);
>> +                                         sbrec_mb_by_dp);
>>
>>          mac_cache_fdb_handle_for_datapath(cache_data, tdp->dp,
>>                                            sbrec_fdb_by_dp_key);
>> @@ -3243,10 +3225,6 @@ mac_cache_sb_datapath_binding_handler(struct
>> engine_node *node, void *data)
>>              engine_ovsdb_node_get_index(
>>                      engine_get_input("SB_mac_binding", node),
>>                      "datapath");
>> -    struct ovsdb_idl_index *sbrec_pb_by_name =
>> -            engine_ovsdb_node_get_index(
>> -                    engine_get_input("SB_port_binding", node),
>> -                    "name");
>>      struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>>              engine_ovsdb_node_get_index(
>>                      engine_get_input("SB_fdb", node),
>> @@ -3274,7 +3252,7 @@ mac_cache_sb_datapath_binding_handler(struct
>> engine_node *node, void *data)
>>
>>      SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_dp, dp_table) {
>>          mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
>> -                                         sbrec_mb_by_dp,
>> sbrec_pb_by_name);
>> +                                         sbrec_mb_by_dp);
>>
>>          mac_cache_fdb_handle_for_datapath(cache_data, sbrec_dp,
>>                                            sbrec_fdb_by_dp_key);
>> @@ -5399,6 +5377,10 @@ main(int argc, char *argv[])
>>                               debug_dump_local_template_vars,
>>                               &template_vars_data->local_templates);
>>
>> +    unixctl_command_register("debug/dump-mac-bindings", "", 0, 0,
>> +                             debug_dump_local_mac_bindings,
>> +                             &mac_cache_data->mac_bindings);
>> +
>>      unixctl_command_register("debug/ignore-startup-delay", "", 0, 0,
>>                               debug_ignore_startup_delay, NULL);
>>
>> @@ -6356,6 +6338,19 @@ debug_dump_local_template_vars(struct unixctl_conn
>> *conn, int argc OVS_UNUSED,
>>      ds_destroy(&tv_str);
>>  }
>>
>> +static void
>> +debug_dump_local_mac_bindings(struct unixctl_conn *conn, int argc
>> OVS_UNUSED,
>> +                               const char *argv[] OVS_UNUSED,
>> +                               void *mac_bindings)
>> +{
>> +    struct ds mb_str = DS_EMPTY_INITIALIZER;
>> +
>> +    ds_put_cstr(&mb_str, "Local MAC bindings:\n");
>> +    mac_bindings_to_string(mac_bindings, &mb_str);
>> +    unixctl_command_reply(conn, ds_cstr(&mb_str));
>> +    ds_destroy(&mb_str);
>> +}
>> +
>>  static void
>>  debug_ignore_startup_delay(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>                             const char *argv[] OVS_UNUSED, void *arg
>> OVS_UNUSED)
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index dfb3130aa2..ca880a1da6 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -1539,19 +1539,20 @@ pinctrl_handle_buffered_packets(const struct
>> ofputil_packet_in *pin,
>>  OVS_REQUIRES(pinctrl_mutex)
>>  {
>>      const struct match *md = &pin->flow_metadata;
>> -    struct mac_binding_data mb_data = (struct mac_binding_data) {
>> -            .dp_key =  ntohll(md->flow.metadata),
>> -            .port_key =  md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0],
>> -            .mac = eth_addr_zero,
>> -    };
>> +    struct mac_binding_data mb_data;
>> +    struct in6_addr ip;
>>
>>      if (is_arp) {
>> -        mb_data.ip = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
>> +        ip = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
>>      } else {
>>          ovs_be128 ip6 = hton128(flow_get_xxreg(&md->flow, 0));
>> -        memcpy(&mb_data.ip, &ip6, sizeof mb_data.ip);
>> +        memcpy(&ip, &ip6, sizeof ip);
>>      }
>>
>> +    mac_binding_data_init(&mb_data, ntohll(md->flow.metadata),
>> +                          md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0],
>> +                          ip, eth_addr_zero);
>> +
>>      struct buffered_packets *bp =
>> buffered_packets_add(&buffered_packets_ctx,
>>                                                         mb_data);
>>      if (!bp) {
>> @@ -4959,9 +4960,14 @@ run_buffered_binding(const struct
>> sbrec_mac_binding_table *mac_binding_table,
>>          const struct sbrec_port_binding *pb = lport_lookup_by_name(
>>              sbrec_port_binding_by_name, smb->logical_port);
>>
>> +        if (!pb || !pb->datapath) {
>> +            continue;
>> +        }
>> +
>>          struct mac_binding_data mb_data;
>> -        if (!mac_binding_data_from_sbrec(&mb_data, smb,
>> -                                         sbrec_port_binding_by_name)) {
>> +
>> +        if (!mac_binding_data_parse(&mb_data, smb->datapath->tunnel_key,
>> +                                    pb->tunnel_key, smb->ip, smb->mac)) {
>>              continue;
>>          }
>>
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 10cd7a79b9..44d58ca160 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -35168,6 +35168,116 @@ OVN_CLEANUP([hv1], [hv2])
>>  AT_CLEANUP
>>  ])
>>
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([MAC binding aging - port deletion])
>> +AT_SKIP_IF([test $HAVE_SCAPY = no])
>> +ovn_start
>> +
>> +net_add n1
>> +sim_add hv1
>> +as hv1
>> +check ovs-vsctl add-br br-phys
>> +ovn_attach n1 br-phys 192.168.0.1
>> +
>> +check ovn-nbctl                                                     \
>> +    -- ls-add ls1                                                   \
>> +    -- ls-add ls2                                                   \
>> +    -- lr-add lr                                                    \
>> +    -- set logical_router lr options:mac_binding_age_threshold=3600 \
>> +    -- lrp-add lr lr-ls1 00:00:00:00:10:00 192.168.10.1/24          \
>> +    -- lrp-add lr lr-ls2 00:00:00:00:20:00 192.168.20.1/24          \
>> +    -- lsp-add ls1 ls1-lr                                           \
>> +    -- lsp-set-type ls1-lr router                                   \
>> +    -- lsp-set-addresses ls1-lr router                              \
>> +    -- lsp-set-options ls1-lr router-port=lr-ls1                    \
>> +    -- lsp-add ls1 vif1                                             \
>> +    -- lsp-set-addresses vif1 "00:00:00:00:10:10 192.168.10.10"     \
>> +    -- lsp-add ls2 ls2-lr                                           \
>> +    -- lsp-set-type ls2-lr router                                   \
>> +    -- lsp-set-addresses ls2-lr router                              \
>> +    -- lsp-set-options ls2-lr router-port=lr-ls2                    \
>> +    -- lsp-add ls2 vif2                                             \
>> +    -- lsp-set-addresses vif2 "00:00:00:00:20:10 192.168.20.10"
>> +
>> +check ovs-vsctl                                      \
>> +    -- add-port br-int vif1                          \
>> +    -- set interface vif1 external-ids:iface-id=vif1 \
>> +    -- add-port br-int vif2                          \
>> +    -- set interface vif2 external-ids:iface-id=vif2
>> +
>> +OVN_POPULATE_ARP
>> +wait_for_ports_up
>> +check ovn-nbctl --wait=hv sync
>> +
>> +dnl Wait for pinctrl thread to be connected.
>> +OVS_WAIT_UNTIL([grep pinctrl hv1/ovn-controller.log | grep -q connected])
>> +
>> +send_garp() {
>> +    hv=$1
>> +    dev=$2
>> +    mac=$3
>> +    ip=$4
>> +
>> +    packet=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${mac}')/ \
>> +                      ARP(op=2, hwsrc='${mac}', hwdst='${mac}',     \
>> +                      psrc='${ip}', pdst='${ip}')")
>> +    as $hv ovs-appctl netdev-dummy/receive $dev $packet
>> +}
>> +
>> +AS_BOX([Remove LRP with learnt MAC_Binding])
>> +dnl Populate MAC Binding entry.
>> +send_garp hv1 vif1 00:00:00:00:10:10 192.168.10.10
>> +wait_row_count mac_binding 1 ip="192.168.10.10" logical_port="lr-ls1"
>> +
>> +dnl Check local mac binding cache.
>> +lr_key=$(fetch_column datapath_binding tunnel_key external_ids:name=lr)
>> +mb_uuid=$(fetch_column mac_binding _uuid logical_port=lr-ls1)
>> +OVS_WAIT_FOR_OUTPUT_UNQUOTED([as hv1 ovn-appctl -t ovn-controller
>> debug/dump-mac-bindings], [0], [dnl
>> +Local MAC bindings:
>> +SB UUID: $mb_uuid, datapath-key: $lr_key, port-key: 0, ip: 192.168.10.10,
>> mac: 00:00:00:00:10:10
>> +])
>> +
>> +dnl Remove router port.  This deletes the mac binding in the same
>> transaction.
>> +check ovn-nbctl --wait=hv lrp-del lr-ls1
>> +
>> +dnl Check local mac binding cache - it should be empty now.
>> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings],
>> [0], [dnl
>> +Local MAC bindings:
>> +])
>> +
>> +AS_BOX([Change LRP tunnel-key with learnt MAC_Binding and remove LRP])
>> +dnl Re-add port.
>> +check ovn-nbctl \
>> +    -- set logical_router_port lr-ls2 options:requested-tnl-key=42
>> +check ovn-nbctl --wait=hv sync
>> +check_column 42 Port_Binding tunnel_key logical_port=lr-ls2
>> +
>> +dnl Populate MAC Binding entry.
>> +send_garp hv1 vif2 00:00:00:00:20:10 192.168.20.10
>> +wait_row_count mac_binding 1 ip="192.168.20.10" logical_port="lr-ls2"
>> +
>> +dnl Check local mac binding cache.
>> +mb_uuid=$(fetch_column mac_binding _uuid logical_port=lr-ls2)
>> +OVS_WAIT_FOR_OUTPUT_UNQUOTED([as hv1 ovn-appctl -t ovn-controller
>> debug/dump-mac-bindings], [0], [dnl
>> +Local MAC bindings:
>> +SB UUID: $mb_uuid, datapath-key: $lr_key, port-key: 0, ip: 192.168.20.10,
>> mac: 00:00:00:00:20:10
>> +])
>> +
>> +dnl Change LRP tunnel key.
>> +check ovn-nbctl --wait=hv set logical_router_port lr-ls2
>> options:requested-tnl-key=43
>> +check_column 43 Port_Binding tunnel_key logical_port=lr-ls2
>> +
>> +dnl Remove router port.  This deletes the mac binding in the same
>> transaction.
>> +check ovn-nbctl --wait=hv lrp-del lr-ls2
>> +dnl Check local mac binding cache - it should be empty now.
>> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings],
>> [0], [dnl
>> +Local MAC bindings:
>> +])
>> +
>> +OVN_CLEANUP([hv1])
>> +AT_CLEANUP
>> +])
>> +
>>  OVN_FOR_EACH_NORTHD([
>>  AT_SETUP([router port type update and then remove])
>>  ovn_start
>> --
>> 2.46.2
>>
>>
> Other than that the fix looks good.
> 
> Acked-by: Ales Musil <amusil@redhat.com>
> 
> Thanks,
> Ales

Thanks,
Dumitru