[MERGED] osmo-bts[master]: fix handover: handle_ph_ra_ind(): evaluate ra_ind before msg...

[Harald Welte]

Harald Welte has submitted this change and it was merged.

Change subject: fix handover: handle_ph_ra_ind(): evaluate ra_ind before 
msgb_trim()
..


fix handover: handle_ph_ra_ind(): evaluate ra_ind before msgb_trim()

Commit c2b4c668f3510b7b0baace749c5a310959010e90
I3b989580cb38082e3fd8fc50a11fedda13991092 introduces evaluation of ra_ind
members below the msgb_trim() call that actually invalidates ra_ind.
A symptom is that it breaks detection of Handover RACH, wich always ends up
with lchan == NULL and interpreting all RACH as chan_nr == 0x88.

Fix: do all evaluation of ra_ind before the msgb_trim(), for osmo-bts-sysmo,
litecell-15 and octphy.

To guard against similar mistakes in the future, set ra_ind = NULL before the
msgb_trim() call.

Related: OS#3045
Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
---
M src/osmo-bts-litecell15/l1_if.c
M src/osmo-bts-octphy/l1_if.c
M src/osmo-bts-sysmo/l1_if.c
3 files changed, 138 insertions(+), 117 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c
index 9e122cd..ea652a1 100644
--- a/src/osmo-bts-litecell15/l1_if.c
+++ b/src/osmo-bts-litecell15/l1_if.c
@@ -997,10 +997,8 @@
struct gsm_bts_role_bts *btsb = bts->role;
struct gsm_lchan *lchan;
struct osmo_phsap_prim *l1sap;
-   uint32_t fn;
-   uint8_t acc_delay = 0;
-   uint16_t ra = 0, is_11bit = 0, burst_type = 0, temp = 0;
int rc;
+   struct ph_rach_ind_param rach_ind_param;
 
/* FIXME: this should be deprecated/obsoleted as it bypasses rach.busy 
counting */
if (ra_ind->measParam.fLinkQuality < btsb->min_qual_rach) {
@@ -1008,12 +1006,7 @@
return 0;
}
 
-   /* the old legacy full-bits acc_delay cannot express negative values */
-   if (ra_ind->measParam.i16BurstTiming > 0)
-   acc_delay = ra_ind->measParam.i16BurstTiming >> 2;
-
dump_meas_res(LOGL_DEBUG, _ind->measParam);
-   burst_type = ra_ind->burstType;
 
if ((ra_ind->msgUnitParam.u8Size != 1) &&
(ra_ind->msgUnitParam.u8Size != 2)) {
@@ -1022,60 +1015,74 @@
return 0;
}
 
+   /* We need to evaluate ra_ind before below msgb_trim(), since that 
invalidates *ra_ind. */
+   rach_ind_param = (struct ph_rach_ind_param) {
+   /* .chan_nr set below */
+   /* .ra set below */
+   .acc_delay = 0,
+   .fn = ra_ind->u32Fn,
+   /* .is_11bit set below */
+   /* .burst_type set below */
+   .rssi = (int8_t) ra_ind->measParam.fRssi,
+   .ber10k = (unsigned int) (ra_ind->measParam.fBer * 1.0),
+   .acc_delay_256bits = ra_ind->measParam.i16BurstTiming * 64,
+   };
+
+   lchan = l1if_hLayer_to_lchan(trx, (uint32_t)ra_ind->hLayer2);
+   if (!lchan || lchan->ts->pchan == GSM_PCHAN_CCCH ||
+   lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4 ||
+   lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4_CBCH)
+   rach_ind_param.chan_nr = 0x88;
+   else
+   rach_ind_param.chan_nr = gsm_lchan2chan_nr(lchan);
+
if (ra_ind->msgUnitParam.u8Size == 2) {
-   is_11bit = 1;
-   ra = ra_ind->msgUnitParam.u8Buffer[0];
+   uint16_t temp;
+   uint16_t ra = ra_ind->msgUnitParam.u8Buffer[0];
ra = ra << 3;
temp = (ra_ind->msgUnitParam.u8Buffer[1] & 0x7);
ra = ra | temp;
+   rach_ind_param.is_11bit = 1;
+   rach_ind_param.ra = ra;
} else {
-   is_11bit = 0;
-   ra = ra_ind->msgUnitParam.u8Buffer[0];
+   rach_ind_param.is_11bit = 0;
+   rach_ind_param.ra = ra_ind->msgUnitParam.u8Buffer[0];
}
 
