Skip to content

Commit

Permalink
northd: Don't detach op->list when it wasn't used.
Browse files Browse the repository at this point in the history
In some scenarios, op->list is not attached anywhere, which makes
attempts to detach it trigger ubsan failure.

ovn/ovs/include/openvswitch/list.h:252:17: runtime error: member access
within null pointer of type 'struct ovs_list'

 #0 0x?? in ovs_list_remove ovn/ovs/include/openvswitch/list.h:252:17
 #1 0x?? in ovn_port_allocate_key ovn/northd/northd.c:4021:13
 #2 0x?? in ls_port_init ovn/northd/northd.c:4321:10
 ovn-org#3 0x?? in ls_port_create ovn/northd/northd.c:4342:10
 ovn-org#4 0x?? in ls_handle_lsp_changes ovn/northd/northd.c:4511:18
 ovn-org#5 0x?? in northd_handle_ls_changes ovn/northd/northd.c:4655:14
 ovn-org#6 0x?? in northd_nb_logical_switch_handler ovn/northd/en-northd.c:150:

This patch makes northd use op->list only as a temporary means for
build_ports logic to track ports that are persisted in both, nb, or sb
only. Now build_ports will always detach ops once done.

Now that op->list is never left attached to a list, we can remove
ovs_list_remove calls for it elsewhere, including where op was never
attached in the first place.

Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
Acked-by: Mark Michelson <mmichels@redhat.com>
  • Loading branch information
booxter authored and numansiddique committed Apr 30, 2024
1 parent 86bf082 commit db4ea9d
Showing 1 changed file with 13 additions and 10 deletions.
23 changes: 13 additions & 10 deletions northd/northd.c
Original file line number Diff line number Diff line change
Expand Up @@ -4117,6 +4117,7 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
sbrec_mirror_table,
op, queue_id_bitmap,
&active_ha_chassis_grps);
ovs_list_remove(&op->list);
}

/* Add southbound record for each unmatched northbound record. */
Expand All @@ -4129,6 +4130,7 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
op, queue_id_bitmap,
&active_ha_chassis_grps);
sbrec_port_binding_set_logical_port(op->sb, op->key);
ovs_list_remove(&op->list);
}

/* Delete southbound records without northbound matches. */
Expand Down Expand Up @@ -4298,7 +4300,7 @@ ovn_port_find_in_datapath(struct ovn_datapath *od,

static bool
ls_port_init(struct ovn_port *op, struct ovsdb_idl_txn *ovnsb_txn,
struct hmap *ls_ports, struct ovn_datapath *od,
struct ovn_datapath *od,
const struct sbrec_port_binding *sb,
const struct sbrec_mirror_table *sbrec_mirror_table,
const struct sbrec_chassis_table *sbrec_chassis_table,
Expand Down Expand Up @@ -4326,11 +4328,6 @@ ls_port_init(struct ovn_port *op, struct ovsdb_idl_txn *ovnsb_txn,
}
/* Assign new tunnel ids where needed. */
if (!ovn_port_allocate_key(sbrec_chassis_table, op)) {
if (op->sb) {
sbrec_port_binding_delete(op->sb);
}
ovs_list_remove(&op->list);
ovn_port_destroy(ls_ports, op);
return false;
}
ovn_port_update_sbrec(ovnsb_txn, sbrec_chassis_by_name,
Expand All @@ -4351,9 +4348,12 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,
struct ovn_port *op = ovn_port_create(ls_ports, key, nbsp, NULL,
NULL);
hmap_insert(&od->ports, &op->dp_node, hmap_node_hash(&op->key_node));
if (!ls_port_init(op, ovnsb_txn, ls_ports, od, sb,
if (!ls_port_init(op, ovnsb_txn, od, sb,
sbrec_mirror_table, sbrec_chassis_table,
sbrec_chassis_by_name, sbrec_chassis_by_hostname)) {
if (op->sb) {
sbrec_port_binding_delete(op->sb);
}
ovn_port_destroy(ls_ports, op);
return NULL;
}
Expand All @@ -4363,7 +4363,6 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports,

static bool
ls_port_reinit(struct ovn_port *op, struct ovsdb_idl_txn *ovnsb_txn,
struct hmap *ls_ports,
const struct nbrec_logical_switch_port *nbsp,
const struct nbrec_logical_router_port *nbrp,
struct ovn_datapath *od,
Expand All @@ -4377,7 +4376,7 @@ ls_port_reinit(struct ovn_port *op, struct ovsdb_idl_txn *ovnsb_txn,
op->sb = sb;
ovn_port_set_nb(op, nbsp, nbrp);
op->l3dgw_port = op->cr_port = NULL;
return ls_port_init(op, ovnsb_txn, ls_ports, od, sb,
return ls_port_init(op, ovnsb_txn, od, sb,
sbrec_mirror_table, sbrec_chassis_table,
sbrec_chassis_by_name, sbrec_chassis_by_hostname);
}
Expand Down Expand Up @@ -4552,12 +4551,16 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
op->visited = true;
continue;
}
if (!ls_port_reinit(op, ovnsb_idl_txn, &nd->ls_ports,
if (!ls_port_reinit(op, ovnsb_idl_txn,
new_nbsp, NULL,
od, sb, ni->sbrec_mirror_table,
ni->sbrec_chassis_table,
ni->sbrec_chassis_by_name,
ni->sbrec_chassis_by_hostname)) {
if (op->sb) {
sbrec_port_binding_delete(op->sb);
}
ovn_port_destroy(&nd->ls_ports, op);
goto fail;
}
add_op_to_northd_tracked_ports(&trk_lsps->updated, op);
Expand Down

0 comments on commit db4ea9d

Please sign in to comment.