LAPDm code issues (ladpm.c)

[Jacob Erlbeck]

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