fixeria has submitted this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/28862 )

Change subject: trxcon: rework trxcon_inst cleanup logic, add trxcon_fsm_pre_term_cb() ......................................................................

trxcon: rework trxcon_inst cleanup logic, add trxcon_fsm_pre_term_cb()

* trxcon_inst_alloc(): allocate trxcon_fsm as a child of the given ctx. * trxcon_inst_alloc(): allocate trxcon_inst as a child of trxcon_fsm. * trxcon_inst_free(): move the cleanup logic to .pre_term() callback of the trxcon_fsm, represented by new trxcon_fsm_pre_term_cb(). * trxcon_allstate_action() properly handle TRXCON_EV_{PHYIF,L2IF}_FAILURE.

Change-Id: I5eb8ef6f62b1dc949dc60eaa558f123b3b93819c Related: OS#5599 --- M src/host/trxcon/src/trxcon.c M src/host/trxcon/src/trxcon_fsm.c 2 files changed, 44 insertions(+), 25 deletions(-)

Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve pespin: Looks good to me, but someone else must approve laforge: Looks good to me, approved

diff --git a/src/host/trxcon/src/trxcon.c b/src/host/trxcon/src/trxcon.c index 35687d7..429d033 100644 --- a/src/host/trxcon/src/trxcon.c +++ b/src/host/trxcon/src/trxcon.c @@ -256,19 +256,22 @@ struct trxcon_inst *trxcon_inst_alloc(void *ctx, unsigned int id) { struct trxcon_inst *trxcon; + struct osmo_fsm_inst *fi;

- trxcon = talloc_zero(ctx, struct trxcon_inst); + fi = osmo_fsm_inst_alloc(&trxcon_fsm_def, ctx, NULL, LOGL_DEBUG, NULL); + OSMO_ASSERT(fi != NULL); + + trxcon = talloc_zero(fi, struct trxcon_inst); OSMO_ASSERT(trxcon != NULL);

- trxcon->fi = osmo_fsm_inst_alloc(&trxcon_fsm_def, tall_trxcon_ctx, - trxcon, LOGL_DEBUG, NULL); - OSMO_ASSERT(trxcon->fi != NULL); + fi->priv = trxcon; + trxcon->fi = fi;

- osmo_fsm_inst_update_id_f(trxcon->fi, "%u", id); + osmo_fsm_inst_update_id_f(fi, "%u", id); trxcon->id = id;

/* Logging context to be used by both l1ctl and l1sched modules */ - trxcon->log_prefix = talloc_asprintf(trxcon, "%s: ", osmo_fsm_inst_name(trxcon->fi)); + trxcon->log_prefix = talloc_asprintf(trxcon, "%s: ", osmo_fsm_inst_name(fi));

/* Init transceiver interface */ trxcon->phyif = trx_if_open(trxcon, @@ -297,18 +300,9 @@

void trxcon_inst_free(struct trxcon_inst *trxcon) { - /* Shutdown the scheduler */ - if (trxcon->sched != NULL) - l1sched_free(trxcon->sched); - /* Close active connections */ - if (trxcon->l2if != NULL) - l1ctl_client_conn_close(trxcon->l2if); - if (trxcon->phyif != NULL) - trx_if_close(trxcon->phyif); - - if (trxcon->fi != NULL) - osmo_fsm_inst_free(trxcon->fi); - talloc_free(trxcon); + if (trxcon == NULL || trxcon->fi == NULL) + return; + osmo_fsm_inst_term(trxcon->fi, OSMO_FSM_TERM_REQUEST, NULL); }

static void l1ctl_conn_accept_cb(struct l1ctl_client *l1c) @@ -334,10 +328,6 @@ return;

osmo_fsm_inst_dispatch(trxcon->fi, TRXCON_EV_L2IF_FAILURE, NULL); - - /* l2if is free()ed by the caller */ - trxcon->l2if = NULL; - trxcon_inst_free(trxcon); }

static void print_usage(const char *app) diff --git a/src/host/trxcon/src/trxcon_fsm.c b/src/host/trxcon/src/trxcon_fsm.c index aa7193a..a77ce68 100644 --- a/src/host/trxcon/src/trxcon_fsm.c +++ b/src/host/trxcon/src/trxcon_fsm.c @@ -46,10 +46,12 @@

switch (event) { case TRXCON_EV_PHYIF_FAILURE: + trxcon->phyif = NULL; + osmo_fsm_inst_term(fi, OSMO_FSM_TERM_ERROR, NULL); + break; case TRXCON_EV_L2IF_FAILURE: - LOGPFSML(fi, LOGL_NOTICE, "Event %s is not handled\n", - osmo_fsm_event_name(&trxcon_fsm_def, event)); - /* TODO: osmo_fsm_inst_term(fi, OSMO_FSM_TERM_ERROR, NULL); */ + trxcon->l2if = NULL; + osmo_fsm_inst_term(fi, OSMO_FSM_TERM_ERROR, NULL); break; case TRXCON_EV_RESET_FULL_REQ: if (fi->state != TRXCON_ST_RESET) @@ -404,6 +406,32 @@ } }

+static void trxcon_fsm_pre_term_cb(struct osmo_fsm_inst *fi, + enum osmo_fsm_term_cause cause) +{ + struct trxcon_inst *trxcon = fi->priv; + + if (trxcon == NULL) + return; + + /* Shutdown the scheduler */ + if (trxcon->sched != NULL) + l1sched_free(trxcon->sched); + /* Close active connections */ + if (trxcon->l2if != NULL) { + /* Avoid use-after-free: both *fi and *trxcon are children of + * the L2IF (L1CTL connection), so we need to re-parent *fi + * to NULL before calling l1ctl_client_conn_close(). */ + talloc_steal(NULL, fi); + l1ctl_client_conn_close(trxcon->l2if); + } + if (trxcon->phyif != NULL) + trx_if_close(trxcon->phyif); + + talloc_free(trxcon); + fi->priv = NULL; +} + static const struct osmo_fsm_state trxcon_fsm_states[] = { [TRXCON_ST_RESET] = { .name = "RESET", @@ -492,6 +520,7 @@ | S(TRXCON_EV_UPDATE_SACCH_CACHE_REQ), .allstate_action = &trxcon_allstate_action, .timer_cb = &trxcon_timer_cb, + .pre_term = &trxcon_fsm_pre_term_cb, };

static __attribute__((constructor)) void on_dso_load(void)