This explicitely allows aliasing of this struct to avoid the warning shown below. Note, that this warning isn't show when '-fno-strict-aliasing' is used.
Addresses: gb/gprs_ns_test.c:85:54: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] --- include/osmocom/gprs/gprs_msgb.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/osmocom/gprs/gprs_msgb.h b/include/osmocom/gprs/gprs_msgb.h index f4c8554..e277696 100644 --- a/include/osmocom/gprs/gprs_msgb.h +++ b/include/osmocom/gprs/gprs_msgb.h @@ -16,7 +16,7 @@ struct libgb_msgb_cb {
/* Identifier of a MS (inside BTS), equal to 'struct sgsn_mm_ctx' */ uint32_t tlli; -} __attribute__((packed)); +} __attribute__((packed, may_alias)); #define LIBGB_MSGB_CB(__msgb) ((struct libgb_msgb_cb *)&((__msgb)->cb[0])) #define msgb_tlli(__x) LIBGB_MSGB_CB(__x)->tlli #define msgb_nsei(__x) LIBGB_MSGB_CB(__x)->nsei
This fixes warnings that are raised by missing includes, missing casts, missing return statements, using printf %lu with uint64_t, and unused symbols. --- tests/auth/milenage_test.c | 4 +++- tests/conv/conv_test.c | 2 ++ tests/sms/sms_test.c | 4 +--- tests/smscb/smscb_test.c | 1 + tests/timer/timer_test.c | 2 +- 5 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/tests/auth/milenage_test.c b/tests/auth/milenage_test.c index 7c996f0..0223764 100644 --- a/tests/auth/milenage_test.c +++ b/tests/auth/milenage_test.c @@ -7,6 +7,8 @@ #include <osmocom/crypt/auth.h> #include <osmocom/core/utils.h>
+int milenage_opc_gen(uint8_t *opc, const uint8_t *k, const uint8_t *op); + static void dump_auth_vec(struct osmo_auth_vector *vec) { printf("RAND:\t%s\n", osmo_hexdump(vec->rand, sizeof(vec->rand))); @@ -88,7 +90,7 @@ int main(int argc, char **argv) if (rc < 0) { printf("AUTS failed\n"); } else { - printf("AUTS success: SEQ.MS = %lu\n", test_aud.u.umts.sqn); + printf("AUTS success: SEQ.MS = %llu\n", (unsigned long long)test_aud.u.umts.sqn); }
opc_test(&test_aud); diff --git a/tests/conv/conv_test.c b/tests/conv/conv_test.c index ab651d4..d9440f9 100644 --- a/tests/conv/conv_test.c +++ b/tests/conv/conv_test.c @@ -347,6 +347,8 @@ ubit_to_sbit(sbit_t *dst, ubit_t *src, int n) dst[i] = src[i] ? -127 : 127; }
+static void sbit_to_ubit(ubit_t *dst, sbit_t *src, int n) __attribute__((unused)); + static void sbit_to_ubit(ubit_t *dst, sbit_t *src, int n) { diff --git a/tests/sms/sms_test.c b/tests/sms/sms_test.c index a79d454..896e134 100644 --- a/tests/sms/sms_test.c +++ b/tests/sms/sms_test.c @@ -266,18 +266,15 @@ static void test_gen_oa(void) int main(int argc, char** argv) { printf("SMS testing\n"); - struct msgb *msg; uint8_t i; uint16_t buffer_size; uint8_t octet_length; int octets_written; uint8_t computed_octet_length; uint8_t septet_length; - uint8_t gsm_septet_length; uint8_t coded[256]; uint8_t tmp[160]; uint8_t septet_data[256]; - uint8_t ud_header[6]; int nchars; char result[256];
@@ -320,6 +317,7 @@ int main(int argc, char** argv)
/* Test: encode multiple SMS */ int number_of_septets = gsm_septet_encode(septet_data, (const char *) test_multiple_encode[0].input); + (void) number_of_septets;
/* SMS part 1 */ memset(tmp, 0x42, sizeof(tmp)); diff --git a/tests/smscb/smscb_test.c b/tests/smscb/smscb_test.c index e10e12d..5925f69 100644 --- a/tests/smscb/smscb_test.c +++ b/tests/smscb/smscb_test.c @@ -21,6 +21,7 @@ #include <osmocom/gsm/protocol/gsm_03_41.h>
#include <stdio.h> +#include <arpa/inet.h>
static uint8_t smscb_msg[] = { 0x40, 0x10, 0x05, 0x0d, 0x01, 0x11 };
diff --git a/tests/timer/timer_test.c b/tests/timer/timer_test.c index bb9a177..b746ace 100644 --- a/tests/timer/timer_test.c +++ b/tests/timer/timer_test.c @@ -115,7 +115,7 @@ static void secondary_timer_fired(void *data) timersub(¤t, &v->stop, &res); if (timercmp(&res, &precision, >)) { fprintf(stderr, "ERROR: timer %p has expired too late!\n", - v->timer); + (void *)&(v->timer)); too_late++; }
This tests the connection establishment by directly calling gprs_ns_rcvmsg() and printing the resulting messages and the NS-VC list. --- tests/Makefile.am | 7 +- tests/gb/gprs_ns_test.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++ tests/gb/gprs_ns_test.ok | 124 +++++++++++++++++++++++ tests/testsuite.at | 6 ++ 4 files changed, 383 insertions(+), 1 deletion(-) create mode 100644 tests/gb/gprs_ns_test.c create mode 100644 tests/gb/gprs_ns_test.ok
diff --git a/tests/Makefile.am b/tests/Makefile.am index ecb2b6c..1bb26ce 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -4,7 +4,8 @@ check_PROGRAMS = timer/timer_test sms/sms_test ussd/ussd_test \ smscb/smscb_test bits/bitrev_test a5/a5_test \ conv/conv_test auth/milenage_test lapd/lapd_test \ gsm0808/gsm0808_test gsm0408/gsm0408_test \ - gb/bssgp_fc_test logging/logging_test fr/fr_test \ + gb/bssgp_fc_test gb/gprs_ns_test \ + logging/logging_test fr/fr_test \ loggingrb/loggingrb_test strrb/strrb_test \ vty/vty_test
@@ -51,6 +52,9 @@ ussd_ussd_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gs gb_bssgp_fc_test_SOURCES = gb/bssgp_fc_test.c gb_bssgp_fc_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gb/libosmogb.la
+gb_gprs_ns_test_SOURCES = gb/gprs_ns_test.c +gb_gprs_ns_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gb/libosmogb.la $(LIBRARY_DL) + logging_logging_test_SOURCES = logging/logging_test.c logging_logging_test_LDADD = $(top_builddir)/src/libosmocore.la
@@ -92,6 +96,7 @@ EXTRA_DIST = testsuite.at $(srcdir)/package.m4 $(TESTSUITE) \ lapd/lapd_test.ok gsm0408/gsm0408_test.ok \ gsm0808/gsm0808_test.ok gb/bssgp_fc_tests.err \ gb/bssgp_fc_tests.ok gb/bssgp_fc_tests.sh \ + gb/gprs_ns_test.ok \ msgfile/msgfile_test.ok msgfile/msgconfig.cfg \ logging/logging_test.ok logging/logging_test.err \ fr/fr_test.ok loggingrb/logging_test.ok \ diff --git a/tests/gb/gprs_ns_test.c b/tests/gb/gprs_ns_test.c new file mode 100644 index 0000000..7423ff9 --- /dev/null +++ b/tests/gb/gprs_ns_test.c @@ -0,0 +1,247 @@ +/* test routines for NS connection handling + * (C) 2013 by Sysmocom GmbH + * Author: Jacob Erlbeck jerlbeck@sysmocom.de + */ + +#undef _GNU_SOURCE +#define _GNU_SOURCE + +#include <stdio.h> +#include <stdlib.h> +#include <stdint.h> +#include <string.h> +#include <getopt.h> +#include <dlfcn.h> +#include <sys/types.h> +#include <sys/socket.h> + +#include <osmocom/core/msgb.h> +#include <osmocom/core/application.h> +#include <osmocom/core/utils.h> +#include <osmocom/core/logging.h> +#include <osmocom/core/talloc.h> +#include <osmocom/gprs/gprs_msgb.h> +#include <osmocom/gprs/gprs_ns.h> +#include <osmocom/gprs/gprs_bssgp.h> + + +/* GPRS Network Service, PDU type: NS_RESET, + * Cause: O&M intervention, NS VCI: 0x1122, NSEI 0x1122 + */ +static const unsigned char gprs_ns_reset[12] = { + 0x02, 0x00, 0x81, 0x01, 0x01, 0x82, 0x11, 0x22, + 0x04, 0x82, 0x11, 0x22 +}; + +/* GPRS Network Service, PDU type: NS_RESET, + * Cause: O&M intervention, NS VCI: 0x3344, NSEI 0x1122 + */ +static const unsigned char gprs_ns_reset_vci2[12] = { + 0x02, 0x00, 0x81, 0x01, 0x01, 0x82, 0x33, 0x44, + 0x04, 0x82, 0x11, 0x22 +}; + +/* GPRS Network Service, PDU type: NS_RESET, + * Cause: O&M intervention, NS VCI: 0x1122, NSEI 0x3344 + */ +static const unsigned char gprs_ns_reset_nsei2[12] = { + 0x02, 0x00, 0x81, 0x01, 0x01, 0x82, 0x11, 0x22, + 0x04, 0x82, 0x33, 0x44 +}; + +/* GPRS Network Service, PDU type: NS_ALIVE */ +static const unsigned char gprs_ns_alive[1] = { + 0x0a +}; + +/* GPRS Network Service, PDU type: NS_STATUS, + * Cause: PDU not compatible with the protocol state + * PDU: NS_ALIVE + */ +static const unsigned char gprs_ns_status_invalid_alive[7] = { + 0x08, 0x00, 0x81, 0x0a, 0x02, 0x81, 0x0a +}; + +/* GPRS Network Service, PDU type: NS_ALIVE_ACK */ +static const unsigned char gprs_ns_alive_ack[1] = { + 0x0b +}; + +/* GPRS Network Service, PDU type: NS_UNBLOCK */ +static const unsigned char gprs_ns_unblock[1] = { + 0x06 +}; + + +/* GPRS Network Service, PDU type: NS_STATUS, + * Cause: PDU not compatible with the protocol state + * PDU: NS_RESET_ACK, NS VCI: 0x1122, NSEI 0x1122 + */ +static const unsigned char gprs_ns_status_invalid_reset_ack[15] = { + 0x08, 0x00, 0x81, 0x0a, 0x02, 0x89, 0x03, 0x01, + 0x82, 0x11, 0x22, 0x04, 0x82, 0x11, 0x22 +}; + +/* GPRS Network Service, PDU type: NS_UNITDATA, BVCI 0 */ +static const unsigned char gprs_bssgp_reset[22] = { + 0x00, 0x00, 0x00, 0x00, 0x22, 0x04, 0x82, 0x4a, + 0x2e, 0x07, 0x81, 0x08, 0x08, 0x88, 0x10, 0x20, + 0x30, 0x40, 0x50, 0x60, 0x10, 0x00 +}; + +int gprs_ns_rcvmsg(struct gprs_ns_inst *nsi, struct msgb *msg, + struct sockaddr_in *saddr, enum gprs_ns_ll ll); + +/* override */ +int gprs_ns_callback(enum gprs_ns_evt event, struct gprs_nsvc *nsvc, + struct msgb *msg, uint16_t bvci) +{ + printf("CALLBACK, event %d, msg length %d, bvci 0x%04x\n%s\n\n", + event, msgb_bssgp_len(msg), bvci, + osmo_hexdump(msgb_bssgph(msg), msgb_bssgp_len(msg))); + return 0; +} + +/* override */ +ssize_t sendto(int sockfd, const void *buf, size_t len, int flags, + const struct sockaddr *dest_addr, socklen_t addrlen) +{ + typedef ssize_t (*sendto_t)(int, const void *, size_t, int, + const struct sockaddr *, socklen_t); + static sendto_t real_sendto = NULL; + + if (!real_sendto) + real_sendto = dlsym(RTLD_NEXT, "sendto"); + + if (sockfd != 0xdead && ((struct sockaddr_in *)dest_addr)->sin_addr.s_addr != htonl(0x01020304)) + return real_sendto(sockfd, buf, len, flags, dest_addr, addrlen); + + printf("RESPONSE, msg length %d\n%s\n\n", len, osmo_hexdump(buf, len)); + + return len; +} + +static int gprs_process_message(struct gprs_ns_inst *nsi, const char *text, struct sockaddr_in *peer, const unsigned char* data, size_t data_len) +{ + struct msgb *msg; + int ret; + if (data_len > NS_ALLOC_SIZE - NS_ALLOC_HEADROOM) { + fprintf(stderr, "message too long: %d\n", data_len); + return -1; + } + + msg = gprs_ns_msgb_alloc(); + memmove(msg->data, data, data_len); + msg->l2h = msg->data; + msgb_put(msg, data_len); + + printf("PROCESSING %s from 0x%08x:%d\n%s\n\n", + text, ntohl(peer->sin_addr.s_addr), ntohs(peer->sin_port), + osmo_hexdump(data, data_len)); + + ret = gprs_ns_rcvmsg(nsi, msg, peer, GPRS_NS_LL_UDP); + + printf("result (%s) = %d\n\n", text, ret); + + msgb_free(msg); + + return ret; +} + +static void gprs_dump_nsi(struct gprs_ns_inst *nsi) +{ + struct gprs_nsvc *nsvc; + + printf("Current NS-VCIs:\n"); + llist_for_each_entry(nsvc, &nsi->gprs_nsvcs, list) { + struct sockaddr_in *peer = &(nsvc->ip.bts_addr); + printf(" VCI 0x%04x, NSEI 0x%04x, peer 0x%08x:%d\n", + nsvc->nsvci, nsvc->nsei, + ntohl(peer->sin_addr.s_addr), ntohs(peer->sin_port) + ); + } + printf("\n"); +} + +static void test_ns() +{ + struct gprs_ns_inst *nsi = gprs_ns_instantiate(gprs_ns_callback, NULL); + struct sockaddr_in peer[4] = {{0},}; + + peer[0].sin_family = AF_INET; + peer[0].sin_port = htons(1111); + peer[0].sin_addr.s_addr = htonl(0x01020304); + peer[1].sin_family = AF_INET; + peer[1].sin_port = htons(2222); + peer[1].sin_addr.s_addr = htonl(0x01020304); + peer[2].sin_family = AF_INET; + peer[2].sin_port = htons(3333); + peer[2].sin_addr.s_addr = htonl(0x01020304); + peer[3].sin_family = AF_INET; + peer[3].sin_port = htons(4444); + peer[3].sin_addr.s_addr = htonl(0x01020304); + + gprs_process_message(nsi, "RESET", &peer[0], + gprs_ns_reset, sizeof(gprs_ns_reset)); + gprs_dump_nsi(nsi); + gprs_process_message(nsi, "ALIVE", &peer[0], + gprs_ns_alive, sizeof(gprs_ns_alive)); + gprs_process_message(nsi, "UNBLOCK", &peer[0], + gprs_ns_unblock, sizeof(gprs_ns_unblock)); + gprs_process_message(nsi, "BSSGP RESET", &peer[0], + gprs_bssgp_reset, sizeof(gprs_bssgp_reset)); + + printf("--- Peer port changes, RESET, message remains unchanged ---\n\n"); + + gprs_process_message(nsi, "RESET", &peer[1], + gprs_ns_reset, sizeof(gprs_ns_reset)); + gprs_dump_nsi(nsi); + + printf("--- Peer port changes, RESET, VCI changes ---\n\n"); + + gprs_process_message(nsi, "RESET", &peer[2], + gprs_ns_reset_vci2, sizeof(gprs_ns_reset_vci2)); + gprs_dump_nsi(nsi); + + printf("--- Peer port changes, RESET, NSEI changes ---\n\n"); + + gprs_process_message(nsi, "RESET", &peer[3], + gprs_ns_reset_nsei2, sizeof(gprs_ns_reset_nsei2)); + gprs_dump_nsi(nsi); + + printf("--- Peer port 3333, RESET, VCI is changed back ---\n\n"); + + gprs_process_message(nsi, "RESET", &peer[2], + gprs_ns_reset, sizeof(gprs_ns_reset)); + gprs_dump_nsi(nsi); + + printf("--- Peer port 4444, RESET, NSEI is changed back ---\n\n"); + + gprs_process_message(nsi, "RESET", &peer[3], + gprs_ns_reset, sizeof(gprs_ns_reset)); + gprs_dump_nsi(nsi); + + gprs_ns_destroy(nsi); + nsi = NULL; +} + + +int bssgp_prim_cb(struct osmo_prim_hdr *oph, void *ctx) +{ + return -1; +} + +static struct log_info info = {}; + +int main(int argc, char **argv) +{ + osmo_init_logging(&info); + log_set_use_color(osmo_stderr_target, 0); + log_set_print_filename(osmo_stderr_target, 0); + + printf("===== NS protocol test START\n"); + test_ns(); + printf("===== NS protocol test END\n\n"); + + exit(EXIT_SUCCESS); +} diff --git a/tests/gb/gprs_ns_test.ok b/tests/gb/gprs_ns_test.ok new file mode 100644 index 0000000..4a44907 --- /dev/null +++ b/tests/gb/gprs_ns_test.ok @@ -0,0 +1,124 @@ +===== NS protocol test START +PROCESSING RESET from 0x01020304:1111 +02 00 81 01 01 82 11 22 04 82 11 22 + +RESPONSE, msg length 1 +0a + +RESPONSE, msg length 9 +03 01 82 11 22 04 82 11 22 + +result (RESET) = 9 + +Current NS-VCIs: + VCI 0x1122, NSEI 0x1122, peer 0x01020304:1111 + +PROCESSING ALIVE from 0x01020304:1111 +0a + +RESPONSE, msg length 1 +0b + +result (ALIVE) = 1 + +PROCESSING UNBLOCK from 0x01020304:1111 +06 + +RESPONSE, msg length 1 +07 + +result (UNBLOCK) = 1 + +PROCESSING BSSGP RESET from 0x01020304:1111 +00 00 00 00 22 04 82 4a 2e 07 81 08 08 88 10 20 30 40 50 60 10 00 + +CALLBACK, event 0, msg length 18, bvci 0x0000 +22 04 82 4a 2e 07 81 08 08 88 10 20 30 40 50 60 10 00 + +result (BSSGP RESET) = 0 + +--- Peer port changes, RESET, message remains unchanged --- + +PROCESSING RESET from 0x01020304:2222 +02 00 81 01 01 82 11 22 04 82 11 22 + +RESPONSE, msg length 1 +0a + +RESPONSE, msg length 9 +03 01 82 11 22 04 82 11 22 + +result (RESET) = 9 + +Current NS-VCIs: + VCI 0x1122, NSEI 0x1122, peer 0x01020304:2222 + +--- Peer port changes, RESET, VCI changes --- + +PROCESSING RESET from 0x01020304:3333 +02 00 81 01 01 82 33 44 04 82 11 22 + +RESPONSE, msg length 1 +0a + +RESPONSE, msg length 9 +03 01 82 33 44 04 82 11 22 + +result (RESET) = 9 + +Current NS-VCIs: + VCI 0x3344, NSEI 0x1122, peer 0x01020304:3333 + +--- Peer port changes, RESET, NSEI changes --- + +PROCESSING RESET from 0x01020304:4444 +02 00 81 01 01 82 11 22 04 82 33 44 + +RESPONSE, msg length 1 +0a + +RESPONSE, msg length 9 +03 01 82 11 22 04 82 33 44 + +result (RESET) = 9 + +Current NS-VCIs: + VCI 0x1122, NSEI 0x3344, peer 0x01020304:4444 + VCI 0x3344, NSEI 0x1122, peer 0x01020304:3333 + +--- Peer port 3333, RESET, VCI is changed back --- + +PROCESSING RESET from 0x01020304:3333 +02 00 81 01 01 82 11 22 04 82 11 22 + +RESPONSE, msg length 1 +0a + +RESPONSE, msg length 9 +03 01 82 11 22 04 82 11 22 + +result (RESET) = 9 + +Current NS-VCIs: + VCI 0x1122, NSEI 0x3344, peer 0x01020304:4444 + VCI 0x1122, NSEI 0x1122, peer 0x01020304:3333 + +--- Peer port 4444, RESET, NSEI is changed back --- + +PROCESSING RESET from 0x01020304:4444 +02 00 81 01 01 82 11 22 04 82 11 22 + +RESPONSE, msg length 1 +0a + +RESPONSE, msg length 9 +03 01 82 11 22 04 82 11 22 + +result (RESET) = 9 + +Current NS-VCIs: + VCI 0x1122, NSEI 0x1122, peer 0x01020304:4444 + VCI 0x1122, NSEI 0x1122, peer 0x01020304:3333 + +===== NS protocol test END + diff --git a/tests/testsuite.at b/tests/testsuite.at index 97d0adc..0685832 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -106,6 +106,12 @@ cat $abs_srcdir/vty/vty_test.ok > expout AT_CHECK([$abs_top_builddir/tests/vty/vty_test], [0], [expout], [ignore]) AT_CLEANUP
+AT_SETUP([gprs-ns]) +AT_KEYWORDS([gprs-ns]) +cat $abs_srcdir/gb/gprs_ns_test.ok > expout +AT_CHECK([$abs_top_builddir/tests/gb/gprs_ns_test], [0], [expout], [ignore]) +AT_CLEANUP + AT_SETUP([bssgp-fc]) AT_KEYWORDS([bssgp-fc]) cat $abs_srcdir/gb/bssgp_fc_tests.ok > expout
This patch refactors gprs_ns_rcvmsg() by moving the parts relevant to the NS messages into the new functions gprs_ns_vc_create() (nsvc object creation) and gprs_ns_process_msg() (main NS automaton). These do not contain code that directly depends on the link layer (they call other functions that still do). This reduces the gprs_ns_rcvmsg() function to calling these two functions and optionally setting up the link layer specific fields of the nsvc. --- include/osmocom/gprs/gprs_ns.h | 9 ++ src/gb/gprs_ns.c | 193 ++++++++++++++++++++++++++++------------ 2 files changed, 146 insertions(+), 56 deletions(-)
diff --git a/include/osmocom/gprs/gprs_ns.h b/include/osmocom/gprs/gprs_ns.h index 8ca5a88..c709312 100644 --- a/include/osmocom/gprs/gprs_ns.h +++ b/include/osmocom/gprs/gprs_ns.h @@ -49,6 +49,15 @@ enum gprs_ns_evt { GPRS_NS_EVT_UNIT_DATA, };
+/*! \brief Osmocom NS VC create status */ +enum gprs_ns_cs { + GPRS_NS_CS_CREATED, /*!< A NSVC object has been created */ + GPRS_NS_CS_FOUND, /*!< A NSVC object has been found */ + GPRS_NS_CS_REJECTED, /*!< Rejected and answered message */ + GPRS_NS_CS_SKIPPED, /*!< Skipped message */ + GPRS_NS_CS_ERROR, /*!< Failed to process message */ +}; + struct gprs_nsvc; /*! \brief Osmocom GPRS callback function type */ typedef int gprs_ns_cb_t(enum gprs_ns_evt event, struct gprs_nsvc *nsvc, diff --git a/src/gb/gprs_ns.c b/src/gb/gprs_ns.c index 5620b3a..e41c118 100644 --- a/src/gb/gprs_ns.c +++ b/src/gb/gprs_ns.c @@ -730,6 +730,13 @@ static int gprs_ns_rx_block(struct gprs_nsvc *nsvc, struct msgb *msg) return gprs_ns_tx_simple(nsvc, NS_PDUT_BLOCK_ACK); }
+int gprs_ns_vc_create(struct gprs_ns_inst *nsi, struct msgb *msg, + struct gprs_nsvc *fallback_nsvc, + struct gprs_nsvc **new_nsvc); + +int gprs_ns_process_msg(struct gprs_ns_inst *nsi, struct msgb *msg, + struct gprs_nsvc *nsvc); + /*! \brief Receive incoming NS message from underlying transport layer * \param nsi NS instance to which the data belongs * \param[in] msg message buffer containing newly-received data @@ -743,72 +750,146 @@ static int gprs_ns_rx_block(struct gprs_nsvc *nsvc, struct msgb *msg) int gprs_ns_rcvmsg(struct gprs_ns_inst *nsi, struct msgb *msg, struct sockaddr_in *saddr, enum gprs_ns_ll ll) { - struct gprs_ns_hdr *nsh = (struct gprs_ns_hdr *) msg->l2h; struct gprs_nsvc *nsvc; int rc = 0;
/* look up the NSVC based on source address */ nsvc = nsvc_by_rem_addr(nsi, saddr); + if (!nsvc) { - struct tlv_parsed tp; - uint16_t nsei; - if (nsh->pdu_type == NS_PDUT_STATUS) { - LOGP(DNS, LOGL_INFO, "Ignoring NS STATUS from %s:%u " - "for non-existing NS-VC\n", - inet_ntoa(saddr->sin_addr), ntohs(saddr->sin_port)); - return 0; - } - /* Only the RESET procedure creates a new NSVC */ - if (nsh->pdu_type != NS_PDUT_RESET) { - /* Since we have no NSVC, we have to use a fake */ - nsvc = nsi->unknown_nsvc; - log_set_context(GPRS_CTX_NSVC, nsvc); - LOGP(DNS, LOGL_INFO, "Rejecting NS PDU type 0x%0x " - "from %s:%u for non-existing NS-VC\n", - nsh->pdu_type, inet_ntoa(saddr->sin_addr), - ntohs(saddr->sin_port)); - nsvc->nsvci = nsvc->nsei = 0xfffe; - nsvc->ip.bts_addr = *saddr; - nsvc->state = NSE_S_ALIVE; + struct gprs_nsvc *fallback_nsvc; + + fallback_nsvc = nsi->unknown_nsvc; + log_set_context(GPRS_CTX_NSVC, fallback_nsvc); + fallback_nsvc->ip.bts_addr = *saddr; + fallback_nsvc->ll = ll; + + rc = gprs_ns_vc_create(nsi, msg, fallback_nsvc, &nsvc); + + switch (rc) { + case GPRS_NS_CS_CREATED: + case GPRS_NS_CS_FOUND: nsvc->ll = ll; -#if 0 - return gprs_ns_tx_reset(nsvc, NS_CAUSE_PDU_INCOMP_PSTATE); -#else - return gprs_ns_tx_status(nsvc, - NS_CAUSE_PDU_INCOMP_PSTATE, 0, - msg); -#endif - } - rc = tlv_parse(&tp, &ns_att_tlvdef, nsh->data, - msgb_l2len(msg) - sizeof(*nsh), 0, 0); - if (rc < 0) { - LOGP(DNS, LOGL_ERROR, "Rx NS RESET Error %d during " - "TLV Parse\n", rc); + break; + case GPRS_NS_CS_SKIPPED: + case GPRS_NS_CS_REJECTED: + break; + default: return rc; } - if (!TLVP_PRESENT(&tp, NS_IE_CAUSE) || - !TLVP_PRESENT(&tp, NS_IE_VCI) || - !TLVP_PRESENT(&tp, NS_IE_NSEI)) { - LOGP(DNS, LOGL_ERROR, "NS RESET Missing mandatory IE\n"); - gprs_ns_tx_status(nsvc, NS_CAUSE_MISSING_ESSENT_IE, 0, - msg); - return -EINVAL; - } - nsei = ntohs(*(uint16_t *)TLVP_VAL(&tp, NS_IE_NSEI)); - /* Check if we already know this NSEI, the remote end might - * simply have changed addresses, or it is a SGSN */ - nsvc = gprs_nsvc_by_nsei(nsi, nsei); - if (!nsvc) { - nsvc = gprs_nsvc_create(nsi, 0xffff); - nsvc->ll = ll; - log_set_context(GPRS_CTX_NSVC, nsvc); - LOGP(DNS, LOGL_INFO, "Creating NS-VC for BSS at %s:%u\n", - inet_ntoa(saddr->sin_addr), ntohs(saddr->sin_port)); - } - /* Update the remote peer IP address/port */ + + rc = 0; + } + + if (nsvc) { nsvc->ip.bts_addr = *saddr; - } else - msgb_nsei(msg) = nsvc->nsei; + rc = gprs_ns_process_msg(nsi, msg, nsvc); + } + + return rc; +} + +const char *gprs_ns_format_peer(struct gprs_nsvc *nsvc) +{ + static char buf[80]; + snprintf(buf, sizeof(buf), "%s:%u", + inet_ntoa(nsvc->ip.bts_addr.sin_addr), + ntohs(nsvc->ip.bts_addr.sin_port)); + + return buf; +} + +/*! \brief Create/get NS-VC independently from underlying transport layer + * \param nsi NS instance to which the data belongs + * \param[in] msg message buffer containing newly-received data + * \param[in] fallback_nsvc is used to send error messages back to the peer + * \param[out] new_nsvc contains a pointer to a NS-VC object if one has + * been created or found + * \returns < 0 in case of error, GPRS_NS_CS_SKIPPED if a message has been + * skipped, GPRS_NS_CS_REJECTED if a message has been rejected and + * answered accordingly, GPRS_NS_CS_CREATED if a new NS-VC object + * has been created and registered, and GPRS_NS_CS_FOUND if an + * existing NS-VC object has been found with the same NSEI. + * + * This contains the initial NS automaton state (NS-VC not yet attached). + */ +int gprs_ns_vc_create(struct gprs_ns_inst *nsi, struct msgb *msg, + struct gprs_nsvc *fallback_nsvc, + struct gprs_nsvc **new_nsvc) +{ + struct gprs_ns_hdr *nsh = (struct gprs_ns_hdr *)msg->l2h; + struct gprs_nsvc *existing_nsvc; + + struct tlv_parsed tp; + uint16_t nsei; + + int rc; + + if (nsh->pdu_type == NS_PDUT_STATUS) { + LOGP(DNS, LOGL_INFO, "Ignoring NS STATUS from %s " + "for non-existing NS-VC\n", + gprs_ns_format_peer(fallback_nsvc)); + return GPRS_NS_CS_SKIPPED; + } + /* Only the RESET procedure creates a new NSVC */ + if (nsh->pdu_type != NS_PDUT_RESET) { + /* Since we have no NSVC, we have to use a fake */ + log_set_context(GPRS_CTX_NSVC, fallback_nsvc); + LOGP(DNS, LOGL_INFO, "Rejecting NS PDU type 0x%0x " + "from %s for non-existing NS-VC\n", + nsh->pdu_type, gprs_ns_format_peer(fallback_nsvc)); + fallback_nsvc->nsvci = fallback_nsvc->nsei = 0xfffe; + fallback_nsvc->state = NSE_S_ALIVE; + + return gprs_ns_tx_status(fallback_nsvc, + NS_CAUSE_PDU_INCOMP_PSTATE, 0, msg); + } + rc = tlv_parse(&tp, &ns_att_tlvdef, nsh->data, + msgb_l2len(msg) - sizeof(*nsh), 0, 0); + if (rc < 0) { + LOGP(DNS, LOGL_ERROR, "Rx NS RESET Error %d during " + "TLV Parse\n", rc); + return rc; + } + if (!TLVP_PRESENT(&tp, NS_IE_CAUSE) || + !TLVP_PRESENT(&tp, NS_IE_VCI) || !TLVP_PRESENT(&tp, NS_IE_NSEI)) { + LOGP(DNS, LOGL_ERROR, "NS RESET Missing mandatory IE\n"); + gprs_ns_tx_status(fallback_nsvc, NS_CAUSE_MISSING_ESSENT_IE, 0, + msg); + return -EINVAL; + } + nsei = ntohs(*(uint16_t *) TLVP_VAL(&tp, NS_IE_NSEI)); + /* Check if we already know this NSEI, the remote end might + * simply have changed addresses, or it is a SGSN */ + existing_nsvc = gprs_nsvc_by_nsei(nsi, nsei); + if (!existing_nsvc) { + *new_nsvc = gprs_nsvc_create(nsi, 0xffff); + log_set_context(GPRS_CTX_NSVC, *new_nsvc); + LOGP(DNS, LOGL_INFO, "Creating NS-VC for BSS at %s\n", + gprs_ns_format_peer(fallback_nsvc)); + + return GPRS_NS_CS_CREATED; + } + + *new_nsvc = existing_nsvc; + return GPRS_NS_CS_FOUND; +} + +/*! \brief Process NS message independently from underlying transport layer + * \param nsi NS instance to which the data belongs + * \param[in] msg message buffer containing newly-received data + * \param[in] nsvc refers to the virtual connection + * \returns 0 in case of success, < 0 in case of error + * + * This contains the main NS automaton. + */ +int gprs_ns_process_msg(struct gprs_ns_inst *nsi, struct msgb *msg, + struct gprs_nsvc *nsvc) +{ + struct gprs_ns_hdr *nsh = (struct gprs_ns_hdr *) msg->l2h; + int rc = 0; + + msgb_nsei(msg) = nsvc->nsei;
log_set_context(GPRS_CTX_NSVC, nsvc);
According to 3GPP TS 08.16, 7.3 "Reset procedure" the entity receiving a NS-RESET PDU responds with a NS-RESET-ACK and 'then' starts the test procedure which essentially means, that a NS-ALIVE gets sent and a timer is started.
Currently the NS-ALIVE is sent before the NS-RESET-ACK.
This patch fixes the implementation by reversing the order in which these messages are sent. --- src/gb/gprs_ns.c | 12 +++++++----- tests/gb/gprs_ns_test.ok | 36 ++++++++++++++++++------------------ 2 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/src/gb/gprs_ns.c b/src/gb/gprs_ns.c index e41c118..bba75bc 100644 --- a/src/gb/gprs_ns.c +++ b/src/gb/gprs_ns.c @@ -684,15 +684,17 @@ static int gprs_ns_rx_reset(struct gprs_nsvc *nsvc, struct msgb *msg) nsvc->nsei = ntohs(*nsei); nsvc->nsvci = ntohs(*nsvci);
- /* start the test procedure */ - gprs_ns_tx_simple(nsvc, NS_PDUT_ALIVE); - nsvc_start_timer(nsvc, NSVC_TIMER_TNS_TEST); - /* inform interested parties about the fact that this NSVC * has received RESET */ ns_osmo_signal_dispatch(nsvc, S_NS_RESET, *cause);
- return gprs_ns_tx_reset_ack(nsvc); + rc = gprs_ns_tx_reset_ack(nsvc); + + /* start the test procedure */ + gprs_ns_tx_simple(nsvc, NS_PDUT_ALIVE); + nsvc_start_timer(nsvc, NSVC_TIMER_TNS_TEST); + + return rc; }
static int gprs_ns_rx_block(struct gprs_nsvc *nsvc, struct msgb *msg) diff --git a/tests/gb/gprs_ns_test.ok b/tests/gb/gprs_ns_test.ok index 4a44907..049b5e6 100644 --- a/tests/gb/gprs_ns_test.ok +++ b/tests/gb/gprs_ns_test.ok @@ -2,12 +2,12 @@ PROCESSING RESET from 0x01020304:1111 02 00 81 01 01 82 11 22 04 82 11 22
-RESPONSE, msg length 1 -0a - RESPONSE, msg length 9 03 01 82 11 22 04 82 11 22
+RESPONSE, msg length 1 +0a + result (RESET) = 9
Current NS-VCIs: @@ -42,12 +42,12 @@ result (BSSGP RESET) = 0 PROCESSING RESET from 0x01020304:2222 02 00 81 01 01 82 11 22 04 82 11 22
-RESPONSE, msg length 1 -0a - RESPONSE, msg length 9 03 01 82 11 22 04 82 11 22
+RESPONSE, msg length 1 +0a + result (RESET) = 9
Current NS-VCIs: @@ -58,12 +58,12 @@ Current NS-VCIs: PROCESSING RESET from 0x01020304:3333 02 00 81 01 01 82 33 44 04 82 11 22
-RESPONSE, msg length 1 -0a - RESPONSE, msg length 9 03 01 82 33 44 04 82 11 22
+RESPONSE, msg length 1 +0a + result (RESET) = 9
Current NS-VCIs: @@ -74,12 +74,12 @@ Current NS-VCIs: PROCESSING RESET from 0x01020304:4444 02 00 81 01 01 82 11 22 04 82 33 44
-RESPONSE, msg length 1 -0a - RESPONSE, msg length 9 03 01 82 11 22 04 82 33 44
+RESPONSE, msg length 1 +0a + result (RESET) = 9
Current NS-VCIs: @@ -91,12 +91,12 @@ Current NS-VCIs: PROCESSING RESET from 0x01020304:3333 02 00 81 01 01 82 11 22 04 82 11 22
-RESPONSE, msg length 1 -0a - RESPONSE, msg length 9 03 01 82 11 22 04 82 11 22
+RESPONSE, msg length 1 +0a + result (RESET) = 9
Current NS-VCIs: @@ -108,12 +108,12 @@ Current NS-VCIs: PROCESSING RESET from 0x01020304:4444 02 00 81 01 01 82 11 22 04 82 11 22
-RESPONSE, msg length 1 -0a - RESPONSE, msg length 9 03 01 82 11 22 04 82 11 22
+RESPONSE, msg length 1 +0a + result (RESET) = 9
Current NS-VCIs:
Currently when a NS-RESET is recevied over a link that has not yet been associated with a NS-VC, the NSEI is used to find an existing NS-VC. If one is found, the reset procedure is initiated.
This behaviour is not conformant with 3GPP TS 08.16 (see chapter 4.2.3) which allows to use several NS-VC between two endpoints in parallel.
The patch changes the implementation to use the NSVCI instead of the NSEI to search for an existing NS-VC object. --- src/gb/gprs_ns.c | 8 ++++---- tests/gb/gprs_ns_test.ok | 7 ++++--- 2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/gb/gprs_ns.c b/src/gb/gprs_ns.c index bba75bc..4414bd5 100644 --- a/src/gb/gprs_ns.c +++ b/src/gb/gprs_ns.c @@ -823,7 +823,7 @@ int gprs_ns_vc_create(struct gprs_ns_inst *nsi, struct msgb *msg, struct gprs_nsvc *existing_nsvc;
struct tlv_parsed tp; - uint16_t nsei; + uint16_t nsvci;
int rc;
@@ -860,10 +860,10 @@ int gprs_ns_vc_create(struct gprs_ns_inst *nsi, struct msgb *msg, msg); return -EINVAL; } - nsei = ntohs(*(uint16_t *) TLVP_VAL(&tp, NS_IE_NSEI)); - /* Check if we already know this NSEI, the remote end might + nsvci = ntohs(*(uint16_t *) TLVP_VAL(&tp, NS_IE_VCI)); + /* Check if we already know this NSVCI, the remote end might * simply have changed addresses, or it is a SGSN */ - existing_nsvc = gprs_nsvc_by_nsei(nsi, nsei); + existing_nsvc = gprs_nsvc_by_nsvci(nsi, nsvci); if (!existing_nsvc) { *new_nsvc = gprs_nsvc_create(nsi, 0xffff); log_set_context(GPRS_CTX_NSVC, *new_nsvc); diff --git a/tests/gb/gprs_ns_test.ok b/tests/gb/gprs_ns_test.ok index 049b5e6..578b905 100644 --- a/tests/gb/gprs_ns_test.ok +++ b/tests/gb/gprs_ns_test.ok @@ -68,6 +68,7 @@ result (RESET) = 9
Current NS-VCIs: VCI 0x3344, NSEI 0x1122, peer 0x01020304:3333 + VCI 0x1122, NSEI 0x1122, peer 0x01020304:2222
--- Peer port changes, RESET, NSEI changes ---
@@ -83,8 +84,8 @@ RESPONSE, msg length 1 result (RESET) = 9
Current NS-VCIs: - VCI 0x1122, NSEI 0x3344, peer 0x01020304:4444 VCI 0x3344, NSEI 0x1122, peer 0x01020304:3333 + VCI 0x1122, NSEI 0x3344, peer 0x01020304:4444
--- Peer port 3333, RESET, VCI is changed back ---
@@ -100,8 +101,8 @@ RESPONSE, msg length 1 result (RESET) = 9
Current NS-VCIs: - VCI 0x1122, NSEI 0x3344, peer 0x01020304:4444 VCI 0x1122, NSEI 0x1122, peer 0x01020304:3333 + VCI 0x1122, NSEI 0x3344, peer 0x01020304:4444
--- Peer port 4444, RESET, NSEI is changed back ---
@@ -117,8 +118,8 @@ RESPONSE, msg length 1 result (RESET) = 9
Current NS-VCIs: - VCI 0x1122, NSEI 0x1122, peer 0x01020304:4444 VCI 0x1122, NSEI 0x1122, peer 0x01020304:3333 + VCI 0x1122, NSEI 0x1122, peer 0x01020304:4444
===== NS protocol test END
Under special circumstances 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.
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 | 45 +++++++++++++++++++++++++++++++--------- tests/gb/gprs_ns_test.ok | 2 -- 3 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/include/osmocom/gprs/gprs_ns.h b/include/osmocom/gprs/gprs_ns.h index c709312..562894a 100644 --- a/include/osmocom/gprs/gprs_ns.h +++ b/include/osmocom/gprs/gprs_ns.h @@ -117,8 +117,11 @@ struct gprs_nsvc { enum nsvc_timer_mode timer_mode; int alive_retries;
+ int dup_deletion_ctr; + unsigned int remote_end_is_sgsn:1; unsigned int persistent:1; + unsigned int nsvci_is_valid:1;
struct rate_ctr_group *ctrg;
diff --git a/src/gb/gprs_ns.c b/src/gb/gprs_ns.c index 4414bd5..8bd1df2 100644 --- a/src/gb/gprs_ns.c +++ b/src/gb/gprs_ns.c @@ -83,6 +83,8 @@
#include "common_vty.h"
+const char *gprs_ns_format_peer(struct gprs_nsvc *nsvc); + static const struct tlv_definition ns_att_tlvdef = { .def = { [NS_IE_CAUSE] = { TLV_TYPE_TvLV, 0 }, @@ -651,8 +653,8 @@ 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; int rc;
rc = tlv_parse(&tp, &ns_att_tlvdef, nsh->data, @@ -671,22 +673,45 @@ 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 */ + struct gprs_nsvc *other_nsvc; + other_nsvc = gprs_nsvc_by_nsvci(nsvc->nsi, nsvci); + + if (other_nsvc) { + /* The NS-VCI is already used by this NS-VC */ + + char *old_peer = strdup(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)); + free(old_peer); + + nsvc->dup_deletion_ctr = 1 + other_nsvc->dup_deletion_ctr; + + /* Delete the 'old' object */ + gprs_nsvc_delete(other_nsvc); + } + }
/* 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;
/* 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.ok b/tests/gb/gprs_ns_test.ok index 578b905..7416f15 100644 --- a/tests/gb/gprs_ns_test.ok +++ b/tests/gb/gprs_ns_test.ok @@ -102,7 +102,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 ---
@@ -118,7 +117,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
Register an osmo signal handler to print a short notice about every SS_L_NS signal that is generated while processing the tests.
Sponsored-by: On-Waves ehf --- include/osmocom/gprs/gprs_ns.h | 3 +++ src/gb/libosmogb.map | 1 + tests/gb/gprs_ns_test.c | 46 +++++++++++++++++++++++++++++++++++++++- tests/gb/gprs_ns_test.ok | 7 ++++++ 4 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/include/osmocom/gprs/gprs_ns.h b/include/osmocom/gprs/gprs_ns.h index c709312..d16068b 100644 --- a/include/osmocom/gprs/gprs_ns.h +++ b/include/osmocom/gprs/gprs_ns.h @@ -176,6 +176,9 @@ void gprs_nsvc_reset(struct gprs_nsvc *nsvc, uint8_t cause); /* Add NS-specific VTY stuff */ int gprs_ns_vty_init(struct gprs_ns_inst *nsi);
+/* Resturn peer info as string (NOTE: the buffer is allocated statically) */ +const char *gprs_ns_format_peer(struct gprs_nsvc *nsvc); + #define NS_ALLOC_SIZE 2048 #define NS_ALLOC_HEADROOM 20 static inline struct msgb *gprs_ns_msgb_alloc(void) diff --git a/src/gb/libosmogb.map b/src/gb/libosmogb.map index 7af085c..0270db8 100644 --- a/src/gb/libosmogb.map +++ b/src/gb/libosmogb.map @@ -54,6 +54,7 @@ gprs_ns_tx_reset; gprs_ns_tx_status; gprs_ns_tx_unblock; gprs_ns_vty_init; +gprs_ns_format_peer;
gprs_nsvc_create; gprs_nsvc_delete; diff --git a/tests/gb/gprs_ns_test.c b/tests/gb/gprs_ns_test.c index d07cc3d..d41fccb 100644 --- a/tests/gb/gprs_ns_test.c +++ b/tests/gb/gprs_ns_test.c @@ -20,11 +20,11 @@ #include <osmocom/core/utils.h> #include <osmocom/core/logging.h> #include <osmocom/core/talloc.h> +#include <osmocom/core/signal.h> #include <osmocom/gprs/gprs_msgb.h> #include <osmocom/gprs/gprs_ns.h> #include <osmocom/gprs/gprs_bssgp.h>
- /* GPRS Network Service, PDU type: NS_RESET, * Cause: O&M intervention, NS VCI: 0x1122, NSEI 0x1122 */ @@ -121,6 +121,49 @@ ssize_t sendto(int sockfd, const void *buf, size_t len, int flags, return len; }
+/* Signal handler for signals from NS layer */ +static int test_signal(unsigned int subsys, unsigned int signal, + void *handler_data, void *signal_data) +{ + struct ns_signal_data *nssd = signal_data; + + if (subsys != SS_L_NS) + return 0; + + switch (signal) { + case S_NS_RESET: + printf("==> got signal NS_RESET, NS-VC 0x%04x/%s\n", + nssd->nsvc->nsvci, + gprs_ns_format_peer(nssd->nsvc)); + break; + + case S_NS_ALIVE_EXP: + printf("==> got signal NS_ALIVE_EXP, NS-VC 0x%04x/%s\n", + nssd->nsvc->nsvci, + gprs_ns_format_peer(nssd->nsvc)); + break; + + case S_NS_BLOCK: + printf("==> got signal NS_BLOCK, NS-VC 0x%04x/%s\n", + nssd->nsvc->nsvci, + gprs_ns_format_peer(nssd->nsvc)); + break; + + case S_NS_UNBLOCK: + printf("==> got signal NS_UNBLOCK, NS-VC 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, + gprs_ns_format_peer(nssd->nsvc)); + break; + } + return 0; +} + static int gprs_process_message(struct gprs_ns_inst *nsi, const char *text, struct sockaddr_in *peer, const unsigned char* data, size_t data_len) { struct msgb *msg; @@ -238,6 +281,7 @@ int main(int argc, char **argv) osmo_init_logging(&info); log_set_use_color(osmo_stderr_target, 0); log_set_print_filename(osmo_stderr_target, 0); + osmo_signal_register_handler(SS_L_NS, &test_signal, NULL);
printf("===== NS protocol test START\n"); test_ns(); diff --git a/tests/gb/gprs_ns_test.ok b/tests/gb/gprs_ns_test.ok index 578b905..01b1bc9 100644 --- a/tests/gb/gprs_ns_test.ok +++ b/tests/gb/gprs_ns_test.ok @@ -2,6 +2,7 @@ PROCESSING RESET from 0x01020304:1111 02 00 81 01 01 82 11 22 04 82 11 22
+==> got signal NS_RESET, NS-VC 0x1122/1.2.3.4:1111 RESPONSE, msg length 9 03 01 82 11 22 04 82 11 22
@@ -24,6 +25,7 @@ result (ALIVE) = 1 PROCESSING UNBLOCK from 0x01020304:1111 06
+==> got signal NS_UNBLOCK, NS-VC 0x1122/1.2.3.4:1111 RESPONSE, msg length 1 07
@@ -42,6 +44,7 @@ result (BSSGP RESET) = 0 PROCESSING RESET from 0x01020304:2222 02 00 81 01 01 82 11 22 04 82 11 22
+==> got signal NS_RESET, NS-VC 0x1122/1.2.3.4:2222 RESPONSE, msg length 9 03 01 82 11 22 04 82 11 22
@@ -58,6 +61,7 @@ Current NS-VCIs: PROCESSING RESET from 0x01020304:3333 02 00 81 01 01 82 33 44 04 82 11 22
+==> got signal NS_RESET, NS-VC 0x3344/1.2.3.4:3333 RESPONSE, msg length 9 03 01 82 33 44 04 82 11 22
@@ -75,6 +79,7 @@ Current NS-VCIs: PROCESSING RESET from 0x01020304:4444 02 00 81 01 01 82 11 22 04 82 33 44
+==> got signal NS_RESET, NS-VC 0x1122/1.2.3.4:4444 RESPONSE, msg length 9 03 01 82 11 22 04 82 33 44
@@ -92,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_RESET, NS-VC 0x1122/1.2.3.4:3333 RESPONSE, msg length 9 03 01 82 11 22 04 82 11 22
@@ -109,6 +115,7 @@ Current NS-VCIs: PROCESSING RESET from 0x01020304:4444 02 00 81 01 01 82 11 22 04 82 11 22
+==> got signal NS_RESET, NS-VC 0x1122/1.2.3.4:4444 RESPONSE, msg length 9 03 01 82 11 22 04 82 11 22
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
On Wed, Oct 09, 2013 at 11:27:05AM +0200, Jacob Erlbeck wrote:
Hi,
This happens when the source port of a NS-VC changes to a new one that has already been used by another NS-VC.
these two patches looks good (minor style/indent issue with the switch/case in the previous patch). We have a semantic change now (deleting nsvc's) and we have ABI changes. So what I would like to do is to merge your changes, bump the SO_VERSION, make a release and then make gbpoxy/OpenBSC depend on the latest libosmocore release.
I think we have some more API changes coming up?
holger
Hi,
On 10/09/2013 03:35 PM, Holger Hans Peter Freyther wrote:
We have a semantic change now (deleting nsvc's) and we have ABI changes. So what I would like to do is to merge your changes, bump the SO_VERSION, make a release and then make gbpoxy/OpenBSC depend on the latest libosmocore release.
I think we have some more API changes coming up?
If we address the usage of NSEI instead of NSVCI at the BSSGP layer we'll probably have.
Jacob
-
Holger Hans Peter Freyther -
Jacob Erlbeck