Skip to content

Commit

Permalink
northd.c: Fix memory leak when falling back to recompute during LSP d…
Browse files Browse the repository at this point in the history 
…eletion.

When multiple LSP deletions are handled in incremental processing, if it
hits a LSP that can't be incrementally processed after incrementally
processing some LSP deletions, it falls back to recompute without
destroying the ovn_port objects that are already handled in the handler,
resulting in memory leaks. See example below, which is detected by the
new test case added by this patch when running with address sanitizer.

=======================

Indirect leak of 936 byte(s) in 3 object(s) allocated from:
    #0 0x55bce7 in calloc (/home/hanzhou/src/ovn/_build_as/northd/ovn-northd+0x55bce7)
    #1 0x773f4e in xcalloc__ /home/hanzhou/src/ovs/_build/../lib/util.c:121:31
    #2 0x773f4e in xzalloc__ /home/hanzhou/src/ovs/_build/../lib/util.c:131:12
    #3 0x773f4e in xzalloc /home/hanzhou/src/ovs/_build/../lib/util.c:165:12
    #4 0x60106c in en_northd_run /home/hanzhou/src/ovn/_build_as/../northd/en-northd.c:137:5
    #5 0x61c6a8 in engine_recompute /home/hanzhou/src/ovn/_build_as/../lib/inc-proc-eng.c:415:5
    #6 0x61bee0 in engine_compute /home/hanzhou/src/ovn/_build_as/../lib/inc-proc-eng.c:454:17
    #7 0x61bee0 in engine_run_node /home/hanzhou/src/ovn/_build_as/../lib/inc-proc-eng.c:503:14
    #8 0x61bee0 in engine_run /home/hanzhou/src/ovn/_build_as/../lib/inc-proc-eng.c:528:9
    #9 0x605e23 in inc_proc_northd_run /home/hanzhou/src/ovn/_build_as/../northd/inc-proc-northd.c:317:9
    #10 0x5fe43b in main /home/hanzhou/src/ovn/_build_as/../northd/ovn-northd.c:934:33
    #11 0x7f473933c1a1 in __libc_start_main (/lib64/libc.so.6+0x281a1)

Indirect leak of 204 byte(s) in 3 object(s) allocated from:
    #0 0x55bea8 in realloc (/home/hanzhou/src/ovn/_build_as/northd/ovn-northd+0x55bea8)
    #1 0x773c7d in xrealloc__ /home/hanzhou/src/ovs/_build/../lib/util.c:147:9
    #2 0x773c7d in xrealloc /home/hanzhou/src/ovs/_build/../lib/util.c:179:12
    #3 0x614bd4 in extract_addresses /home/hanzhou/src/ovn/_build_as/../lib/ovn-util.c:228:12
    #4 0x614bd4 in extract_lsp_addresses /home/hanzhou/src/ovn/_build_as/../lib/ovn-util.c:243:20
    #5 0x5c8d90 in parse_lsp_addrs /home/hanzhou/src/ovn/_build_as/../northd/northd.c:2468:21
    #6 0x5b2ebf in join_logical_ports /home/hanzhou/src/ovn/_build_as/../northd/northd.c:2594:13
    #7 0x5b2ebf in build_ports /home/hanzhou/src/ovn/_build_as/../northd/northd.c:4711:5
    #8 0x5b2ebf in ovnnb_db_run /home/hanzhou/src/ovn/_build_as/../northd/northd.c:17376:5
    #9 0x60106c in en_northd_run /home/hanzhou/src/ovn/_build_as/../northd/en-northd.c:137:5
    #10 0x61c6a8 in engine_recompute /home/hanzhou/src/ovn/_build_as/../lib/inc-proc-eng.c:415:5
    #11 0x61bee0 in engine_compute /home/hanzhou/src/ovn/_build_as/../lib/inc-proc-eng.c:454:17
    #12 0x61bee0 in engine_run_node /home/hanzhou/src/ovn/_build_as/../lib/inc-proc-eng.c:503:14
    #13 0x61bee0 in engine_run /home/hanzhou/src/ovn/_build_as/../lib/inc-proc-eng.c:528:9
    #14 0x605e23 in inc_proc_northd_run /home/hanzhou/src/ovn/_build_as/../northd/inc-proc-northd.c:317:9
    #15 0x5fe43b in main /home/hanzhou/src/ovn/_build_as/../northd/ovn-northd.c:934:33
    #16 0x7f473933c1a1 in __libc_start_main (/lib64/libc.so.6+0x281a1)
