Currently the BVCI is not set in all invocations to bssgp_tx_status() when the cause is UNKNOWN_BVCI.
This patch adds the argument where it is missing.
It also adds a check for compliance (GSM 08.18, 10.4.14.1) to bssgp_tx_status() to emit errors when the following requirement is not fulfilled: The BVCI must be included if (and only if) the cause is either "BVCI blocked" or "BVCI unknown".
Sponsored-by: On-Waves ehf --- src/gb/gprs_bssgp.c | 2 +- src/gb/gprs_bssgp_util.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c index 5ef1887..b8c6c74 100644 --- a/src/gb/gprs_bssgp.c +++ b/src/gb/gprs_bssgp.c @@ -998,7 +998,7 @@ int bssgp_rcvmsg(struct msgb *msg) LOGP(DBSSGP, LOGL_NOTICE, "NSEI=%u/BVCI=%u Rejecting PDU " "type %u for unknown BVCI\n", msgb_nsei(msg), ns_bvci, pdu_type); - return bssgp_tx_status(BSSGP_CAUSE_UNKNOWN_BVCI, NULL, msg); + return bssgp_tx_status(BSSGP_CAUSE_UNKNOWN_BVCI, &ns_bvci, msg); }
if (bctx) { diff --git a/src/gb/gprs_bssgp_util.c b/src/gb/gprs_bssgp_util.c index ae4647e..261e0b0 100644 --- a/src/gb/gprs_bssgp_util.c +++ b/src/gb/gprs_bssgp_util.c @@ -99,6 +99,20 @@ int bssgp_tx_status(uint8_t cause, uint16_t *bvci, struct msgb *orig_msg) struct bssgp_normal_hdr *bgph = (struct bssgp_normal_hdr *) msgb_put(msg, sizeof(*bgph));
+ /* GSM 08.18, 10.4.14.1: The BVCI must be included if (and only if) the + cause is either "BVCI blocked" or "BVCI unknown" */ + if (cause == BSSGP_CAUSE_UNKNOWN_BVCI || cause == BSSGP_CAUSE_BVCI_BLOCKED) { + if (bvci == NULL) + LOGP(DBSSGP, LOGL_ERROR, "BSSGP Tx STATUS, cause=%s: " + "missing conditional BVCI\n", + bssgp_cause_str(cause)); + } else { + if (bvci != NULL) + LOGP(DBSSGP, LOGL_ERROR, "BSSGP Tx STATUS, cause=%s: " + "unexpected conditional BVCI\n", + bssgp_cause_str(cause)); + } + LOGP(DBSSGP, LOGL_NOTICE, "BSSGP BVCI=%u Tx STATUS, cause=%s\n", bvci ? *bvci : 0, bssgp_cause_str(cause)); msgb_nsei(msg) = msgb_nsei(orig_msg);
Currently BSSGP messages with an NS BVCI of 0 (signalling) are discarded if they aren't RESET messages. Thus valid signalling messages (e.g. BLOCK) are not handled properly, because the BVCI IE is ignored if it present. Instead a STATUS message referring to BVCI 0 (instead of the BVCI used in the BLOCK message) is returned.
This patch changes the implementation to use the BVCI contained in the BVCI IE if that is present in a signalling message.
It fixes BSSGP BLOCK/UNBLOCK for the osmo-sgsn.
Note that signalling messages without an BVCI IE (e.g. SUSPEND/RESUME) are still rejected.
Ticket: OW#1205 Sponsored-by: On-Waves ehf --- src/gb/gprs_bssgp.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c index b8c6c74..0e9fd38 100644 --- a/src/gb/gprs_bssgp.c +++ b/src/gb/gprs_bssgp.c @@ -976,6 +976,7 @@ int bssgp_rcvmsg(struct msgb *msg) struct bssgp_bvc_ctx *bctx; uint8_t pdu_type = bgph->pdu_type; uint16_t ns_bvci = msgb_bvci(msg); + uint16_t bvci = ns_bvci; int data_len; int rc = 0;
@@ -991,14 +992,17 @@ int bssgp_rcvmsg(struct msgb *msg) rc = bssgp_tlv_parse(&tp, budh->data, data_len); }
+ if (bvci == BVCI_SIGNALLING && TLVP_PRESENT(&tp, BSSGP_IE_BVCI)) + bvci = ntohs(*(uint16_t *)TLVP_VAL(&tp, BSSGP_IE_BVCI)); + /* look-up or create the BTS context for this BVC */ - bctx = btsctx_by_bvci_nsei(ns_bvci, msgb_nsei(msg)); + bctx = btsctx_by_bvci_nsei(bvci, msgb_nsei(msg)); /* Only a RESET PDU can create a new BVC context */ if (!bctx && pdu_type != BSSGP_PDUT_BVC_RESET) { LOGP(DBSSGP, LOGL_NOTICE, "NSEI=%u/BVCI=%u Rejecting PDU " - "type %u for unknown BVCI\n", msgb_nsei(msg), ns_bvci, + "type %u for unknown BVCI\n", msgb_nsei(msg), bvci, pdu_type); - return bssgp_tx_status(BSSGP_CAUSE_UNKNOWN_BVCI, &ns_bvci, msg); + return bssgp_tx_status(BSSGP_CAUSE_UNKNOWN_BVCI, &bvci, msg); }
if (bctx) {
On Tue, Sep 23, 2014 at 01:28:22PM +0200, Jacob Erlbeck wrote:
Currently BSSGP messages with an NS BVCI of 0 (signalling) are discarded if they aren't RESET messages. Thus valid signalling messages (e.g. BLOCK) are not handled properly, because the BVCI IE is ignored if it present. Instead a STATUS message referring to BVCI
^^^^^ "if present"?
Currently sending SUSPEND/RESUME messages to this function (like it is done in the osmo-sgsn) results in STATUS messages complaining about an unknown BVCI. The reason is, that these messages rely on a TLLI/RAI pair to identify the context and do not contain an explicit BVCI.
This patch modifies bssgp_rcvmsg() to only complain about and unknown BVCI if one is given but a matching context is not found (except for RESET messages). The ctx argument is removed from the functions handling SUSPEND and RESUME since it will always be NULL then.
TODO: - test cases - end-to-end test
Sponsored-by: On-Waves ehf --- src/gb/gprs_bssgp.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c index 0e9fd38..eee638c 100644 --- a/src/gb/gprs_bssgp.c +++ b/src/gb/gprs_bssgp.c @@ -398,32 +398,32 @@ static int bssgp_rx_ul_ud(struct msgb *msg, struct tlv_parsed *tp, return bssgp_prim_cb(&gbp.oph, NULL); }
-static int bssgp_rx_suspend(struct msgb *msg, struct tlv_parsed *tp, - struct bssgp_bvc_ctx *ctx) +static int bssgp_rx_suspend(struct msgb *msg, struct tlv_parsed *tp) { struct osmo_bssgp_prim gbp; struct gprs_ra_id raid; uint32_t tlli; + uint16_t ns_bvci = msgb_bvci(msg); int rc;
if (!TLVP_PRESENT(tp, BSSGP_IE_TLLI) || !TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA)) { LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx SUSPEND " - "missing mandatory IE\n", ctx->bvci); + "missing mandatory IE\n", ns_bvci); return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg); }
tlli = ntohl(*(uint32_t *)TLVP_VAL(tp, BSSGP_IE_TLLI));
DEBUGP(DBSSGP, "BSSGP BVCI=%u TLLI=0x%08x Rx SUSPEND\n", - ctx->bvci, tlli); + ns_bvci, tlli);
gsm48_parse_ra(&raid, TLVP_VAL(tp, BSSGP_IE_ROUTEING_AREA));
/* Inform GMM about the SUSPEND request */ memset(&gbp, 0, sizeof(gbp)); gbp.nsei = msgb_nsei(msg); - gbp.bvci = ctx->bvci; + gbp.bvci = ns_bvci; gbp.tlli = tlli; gbp.ra_id = &raid; osmo_prim_init(&gbp.oph, SAP_BSSGP_GMM, PRIM_BSSGP_GMM_SUSPEND, @@ -438,34 +438,34 @@ static int bssgp_rx_suspend(struct msgb *msg, struct tlv_parsed *tp, return 0; }
-static int bssgp_rx_resume(struct msgb *msg, struct tlv_parsed *tp, - struct bssgp_bvc_ctx *ctx) +static int bssgp_rx_resume(struct msgb *msg, struct tlv_parsed *tp) { struct osmo_bssgp_prim gbp; struct gprs_ra_id raid; uint32_t tlli; uint8_t suspend_ref; + uint16_t ns_bvci = msgb_bvci(msg); int rc;
if (!TLVP_PRESENT(tp, BSSGP_IE_TLLI) || !TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA) || !TLVP_PRESENT(tp, BSSGP_IE_SUSPEND_REF_NR)) { LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx RESUME " - "missing mandatory IE\n", ctx->bvci); + "missing mandatory IE\n", ns_bvci); return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg); }
tlli = ntohl(*(uint32_t *)TLVP_VAL(tp, BSSGP_IE_TLLI)); suspend_ref = *TLVP_VAL(tp, BSSGP_IE_SUSPEND_REF_NR);
- DEBUGP(DBSSGP, "BSSGP BVCI=%u TLLI=0x%08x Rx RESUME\n", ctx->bvci, tlli); + DEBUGP(DBSSGP, "BSSGP BVCI=%u TLLI=0x%08x Rx RESUME\n", ns_bvci, tlli);
gsm48_parse_ra(&raid, TLVP_VAL(tp, BSSGP_IE_ROUTEING_AREA));
/* Inform GMM about the RESUME request */ memset(&gbp, 0, sizeof(gbp)); gbp.nsei = msgb_nsei(msg); - gbp.bvci = ctx->bvci; + gbp.bvci = ns_bvci; gbp.tlli = tlli; gbp.ra_id = &raid; gbp.u.resume.suspend_ref = suspend_ref; @@ -885,23 +885,26 @@ static int bssgp_rx_sign(struct msgb *msg, struct tlv_parsed *tp, uint8_t pdu_type = bgph->pdu_type; int rc = 0; uint16_t ns_bvci = msgb_bvci(msg); + uint16_t bvci = bctx ? bctx->bvci : ns_bvci;
switch (bgph->pdu_type) { case BSSGP_PDUT_SUSPEND: /* MS wants to suspend */ - rc = bssgp_rx_suspend(msg, tp, bctx); + rc = bssgp_rx_suspend(msg, tp); break; case BSSGP_PDUT_RESUME: /* MS wants to resume */ - rc = bssgp_rx_resume(msg, tp, bctx); + rc = bssgp_rx_resume(msg, tp); break; case BSSGP_PDUT_FLUSH_LL_ACK: /* BSS informs us it has performed LL FLUSH */ - DEBUGP(DBSSGP, "BSSGP Rx BVCI=%u FLUSH LL ACK\n", bctx->bvci); + DEBUGP(DBSSGP, "BSSGP Rx BVCI=%u FLUSH LL ACK\n", bvci); /* FIXME: send NM_FLUSH_LL.res to NM */ break; case BSSGP_PDUT_LLC_DISCARD: /* BSS informs that some LLC PDU's have been discarded */ + if (!bctx) + goto err_no_bvci; rc = bssgp_rx_llc_disc(msg, tp, bctx); break; case BSSGP_PDUT_BVC_BLOCK: @@ -935,7 +938,7 @@ static int bssgp_rx_sign(struct msgb *msg, struct tlv_parsed *tp, break; case BSSGP_PDUT_STATUS: /* Some exception has occurred */ - DEBUGP(DBSSGP, "BSSGP BVCI=%u Rx BVC STATUS\n", bctx->bvci); + DEBUGP(DBSSGP, "BSSGP BVCI=%u Rx BVC STATUS\n", bvci); /* FIXME: send NM_STATUS.ind to NM */ break; /* those only exist in the SGSN -> BSS direction */ @@ -950,18 +953,20 @@ static int bssgp_rx_sign(struct msgb *msg, struct tlv_parsed *tp, case BSSGP_PDUT_BVC_UNBLOCK_ACK: case BSSGP_PDUT_SGSN_INVOKE_TRACE: DEBUGP(DBSSGP, "BSSGP BVCI=%u Rx PDU type 0x%02x only exists " - "in DL\n", bctx->bvci, pdu_type); + "in DL\n", bvci, pdu_type); bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg); rc = -EINVAL; break; default: DEBUGP(DBSSGP, "BSSGP BVCI=%u Rx PDU type 0x%02x unknown\n", - bctx->bvci, pdu_type); + bvci, pdu_type); rc = bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg); break; }
return rc; +err_no_bvci: + LOGP(DBSSGP, LOGL_ERROR, "BSSGP missing mandatory BVCI\n"); err_mand_ie: return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg); } @@ -997,8 +1002,10 @@ int bssgp_rcvmsg(struct msgb *msg)
/* look-up or create the BTS context for this BVC */ bctx = btsctx_by_bvci_nsei(bvci, msgb_nsei(msg)); - /* Only a RESET PDU can create a new BVC context */ - if (!bctx && pdu_type != BSSGP_PDUT_BVC_RESET) { + /* Only a RESET PDU can create a new BVC context, + * otherwise it must be registered if a BVCI is given */ + if (!bctx && bvci != BVCI_SIGNALLING && + pdu_type != BSSGP_PDUT_BVC_RESET) { LOGP(DBSSGP, LOGL_NOTICE, "NSEI=%u/BVCI=%u Rejecting PDU " "type %u for unknown BVCI\n", msgb_nsei(msg), bvci, pdu_type);
Oops, I didn't intend to send this one, but comments are welcome anyway.
Jacob
On 23.09.2014 13:28, Jacob Erlbeck wrote:
Currently sending SUSPEND/RESUME messages to this function (like it is done in the osmo-sgsn) results in STATUS messages complaining about an unknown BVCI. The reason is, that these messages rely on a TLLI/RAI pair to identify the context and do not contain an explicit BVCI.
This patch modifies bssgp_rcvmsg() to only complain about and unknown BVCI if one is given but a matching context is not found (except for RESET messages). The ctx argument is removed from the functions handling SUSPEND and RESUME since it will always be NULL then.
TODO:
- test cases
- end-to-end test
Sponsored-by: On-Waves ehf
Currently the bssgph field is not set when using the bssgp_tx_* functions. This hinders unit testing of generated messages.
This patch initializes the bssgph field directly after allocation a new bssgp msgb in bssgp_msgb_alloc() so that it is set by default.
Sponsored-by: On-Waves ehf --- src/gb/gprs_bssgp_util.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/gb/gprs_bssgp_util.c b/src/gb/gprs_bssgp_util.c index 261e0b0..604764c 100644 --- a/src/gb/gprs_bssgp_util.c +++ b/src/gb/gprs_bssgp_util.c @@ -70,7 +70,9 @@ const char *bssgp_cause_str(enum gprs_bssgp_cause cause)
struct msgb *bssgp_msgb_alloc(void) { - return msgb_alloc_headroom(4096, 128, "BSSGP"); + struct msgb *msg = msgb_alloc_headroom(4096, 128, "BSSGP"); + msgb_bssgph(msg) = msg->data; + return msg; }
/* Transmit a simple response such as BLOCK/UNBLOCK/RESET ACK/NACK */
-
Holger Hans Peter Freyther -
Jacob Erlbeck