Change in osmo-pcu[master]: Fix MS ending up with assigned imsi 000

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

pespin gerrit-no-reply at lists.osmocom.org
Fri Nov 12 17:25:58 UTC 2021


pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-pcu/+/26222 )


Change subject: Fix MS ending up with assigned imsi 000
......................................................................

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
---
M src/gprs_bssgp_pcu.c
M src/gprs_ms.c
M src/gprs_ms.h
M src/gprs_ms_storage.cpp
M src/tbf_dl.cpp
M tests/tbf/TbfTest.cpp
6 files changed, 35 insertions(+), 19 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/22/26222/1

diff --git a/src/gprs_bssgp_pcu.c b/src/gprs_bssgp_pcu.c
index 4328e07..8cb5302 100644
--- a/src/gprs_bssgp_pcu.c
+++ b/src/gprs_bssgp_pcu.c
@@ -108,12 +108,8 @@
 	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 @@
 			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 @@
 				"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 0d6be4d..5e75d06 100644
--- a/src/gprs_ms.c
+++ b/src/gprs_ms.c
@@ -522,6 +522,18 @@
 	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 4438a4c..c579cf5 100644
--- a/src/gprs_ms.h
+++ b/src/gprs_ms.h
@@ -132,6 +132,7 @@
 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 @@
 	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 6245ed9..db3a7f9 100644
--- a/src/gprs_ms_storage.cpp
+++ b/src/gprs_ms_storage.cpp
@@ -29,8 +29,6 @@
 	#include <osmocom/gsm/gsm48.h>
 }
 
-#define GPRS_UNDEFINED_IMSI "000"
-
 static void ms_storage_ms_idle_cb(struct GprsMs *ms)
 {
 	llist_del(&ms->list);
@@ -89,10 +87,10 @@
 
 	/* 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 05d5ad3..20811ac 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -296,7 +296,9 @@
 	/* 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 @@
 			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 @@
 
 	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);
@@ -618,8 +621,7 @@
 		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());
+		pgroup = ms_paging_group(ms());
 		bts_snd_dl_ass(bts, this, pgroup);
 	}
 }
diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp
index 8700347..9bea528 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -1718,7 +1718,7 @@
 	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);
 	}

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/26222
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I4c0838b26ede58e4b711410eee2a8e4f71e9414b
Gerrit-Change-Number: 26222
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211112/f10a8360/attachment.htm>


More information about the gerrit-log mailing list