From 664997fd28fe2ffc78ee696c97792d86b461de66 Mon Sep 17 00:00:00 2001 From: Spencer Sevilla Date: Mon, 30 Jan 2023 15:56:08 -0800 Subject: [PATCH] [mme] turn various asserts into checks with error-handling (#69) --- README.md | 5 ++++- src/mme/mme-s11-handler.c | 46 +++++++++++++++++++++++++++------------ src/mme/nas-path.c | 7 ++++++ 3 files changed, 43 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 9a01e720f3..71c27a7c3c 100644 --- a/README.md +++ b/README.md @@ -34,4 +34,7 @@ Wherever possible, we have changed PFCP messages/operations from an action-based When a UPS re-associates itself with the CPS after a period of disconnectivity, their respective states may have diverged substantially. We handle this using a reassociation process wherein (1) the CPS sends a Session-Set-Delete message, effectively instructing the UPS to delete any/all current sessions, and then (2) the CPS re-sends a Session Establishment message for each active session. This creates a bit more chatter over the wire than you might expect, but works incredibly well in our context. ## Fine-Grained Timers -Stock open5gs has one configurable parameter (`message.duration`) which sets the timeout value for *all* messages sent or received by the program in question. We turned this parameter into several different ones pertaining to each message protocol to allow us to have finer-grained control over these timers, which is highly recommended by the 3GPP. Specifically, `message.duration` has been replaced by `message.sbi_duration`, `message.gtp_duration`, `message.pfcp_duration` and `message.diameter_timeout`. \ No newline at end of file +Stock open5gs has one configurable parameter (`message.duration`) which sets the timeout value for *all* messages sent or received by the program in question. We turned this parameter into several different ones pertaining to each message protocol to allow us to have finer-grained control over these timers, which is highly recommended by the 3GPP. Specifically, `message.duration` has been replaced by `message.sbi_duration`, `message.gtp_duration`, `message.pfcp_duration` and `message.diameter_timeout`. + +## Stability Fixes +We have caught and fixed a large number of small bugs that threaten stability, generally asserts() that are not always true. Our team is firmly committed to contributing all such fixes to the main open5gs project, and these commits are either already upstreamed or will be soon. diff --git a/src/mme/mme-s11-handler.c b/src/mme/mme-s11-handler.c index 32d8dc994a..b80b8ba04c 100644 --- a/src/mme/mme-s11-handler.c +++ b/src/mme/mme-s11-handler.c @@ -112,16 +112,6 @@ void mme_s11_handle_create_session_response( ogs_assert(sess); mme_ue = sess->mme_ue; ogs_assert(mme_ue); - source_ue = sgw_ue_cycle(mme_ue->sgw_ue); - ogs_assert(source_ue); - - if (create_action == OGS_GTP_CREATE_IN_PATH_SWITCH_REQUEST) { - target_ue = sgw_ue_cycle(source_ue->target_ue); - ogs_assert(target_ue); - } else { - target_ue = source_ue; - ogs_assert(target_ue); - } rv = ogs_gtp_xact_commit(xact); ogs_expect_or_return(rv == OGS_OK); @@ -136,6 +126,23 @@ void mme_s11_handle_create_session_response( cause_value = OGS_GTP2_CAUSE_CONTEXT_NOT_FOUND; } + source_ue = sgw_ue_cycle(mme_ue->sgw_ue); + if (!source_ue) { + ogs_error("Cannot find source_ue context"); + cause_value = OGS_GTP2_CAUSE_CONTEXT_NOT_FOUND; + } + + if (create_action == OGS_GTP_CREATE_IN_PATH_SWITCH_REQUEST) { + // if source_ue == null we'll catch below + if (source_ue) { + target_ue = sgw_ue_cycle(source_ue->target_ue); + ogs_assert(target_ue); + } + } else { + target_ue = source_ue; + // if source_ue == null we'll catch below + } + if (cause_value != OGS_GTP2_CAUSE_REQUEST_ACCEPTED) { if (create_action == OGS_GTP_CREATE_IN_ATTACH_REQUEST) { ogs_error("[%s] Attach reject", mme_ue->imsi_bcd); @@ -437,8 +444,6 @@ void mme_s11_handle_modify_bearer_response( modify_action = xact->modify_action; mme_ue = xact->data; ogs_assert(mme_ue); - sgw_ue = sgw_ue_cycle(mme_ue->sgw_ue); - ogs_assert(sgw_ue); rv = ogs_gtp_xact_commit(xact); ogs_expect_or_return(rv == OGS_OK); @@ -453,6 +458,12 @@ void mme_s11_handle_modify_bearer_response( cause_value = OGS_GTP2_CAUSE_CONTEXT_NOT_FOUND; } + sgw_ue = sgw_ue_cycle(mme_ue->sgw_ue); + if (!sgw_ue) { + ogs_error("Cannot find sgw_ue context"); + cause_value = OGS_GTP2_CAUSE_CONTEXT_NOT_FOUND; + } + if (cause_value != OGS_GTP2_CAUSE_REQUEST_ACCEPTED) { mme_send_delete_session_or_mme_ue_context_release(mme_ue); return; @@ -1116,8 +1127,6 @@ void mme_s11_handle_release_access_bearers_response( ogs_assert(action); mme_ue = xact->data; ogs_assert(mme_ue); - sgw_ue = sgw_ue_cycle(mme_ue->sgw_ue); - ogs_assert(sgw_ue); rv = ogs_gtp_xact_commit(xact); ogs_expect_or_return(rv == OGS_OK); @@ -1127,6 +1136,15 @@ void mme_s11_handle_release_access_bearers_response( ***********************/ if (!mme_ue_from_teid) { ogs_error("No Context in TEID"); + mme_send_delete_session_or_mme_ue_context_release(mme_ue); + return; + } + + sgw_ue = sgw_ue_cycle(mme_ue->sgw_ue); + if (!sgw_ue) { + ogs_error("Cannot find sgw_ue"); + mme_send_delete_session_or_mme_ue_context_release(mme_ue); + return; } /******************** diff --git a/src/mme/nas-path.c b/src/mme/nas-path.c index 67b616b643..354c3b8844 100644 --- a/src/mme/nas-path.c +++ b/src/mme/nas-path.c @@ -131,6 +131,7 @@ int nas_eps_send_attach_reject(mme_ue_t *mme_ue, ogs_nas_emm_cause_t emm_cause, ogs_nas_esm_cause_t esm_cause) { int rv; + enb_ue_t *enb_ue = NULL; mme_sess_t *sess = NULL; ogs_pkbuf_t *esmbuf = NULL, *emmbuf = NULL; @@ -139,6 +140,12 @@ int nas_eps_send_attach_reject(mme_ue_t *mme_ue, ogs_debug("[%s] Attach reject", mme_ue->imsi_bcd); ogs_debug(" Cause[%d]", emm_cause); + enb_ue = enb_ue_cycle(mme_ue->enb_ue); + if (!enb_ue) { + ogs_error("S1 context has already been removed"); + return OGS_OK; + } + sess = mme_sess_first(mme_ue); if (sess) { esmbuf = esm_build_pdn_connectivity_reject(