From 5deac1404d2fb1c805218b19efcbaeedeb3ec23b Mon Sep 17 00:00:00 2001 From: Pau Espin Pedrol Date: Fri, 12 Nov 2021 18:01:50 +0100 Subject: [PATCH] Fix MS ending up with assigned imsi 000 The whole paging path and data structre is cleaned up. New MS helpers ms_imsi_is_valid() and ms_paging_group() are introduced to help in the process and keep implementation details inside GprsMs class. Related: OS#5303 Change-Id: I4c0838b26ede58e4b711410eee2a8e4f71e9414b --- src/bts.cpp | 3 ++- src/bts.h | 2 +- src/gprs_bssgp_pcu.c | 16 +++++++--------- src/gprs_ms.c | 12 ++++++++++++ src/gprs_ms.h | 6 ++++++ src/gprs_ms_storage.cpp | 6 ++---- src/tbf_dl.cpp | 14 +++++++------- src/tbf_fsm.c | 8 ++------ tests/tbf/TbfTest.cpp | 2 +- 9 files changed, 40 insertions(+), 29 deletions(-) diff --git a/src/bts.cpp b/src/bts.cpp index dc20259c..6fabc90c 100644 --- a/src/bts.cpp +++ b/src/bts.cpp @@ -1037,10 +1037,11 @@ int bts_rcv_ptcch_rach(struct gprs_rlcmac_bts *bts, const struct rach_ind_params return 0; } -void bts_snd_dl_ass(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *tbf, uint16_t pgroup) +void bts_snd_dl_ass(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *tbf) { uint8_t trx_no = tbf->trx->trx_no; uint8_t ts_no = tbf->first_ts; + uint16_t pgroup = ms_paging_group(tbf_ms(tbf)); int plen; LOGPTBF(tbf, LOGL_INFO, "TX: START Immediate Assignment Downlink (PCH)\n"); diff --git a/src/bts.h b/src/bts.h index 3a58c636..f947a558 100644 --- a/src/bts.h +++ b/src/bts.h @@ -301,7 +301,7 @@ uint32_t bts_rfn_to_fn(const struct gprs_rlcmac_bts *bts, int32_t rfn); struct gprs_rlcmac_dl_tbf *bts_dl_tbf_by_tfi(struct gprs_rlcmac_bts *bts, uint8_t tfi, uint8_t trx, uint8_t ts); struct gprs_rlcmac_ul_tbf *bts_ul_tbf_by_tfi(struct gprs_rlcmac_bts *bts, uint8_t tfi, uint8_t trx, uint8_t ts); -void bts_snd_dl_ass(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *tbf, uint16_t pgroup); +void bts_snd_dl_ass(struct gprs_rlcmac_bts *bts, struct gprs_rlcmac_tbf *tbf); void bts_set_current_frame_number(struct gprs_rlcmac_bts *bts, uint32_t frame_number); void bts_set_current_block_frame_number(struct gprs_rlcmac_bts *bts, int frame_number); diff --git a/src/gprs_bssgp_pcu.c b/src/gprs_bssgp_pcu.c index 4328e079..8cb53027 100644 --- a/src/gprs_bssgp_pcu.c +++ b/src/gprs_bssgp_pcu.c @@ -108,12 +108,8 @@ static int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp) uint8_t egprs_ms_class = 0; int rc; MS_Radio_Access_capability_t rac; - /* TODO: is it really necessary to initialize this as a "000" IMSI? It seems, the function should just return an - * error if no IMSI IE was found. */ - struct osmo_mobile_identity mi_imsi = { - .type = GSM_MI_TYPE_TMSI, - }; - OSMO_STRLCPY_ARRAY(mi_imsi.imsi, "000"); + const char *imsi = NULL; + struct osmo_mobile_identity mi_imsi; budh = (struct bssgp_ud_hdr *)msgb_bssgph(msg); tlli = ntohl(budh->tlli); @@ -144,6 +140,7 @@ static int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp) LOGP(DBSSGP, LOGL_NOTICE, "Failed to parse IMSI IE (rc=%d)\n", rc); return bssgp_tx_status(BSSGP_CAUSE_COND_IE_ERR, NULL, msg); } + imsi = &mi_imsi.imsi[0]; } /* parse ms radio access capability */ @@ -180,10 +177,11 @@ static int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp) "TLLI (old) IE\n"); } - LOGP(DBSSGP, LOGL_INFO, "LLC [SGSN -> PCU] = TLLI: 0x%08x IMSI: %s len: %d\n", tlli, mi_imsi.imsi, len); + LOGP(DBSSGP, LOGL_INFO, "LLC [SGSN -> PCU] = TLLI: 0x%08x IMSI: %s len: %d\n", + tlli, imsi ? : "none", len); - return dl_tbf_handle(the_pcu->bssgp.bts, tlli, tlli_old, mi_imsi.imsi, - ms_class, egprs_ms_class, delay_csec, data, len); + return dl_tbf_handle(the_pcu->bssgp.bts, tlli, tlli_old, imsi, ms_class, + egprs_ms_class, delay_csec, data, len); } /* 3GPP TS 48.018 Table 10.3.2. Returns 0 on success, suggested BSSGP cause otherwise */ diff --git a/src/gprs_ms.c b/src/gprs_ms.c index 0d6be4d5..5e75d067 100644 --- a/src/gprs_ms.c +++ b/src/gprs_ms.c @@ -522,6 +522,18 @@ void ms_set_imsi(struct GprsMs *ms, const char *imsi) osmo_strlcpy(ms->imsi, imsi, sizeof(ms->imsi)); } +uint16_t ms_paging_group(struct GprsMs *ms) +{ + uint16_t pgroup; + if (!ms_imsi_is_valid(ms)) + return 0; /* 000 is the special "all paging" group */ + if ((pgroup = imsi2paging_group(ms_imsi(ms))) > 999) { + LOGPMS(ms, DRLCMAC, LOGL_ERROR, "IMSI to paging group failed!\n"); + return 0; + } + return pgroup; +} + void ms_set_ta(struct GprsMs *ms, uint8_t ta_) { if (ta_ == ms->ta) diff --git a/src/gprs_ms.h b/src/gprs_ms.h index 4438a4c9..c579cf5a 100644 --- a/src/gprs_ms.h +++ b/src/gprs_ms.h @@ -132,6 +132,7 @@ void ms_detach_tbf(struct GprsMs *ms, struct gprs_rlcmac_tbf *tbf); void ms_set_tlli(struct GprsMs *ms, uint32_t tlli); bool ms_confirm_tlli(struct GprsMs *ms, uint32_t tlli); void ms_set_imsi(struct GprsMs *ms, const char *imsi); +uint16_t ms_paging_group(struct GprsMs *ms); void ms_update_l1_meas(struct GprsMs *ms, const struct pcu_l1_meas *meas); @@ -186,6 +187,11 @@ static inline const char *ms_imsi(const struct GprsMs *ms) return ms->imsi; } +static inline bool ms_imsi_is_valid(const struct GprsMs *ms) +{ + return ms->imsi[0] != '\0'; +} + static inline uint8_t ms_ta(const struct GprsMs *ms) { return ms->ta; diff --git a/src/gprs_ms_storage.cpp b/src/gprs_ms_storage.cpp index 6245ed9f..db3a7f96 100644 --- a/src/gprs_ms_storage.cpp +++ b/src/gprs_ms_storage.cpp @@ -29,8 +29,6 @@ extern "C" { #include } -#define GPRS_UNDEFINED_IMSI "000" - static void ms_storage_ms_idle_cb(struct GprsMs *ms) { llist_del(&ms->list); @@ -89,10 +87,10 @@ GprsMs *GprsMsStorage::get_ms(uint32_t tlli, uint32_t old_tlli, const char *imsi /* not found by TLLI */ - if (imsi && imsi[0] && strcmp(imsi, GPRS_UNDEFINED_IMSI) != 0) { + if (imsi && imsi[0] != '\0') { llist_for_each(tmp, &m_list) { ms = llist_entry(tmp, typeof(*ms), list); - if (strcmp(imsi, ms_imsi(ms)) == 0) + if (ms_imsi_is_valid(ms) && strcmp(imsi, ms_imsi(ms)) == 0) return ms; } } diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp index 05d5ad36..6e898dad 100644 --- a/src/tbf_dl.cpp +++ b/src/tbf_dl.cpp @@ -296,7 +296,9 @@ int dl_tbf_handle(struct gprs_rlcmac_bts *bts, /* check for existing TBF */ ms = bts_ms_store(bts)->get_ms(tlli, tlli_old, imsi); - if (ms && strlen(ms_imsi(ms)) == 0) { + /* If we got MS by TLLI above let's see if we already have another MS + * object identified by IMSI and merge them */ + if (ms && !ms_imsi_is_valid(ms) && imsi) { ms_old = bts_ms_store(bts)->get_ms(0, 0, imsi); if (ms_old && ms_old != ms) { /* The TLLI has changed (RAU), so there are two MS @@ -310,7 +312,7 @@ int dl_tbf_handle(struct gprs_rlcmac_bts *bts, if (!ms_dl_tbf(ms) && ms_dl_tbf(ms_old)) { LOGP(DTBF, LOGL_NOTICE, "IMSI %s, old TBF %s: moving DL TBF to new MS object\n", - imsi, ms_dl_tbf(ms_old)->name()); + imsi ? : "unknown", ms_dl_tbf(ms_old)->name()); dl_tbf = ms_dl_tbf(ms_old); /* Move the DL TBF to the new MS */ dl_tbf->set_ms(ms); @@ -323,7 +325,8 @@ int dl_tbf_handle(struct gprs_rlcmac_bts *bts, if (!ms) ms = bts_alloc_ms(bts, ms_class, egprs_ms_class); - ms_set_imsi(ms, imsi); + if (imsi) + ms_set_imsi(ms, imsi); ms_confirm_tlli(ms, tlli); if (!ms_ms_class(ms) && ms_class) { ms_set_ms_class(ms, ms_class); @@ -599,7 +602,6 @@ struct msgb *gprs_rlcmac_dl_tbf::create_dl_acked_block(uint32_t fn, uint8_t ts, /* depending on the current TBF, we assign on PACCH or AGCH */ void gprs_rlcmac_dl_tbf::trigger_ass(struct gprs_rlcmac_tbf *old_tbf) { - uint16_t pgroup; /* stop pending timer */ stop_timers("assignment (DL-TBF)"); @@ -618,9 +620,7 @@ void gprs_rlcmac_dl_tbf::trigger_ass(struct gprs_rlcmac_tbf *old_tbf) osmo_fsm_inst_dispatch(this->state_fsm.fi, TBF_EV_ASSIGN_ADD_CCCH, NULL); /* send immediate assignment */ - if ((pgroup = imsi2paging_group(imsi())) > 999) - LOGPTBFDL(this, LOGL_ERROR, "IMSI to paging group failed! (%s)\n", imsi()); - bts_snd_dl_ass(bts, this, pgroup); + bts_snd_dl_ass(bts, this); } } diff --git a/src/tbf_fsm.c b/src/tbf_fsm.c index af2b34ef..39f20805 100644 --- a/src/tbf_fsm.c +++ b/src/tbf_fsm.c @@ -214,15 +214,11 @@ static void st_flow(struct osmo_fsm_inst *fi, uint32_t event, void *data) if ((ctx->state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH)) && !(ctx->state_flags & (1 << GPRS_RLCMAC_FLAG_DL_ACK))) { struct GprsMs *ms = tbf_ms(ctx->tbf); - const char *imsi = ms_imsi(ms); - uint16_t pgroup; LOGPTBF(ctx->tbf, LOGL_DEBUG, "Re-send downlink assignment on PCH (IMSI=%s)\n", - imsi); + ms_imsi_is_valid(ms) ? ms_imsi(ms) : ""); tbf_fsm_state_chg(fi, TBF_ST_ASSIGN); /* send immediate assignment */ - if ((pgroup = imsi2paging_group(imsi)) > 999) - LOGPTBF(ctx->tbf, LOGL_ERROR, "IMSI to paging group failed! (%s)\n", imsi); - bts_snd_dl_ass(ms->bts, ctx->tbf, pgroup); + bts_snd_dl_ass(ms->bts, ctx->tbf); } break; case TBF_EV_LAST_DL_DATA_SENT: diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp index 8700347d..9bea528b 100644 --- a/tests/tbf/TbfTest.cpp +++ b/tests/tbf/TbfTest.cpp @@ -1718,7 +1718,7 @@ static void send_dl_data(struct gprs_rlcmac_bts *bts, uint32_t tlli, const char OSMO_ASSERT(ms != NULL); OSMO_ASSERT(ms_dl_tbf(ms) != NULL); - if (imsi[0] && strcmp(imsi, "000") != 0) { + if (imsi[0] != '\0') { ms2 = bts_ms_by_tlli(bts, tlli, GSM_RESERVED_TMSI); OSMO_ASSERT(ms == ms2); }