Avoid use after free in scenario when controller received LB deletion
after the DB was reconnected. The reconnect led to idl clearing up
the "old" structs, one of them being the LB. However, during recompute
the struct was referenced when it was already gone.
Clear the whole objdep_mgr instead of going one-by-one during recompute.
==143949==ERROR: AddressSanitizer: heap-use-after-free
READ of size 4 at 0x5130000280d0 thread T0
0 0x61c3c9 in lb_data_local_lb_remove controller/ovn-controller.c:2978:5
1 0x5fd4df in en_lb_data_run controller/ovn-controller.c:3063:9
2 0x6fe0d9 in engine_recompute lib/inc-proc-eng.c:415:5
3 0x6fbdc2 in engine_run_node lib/inc-proc-eng.c:477:9
4 0x6fbdc2 in engine_run lib/inc-proc-eng.c:528:9
5 0x5f39a0 in main controller/ovn-controller.c
Fixes: 8382127186bf ("controller: Store load balancer data in separate node")
Reported-at: https://issues.redhat.com/browse/FDP-610
Signed-off-by: Ales Musil <amu...@redhat.com>
---
controller/ovn-controller.c | 20 +++++++++----------
tests/ovn-controller.at | 38 +++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 10 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 23269af83..65b9ba8e5 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2972,7 +2972,7 @@ lb_data_local_lb_add(struct ed_type_lb_data *lb_data,
static void
lb_data_local_lb_remove(struct ed_type_lb_data *lb_data,
- struct ovn_controller_lb *lb, bool tracked)
+ struct ovn_controller_lb *lb)
{
const struct uuid *uuid = &lb->slb->header_.uuid;
@@ -2981,12 +2981,8 @@ lb_data_local_lb_remove(struct ed_type_lb_data *lb_data,
lb_data_removed_five_tuples_add(lb_data, lb);
- if (tracked) {
- hmap_insert(&lb_data->old_lbs, &lb->hmap_node, uuid_hash(uuid));
- uuidset_insert(&lb_data->deleted, uuid);
- } else {
- ovn_controller_lb_destroy(lb);
- }
+ hmap_insert(&lb_data->old_lbs, &lb->hmap_node, uuid_hash(uuid));
+ uuidset_insert(&lb_data->deleted, uuid);
}
static bool
@@ -3011,7 +3007,7 @@ lb_data_handle_changed_ref(enum objdep_type type, const
char *res_name,
continue;
}
- lb_data_local_lb_remove(lb_data, lb, true);
+ lb_data_local_lb_remove(lb_data, lb);
const struct sbrec_load_balancer *sbrec_lb =
sbrec_load_balancer_table_get_for_uuid(ctx_in->lb_table, uuid);
@@ -3057,9 +3053,13 @@ en_lb_data_run(struct engine_node *node, void *data)
const struct sbrec_load_balancer_table *lb_table =
EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
+ objdep_mgr_clear(&lb_data->deps_mgr);
+
struct ovn_controller_lb *lb;
HMAP_FOR_EACH_SAFE (lb, hmap_node, &lb_data->local_lbs) {
- lb_data_local_lb_remove(lb_data, lb, false);
+ hmap_remove(&lb_data->local_lbs, &lb->hmap_node);
+ lb_data_removed_five_tuples_add(lb_data, lb);
+ ovn_controller_lb_destroy(lb);
}
const struct sbrec_load_balancer *sbrec_lb;
@@ -3097,7 +3097,7 @@ lb_data_sb_load_balancer_handler(struct engine_node
*node, void *data)
continue;
}
- lb_data_local_lb_remove(lb_data, lb, true);
+ lb_data_local_lb_remove(lb_data, lb);
}
if (sbrec_load_balancer_is_deleted(sbrec_lb) ||
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 27cec2aec..cecbc190b 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -2973,3 +2973,41 @@ priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.4
actions=load:0x1->OXM_OF
OVN_CLEANUP([hv1])
AT_CLEANUP
+
+AT_SETUP([ovn-controller - LB remove after disconnect])
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+check ovs-vsctl -- add-port br-int vif1 -- \
+ set interface vif1 external-ids:iface-id=lsp
+
+check ovs-vsctl set Open_vSwitch .
external-ids:ovn-remote-probe-interval="5000"
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl lsp-add ls lsp \
+-- lsp-set-addresses lsp "f0:00:00:00:00:01 172.16.0.10"
+
+check ovn-nbctl lb-add lb 192.168.100.100 172.16.0.10
+check ovn-nbctl ls-lb-add ls lb
+
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+sleep_sb
+OVS_WAIT_UNTIL([grep -q 'OVNSB commit failed' hv1/ovn-controller.log])
+
+sleep_controller hv1
+wake_up_sb
+
+ovn-nbctl lb-del lb
+
+wake_up_controller hv1
+check ovn-nbctl --wait=hv sync
+
+OVN_CLEANUP([hv1
+/no response to inactivity probe after .* seconds, disconnecting/d])
+AT_CLEANUP
--
2.44.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
