On Thu, Mar 09, 2023 at 07:17:15AM +0100, Ales Musil wrote: > 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 GDP SNAT > 2) The GARP is delayed on R2 because it's multicast > 3) Some traffic that get's buffered on S2 > 4) An ARP is sent as consequence of the buffering > 5) The delayed MAC binding is added to SB > 6) Response for the ARP is ignored because the MAC binding > already exists > 7) The buffered packet is never sent out and times out > > In order to prevent this behavior add new node that will > keep track of all recently changed MAC bindings. Those > recently changed MAC bindings are kept around for a longer > time than the buffered packets which should ensure that we > can find them even if they are created before the packet > is actually buffered. > > At the same time simplify the packet buffering process > and move it to mac-learn module.
This patch is a bit long. It may be easier to review if it was broken up somehow. > Signed-off-by: Ales Musil <[email protected]> ... > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 7dcbfd252..c03b2af4c 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c ... > @@ -4305,6 +4306,185 @@ pflow_output_debug_handler(struct engine_node *node, > void *data) > return true; > } > > +static void > +recent_mac_bindings_add(struct hmap *recent_mbs, > + const struct sbrec_mac_binding *smb, > + const struct hmap *local_datapaths, > + struct ovsdb_idl_index *sbrec_port_binding_by_name) > +{ > + ovs_be32 ip4; > + struct in6_addr ip; > + struct eth_addr mac; > + const struct sbrec_port_binding *pb; > + > + if (!get_local_datapath(local_datapaths, smb->datapath->tunnel_key)) { > + return; > + } > + > + pb = lport_lookup_by_name(sbrec_port_binding_by_name, smb->logical_port); > + if (!pb) { > + return; > + } > + > + if (!eth_addr_from_string(smb->mac, &mac)) { > + return; > + } > + > + if (strchr(smb->ip, '.')) { > + if (!ip_parse(smb->ip, &ip4)) { > + return; > + } > + ip = in6_addr_mapped_ipv4(ip4); > + } else { > + if (!ipv6_parse(smb->ip, &ip)) { > + return; > + } > + } Not strictly related to this patch, but this feels ripe for a helper. ... > +static void > +en_recent_mac_bindings_run(struct engine_node *node, void *data) > +{ > + struct hmap *recent_mbs = data; > + struct ed_type_runtime_data *rt_data = > + engine_get_input_data("runtime_data", node); > + const struct sbrec_mac_binding_table *mac_binding_table = > + EN_OVSDB_GET(engine_get_input("SB_mac_binding", node)); > + struct ovsdb_idl_index *sbrec_port_binding_by_name = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_port_binding", node), > + "name"); > + > + const struct sbrec_mac_binding *smb; Maybe it's just me, but the following seems somewhat easier to read: struct hmap *recent_mbs = data; const struct sbrec_mac_binding *smb; struct ed_type_runtime_data *rt_data; const struct sbrec_mac_binding_table *mac_binding_table; struct ovsdb_idl_index *sbrec_port_binding_by_name; rt_data = engine_get_input_data("runtime_data", node); mac_binding_table = EN_OVSDB_GET(engine_get_input("SB_mac_binding", node)); sbrec_port_binding_by_name = engine_ovsdb_node_get_index(engine_get_input("SB_port_binding", node), "name"); Also this setup and call invocatoion of SBREC_MAC_BINDING_TABLE_FOR_EACH seems to be repeated several times. Perhaps some sort of helper macro could be considered. > + SBREC_MAC_BINDING_TABLE_FOR_EACH (smb, mac_binding_table) { > + recent_mac_bindings_add(recent_mbs, smb, &rt_data->local_datapaths, > + sbrec_port_binding_by_name); > + } > +} > +static bool > +recent_mac_bindings_mac_binding_handler(struct engine_node *node, void *data) > +{ > + struct hmap *recent_mbs = data; > + struct ed_type_runtime_data *rt_data = > + engine_get_input_data("runtime_data", node); > + const struct sbrec_mac_binding_table *mac_binding_table = > + EN_OVSDB_GET(engine_get_input("SB_mac_binding", node)); > + struct ovsdb_idl_index *sbrec_port_binding_by_name = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_port_binding", node), > + "name"); > + > + const struct sbrec_mac_binding *smb; > + SBREC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (smb, mac_binding_table) { > + /* The deleted entry will expire eventually. */ > + if (sbrec_mac_binding_is_deleted(smb)) { > + continue; > + } > + > + recent_mac_bindings_add(recent_mbs, smb, &rt_data->local_datapaths, > + sbrec_port_binding_by_name); > + } > + > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > + > +static bool > +recent_mac_bindings_runtime_data_handler(struct engine_node *node, void > *data) > +{ > + struct hmap *recent_mbs = data; > + struct ed_type_runtime_data *rt_data = > + engine_get_input_data("runtime_data", node); > + struct ovsdb_idl_index *smb_by_datapath = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_mac_binding", node), > + "datapath"); > + struct ovsdb_idl_index *sbrec_port_binding_by_name = > + engine_ovsdb_node_get_index( > + engine_get_input("SB_port_binding", node), > + "name"); > + > + if (!rt_data->tracked) { > + return false; > + } > + > + struct tracked_datapath *tdp; > + HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) { > + if (tdp->tracked_type != TRACKED_RESOURCE_NEW) { > + continue; > + } > + > + struct sbrec_mac_binding *mb_index_row = > + sbrec_mac_binding_index_init_row(smb_by_datapath); > + sbrec_mac_binding_index_set_datapath(mb_index_row, tdp->dp); > + > + const struct sbrec_mac_binding *smb; > + SBREC_MAC_BINDING_FOR_EACH_EQUAL (smb, mb_index_row, > smb_by_datapath) { > + recent_mac_bindings_add(recent_mbs, smb, > &rt_data->local_datapaths, > + sbrec_port_binding_by_name); > + } > + > + sbrec_mac_binding_index_destroy_row(mb_index_row); > + } > + > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > + > static void * > en_controller_output_init(struct engine_node *node OVS_UNUSED, > struct engine_arg *arg OVS_UNUSED) ... _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
