pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/40264?usp=email )
Change subject: Convert struct hnbgw_cnpool to be a talloc context ......................................................................
Convert struct hnbgw_cnpool to be a talloc context
We currently have some memory corruption somewhere in osmo-hnbgw in production which we need to figure out. The previous talloc context hierarchy was really messy, and it was probably a left over of previous states of the code where we didn't have support for several sccp clients, etc. This patch is a step towards clarifying that hierarchy.
Change-Id: I62a511e3ea0254d0327f2664d504b9195416bc76 --- M include/osmocom/hnbgw/hnbgw.h M include/osmocom/hnbgw/hnbgw_cn.h M src/osmo-hnbgw/hnbgw.c M src/osmo-hnbgw/hnbgw_cn.c M src/osmo-hnbgw/hnbgw_sccp.c M src/osmo-hnbgw/hnbgw_vty.c M src/osmo-hnbgw/osmo_hnbgw_main.c 7 files changed, 81 insertions(+), 68 deletions(-)
Approvals: Jenkins Builder: Verified osmith: Looks good to me, approved
diff --git a/include/osmocom/hnbgw/hnbgw.h b/include/osmocom/hnbgw/hnbgw.h index eb9c61c..685bc12 100644 --- a/include/osmocom/hnbgw/hnbgw.h +++ b/include/osmocom/hnbgw/hnbgw.h @@ -133,9 +133,9 @@ struct llist_head users;
/* Pool of core network peers: MSCs for IuCS */ - struct hnbgw_cnpool cnpool_iucs; + struct hnbgw_cnpool *cnpool_iucs; /* Pool of core network peers: SGSNs for IuPS */ - struct hnbgw_cnpool cnpool_iups; + struct hnbgw_cnpool *cnpool_iups; } sccp; /* MGW pool, also includes the single MGCP client as fallback if no * pool is configured. */ diff --git a/include/osmocom/hnbgw/hnbgw_cn.h b/include/osmocom/hnbgw/hnbgw_cn.h index 5ca5675..fe4f7bb 100644 --- a/include/osmocom/hnbgw/hnbgw_cn.h +++ b/include/osmocom/hnbgw/hnbgw_cn.h @@ -56,11 +56,7 @@ struct rate_ctr_group *ctrs; };
-extern const struct rate_ctr_group_desc iucs_ctrg_desc; -extern const struct rate_ctr_group_desc iups_ctrg_desc; - -extern const struct rate_ctr_group_desc msc_ctrg_desc; -extern const struct rate_ctr_group_desc sgsn_ctrg_desc; +struct hnbgw_cnpool *hnbgw_cnpool_alloc(RANAP_CN_DomainIndicator_t domain);
struct hnbgw_cnlink *hnbgw_cnlink_select(struct hnbgw_context_map *map);
diff --git a/src/osmo-hnbgw/hnbgw.c b/src/osmo-hnbgw/hnbgw.c index c46ddad..d237928 100644 --- a/src/osmo-hnbgw/hnbgw.c +++ b/src/osmo-hnbgw/hnbgw.c @@ -235,35 +235,8 @@ g_hnbgw->config.pfcp.remote_port = OSMO_PFCP_PORT; #endif
- g_hnbgw->sccp.cnpool_iucs = (struct hnbgw_cnpool){ - .domain = DOMAIN_CS, - .pool_name = "iucs", - .peer_name = "msc", - .default_remote_pc = DEFAULT_PC_MSC, - .vty = { - .nri_bitlen = OSMO_NRI_BITLEN_DEFAULT, - .null_nri_ranges = osmo_nri_ranges_alloc(g_hnbgw), - }, - .cnlink_ctrg_desc = &msc_ctrg_desc, - - .ctrs = rate_ctr_group_alloc(g_hnbgw, &iucs_ctrg_desc, 0), - }; - INIT_LLIST_HEAD(&g_hnbgw->sccp.cnpool_iucs.cnlinks); - - g_hnbgw->sccp.cnpool_iups = (struct hnbgw_cnpool){ - .domain = DOMAIN_PS, - .pool_name = "iups", - .peer_name = "sgsn", - .default_remote_pc = DEFAULT_PC_SGSN, - .vty = { - .nri_bitlen = OSMO_NRI_BITLEN_DEFAULT, - .null_nri_ranges = osmo_nri_ranges_alloc(g_hnbgw), - }, - .cnlink_ctrg_desc = &sgsn_ctrg_desc, - - .ctrs = rate_ctr_group_alloc(g_hnbgw, &iups_ctrg_desc, 0), - }; - INIT_LLIST_HEAD(&g_hnbgw->sccp.cnpool_iups.cnlinks); + g_hnbgw->sccp.cnpool_iucs = hnbgw_cnpool_alloc(DOMAIN_CS); + g_hnbgw->sccp.cnpool_iups = hnbgw_cnpool_alloc(DOMAIN_PS);
osmo_timer_setup(&g_hnbgw->store_uptime_timer, hnbgw_store_hnb_uptime, g_hnbgw); osmo_timer_schedule(&g_hnbgw->store_uptime_timer, STORE_UPTIME_INTERVAL, 0); diff --git a/src/osmo-hnbgw/hnbgw_cn.c b/src/osmo-hnbgw/hnbgw_cn.c index e03f3c7..4420157 100644 --- a/src/osmo-hnbgw/hnbgw_cn.c +++ b/src/osmo-hnbgw/hnbgw_cn.c @@ -42,10 +42,6 @@ #include <osmocom/hnbgw/hnbgw_cn.h> #include <osmocom/hnbgw/context_map.h>
-/*********************************************************************** - * Incoming primitives from SCCP User SAP - ***********************************************************************/ - static bool addr_has_pc_and_ssn(const struct osmo_sccp_addr *addr) { if (!(addr->presence & OSMO_SCCP_ADDR_T_SSN)) @@ -94,7 +90,7 @@ cnpool->use.nri_bitlen = cnpool->vty.nri_bitlen;
osmo_nri_ranges_free(cnpool->use.null_nri_ranges); - cnpool->use.null_nri_ranges = osmo_nri_ranges_alloc(g_hnbgw); + cnpool->use.null_nri_ranges = osmo_nri_ranges_alloc(cnpool); llist_for_each_entry(r, &cnpool->vty.null_nri_ranges->entries, entry) osmo_nri_ranges_add(cnpool->use.null_nri_ranges, r); } @@ -269,7 +265,7 @@ */ struct hnbgw_cnlink *hnbgw_cnlink_select(struct hnbgw_context_map *map) { - struct hnbgw_cnpool *cnpool = map->is_ps ? &g_hnbgw->sccp.cnpool_iups : &g_hnbgw->sccp.cnpool_iucs; + struct hnbgw_cnpool *cnpool = map->is_ps ? g_hnbgw->sccp.cnpool_iups : g_hnbgw->sccp.cnpool_iucs; struct hnbgw_cnlink *cnlink; struct hnbgw_cnlink *round_robin_next = NULL; struct hnbgw_cnlink *round_robin_first = NULL; @@ -578,3 +574,51 @@ ARRAY_SIZE(cnpool_ctr_description), cnpool_ctr_description, }; + +static int hnbgw_cnpool_talloc_destructor(struct hnbgw_cnpool *cnpool) +{ + struct hnbgw_cnlink *cnlink; + osmo_nri_ranges_free(cnpool->vty.null_nri_ranges); + cnpool->vty.null_nri_ranges = NULL; + + while ((cnlink = llist_first_entry_or_null(&cnpool->cnlinks, struct hnbgw_cnlink, entry))) + hnbgw_cnlink_term_and_free(cnlink); + return 0; +} + +struct hnbgw_cnpool *hnbgw_cnpool_alloc(RANAP_CN_DomainIndicator_t domain) +{ + struct hnbgw_cnpool *cnpool = talloc_zero(g_hnbgw, struct hnbgw_cnpool); + OSMO_ASSERT(cnpool); + + cnpool->domain = domain; + cnpool->vty = (struct hnbgw_cnpool_cfg){ + .nri_bitlen = OSMO_NRI_BITLEN_DEFAULT, + .null_nri_ranges = osmo_nri_ranges_alloc(cnpool), + }; + OSMO_ASSERT(cnpool->vty.null_nri_ranges); + INIT_LLIST_HEAD(&cnpool->cnlinks); + + talloc_set_destructor(cnpool, hnbgw_cnpool_talloc_destructor); + + switch (domain) { + case DOMAIN_CS: + cnpool->pool_name = "iucs"; + cnpool->peer_name = "msc"; + cnpool->default_remote_pc = DEFAULT_PC_MSC; + cnpool->cnlink_ctrg_desc = &msc_ctrg_desc; + cnpool->ctrs = rate_ctr_group_alloc(cnpool, &iucs_ctrg_desc, 0); + break; + case DOMAIN_PS: + cnpool->pool_name = "iups"; + cnpool->peer_name = "sgsn"; + cnpool->default_remote_pc = DEFAULT_PC_SGSN; + cnpool->cnlink_ctrg_desc = &sgsn_ctrg_desc; + cnpool->ctrs = rate_ctr_group_alloc(cnpool, &iups_ctrg_desc, 0); + break; + default: + OSMO_ASSERT(0); + } + + return cnpool; +} diff --git a/src/osmo-hnbgw/hnbgw_sccp.c b/src/osmo-hnbgw/hnbgw_sccp.c index 434c58e..ed58b78 100644 --- a/src/osmo-hnbgw/hnbgw_sccp.c +++ b/src/osmo-hnbgw/hnbgw_sccp.c @@ -54,11 +54,11 @@ const struct osmo_sccp_addr *remote_addr) { struct hnbgw_cnlink *cnlink; - llist_for_each_entry(cnlink, &g_hnbgw->sccp.cnpool_iucs.cnlinks, entry) { + llist_for_each_entry(cnlink, &g_hnbgw->sccp.cnpool_iucs->cnlinks, entry) { if (cnlink_matches(cnlink, hsu, remote_addr)) return cnlink; } - llist_for_each_entry(cnlink, &g_hnbgw->sccp.cnpool_iups.cnlinks, entry) { + llist_for_each_entry(cnlink, &g_hnbgw->sccp.cnpool_iups->cnlinks, entry) { if (cnlink_matches(cnlink, hsu, remote_addr)) return cnlink; } @@ -188,9 +188,9 @@ static struct hnbgw_cnlink *cnlink_find_by_remote_pc(struct osmo_ss7_instance *cs7, uint32_t pc) { struct hnbgw_cnlink *cnlink; - cnlink = _cnlink_find_by_remote_pc(&g_hnbgw->sccp.cnpool_iucs, cs7, pc); + cnlink = _cnlink_find_by_remote_pc(g_hnbgw->sccp.cnpool_iucs, cs7, pc); if (!cnlink) - cnlink = _cnlink_find_by_remote_pc(&g_hnbgw->sccp.cnpool_iups, cs7, pc); + cnlink = _cnlink_find_by_remote_pc(g_hnbgw->sccp.cnpool_iups, cs7, pc); return cnlink; }
diff --git a/src/osmo-hnbgw/hnbgw_vty.c b/src/osmo-hnbgw/hnbgw_vty.c index bfe0c73..52c5ba6 100644 --- a/src/osmo-hnbgw/hnbgw_vty.c +++ b/src/osmo-hnbgw/hnbgw_vty.c @@ -86,7 +86,7 @@ "iucs", "Configure IuCS options") { vty->node = IUCS_NODE; - vty->index = &g_hnbgw->sccp.cnpool_iucs; + vty->index = g_hnbgw->sccp.cnpool_iucs; return CMD_SUCCESS; }
@@ -100,7 +100,7 @@ "iups", "Configure IuPS options") { vty->node = IUPS_NODE; - vty->index = &g_hnbgw->sccp.cnpool_iups; + vty->index = g_hnbgw->sccp.cnpool_iups; return CMD_SUCCESS; }
@@ -167,10 +167,10 @@ { struct hnbgw_cnlink *cnlink; vty_out(vty, "IuCS: "); - llist_for_each_entry(cnlink, &g_hnbgw->sccp.cnpool_iucs.cnlinks, entry) + llist_for_each_entry(cnlink, &g_hnbgw->sccp.cnpool_iucs->cnlinks, entry) _show_cnlink(vty, cnlink); vty_out(vty, "IuPS: "); - llist_for_each_entry(cnlink, &g_hnbgw->sccp.cnpool_iups.cnlinks, entry) + llist_for_each_entry(cnlink, &g_hnbgw->sccp.cnpool_iups->cnlinks, entry) _show_cnlink(vty, cnlink); return CMD_SUCCESS; } @@ -511,7 +511,7 @@ "Configure an IuCS link to an MSC\n" "MSC nr\n") { - return cnlink_nr(vty, &g_hnbgw->sccp.cnpool_iucs, argc, argv); + return cnlink_nr(vty, g_hnbgw->sccp.cnpool_iucs, argc, argv); }
/* 'sgsn 0' */ @@ -520,7 +520,7 @@ "Configure an IuPS link to an SGSN\n" "SGSN nr\n") { - return cnlink_nr(vty, &g_hnbgw->sccp.cnpool_iups, argc, argv); + return cnlink_nr(vty, g_hnbgw->sccp.cnpool_iups, argc, argv); }
/* 'msc 0' / 'remote-addr my-msc' and @@ -716,14 +716,14 @@ * nri null add ... */ vty_out(vty, "hnbgw%s", VTY_NEWLINE); - cnpool_write_nri(vty, &g_hnbgw->sccp.cnpool_iucs, true); - cnpool_write_nri(vty, &g_hnbgw->sccp.cnpool_iups, true); + cnpool_write_nri(vty, g_hnbgw->sccp.cnpool_iucs, true); + cnpool_write_nri(vty, g_hnbgw->sccp.cnpool_iups, true);
/* msc 0 * nri add ... */ - cnlinks_write_nri(vty, &g_hnbgw->sccp.cnpool_iucs, true); - cnlinks_write_nri(vty, &g_hnbgw->sccp.cnpool_iups, true); + cnlinks_write_nri(vty, g_hnbgw->sccp.cnpool_iucs, true); + cnlinks_write_nri(vty, g_hnbgw->sccp.cnpool_iups, true); return CMD_SUCCESS; }
@@ -739,9 +739,9 @@ { struct hnbgw_cnpool *cnpool; if (!strcmp("msc", argv[0])) - cnpool = &g_hnbgw->sccp.cnpool_iucs; + cnpool = g_hnbgw->sccp.cnpool_iucs; else - cnpool = &g_hnbgw->sccp.cnpool_iups; + cnpool = g_hnbgw->sccp.cnpool_iups; cnpool->round_robin_next_nr = atoi(argv[1]); return CMD_SUCCESS; } @@ -760,9 +760,9 @@ int nr = atoi(argv[1]);
if (!strcmp("msc", msc_sgsn)) - cnpool = &g_hnbgw->sccp.cnpool_iucs; + cnpool = g_hnbgw->sccp.cnpool_iucs; else - cnpool = &g_hnbgw->sccp.cnpool_iups; + cnpool = g_hnbgw->sccp.cnpool_iups;
cnlink = cnlink_get_nr(cnpool, nr, false); if (!cnlink) { @@ -801,11 +801,11 @@ { struct hnbgw_cnpool *cnpool;
- cnpool = &g_hnbgw->sccp.cnpool_iucs; + cnpool = g_hnbgw->sccp.cnpool_iucs; hnbgw_cnpool_apply_cfg(cnpool); hnbgw_cnpool_cnlinks_start_or_restart(cnpool);
- cnpool = &g_hnbgw->sccp.cnpool_iups; + cnpool = g_hnbgw->sccp.cnpool_iups; hnbgw_cnpool_apply_cfg(cnpool); hnbgw_cnpool_cnlinks_start_or_restart(cnpool);
@@ -1014,8 +1014,8 @@ llist_for_each_entry(hnbp, &g_hnbgw->hnb_persistent_list, list) write_one_hnbp(vty, hnbp);
- _config_write_cnpool(vty, &g_hnbgw->sccp.cnpool_iucs); - _config_write_cnpool(vty, &g_hnbgw->sccp.cnpool_iups); + _config_write_cnpool(vty, g_hnbgw->sccp.cnpool_iucs); + _config_write_cnpool(vty, g_hnbgw->sccp.cnpool_iups);
if (g_hnbgw->config.nft_kpi.enable) vty_out(vty, " nft-kpi%s%s%s", @@ -1065,13 +1065,13 @@
static int config_write_msc(struct vty *vty) { - _config_write_cnlink(vty, &g_hnbgw->sccp.cnpool_iucs); + _config_write_cnlink(vty, g_hnbgw->sccp.cnpool_iucs); return CMD_SUCCESS; }
static int config_write_sgsn(struct vty *vty) { - _config_write_cnlink(vty, &g_hnbgw->sccp.cnpool_iups); + _config_write_cnlink(vty, g_hnbgw->sccp.cnpool_iups); return CMD_SUCCESS; }
diff --git a/src/osmo-hnbgw/osmo_hnbgw_main.c b/src/osmo-hnbgw/osmo_hnbgw_main.c index 845002f..0f932d9 100644 --- a/src/osmo-hnbgw/osmo_hnbgw_main.c +++ b/src/osmo-hnbgw/osmo_hnbgw_main.c @@ -345,8 +345,8 @@ #endif }
- hnbgw_cnpool_start(&g_hnbgw->sccp.cnpool_iucs); - hnbgw_cnpool_start(&g_hnbgw->sccp.cnpool_iups); + hnbgw_cnpool_start(g_hnbgw->sccp.cnpool_iucs); + hnbgw_cnpool_start(g_hnbgw->sccp.cnpool_iups);
if (hnbgw_cmdline_config.daemonize) { rc = osmo_daemonize();
-
pespin