LAPDm code issues (ladpm.c)

Hi LAPDm guys, while I was digging into the LAPDm code to find out, why a broken SABM message is sent on a SAPI3 establish SACCH request during an active call (leading to a delay of 3s between RSL EST request and response), I stumbled over the following: The initial SABM message (not the retransmitted one after T200) has a non-zero length and ends with 3 bytes that have been taken from the end of the RSL EST REQ message. The MS does not answer to this. Interestingly the second SABM message that gets sent after T200 (2s) has a length field of 0 and no trailing garbage. The difference lies in the way the msgb is handled. In rslms_rx_rll_est_req() the msg buffer passed from RSL is being used. In the case of IPA, the l2h is prepended by the 3 byte IPA header. Since all code in lapdm.c that handles RSL message seem to assume, that msg->data == msg->l2h, length computation is done based on that assumption in some places. In addition, msgb_pull() is used in a way that would also lead to undefined results in this case, e.g. in msgb_pull_l2h() just the difference between l2h and l3h is pulled from the beginning (msg->data). The main difficulty to find this, was that much msgb handling is done by manual access to the msgb fields. I'd really favor the use of the predefined macros/inlines instead of meddling around with the fields. I've added a function msgb_pull_to_l3() to msgb.h which just skips over everything in front of l3 (and therefore invalidates l2h and l1h) and replaced all calls to msgb_pull_l2h() by calls to msgb_pull_to_l3(). In addition, I replaced manual l3 length adjustment by calls to msgb_trim(). That alone fixed the SABM issue described above. See the jerlbeck/fixes/lapd-sms branch for details. But AFAICS there is still something to do: - The remaining msgb_pull() need to be checked (at least the one in l2_ph_data_ind() looks suspicious to me. - L3 length computation should be done with the macros, how it is done in lapdm_send_ph_data_req() is broken. - Why does lapd_msg_ctx have a length field that is not used? - ladp_test.c should be extended to check other execution paths, too. I've tried it for dummy packets and I didn't get it working without failing assertions (see below) - How l2/data/.. in a msg are expected to be used/set should be documented somewhere. - It should be clarified, whether all abis driver should reset the msg to start with l2. Cheers Jacob ====== The following patch still breaks the assertions, at least changing the l3len computation in lapdm_send_ph_data_req() influences but doesn't fix it. --- a/tests/lapd/lapd_test.c +++ b/tests/lapd/lapd_test.c @@ -123,8 +123,10 @@ static struct msgb *create_empty_msg(void) static struct msgb *create_dummy_data_req(void) { struct msgb *msg; + const int dummy_l1len = 3; msg = msgb_from_array(dummy1, sizeof(dummy1)); + msgb_push(msg, dummy_l1len); rsl_rll_push_l3(msg, RSL_MT_DATA_REQ, 0, 0, 1); return msg; }