diff mbox series

[ovs-dev] binding: Cleanup gateway port local binding in runtime data.

Message ID 1605886672-28311-1-git-send-email-dceara@redhat.com
State New
Headers show
Series [ovs-dev] binding: Cleanup gateway port local binding in runtime data. | expand

Commit Message

Dumitru Ceara Nov. 20, 2020, 3:37 p.m. UTC
When a port binding of type "l3gateway" is claimed its remote peer
port_binding is also stored in local_datapath.peer_ports[].remote.

If the remote peer port_binding is deleted first (i.e., before the local
"l3gateway" one) then we need to remove the complete
local_datapath.peer_ports[] entry in order to avoid ending up using
dangling pointers to already freed port bindings.

Also, properly reset local_datapath->has_local_l3gateway in
remove_pb_from_local_datapath().

Ilya reported this issue found by AddressSanitizer during his testing:

==1816017==ERROR: AddressSanitizer: heap-use-after-free on address 0x6140000cb170 at pc 0x0000005ab574 bp 0x7fff68925a30 sp 0x7fff68925a28
READ of size 8 at 0x6140000cb170 thread T0
    #0 0x5ab573 in put_replace_chassis_mac_flows git/ovn/controller/physical.c:550:9
    #1 0x5a65eb in consider_port_binding git/ovn/controller/physical.c:1168:13
    #2 0x5a8764 in physical_run git/ovn/controller/physical.c:1607:9
    #3 0x5a0064 in flow_output_physical_flow_changes_handler git/ovn/controller/ovn-controller.c:2127:9
    #4 0x5db423 in engine_compute git/ovn/lib/inc-proc-eng.c:306:18
    #5 0x5dae1f in engine_run_node git/ovn/lib/inc-proc-eng.c:352:14
    #6 0x5dac74 in engine_run git/ovn/lib/inc-proc-eng.c:377:9
    #7 0x59ad64 in main git/ovn/controller/ovn-controller.c
    #8 0x7f39fa6491a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
    #9 0x480b2d in _start (git/ovn/controller/ovn-controller+0x480b2d)

0x6140000cb170 is located 304 bytes inside of 408-byte region [0x6140000cb040,0x6140000cb1d8)
freed by thread T0 here:
    #0 0x520d07 in free (git/ovn/controller/ovn-controller+0x520d07)
    #1 0x712de7 in ovsdb_idl_db_track_clear git/ovs/lib/ovsdb-idl.c:1984:21
    #2 0x59b5cd in main git/ovn/controller/ovn-controller.c:2762:9
    #3 0x7f39fa6491a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)

Reported-by: Ilya Maximets <i.maximets@ovn.org>
Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS interface in runtime_data.")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 controller/binding.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 5 deletions(-)
diff mbox series

Patch

diff --git a/controller/binding.c b/controller/binding.c
index eb92679..cb60c5d 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1523,21 +1523,41 @@  binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
 }
 
 static const struct sbrec_port_binding *
-get_peer_lport(const struct sbrec_port_binding *pb,
-               struct binding_ctx_in *b_ctx_in)
+get_peer_lport__(const struct sbrec_port_binding *pb,
+                 struct binding_ctx_in *b_ctx_in)
 {
     const char *peer_name = smap_get(&pb->options, "peer");
-    if (strcmp(pb->type, "patch") || !peer_name) {
+
+    if (!peer_name) {
         return NULL;
     }
 
     const struct sbrec_port_binding *peer;
     peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
                                 peer_name);
-
     return (peer && peer->datapath) ? peer : NULL;
 }
 
+static const struct sbrec_port_binding *
+get_l3gw_peer_lport(const struct sbrec_port_binding *pb,
+                    struct binding_ctx_in *b_ctx_in)
+{
+    if (strcmp(pb->type, "l3gateway")) {
+        return NULL;
+    }
+    return get_peer_lport__(pb, b_ctx_in);
+}
+
+static const struct sbrec_port_binding *
+get_peer_lport(const struct sbrec_port_binding *pb,
+               struct binding_ctx_in *b_ctx_in)
+{
+    if (strcmp(pb->type, "patch")) {
+        return NULL;
+    }
+    return get_peer_lport__(pb, b_ctx_in);
+}
+
 /* This function adds the local datapath of the 'peer' of
  * lport 'pb' to the local datapaths if it is not yet added.
  */
@@ -1654,7 +1674,9 @@  remove_pb_from_local_datapath(const struct sbrec_port_binding *pb,
                                          pb->logical_port)) {
             ld->localnet_port = NULL;
         }
-    } else if (!strcmp(pb->type, "l3gateway")) {
+    }
+
+    if (!strcmp(pb->type, "l3gateway")) {
         const char *chassis_id = smap_get(&pb->options,
                                           "l3gateway-chassis");
         if (chassis_id && !strcmp(chassis_id, chassis_rec->name)) {
@@ -1956,12 +1978,27 @@  handle_deleted_lport(const struct sbrec_port_binding *pb,
                      struct binding_ctx_in *b_ctx_in,
                      struct binding_ctx_out *b_ctx_out)
 {
+    /* If the binding is local, remove it. */
     struct local_datapath *ld =
         get_local_datapath(b_ctx_out->local_datapaths,
                            pb->datapath->tunnel_key);
     if (ld) {
         remove_pb_from_local_datapath(pb, b_ctx_in->chassis_rec,
                                       b_ctx_out, ld);
+        return;
+    }
+
+    /* If the binding is not local, if 'pb' is a L3 gateway port, we should
+     * remove its peer, if that one is local.
+     */
+    pb = get_l3gw_peer_lport(pb, b_ctx_in);
+    if (pb) {
+        ld = get_local_datapath(b_ctx_out->local_datapaths,
+                                pb->datapath->tunnel_key);
+        if (ld) {
+            remove_pb_from_local_datapath(pb, b_ctx_in->chassis_rec, b_ctx_out,
+                                          ld);
+        }
     }
 }