[PATCH 2/4] gprs: Fix bssgp_rcvmsg to handle signalling msgs with BVCI IE

[Jacob Erlbeck]

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) {

[Jacob Erlbeck]

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 */

[Jacob Erlbeck]

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);

[Jacob Erlbeck]

Oops, I didn't intend to send this one, but comments are welcome anyway.

Jacob

Post by Jacob Erlbeck
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.
- test cases
- end-to-end test
Sponsored-by: On-Waves ehf

[Holger Hans Peter Freyther]

Post by Jacob Erlbeck
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"?