-   fn = ra_ind->u32Fn;
+   /* the old legacy full-bits acc_delay cannot express negative values */
+   if (ra_ind->measParam.i16BurstTiming > 0)
+   rach_ind_param.acc_delay = ra_ind->measParam.i16BurstTiming >> 
2;
+
+   /* mapping of the burst type, the values are specific to
+* osmo-bts-litecell15 */
+   switch (ra_ind->burstType) {
+   case GsmL1_BurstType_Access_0:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_ACCESS_0;
+   break;
+   case GsmL1_BurstType_Access_1:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_ACCESS_1;
+   break;
+   case GsmL1_BurstType_Access_2:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_ACCESS_2;
+   break;
+   default:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_NONE;
+   break;
+   }
+
+   /* msgb_trim() 

osmo-bts[master]: fix handover: handle_ph_ra_ind(): evaluate ra_ind before msg...

[Harald Welte]

Patch Set 2:

merge this now, we can still improve further upon it later

-- 
To view, visit https://gerrit.osmocom.org/7169
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
Gerrit-PatchSet: 2
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-bts[master]: fix handover: handle_ph_ra_ind(): evaluate ra_ind before msg...

[Harald Welte]

Patch Set 2: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/7169
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
Gerrit-PatchSet: 2
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-bts[master]: fix handover: handle_ph_ra_ind(): evaluate ra_ind before msg...

[Harald Welte]

Patch Set 2:

just noting that in general we could also consider to either
a) not re-cycle the msgb with msgb_trim() but allocate a new one
b) have the L1SAP primitive on the stack and pass that up (not sure if L1SAP 
currently supports this for all primitives, but I know at least in one case 
this is already being done somewhere I believe in octphy.

-- 
To view, visit https://gerrit.osmocom.org/7169
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
Gerrit-PatchSet: 2
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-bts[master]: fix handover: handle_ph_ra_ind(): evaluate ra_ind before msg...

[Neels Hofmeyr]

Patch Set 1:

NOTE: I HAVE ONLY TESTED ON SYSMOBTS. We should still test on octphy and 
litecell15. litecell15 probably is fine because of practically identical code, 
but octphy might not be.

-- 
To view, visit https://gerrit.osmocom.org/7169
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
Gerrit-PatchSet: 1
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

[PATCH] osmo-bts[master]: fix handover: handle_ph_ra_ind(): evaluate ra_ind before msg...

[Neels Hofmeyr]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/7169

to look at the new patch set (#2).

fix handover: handle_ph_ra_ind(): evaluate ra_ind before msgb_trim()

Commit c2b4c668f3510b7b0baace749c5a310959010e90
I3b989580cb38082e3fd8fc50a11fedda13991092 introduces evaluation of ra_ind
members below the msgb_trim() call that actually invalidates ra_ind.
A symptom is that it breaks detection of Handover RACH, wich always ends up
with lchan == NULL and interpreting all RACH as chan_nr == 0x88.

Fix: do all evaluation of ra_ind before the msgb_trim(), for osmo-bts-sysmo,
litecell-15 and octphy.

To guard against similar mistakes in the future, set ra_ind = NULL before the
msgb_trim() call.

Related: OS#3045
Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
---
M src/osmo-bts-litecell15/l1_if.c
M src/osmo-bts-octphy/l1_if.c
M src/osmo-bts-sysmo/l1_if.c
3 files changed, 138 insertions(+), 117 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/69/7169/2

diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c
index 9e122cd..ea652a1 100644
--- a/src/osmo-bts-litecell15/l1_if.c
+++ b/src/osmo-bts-litecell15/l1_if.c
@@ -997,10 +997,8 @@
struct gsm_bts_role_bts *btsb = bts->role;
struct gsm_lchan *lchan;
struct osmo_phsap_prim *l1sap;
-   uint32_t fn;
-   uint8_t acc_delay = 0;
-   uint16_t ra = 0, is_11bit = 0, burst_type = 0, temp = 0;
int rc;
+   struct ph_rach_ind_param rach_ind_param;
 
/* FIXME: this should be deprecated/obsoleted as it bypasses rach.busy 
counting */
if (ra_ind->measParam.fLinkQuality < btsb->min_qual_rach) {
@@ -1008,12 +1006,7 @@
return 0;
}
 
