There was a race within packet buffering that could result in first packt being dropped. It could happen under following conditions and topology: S1 == R1 == public == R2 == S2 SNAT on R1 and DGP on port connecting R1 with public.
1) The GARP is sent for the DGP SNAT 2) The GARP is delayed on R2 because it's multicast 3) The MAC binding is added to SB 4) Some traffic that gets buffered on S2 5) An ARP is sent as consequence of the buffering 6) Response for the ARP is ignored on lflow level, because the MAC binding already exists 7) The buffered packet is never sent out and times out In order to prevent this behavior do not use tracked database changes and search for the MAC binding in the DB instead. Signed-off-by: Ales Musil <[email protected]> --- v6: Rebase on top of current main. Do not use the I-P node, but do the lookup separately for every buffered packet. --- controller/mac-learn.c | 72 +++++++++++++--------------------- controller/mac-learn.h | 9 ++++- controller/ovn-controller.c | 2 - controller/pinctrl.c | 78 ++++++++++++++++++++----------------- controller/pinctrl.h | 1 - 5 files changed, 78 insertions(+), 84 deletions(-) diff --git a/controller/mac-learn.c b/controller/mac-learn.c index 296a70242..c1160e735 100644 --- a/controller/mac-learn.c +++ b/controller/mac-learn.c @@ -43,8 +43,6 @@ static struct fdb_entry *fdb_entry_find(struct hmap *fdbs, uint32_t dp_key, static struct buffered_packets * buffered_packets_find(struct buffered_packets_ctx *ctx, uint64_t dp_key, uint64_t port_key, struct in6_addr *ip, uint32_t hash); -static void ovn_buffered_packets_remove(struct buffered_packets_ctx *ctx, - struct buffered_packets *bp); /* mac_binding functions. */ void @@ -215,7 +213,7 @@ ovn_buffered_packets_add(struct buffered_packets_ctx *ctx, uint64_t dp_key, ovs_list_init(&bp->queue); } - bp->expire = time_msec() + OVN_BUFFERED_PACKETS_TIMEOUT_MS; + bp->expire_at_ms= time_msec() + OVN_BUFFERED_PACKETS_TIMEOUT_MS; return bp; } @@ -233,39 +231,37 @@ ovn_buffered_packets_packet_data_enqueue(struct buffered_packets *bp, ovs_list_push_back(&bp->queue, &pd->node); } -void -ovn_buffered_packets_ctx_run(struct buffered_packets_ctx *ctx, - const struct mac_bindings_map *recent_mbs) +bool +ovn_buffered_packets_expired(const struct buffered_packets *bp, long long now) { - long long now = time_msec(); - - struct buffered_packets *bp; - - HMAP_FOR_EACH_SAFE (bp, hmap_node, &ctx->buffered_packets) { - /* Remove expired buffered packets. */ - if (now > bp->expire) { - ovn_buffered_packets_remove(ctx, bp); - continue; - } - - uint32_t hash = keys_ip_hash(bp->dp_key, bp->port_key, &bp->ip); - struct mac_binding *mb = mac_binding_find(recent_mbs, bp->dp_key, - bp->port_key, &bp->ip, hash); - - if (!mb) { - continue; - } + return now >= bp->expire_at_ms; +} - struct packet_data *pd; - LIST_FOR_EACH_POP (pd, node, &bp->queue) { - struct eth_header *eth = dp_packet_data(pd->p); - eth->eth_dst = mb->mac; +void +ovn_buffered_packets_set_ready(struct buffered_packets_ctx *ctx, + struct buffered_packets *bp, + struct eth_addr mac) +{ + struct packet_data *pd; + LIST_FOR_EACH_POP (pd, node, &bp->queue) { + struct eth_header *eth = dp_packet_data(pd->p); + eth->eth_dst = mac; - ovs_list_push_back(&ctx->ready_packets_data, &pd->node); - } + ovs_list_push_back(&ctx->ready_packets_data, &pd->node); + } +} - ovn_buffered_packets_remove(ctx, bp); +void +ovn_buffered_packets_remove(struct buffered_packets_ctx *ctx, + struct buffered_packets *bp) +{ + struct packet_data *pd; + LIST_FOR_EACH_POP (pd, node, &bp->queue) { + ovn_packet_data_destroy(pd); } + + hmap_remove(&ctx->buffered_packets, &bp->hmap_node); + free(bp); } bool @@ -365,17 +361,3 @@ buffered_packets_find(struct buffered_packets_ctx *ctx, uint64_t dp_key, return NULL; } - -static void -ovn_buffered_packets_remove(struct buffered_packets_ctx *ctx, - struct buffered_packets *bp) -{ - struct packet_data *pd; - - LIST_FOR_EACH_POP (pd, node, &bp->queue) { - ovn_packet_data_destroy(pd); - } - - hmap_remove(&ctx->buffered_packets, &bp->hmap_node); - free(bp); -} diff --git a/controller/mac-learn.h b/controller/mac-learn.h index bc99d09f3..8e64ffd56 100644 --- a/controller/mac-learn.h +++ b/controller/mac-learn.h @@ -102,7 +102,7 @@ struct buffered_packets { struct ovs_list queue; /* Timestamp in ms when the buffered packet should expire. */ - long long int expire; + long long int expire_at_ms; }; struct buffered_packets_ctx { @@ -121,6 +121,13 @@ ovn_buffered_packets_add(struct buffered_packets_ctx *ctx, uint64_t dp_key, uint64_t port_key, struct in6_addr ip); void ovn_buffered_packets_packet_data_enqueue(struct buffered_packets *bp, struct packet_data *pd); +bool ovn_buffered_packets_expired(const struct buffered_packets *bp, + long long now); +void ovn_buffered_packets_set_ready(struct buffered_packets_ctx *ctx, + struct buffered_packets *bp, + struct eth_addr mac); +void ovn_buffered_packets_remove(struct buffered_packets_ctx *ctx, + struct buffered_packets *bp); void ovn_buffered_packets_ctx_run(struct buffered_packets_ctx *ctx, const struct mac_bindings_map *recent_mbs); diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index de90025f0..356dbc77f 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5185,8 +5185,6 @@ main(int argc, char *argv[]) ovnsb_idl_loop.idl), sbrec_service_monitor_table_get( ovnsb_idl_loop.idl), - sbrec_mac_binding_table_get( - ovnsb_idl_loop.idl), sbrec_bfd_table_get(ovnsb_idl_loop.idl), br_int, chassis, &runtime_data->local_datapaths, diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 1694b0445..fcd9f5946 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -181,8 +181,10 @@ static struct pinctrl pinctrl; static void init_buffered_packets_ctx(void); static void destroy_buffered_packets_ctx(void); static void -run_buffered_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, - const struct sbrec_mac_binding_table *mac_binding_table) +run_buffered_binding(struct ovsdb_idl_index *sbrec_port_binding_by_key, + struct ovsdb_idl_index *sbrec_datapath_binding_by_key, + struct ovsdb_idl_index *sbrec_port_binding_by_name, + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip) OVS_REQUIRES(pinctrl_mutex); static void pinctrl_handle_put_mac_binding(const struct flow *md, @@ -3470,7 +3472,6 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sbrec_dns_table *dns_table, const struct sbrec_controller_event_table *ce_table, const struct sbrec_service_monitor_table *svc_mon_table, - const struct sbrec_mac_binding_table *mac_binding_table, const struct sbrec_bfd_table *bfd_table, const struct ovsrec_bridge *br_int, const struct sbrec_chassis *chassis, @@ -3500,7 +3501,10 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, sbrec_port_binding_by_key, sbrec_igmp_groups, sbrec_ip_multicast_opts); - run_buffered_binding(sbrec_port_binding_by_name, mac_binding_table); + run_buffered_binding(sbrec_port_binding_by_key, + sbrec_datapath_binding_by_key, + sbrec_port_binding_by_name, + sbrec_mac_binding_by_lport_ip); sync_svc_monitors(ovnsb_idl_txn, svc_mon_table, sbrec_port_binding_by_name, chassis); bfd_monitor_run(ovnsb_idl_txn, bfd_table, sbrec_port_binding_by_name, @@ -4291,63 +4295,67 @@ run_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn, } static void -run_buffered_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, - const struct sbrec_mac_binding_table *mac_binding_table) +run_buffered_binding(struct ovsdb_idl_index *sbrec_port_binding_by_key, + struct ovsdb_idl_index *sbrec_datapath_binding_by_key, + struct ovsdb_idl_index *sbrec_port_binding_by_name, + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip) OVS_REQUIRES(pinctrl_mutex) { if (!ovn_buffered_packets_ctx_has_packets(&buffered_packets_ctx)) { return; } - struct mac_bindings_map recent_mbs; - ovn_mac_bindings_map_init(&recent_mbs, 0); + struct ds ip = DS_EMPTY_INITIALIZER; + long long now = time_msec(); - const struct sbrec_mac_binding *smb; - SBREC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (smb, 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) { + struct buffered_packets *bp; + HMAP_FOR_EACH_SAFE (bp, hmap_node, + &buffered_packets_ctx.buffered_packets) { + if (ovn_buffered_packets_expired(bp, now)) { + ovn_buffered_packets_remove(&buffered_packets_ctx, bp); continue; } - struct in6_addr ip; - if (!ip46_parse(smb->ip, &ip)) { - continue; + const struct sbrec_port_binding *pb = lport_lookup_by_key( + sbrec_datapath_binding_by_key, sbrec_port_binding_by_key, + bp->dp_key, bp->port_key); + + if (!pb) { + continue; } - struct eth_addr mac; - if (!eth_addr_from_string(smb->mac, &mac)) { - continue; + if (!strcmp(pb->type, "chassisredirect")) { + const char *dgp_name = smap_get_def(&pb->options, + "distributed-port", ""); + pb = lport_lookup_by_name(sbrec_port_binding_by_name, dgp_name); + if (!pb) { + continue; + } } - ovn_mac_binding_add(&recent_mbs, smb->datapath->tunnel_key, - pb->tunnel_key, &ip, mac, 0); + ipv6_format_mapped(&bp->ip, &ip); + const struct sbrec_mac_binding *smb = mac_binding_lookup( + sbrec_mac_binding_by_lport_ip, pb->logical_port, ds_cstr(&ip)); + ds_clear(&ip); - const char *redirect_port = - smap_get(&pb->options, "chassis-redirect-port"); - if (!redirect_port) { + if (!smb) { continue; } - pb = lport_lookup_by_name(sbrec_port_binding_by_name, redirect_port); - if (!pb || pb->datapath->tunnel_key != smb->datapath->tunnel_key || - strcmp(pb->type, "chassisredirect")) { + struct eth_addr mac; + if (!eth_addr_from_string(smb->mac, &mac)) { continue; } - /* Add the same entry also for chassisredirect port as the buffered - * traffic might be buffered on the cr port. */ - ovn_mac_binding_add(&recent_mbs, smb->datapath->tunnel_key, - pb->tunnel_key, &ip, mac, 0); + ovn_buffered_packets_set_ready(&buffered_packets_ctx, bp, mac); + ovn_buffered_packets_remove(&buffered_packets_ctx, bp); } - ovn_buffered_packets_ctx_run(&buffered_packets_ctx, &recent_mbs); - - ovn_mac_bindings_map_destroy(&recent_mbs); - if (ovn_buffered_packets_ctx_is_ready_to_send(&buffered_packets_ctx)) { notify_pinctrl_handler(); } + + ds_destroy(&ip); } static void diff --git a/controller/pinctrl.h b/controller/pinctrl.h index 279a49fbc..7c0743223 100644 --- a/controller/pinctrl.h +++ b/controller/pinctrl.h @@ -51,7 +51,6 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sbrec_dns_table *, const struct sbrec_controller_event_table *, const struct sbrec_service_monitor_table *, - const struct sbrec_mac_binding_table *, const struct sbrec_bfd_table *, const struct ovsrec_bridge *, const struct sbrec_chassis *, const struct hmap *local_datapaths, -- 2.40.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
