On Mon, May 6, 2024 at 6:37 AM Ales Musil <[email protected]> wrote:
>
> 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 <[email protected]>
Thanks. Applied the patch to main and backported until 23.03
Numan
> ---
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
