sgsn: Don't allow mmctx == NULL in sgsn_update_subscriber_data

Currently, sgsn_update_subscriber_data can be called with mmctx ==
NULL and will find and associate the right context (if present) based
on the subscriber's IMSI. This will not happen in regular use
any more, since sgsn_update_subscriber_data will only be called when
subscribers are used (auth mode 'remote') and in this case
gprs_subscr_get_or_create_by_mmctx will already be called by
sgsn_auth_request. Therefore, MM context and subscriber are always
associated except for some test cases and experimental VTY usage.
The current implementation of sgsn_update_subscriber_data also causes
additional complexity for the deletion on MM contexts to avoid a
ipossible double-free MM contexts.

This commit removes the MM context <-> subscriber association code
from sgsn_update_subscriber_data. That function must always be called
with mmctx != NULL, now. To avoid problems with VTY and test usage,
the calling subscriber function now only call
sgsn_update_subscriber_data when mmctx != NULL, since the purpose of
that function is to update that state of an existing MM context after
subscriber data has been changed.

Sponsored-by: On-Waves ehf

4 files changed, 18 insertions, 33 deletions

diff --git a/openbsc/include/openbsc/gprs_sgsn.h b/openbsc/include/openbsc/gprs_sgsn.h
index 533117a74..ce73e0189 100644
--- a/openbsc/include/openbsc/gprs_sgsn.h
+++ b/openbsc/include/openbsc/gprs_sgsn.h
@@ -339,8 +339,7 @@ int gprs_subscr_query_auth_info(struct gsm_subscriber *subscr);
 int gprs_subscr_location_update(struct gsm_subscriber *subscr);
 
 /* Called on subscriber data updates */
-void sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx,
-				 struct gsm_subscriber *subscr);
+void sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx);
 
 int gprs_sndcp_vty_init(void);
 struct sgsn_instance;
diff --git a/openbsc/src/gprs/gprs_sgsn.c b/openbsc/src/gprs/gprs_sgsn.c
index 781be3ffb..14b925400 100644
--- a/openbsc/src/gprs/gprs_sgsn.c
+++ b/openbsc/src/gprs/gprs_sgsn.c
@@ -487,28 +487,11 @@ int sgsn_force_reattach_oldmsg(struct msgb *oldmsg)
 	return gsm0408_gprs_force_reattach_oldmsg(oldmsg);
 }
 
-void sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx,
-				 struct gsm_subscriber *subscr)
+void sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx)
 {
-	if (!mmctx && subscr && strlen(subscr->imsi) > 0) {
-		mmctx = sgsn_mm_ctx_by_imsi(subscr->imsi);
-		OSMO_ASSERT(!mmctx || !mmctx->subscr || mmctx->subscr == subscr);
-	}
-
-	if (!mmctx) {
-		LOGP(DMM, LOGL_INFO,
-		     "Subscriber data update for unregistered MM context, IMSI %s\n",
-		     subscr->imsi);
-		return;
-	}
-
+	OSMO_ASSERT(mmctx != NULL);
 	LOGMMCTXP(LOGL_INFO, mmctx, "Subscriber data update\n");
 
-	if (!subscr->sgsn_data->mm && !mmctx->subscr) {
-		mmctx->subscr =	subscr_get(subscr);
-		mmctx->subscr->sgsn_data->mm = mmctx;
-	}
-
 	sgsn_auth_update(mmctx);
 }
 
diff --git a/openbsc/src/gprs/gprs_subscriber.c b/openbsc/src/gprs/gprs_subscriber.c
index ee6c47786..ea5d1d8d6 100644
--- a/openbsc/src/gprs/gprs_subscriber.c
+++ b/openbsc/src/gprs/gprs_subscriber.c
@@ -614,7 +614,8 @@ void gprs_subscr_update(struct gsm_subscriber *subscr)
 	subscr->flags &= ~GPRS_SUBSCRIBER_UPDATE_LOCATION_PENDING;
 	subscr->flags &= ~GSM_SUBSCRIBER_FIRST_CONTACT;
 
-	sgsn_update_subscriber_data(subscr->sgsn_data->mm, subscr);
+	if (subscr->sgsn_data->mm)
+		sgsn_update_subscriber_data(subscr->sgsn_data->mm);
 }
 
 void gprs_subscr_update_auth_info(struct gsm_subscriber *subscr)
@@ -625,7 +626,8 @@ void gprs_subscr_update_auth_info(struct gsm_subscriber *subscr)
 	subscr->flags &= ~GPRS_SUBSCRIBER_UPDATE_AUTH_INFO_PENDING;
 	subscr->flags &= ~GSM_SUBSCRIBER_FIRST_CONTACT;
 
-	sgsn_update_subscriber_data(subscr->sgsn_data->mm, subscr);
+	if (subscr->sgsn_data->mm)
+		sgsn_update_subscriber_data(subscr->sgsn_data->mm);
 }
 
 struct gsm_subscriber *gprs_subscr_get_or_create_by_mmctx(struct sgsn_mm_ctx *mmctx)
diff --git a/openbsc/tests/sgsn/sgsn_test.c b/openbsc/tests/sgsn/sgsn_test.c
index 5d142da90..da7da855f 100644
--- a/openbsc/tests/sgsn/sgsn_test.c
+++ b/openbsc/tests/sgsn/sgsn_test.c
@@ -61,14 +61,13 @@ int bssgp_tx_dl_ud(struct msgb *msg, uint16_t pdu_lifetime,
 }
 
 /* override, requires '-Wl,--wrap=sgsn_update_subscriber_data' */
-void __real_sgsn_update_subscriber_data(struct sgsn_mm_ctx *, struct gsm_subscriber *);
-void (*update_subscriber_data_cb)(struct sgsn_mm_ctx *, struct gsm_subscriber *) =
+void __real_sgsn_update_subscriber_data(struct sgsn_mm_ctx *);
+void (*update_subscriber_data_cb)(struct sgsn_mm_ctx *) =
     &__real_sgsn_update_subscriber_data;
 
-void __wrap_sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx,
-					struct gsm_subscriber *subscr)
+void __wrap_sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx)
 {
-	(*update_subscriber_data_cb)(mmctx, subscr);
+	(*update_subscriber_data_cb)(mmctx);
 }
 
 /* override, requires '-Wl,--wrap=gprs_subscr_request_update_location' */
@@ -191,12 +190,12 @@ static void test_llme(void)
 }
 
 struct gsm_subscriber *last_updated_subscr = NULL;
-void my_dummy_sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx,
-					  struct gsm_subscriber *subscr)
+void my_dummy_sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx)
 {
+	OSMO_ASSERT(mmctx);
 	fprintf(stderr, "Called %s, mmctx = %p, subscr = %p\n",
-		__func__, mmctx, subscr);
-	last_updated_subscr = subscr;
+		__func__, mmctx, mmctx->subscr);
+	last_updated_subscr = mmctx->subscr;
 }
 
 static void assert_subscr(const struct gsm_subscriber *subscr, const char *imsi)
@@ -266,7 +265,9 @@ static void test_subscriber(void)
 	/* Update entry 1 */
 	last_updated_subscr = NULL;
 	gprs_subscr_update(s1);
-	OSMO_ASSERT(last_updated_subscr == s1);
+	OSMO_ASSERT(last_updated_subscr == NULL);
+	OSMO_ASSERT(s1->sgsn_data->mm == NULL);
+	OSMO_ASSERT((s1->flags & GSM_SUBSCRIBER_FIRST_CONTACT) == 0);
 
 	/* There is no subscriber cache. Verify it */
 	gprs_subscr_cleanup(s1);