If a logical switch port is added and connected to a logical router
port (through options: router-port) before the router port is
created, then this might cause further issues such as segmentation
violation when the switch and router ports are deleted.

Signed-off-by: Xavier Simonart <[email protected]>

---
v2: handled Han's comments (avoid wasting CPU cycles searching for peer_ld)
---
 controller/local_data.c | 36 ++++++++++++++----------------------
 tests/ovn.at            | 30 ++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/controller/local_data.c b/controller/local_data.c
index 7f874fc19..669e686ab 100644
--- a/controller/local_data.c
+++ b/controller/local_data.c
@@ -34,7 +34,7 @@
 
 VLOG_DEFINE_THIS_MODULE(ldata);
 
-static void add_local_datapath__(
+static struct local_datapath *add_local_datapath__(
     struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
     struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
     struct ovsdb_idl_index *sbrec_port_binding_by_name,
@@ -194,17 +194,7 @@ add_local_datapath_peer_port(
         return;
     }
 
-    bool present = false;
-    for (size_t i = 0; i < ld->n_peer_ports; i++) {
-        if (ld->peer_ports[i].local == pb) {
-            present = true;
-            break;
-        }
-    }
-
-    if (!present) {
-        local_datapath_peer_port_add(ld, pb, peer);
-    }
+    local_datapath_peer_port_add(ld, pb, peer);
 
     struct local_datapath *peer_ld =
         get_local_datapath(local_datapaths,
@@ -218,12 +208,6 @@ add_local_datapath_peer_port(
         return;
     }
 
-    for (size_t i = 0; i < peer_ld->n_peer_ports; i++) {
-        if (peer_ld->peer_ports[i].local == peer) {
-            return;
-        }
-    }
-
     local_datapath_peer_port_add(peer_ld, peer, pb);
 }
 
@@ -541,7 +525,7 @@ chassis_tunnel_find(const struct hmap *chassis_tunnels, 
const char *chassis_id,
 }
 
 /* static functions. */
-static void
+static struct local_datapath *
 add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
                      struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
                      struct ovsdb_idl_index *sbrec_port_binding_by_name,
@@ -553,7 +537,7 @@ add_local_datapath__(struct ovsdb_idl_index 
*sbrec_datapath_binding_by_key,
     uint32_t dp_key = dp->tunnel_key;
     struct local_datapath *ld = get_local_datapath(local_datapaths, dp_key);
     if (ld) {
-        return;
+        return ld;
     }
 
     ld = local_datapath_alloc(dp);
@@ -568,7 +552,7 @@ add_local_datapath__(struct ovsdb_idl_index 
*sbrec_datapath_binding_by_key,
     if (depth >= 100) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
         VLOG_WARN_RL(&rl, "datapaths nested too deep");
-        return;
+        return ld;
     }
 
     struct sbrec_port_binding *target =
@@ -589,12 +573,14 @@ add_local_datapath__(struct ovsdb_idl_index 
*sbrec_datapath_binding_by_key,
                 if (peer && peer->datapath) {
                     if (need_add_patch_peer_to_local(
                             sbrec_port_binding_by_name, pb, chassis)) {
-                        add_local_datapath__(sbrec_datapath_binding_by_key,
+                        struct local_datapath *peer_ld =
+                            add_local_datapath__(sbrec_datapath_binding_by_key,
                                              sbrec_port_binding_by_datapath,
                                              sbrec_port_binding_by_name,
                                              depth + 1, peer->datapath,
                                              chassis, local_datapaths,
                                              tracked_datapaths);
+                        local_datapath_peer_port_add(peer_ld, peer, pb);
                     }
                     local_datapath_peer_port_add(ld, pb, peer);
                 }
@@ -602,6 +588,7 @@ add_local_datapath__(struct ovsdb_idl_index 
*sbrec_datapath_binding_by_key,
         }
     }
     sbrec_port_binding_index_destroy_row(target);
+    return ld;
 }
 
 static struct tracked_datapath *
@@ -622,6 +609,11 @@ local_datapath_peer_port_add(struct local_datapath *ld,
                              const struct sbrec_port_binding *local,
                              const struct sbrec_port_binding *remote)
 {
+    for (size_t i = 0; i < ld->n_peer_ports; i++) {
+        if (ld->peer_ports[i].local == local) {
+            return;
+        }
+    }
     ld->n_peer_ports++;
     if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
         size_t old_n_ports = ld->n_allocated_peer_ports;
diff --git a/tests/ovn.at b/tests/ovn.at
index bba2c9c1d..ae0918d55 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -32580,3 +32580,33 @@ check ovn-nbctl --wait=hv sync
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([router port add then remove - lsp first])
+ovn_start
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+    set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
+    options:tx_pcap=hv1/vif1-tx.pcap \
+    options:rxq_pcap=hv1/vif1-rx.pcap \
+    ofport-request=1
+
+check ovn-nbctl ls-add sw0
+check ovn-nbctl lr-add ro0
+check ovn-nbctl lsp-add sw0 sw0-p1
+check ovn-nbctl lsp-add sw0 lsp
+check ovn-nbctl lsp-set-type lsp router
+check ovn-nbctl lsp-set-options lsp router-port=lrp
+check ovn-nbctl lsp-set-addresses lsp  00:00:00:00:00:1
+check ovn-nbctl --wait=hv lrp-add ro0 lrp 00:00:00:00:00:1 
aef0:0:0:0:0:0:0:1/64
+check ovn-nbctl --wait=hv lsp-del lsp
+check ovn-nbctl lrp-del lrp
+check ovn-nbctl --wait=hv sync
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
-- 
2.31.1

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to