...

Fixes: b337750 ("northd: Incremental processing of VIF changes in 'northd' node.")
Reported-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Han Zhou <hzhou@ovn.org>
Acked-by: Dumitru Ceara <dceara@redhat.com>
  • Loading branch information
@hzhou8
hzhou8 committed Jun 30, 2023
1 parent 056f66e commit a595d6d
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 3 deletions.
11 changes: 8 additions & 3 deletions northd/northd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5037,6 +5037,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
{
const struct nbrec_logical_switch *changed_ls;
struct ls_change *ls_change = NULL;
struct ovn_port *op;

NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls,
ni->nbrec_logical_switch_table) {
Expand Down Expand Up @@ -5067,7 +5068,6 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
ovs_list_init(&ls_change->deleted_ports);
ovs_list_init(&ls_change->updated_ports);

struct ovn_port *op;
HMAP_FOR_EACH (op, dp_node, &od->ports) {
op->visited = false;
}
Expand Down Expand Up @@ -5133,12 +5133,12 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) {
if (!op->visited) {
if (!op->lsp_can_be_inc_processed) {
goto fail;
goto fail_clean_deleted;
}
if (sset_contains(&nd->svc_monitor_lsps, op->key)) {
/* This port was used for svc monitor, which may be
* impacted by this deletion. Fallback to recompute. */
goto fail;
goto fail_clean_deleted;
}
ovs_list_push_back(&ls_change->deleted_ports,
&op->list);
Expand All @@ -5165,6 +5165,11 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
}
return true;

fail_clean_deleted:
LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) {
ovn_port_destroy_orphan(op);
}

fail:
free(ls_change);
destroy_northd_data_tracked_changes(nd);
Expand Down
46 changes: 46 additions & 0 deletions tests/ovn-northd.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9559,6 +9559,52 @@ OVN_CLEANUP([hv1])
AT_CLEANUP
])

OVN_FOR_EACH_NORTHD_NO_HV([
AT_SETUP([LSP incremental processing fallback to recompute])
ovn_start

net_add n1
sim_add hv1
as hv1
ovs-vsctl add-br br-phys
ovn_attach n1 br-phys 192.168.0.11

check ovn-nbctl --wait=hv ls-add ls0

# Add ports that should be incrementally processed
n=9
for i in $(seq $n); do
check ovn-nbctl --wait=hv lsp-add ls0 lsp0-$i -- lsp-set-addresses lsp0-$i "aa:aa:aa:00:00:0$i 192.168.0.1$i"
ovs-vsctl add-port br-int lsp0-$i -- set interface lsp0-$i external_ids:iface-id=lsp0-$i
done

# Add a port that should not be incrementally processed (because of the
# "unknown" that is not supported by i-p for now).
check ovn-nbctl --wait=hv lsp-add ls0 lsp0-foo -- lsp-set-addresses lsp0-foo "unknown"
ovs-vsctl add-port br-int lsp0-foo -- set interface lsp0-foo external_ids:iface-id=lsp0-foo

wait_for_ports_up
check ovn-nbctl --wait=hv sync
wait_row_count port_binding $(($n + 1))

# Delete multiple ports, and one of them not incrementally processible. This is
# to trigger partial I-P and then fall back to recompute.
check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
args="--wait=hv lsp-del lsp0-foo"
for i in $(seq $n); do
args="$args -- lsp-del lsp0-$i"
done
check ovn-nbctl $args

wait_row_count Port_Binding 0
northd_recomp=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute)
echo northd_recomp $northd_recomp
AT_CHECK([test $northd_recomp -ge 1])

OVN_CLEANUP([hv1])
AT_CLEANUP
])

OVN_FOR_EACH_NORTHD_NO_HV([
AT_SETUP([check fip and lb flows])
AT_KEYWORDS([fip-lb-flows])
Expand Down

0 comments on commit a595d6d

Please sign in to comment.