diff mbox series

[ovs-dev,v6,2/6] northd: Don't detach op->list when it wasn't used.

Message ID 20240412015727.4152034-3-ihrachys@redhat.com
State Superseded, archived
Headers show
Series Correct tunnel ids exhaustion scenario. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Ihar Hrachyshka April 12, 2024, 1:57 a.m. UTC 
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
 #3 0x?? in ls_port_create ovn/northd/northd.c:4342:10
 #4 0x?? in ls_handle_lsp_changes ovn/northd/northd.c:4511:18
 #5 0x?? in northd_handle_ls_changes ovn/northd/northd.c:4655:14
 #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>
---
 northd/northd.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index f2406890c..4cea669cf 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -4111,6 +4111,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. */
@@ -4123,6 +4124,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. */
@@ -4292,7 +4294,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,
@@ -4320,11 +4322,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,
@@ -4345,9 +4342,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;
     }
@@ -4357,7 +4357,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,
@@ -4371,7 +4370,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);
 }
@@ -4546,12 +4545,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);