Unbounded AGCH queue in OsmoBTS

[Ivan Kluchnikov]

Hi Holger,

Thank you for review, I will fix my patches today or tomorrow and send
you new version asap.

2014-02-11 14:14 GMT+04:00 Holger Hans Peter Freyther <holger at freyther.de>:
> On Tue, Feb 11, 2014 at 11:13:35AM +0400, Ivan Kluchnikov wrote:
>
> Good Morning,
>
>> The attached patches for openbsc and osmo-bts finally fix issues which
>> were discussed above.
>
> what a surprise. Jacob has started implementing AGCH queue handling
> as well. So I will let him comment on the BTS part.
>
>
>
> OpenBSC:
>> +
>> +     /* bts didn't send IMM_ASSIGN, so we should release allocated channel */
>> +     ia = (struct gsm48_imm_ass *) (rqd_hdr->data + 2);
>
> Please add a size check that the mandatory element actually fits
> and use early returns.
>
>> +     if (ia->msg_type == GSM48_MT_RR_IMM_ASS) {
>> +             chan_nr = ia->chan_desc.chan_nr;
>> +             lchan = lchan_lookup(trx, chan_nr);
>
> same thing for the lchan. Verify it was found and that the state
> is actually the right one.
>
>
>> +             rsl_rf_chan_release(lchan, 1, SACCH_DEACTIVATE);
>
> Maybe use rsl_direct_rf_release. By definition there is no one listening
> on the SACCH. So there is not point in going through the normal release
> procedure.
>
>
>> +/* 8.5.4 DELETE INDICATION */
>> +int rsl_tx_delete_ind(struct gsm_bts *bts, uint8_t len, uint8_t *val)
>> +{
>> +     struct msgb *msg;
>> +
>> +     msg = rsl_msgb_alloc(sizeof(struct abis_rsl_cchan_hdr));
>> +     if (!msg)
>> +             return -ENOMEM;
>> +     rsl_cch_push_hdr(msg, RSL_MT_DELETE_IND, RSL_CHAN_PCH_AGCH);
>> +     msgb_tlv_put(msg, RSL_IE_FULL_IMM_ASS_INFO, len, val);
>> +     msg->trx = bts->c0;
>
> Have you manually tested this with multi-trx support and the channel
> being on the second trx? The lchan_lookup in OpenBSC will be done using
> the "bts->c0"?
>
>
>> -             msgb_free(msg);
>> +             rsl_tx_delete_ind(trx->bts, msg->len, msg->data);
>> +             return -ENOMEM;
>
> Does it leak now? or was it a double free before?
>



-- 
Regards,
Ivan Kluchnikov.
http://fairwaves.ru