-   /* the old legacy full-bits acc_delay cannot express negative values */
-   if (ra_ind->measParam.i16BurstTiming > 0)
-   acc_delay = ra_ind->measParam.i16BurstTiming >> 2;
-
dump_meas_res(LOGL_DEBUG, _ind->measParam);
-   burst_type = ra_ind->burstType;
 
if ((ra_ind->msgUnitParam.u8Size != 1) &&
(ra_ind->msgUnitParam.u8Size != 2)) {
@@ -1022,60 +1015,74 @@
return 0;
}
 
+   /* We need to evaluate ra_ind before below msgb_trim(), since that 
invalidates *ra_ind. */
+   rach_ind_param = (struct ph_rach_ind_param) {
+   /* .chan_nr set below */
+   /* .ra set below */
+   .acc_delay = 0,
+   .fn = ra_ind->u32Fn,
+   /* .is_11bit set below */
+   /* .burst_type set below */
+   .rssi = (int8_t) ra_ind->measParam.fRssi,
+   .ber10k = (unsigned int) (ra_ind->measParam.fBer * 1.0),
+   .acc_delay_256bits = ra_ind->measParam.i16BurstTiming * 64,
+   };
+
+   lchan = l1if_hLayer_to_lchan(trx, (uint32_t)ra_ind->hLayer2);
+   if (!lchan || lchan->ts->pchan == GSM_PCHAN_CCCH ||
+   lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4 ||
+   lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4_CBCH)
+   rach_ind_param.chan_nr = 0x88;
+   else
+   rach_ind_param.chan_nr = gsm_lchan2chan_nr(lchan);
+
if (ra_ind->msgUnitParam.u8Size == 2) {
-   is_11bit = 1;
-   ra = ra_ind->msgUnitParam.u8Buffer[0];
+   uint16_t temp;
+   uint16_t ra = ra_ind->msgUnitParam.u8Buffer[0];
ra = ra << 3;
temp = (ra_ind->msgUnitParam.u8Buffer[1] & 0x7);
ra = ra | temp;
+   rach_ind_param.is_11bit = 1;
+   rach_ind_param.ra = ra;
} else {
-   is_11bit = 0;
-   ra = ra_ind->msgUnitParam.u8Buffer[0];
+   rach_ind_param.is_11bit = 0;
+   rach_ind_param.ra = ra_ind->msgUnitParam.u8Buffer[0];
}
 
-   fn = ra_ind->u32Fn;
+   /* the old legacy full-bits acc_delay cannot express negative values */
+   if (ra_ind->measParam.i16BurstTiming > 0)
+   rach_ind_param.acc_delay = ra_ind->measParam.i16BurstTiming >> 
2;
+
+   /* mapping of the burst type, the values are specific to
+* osmo-bts-litecell15 */
+   switch (ra_ind->burstType) {
+   case GsmL1_BurstType_Access_0:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_ACCESS_0;
+   break;
+   case GsmL1_BurstType_Access_1:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_ACCESS_1;
+   break;
+   case GsmL1_BurstType_Access_2:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_ACCESS_2;
+   break;
+   default:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_NONE;
+   break;
+   }
+
+   /* msgb_trim() invalidates ra_ind, make that abundantly clear: */
+   ra_ind = NULL;

[PATCH] osmo-bts[master]: fix handover: handle_ph_ra_ind(): evaluate ra_ind before msg...

[Neels Hofmeyr]

Review at  https://gerrit.osmocom.org/7169

fix handover: handle_ph_ra_ind(): evaluate ra_ind before msgb_trim()

Commit c2b4c668f3510b7b0baace749c5a310959010e90
I3b989580cb38082e3fd8fc50a11fedda13991092 introduces evaluation of ra_ind
members below the msgb_trim() call that actually invalidates ra_ind.
A symptom is that it breaks detection of Handover RACH, wich always ends up
with lchan == NULL and interpreting all RACH as chan_nr == 0x88.

Fix: do all evaluation of ra_ind before the msgb_trim(), for osmo-bts-sysmo,
litecell-15 and octphy.

To guard against similar mistakes in the future, set ra_ind = NULL before the
msgb_trim() call.

Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
---
M src/osmo-bts-litecell15/l1_if.c
M src/osmo-bts-octphy/l1_if.c
M src/osmo-bts-sysmo/l1_if.c
3 files changed, 138 insertions(+), 117 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/69/7169/1

diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c
index 9e122cd..ea652a1 100644
--- a/src/osmo-bts-litecell15/l1_if.c
+++ b/src/osmo-bts-litecell15/l1_if.c
@@ -997,10 +997,8 @@
struct gsm_bts_role_bts *btsb = bts->role;
struct gsm_lchan *lchan;
struct osmo_phsap_prim *l1sap;
-   uint32_t fn;
-   uint8_t acc_delay = 0;
-   uint16_t ra = 0, is_11bit = 0, burst_type = 0, temp = 0;
int rc;
+   struct ph_rach_ind_param rach_ind_param;
 
/* FIXME: this should be deprecated/obsoleted as it bypasses rach.busy 
counting */
if (ra_ind->measParam.fLinkQuality < btsb->min_qual_rach) {
@@ -1008,12 +1006,7 @@
return 0;
}
 
-   /* the old legacy full-bits acc_delay cannot express negative values */
-   if (ra_ind->measParam.i16BurstTiming > 0)
-   acc_delay = ra_ind->measParam.i16BurstTiming >> 2;
-
dump_meas_res(LOGL_DEBUG, _ind->measParam);
-   burst_type = ra_ind->burstType;
 
if ((ra_ind->msgUnitParam.u8Size != 1) &&
(ra_ind->msgUnitParam.u8Size != 2)) {
@@ -1022,60 +1015,74 @@
return 0;
}
 
+   /* We need to evaluate ra_ind before below msgb_trim(), since that 
invalidates *ra_ind. */
+   rach_ind_param = (struct ph_rach_ind_param) {
+   /* .chan_nr set below */
+   /* .ra set below */
+   .acc_delay = 0,
+   .fn = ra_ind->u32Fn,
+   /* .is_11bit set below */
+   /* .burst_type set below */
+   .rssi = (int8_t) ra_ind->measParam.fRssi,
+   .ber10k = (unsigned int) (ra_ind->measParam.fBer * 1.0),
+   .acc_delay_256bits = ra_ind->measParam.i16BurstTiming * 64,
+   };
+
+   lchan = l1if_hLayer_to_lchan(trx, (uint32_t)ra_ind->hLayer2);
+   if (!lchan || lchan->ts->pchan == GSM_PCHAN_CCCH ||
+   lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4 ||
+   lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4_CBCH)
+   rach_ind_param.chan_nr = 0x88;
+   else
+   rach_ind_param.chan_nr = gsm_lchan2chan_nr(lchan);
+
if (ra_ind->msgUnitParam.u8Size == 2) {
-   is_11bit = 1;
-   ra = ra_ind->msgUnitParam.u8Buffer[0];
+   uint16_t temp;
+   uint16_t ra = ra_ind->msgUnitParam.u8Buffer[0];
ra = ra << 3;
temp = (ra_ind->msgUnitParam.u8Buffer[1] & 0x7);
ra = ra | temp;
+   rach_ind_param.is_11bit = 1;
+   rach_ind_param.ra = ra;
} else {
-   is_11bit = 0;
-   ra = ra_ind->msgUnitParam.u8Buffer[0];
+   rach_ind_param.is_11bit = 0;
+   rach_ind_param.ra = ra_ind->msgUnitParam.u8Buffer[0];
}
 
-   fn = ra_ind->u32Fn;
+   /* the old legacy full-bits acc_delay cannot express negative values */
+   if (ra_ind->measParam.i16BurstTiming > 0)
+   rach_ind_param.acc_delay = ra_ind->measParam.i16BurstTiming >> 
2;
+
+   /* mapping of the burst type, the values are specific to
+* osmo-bts-litecell15 */
+   switch (ra_ind->burstType) {
+   case GsmL1_BurstType_Access_0:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_ACCESS_0;
+   break;
+   case GsmL1_BurstType_Access_1:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_ACCESS_1;
+   break;
+   case GsmL1_BurstType_Access_2:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_ACCESS_2;
+   break;
+   default:
+   rach_ind_param.burst_type =
+   GSM_L1_BURST_TYPE_NONE;
+   break;
+   }
+
+   /* msgb_trim() invalidates ra_ind, make that abundantly clear: */
+   ra_ind = NULL;
rc = msgb_trim(l1p_msg, sizeof(*l1sap));
if (rc < 0)
MSGB_ABORT(l1p_msg, "No room for primitive