From 6ecafef28f74ddae6c6db3458ca3d84179cdf953 Mon Sep 17 00:00:00 2001 From: Holger Hans Peter Freyther Date: Fri, 13 Jan 2012 05:46:26 +0800 Subject: [PATCH 1/3] lapd: Mention the L3 size of the payload being sent --- src/gsm/lapd_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/gsm/lapd_core.c b/src/gsm/lapd_core.c index b1b5a1ba6..74ffef300 100644 --- a/src/gsm/lapd_core.c +++ b/src/gsm/lapd_core.c @@ -1731,7 +1731,8 @@ static int lapd_data_req(struct osmo_dlsap_prim *dp, struct lapd_msg_ctx *lctx) struct lapd_datalink *dl = lctx->dl; struct msgb *msg = dp->oph.msg; - LOGP(DLLAPD, LOGL_INFO, "writing message to send-queue\n"); + LOGP(DLLAPD, LOGL_INFO, + "writing message to send-queue: l3len: %d\n", msgb_l3len(msg)); /* Write data into the send queue */ msgb_enqueue(&dl->send_queue, msg); From 90656dbd00c32b56a6082d7baf4fc752adcf85dd Mon Sep 17 00:00:00 2001 From: Holger Hans Peter Freyther Date: Fri, 13 Jan 2012 05:49:29 +0800 Subject: [PATCH 2/3] lapd: Warn if someone attempts to send an empty message DATA REQ with a msgb_l3len(msg) == 0 message does not make any sense, log an error and return immediately before attempting to send an empty I frame in lapd_send_i. --- src/gsm/lapd_core.c | 7 +++++++ tests/lapd/lapd_test.c | 15 +++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/gsm/lapd_core.c b/src/gsm/lapd_core.c index 74ffef300..fb79a2f0f 100644 --- a/src/gsm/lapd_core.c +++ b/src/gsm/lapd_core.c @@ -1731,6 +1731,13 @@ static int lapd_data_req(struct osmo_dlsap_prim *dp, struct lapd_msg_ctx *lctx) struct lapd_datalink *dl = lctx->dl; struct msgb *msg = dp->oph.msg; + if (msgb_l3len(msg) == 0) { + LOGP(DLLAPD, LOGL_ERROR, + "writing an empty message is not possible.\n"); + msgb_free(msg); + return -1; + } + LOGP(DLLAPD, LOGL_INFO, "writing message to send-queue: l3len: %d\n", msgb_l3len(msg)); diff --git a/tests/lapd/lapd_test.c b/tests/lapd/lapd_test.c index 8c6b0df2e..c924dbf9e 100644 --- a/tests/lapd/lapd_test.c +++ b/tests/lapd/lapd_test.c @@ -99,6 +99,16 @@ static struct msgb *create_mm_id_req(void) return msg; } +static struct msgb *create_empty_msg(void) +{ + struct msgb *msg; + + msg = msgb_from_array(NULL, 0); + ASSERT(msgb_l3len(msg) == 0); + rsl_rll_push_l3(msg, RSL_MT_DATA_REQ, 0, 0, 1); + return msg; +} + static struct msgb *create_dummy_data_req(void) { struct msgb *msg; @@ -284,6 +294,11 @@ static void test_lapdm_polling() rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); ASSERT(rc < 0); + /* check sending an empty L3 message fails */ + rc = lapdm_rslms_recvmsg(create_empty_msg(), &bts_to_ms_channel); + ASSERT(rc == -1); + ASSERT(test_state.ms_read == 2); + /* clean up */ lapdm_channel_exit(&bts_to_ms_channel); lapdm_channel_exit(&ms_to_bts_channel); From 3a5f08c221b32381623e50095de2751183e994c4 Mon Sep 17 00:00:00 2001 From: Holger Hans Peter Freyther Date: Thu, 12 Jan 2012 23:12:28 +0100 Subject: [PATCH 3/3] lapdm: Make sure that the msgb_l3len(msg) == length... This code should not play with the internals of the msgb like this, this code got introduced in af48bed55607931307 and is breaking the osmo-bts usecase of forwarding an RSL message. Add a test case that fails without the new code. I would prefer if we could get rid of the manipulating the msgb like this, it is prone to errors like this one. --- src/gsm/lapdm.c | 8 ++++---- tests/lapd/lapd_test.c | 18 +++++++++++------- tests/lapd/lapd_test.ok | 4 ++-- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/gsm/lapdm.c b/src/gsm/lapdm.c index 3d2f3d831..e9ce881b8 100644 --- a/src/gsm/lapdm.c +++ b/src/gsm/lapdm.c @@ -792,7 +792,7 @@ static int rslms_rx_rll_est_req(struct msgb *msg, struct lapdm_datalink *dl) /* Remove RLL header from msgb and set length to L3-info */ msgb_pull_l2h(msg); msg->len = length; - msg->tail = msg->data + length; + msg->tail = msg->l3h + length; /* prepare prim */ osmo_prim_init(&dp.oph, 0, PRIM_DL_EST, PRIM_OP_REQUEST, msg); @@ -845,7 +845,7 @@ static int rslms_rx_rll_udata_req(struct msgb *msg, struct lapdm_datalink *dl) /* Remove RLL header from msgb and set length to L3-info */ msgb_pull_l2h(msg); msg->len = length; - msg->tail = msg->data + length; + msg->tail = msg->l3h + length; /* Push L1 + LAPDm header on msgb */ msg->l2h = msgb_push(msg, 4 + !ui_bts); @@ -881,7 +881,7 @@ static int rslms_rx_rll_data_req(struct msgb *msg, struct lapdm_datalink *dl) /* Remove RLL header from msgb and set length to L3-info */ msgb_pull_l2h(msg); msg->len = length; - msg->tail = msg->data + length; + msg->tail = msg->l3h + length; /* prepare prim */ osmo_prim_init(&dp.oph, 0, PRIM_DL_DATA, PRIM_OP_REQUEST, msg); @@ -938,7 +938,7 @@ static int rslms_rx_rll_res_req(struct msgb *msg, struct lapdm_datalink *dl) /* Remove RLL header from msgb and set length to L3-info */ msgb_pull_l2h(msg); msg->len = length; - msg->tail = msg->data + length; + msg->tail = msg->l3h + length; /* prepare prim */ osmo_prim_init(&dp.oph, 0, (msg_type == RSL_MT_RES_REQ) ? PRIM_DL_RES diff --git a/tests/lapd/lapd_test.c b/tests/lapd/lapd_test.c index c924dbf9e..d58bec655 100644 --- a/tests/lapd/lapd_test.c +++ b/tests/lapd/lapd_test.c @@ -73,8 +73,8 @@ static const uint8_t cm_padded[] = { }; static const uint8_t mm[] = { - 0x05, 0x24, 0x31, 0x03, 0x50, 0x18, 0x93, 0x08, - 0x29, 0x47, 0x80, 0x00, + 0x00, 0x0c, 0x00, 0x03, 0x01, 0x01, 0x20, 0x02, + 0x00, 0x0b, 0x00, 0x03, 0x05, 0x04, 0x0d }; static const uint8_t dummy1[] = { @@ -95,7 +95,11 @@ static struct msgb *create_mm_id_req(void) struct msgb *msg; msg = msgb_from_array(mm, sizeof(mm)); - rsl_rll_push_l3(msg, RSL_MT_DATA_REQ, 0, 0, 1); + msg->l2h = msg->data + 3; + ASSERT(msgb_l2len(msg) == 12); + msg->l3h = msg->l2h + 6; + ASSERT(msgb_l3len(msg) == 6); + return msg; } @@ -204,9 +208,9 @@ static int ms_to_bts_tx_cb(struct msgb *msg, struct lapdm_entity *le, void *_ctx /* ASSERT(msg->data[7] == 0x0 && msg->data[8] == 0x9c); */ /* this should be 0x0 and 0x0... but we have a bug */ } else if (state->ms_read == 1) { - printf("MS: Verifying incoming MM message.\n"); - ASSERT(msgb_l3len(msg) == ARRAY_SIZE(mm)); - ASSERT(memcmp(msg->l3h, mm, msgb_l3len(msg)) == 0); + printf("MS: Verifying incoming MM message: %d\n", msgb_l3len(msg)); + ASSERT(msgb_l3len(msg) == 3); + ASSERT(memcmp(msg->l3h, &mm[12], msgb_l3len(msg)) == 0); } else { printf("MS: Do not know to verify: %d\n", state->ms_read); } @@ -281,7 +285,7 @@ static void test_lapdm_polling() lapdm_rslms_recvmsg(create_dummy_data_req(), &ms_to_bts_channel); - /* 4. And back to the MS */ + /* 4. And back to the MS, but let's move data/l2h apart */ ASSERT(test_state.bts_read == 2) ASSERT(test_state.ms_read == 2); rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); diff --git a/tests/lapd/lapd_test.ok b/tests/lapd/lapd_test.ok index 058ac3d77..d67a0a80a 100644 --- a/tests/lapd/lapd_test.ok +++ b/tests/lapd/lapd_test.ok @@ -9,8 +9,8 @@ ms_to_bts_tx_cb: BTS->MS(us) message 9 MS: Verifying incoming primitive. Sending back to MS -ms_to_bts_tx_cb: BTS->MS(us) message 21 -MS: Verifying incoming MM message. +ms_to_bts_tx_cb: BTS->MS(us) message 12 +MS: Verifying incoming MM message: 3 ms_to_bts_l1_cb: MS(us) -> BTS prim message Sending back to BTS