From 0440a083d2bb024fe3a8263f72c7c749cc9fae5a Mon Sep 17 00:00:00 2001 From: Ales Musil Date: Mon, 6 May 2024 12:37:20 +0200 Subject: [PATCH] controller: Avoid use after free in LB I-P. 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 Signed-off-by: Numan Siddique (cherry picked from commit e5d093cc243e1ad478eaeddad445523a56f2f676) --- 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 113d3e05c6..ad0d79814b 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -2962,7 +2962,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; @@ -2971,12 +2971,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 @@ -3001,7 +2997,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); @@ -3047,9 +3043,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; @@ -3087,7 +3087,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 10aff7b671..cfdc7b6e21 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -2907,3 +2907,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