[PATCH 2/2] gb: Fix gprs_ns_rx_reset to not create NS-VC duplicates

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/OpenBSC@lists.osmocom.org/.

Jacob Erlbeck jerlbeck at sysmocom.de
Wed Oct 9 09:27:05 UTC 2013


Under special circumstances (see below) receiving a NS-RESET leads to
duplicated NS-VC entries.

This patch changes gprs_ns_rx_reset() to check for this case and to
eventually delete the old entry. A new counter NS_CTR_REPLACED is
incremented each time when the NS-VC object is replaced. A new signal
S_NS_REPLACED is added which gets dispatched in this case, too.

This happens when the source port of a NS-VC changes to a new one
that has already been used by another NS-VC.
---
 include/osmocom/gprs/gprs_ns.h |    3 ++
 src/gb/gprs_ns.c               |   83 ++++++++++++++++++++++++++++++++--------
 tests/gb/gprs_ns_test.c        |    9 +++++
 tests/gb/gprs_ns_test.ok       |    3 +-
 4 files changed, 79 insertions(+), 19 deletions(-)

diff --git a/include/osmocom/gprs/gprs_ns.h b/include/osmocom/gprs/gprs_ns.h
index d16068b..cfcf8e2 100644
--- a/include/osmocom/gprs/gprs_ns.h
+++ b/include/osmocom/gprs/gprs_ns.h
@@ -119,6 +119,7 @@ struct gprs_nsvc {
 
 	unsigned int remote_end_is_sgsn:1;
 	unsigned int persistent:1;
+	unsigned int nsvci_is_valid:1;
 
 	struct rate_ctr_group *ctrg;
 
@@ -191,10 +192,12 @@ enum signal_ns {
 	S_NS_BLOCK,
 	S_NS_UNBLOCK,
 	S_NS_ALIVE_EXP,	/* Tns-alive expired more than N times */
+	S_NS_REPLACED, /* nsvc object is replaced (sets old_nsvc) */
 };
 
 struct ns_signal_data {
 	struct gprs_nsvc *nsvc;
+	struct gprs_nsvc *old_nsvc;
 	uint8_t cause;
 };
 
diff --git a/src/gb/gprs_ns.c b/src/gb/gprs_ns.c
index b5f91c4..03f39ef 100644
--- a/src/gb/gprs_ns.c
+++ b/src/gb/gprs_ns.c
@@ -100,15 +100,17 @@ enum ns_ctr {
 	NS_CTR_BYTES_OUT,
 	NS_CTR_BLOCKED,
 	NS_CTR_DEAD,
+	NS_CTR_REPLACED,
 };
 
 static const struct rate_ctr_desc nsvc_ctr_description[] = {
-	{ "packets.in", "Packets at NS Level ( In)" },
-	{ "packets.out","Packets at NS Level (Out)" },
-	{ "bytes.in",	"Bytes at NS Level   ( In)" },
-	{ "bytes.out",	"Bytes at NS Level   (Out)" },
-	{ "blocked",	"NS-VC Block count        " },
-	{ "dead",	"NS-VC gone dead count    " },
+	{ "packets.in", "Packets at NS Level  ( In)" },
+	{ "packets.out","Packets at NS Level  (Out)" },
+	{ "bytes.in",	"Bytes at NS Level    ( In)" },
+	{ "bytes.out",	"Bytes at NS Level    (Out)" },
+	{ "blocked",	"NS-VC Block count         " },
+	{ "dead",	"NS-VC gone dead count     " },
+	{ "replaced",	"NS-VC replaced other count" },
 };
 
 static const struct rate_ctr_group_desc nsvc_ctrg_desc = {
@@ -198,7 +200,7 @@ void gprs_nsvc_delete(struct gprs_nsvc *nsvc)
 static void ns_osmo_signal_dispatch(struct gprs_nsvc *nsvc, unsigned int signal,
 			       uint8_t cause)
 {
-	struct ns_signal_data nssd;
+	struct ns_signal_data nssd = {0};
 
 	nssd.nsvc = nsvc;
 	nssd.cause = cause;
@@ -206,6 +208,16 @@ static void ns_osmo_signal_dispatch(struct gprs_nsvc *nsvc, unsigned int signal,
 	osmo_signal_dispatch(SS_L_NS, signal, &nssd);
 }
 
+static void ns_osmo_signal_dispatch_replaced(struct gprs_nsvc *nsvc, struct gprs_nsvc *old_nsvc)
+{
+	struct ns_signal_data nssd = {0};
+
+	nssd.nsvc = nsvc;
+	nssd.old_nsvc = old_nsvc;
+
+	osmo_signal_dispatch(SS_L_NS, S_NS_REPLACED, &nssd);
+}
+
 /* Section 10.3.2, Table 13 */
 static const struct value_string ns_cause_str[] = {
 	{ NS_CAUSE_TRANSIT_FAIL,	"Transit network failure" },
@@ -651,8 +663,9 @@ static int gprs_ns_rx_reset(struct gprs_nsvc *nsvc, struct msgb *msg)
 {
 	struct gprs_ns_hdr *nsh = (struct gprs_ns_hdr *) msg->l2h;
 	struct tlv_parsed tp;
-	uint8_t *cause;
-	uint16_t *nsvci, *nsei;
+	uint8_t cause;
+	uint16_t nsvci, nsei;
+	struct gprs_nsvc *other_nsvc = NULL;
 	int rc;
 
 	rc = tlv_parse(&tp, &ns_att_tlvdef, nsh->data,
@@ -671,22 +684,58 @@ static int gprs_ns_rx_reset(struct gprs_nsvc *nsvc, struct msgb *msg)
 		return -EINVAL;
 	}
 
-	cause = (uint8_t *) TLVP_VAL(&tp, NS_IE_CAUSE);
-	nsvci = (uint16_t *) TLVP_VAL(&tp, NS_IE_VCI);
-	nsei = (uint16_t *) TLVP_VAL(&tp, NS_IE_NSEI);
+	cause = *(uint8_t  *) TLVP_VAL(&tp, NS_IE_CAUSE);
+	nsvci = ntohs(*(uint16_t *) TLVP_VAL(&tp, NS_IE_VCI));
+	nsei  = ntohs(*(uint16_t *) TLVP_VAL(&tp, NS_IE_NSEI));
+
+	LOGP(DNS, LOGL_INFO, "NSVCI=%u%s Rx NS RESET (NSEI=%u, NSVCI=%u, cause=%s)\n",
+	     nsvc->nsvci, nsvc->nsvci_is_valid ? "" : "(invalid)",
+	     nsei, nsvci, gprs_ns_cause_str(cause));
 
-	LOGP(DNS, LOGL_INFO, "NSEI=%u Rx NS RESET (NSVCI=%u, cause=%s)\n",
-		nsvc->nsvci, nsvc->nsei, gprs_ns_cause_str(*cause));
+	if (nsvc->nsvci_is_valid && nsvc->nsvci != nsvci) {
+		/* NS-VCI has changed */
+		other_nsvc = gprs_nsvc_by_nsvci(nsvc->nsi, nsvci);
+
+		if (other_nsvc) {
+			/* The NS-VCI is already used by this NS-VC */
+
+			struct rate_ctr_group *tmp_ctrg;
+			char *old_peer =
+				talloc_strdup(nsvc, gprs_ns_format_peer(other_nsvc));
+
+			LOGP(DNS, LOGL_INFO,
+			     "NS-VC changed link (NSVCI=%u) from %s to %s\n",
+			     nsvci, old_peer, gprs_ns_format_peer(nsvc));
+
+			talloc_free(old_peer);
+
+			/* Exchange the counters */
+			tmp_ctrg = nsvc->ctrg;
+			nsvc->ctrg = talloc_move(nsvc, &other_nsvc->ctrg);
+			other_nsvc->ctrg = talloc_move(other_nsvc, &tmp_ctrg);
+
+			rate_ctr_inc(&nsvc->ctrg->ctr[NS_CTR_REPLACED]);
+		}
+	}
 
 	/* Mark NS-VC as blocked and alive */
 	nsvc->state = NSE_S_BLOCKED | NSE_S_ALIVE;
 
-	nsvc->nsei = ntohs(*nsei);
-	nsvc->nsvci = ntohs(*nsvci);
+	nsvc->nsei  = nsei;
+	nsvc->nsvci = nsvci;
+	nsvc->nsvci_is_valid = 1;
+
+	if (other_nsvc) {
+		ns_osmo_signal_dispatch_replaced(nsvc, other_nsvc);
+
+		/* Delete the 'old' object */
+		gprs_nsvc_delete(other_nsvc);
+		other_nsvc = NULL;
+	}
 
 	/* inform interested parties about the fact that this NSVC
 	 * has received RESET */
-	ns_osmo_signal_dispatch(nsvc, S_NS_RESET, *cause);
+	ns_osmo_signal_dispatch(nsvc, S_NS_RESET, cause);
 
 	rc = gprs_ns_tx_reset_ack(nsvc);
 
diff --git a/tests/gb/gprs_ns_test.c b/tests/gb/gprs_ns_test.c
index d41fccb..67e2eb9 100644
--- a/tests/gb/gprs_ns_test.c
+++ b/tests/gb/gprs_ns_test.c
@@ -155,6 +155,15 @@ static int test_signal(unsigned int subsys, unsigned int signal,
 			       gprs_ns_format_peer(nssd->nsvc));
 			break;
 
+		case S_NS_REPLACED:
+			printf("==> got signal NS_REPLACED: 0x%04x/%s",
+			       nssd->old_nsvc->nsvci,
+			       gprs_ns_format_peer(nssd->old_nsvc));
+			printf(" -> 0x%04x/%s\n",
+			       nssd->nsvc->nsvci,
+			       gprs_ns_format_peer(nssd->nsvc));
+			break;
+
 		default:
 			printf("==> got signal %d, NS-VC 0x%04x/%s\n", signal,
 			       nssd->nsvc->nsvci,
diff --git a/tests/gb/gprs_ns_test.ok b/tests/gb/gprs_ns_test.ok
index 01b1bc9..cc9e76c 100644
--- a/tests/gb/gprs_ns_test.ok
+++ b/tests/gb/gprs_ns_test.ok
@@ -97,6 +97,7 @@ Current NS-VCIs:
 PROCESSING RESET from 0x01020304:3333
 02 00 81 01 01 82 11 22 04 82 11 22 
 
+==> got signal NS_REPLACED: 0x1122/1.2.3.4:4444 -> 0x1122/1.2.3.4:3333
 ==> got signal NS_RESET, NS-VC 0x1122/1.2.3.4:3333
 RESPONSE, msg length 9
 03 01 82 11 22 04 82 11 22 
@@ -108,7 +109,6 @@ result (RESET) = 9
 
 Current NS-VCIs:
     VCI 0x1122, NSEI 0x1122, peer 0x01020304:3333
-    VCI 0x1122, NSEI 0x3344, peer 0x01020304:4444
 
 --- Peer port 4444, RESET, NSEI is changed back ---
 
@@ -125,7 +125,6 @@ RESPONSE, msg length 1
 result (RESET) = 9
 
 Current NS-VCIs:
-    VCI 0x1122, NSEI 0x1122, peer 0x01020304:3333
     VCI 0x1122, NSEI 0x1122, peer 0x01020304:4444
 
 ===== NS protocol test END
-- 
1.7.9.5





More information about the OpenBSC mailing list