Commit Graph

1996 Commits

Author SHA1 Message Date
Pau Espin 49a2f404e8 replace dl_ass_state with osmocom FSM
Related: OS#2709
Change-Id: Ia33418478e17986a316ffda48b091030f53fa371
2021-08-23 17:14:22 +02:00
Pau Espin 432d4f3b89 tbf: Drop unuseful flag GPRS_RLCMAC_FLAG_TO_UL_ASS
The flag is only used to print some uninteresting stuff, let's drop it
in order to simplify code. We can add later whatever we want in the new
shiny FSM.

Change-Id: I20aa7f83cc4f32de129e64c74a91745b983a7b16
2021-08-23 17:14:22 +02:00
Pau Espin ab8fba3a20 tbf: Reimplement rlcmac_diag() and make it available from C
We never use the std:string anyway, we always call .c_str() to log using
osmocom logging system.
Furthermore, we'll need to use it from C code soon (next commit).

Change-Id: I3ad66f9f3f4d55d11da3a3b8b38656ae2dd50603
2021-08-23 17:14:22 +02:00
Pau Espin 6ad11a6990 Replace ul_ass_state with osmocom FSM
Related: OS#2709
Change-Id: Id414eafe9c04a9a8759c6fb1a483bf2ee093a4d2
2021-08-23 17:14:22 +02:00
Pau Espin b0ead922a1 tbf_free: Get rid of uneeded tbf_state transition
We are freeing the object immediately afterwards anyway, so no need to
pretend it went through the normal state release.
Leaving current state as it is actually provides more information on
what was the status/state at the time the TBF had to be freed.

Change-Id: I3016caaccc2c43e1e300f3c6042d69f8adcd9d69
2021-08-23 17:14:22 +02:00
Pau Espin 284711d627 Get rid of tbf_dl:abort()
Having that code in a separate function is confusing and adds code
complexity since it looks like an entry point to start feeing a TBF, but
it simply some (not yet really useful) set of instructions to be called
one 1 code path in tbf_free.
Let's move it there, this way it becomes clear tbf_free() is THE place
to be (if you want to get rid of a TBF).

Change-Id: I30febf4d21a0bfab37524c07598bbb0dd32f7f65
2021-08-23 17:14:22 +02:00
Pau Espin 131deb059f Move rate_ctr free to tbf subclass destructor
This way we clean up tbf_free entry point, and leave memory freeing for
later on at the end when talloc_free is called.

Change-Id: I1c45e3296e565725bcbbca391d9518772fffa89d
2021-08-23 17:14:22 +02:00
Pau Espin 9dacf0b35b Remove duplicate call to gprs_rlcmac_lost_rep
Function is already called by gprs_rlcmac_received_lost(), so next call
following it will be sum=0 and return EINVAL.

Change-Id: I015ba16d18fdd6e2441ec3c256b5ac88771d7a8b
2021-08-23 17:14:22 +02:00
Pau Espin 8c4f978483 Drop logging last mas report before freeing TBF
There's no much use in logging it since anyway we are immediately
getting rid of it.

Change-Id: I9b712f720b5874886cc19d998fb8fcd0e618d590
2021-08-23 17:14:21 +02:00
Pau Espin 62e06f92e9 Put dl_tbf::cleanup into destructor
It's fine to always attemt dropping the timer since it's set up in the
constructor.
This also drps the double function call abort()+cleanup() which is
confusing.

Change-Id: Ia2aaa43bd8faacf09fe4b36b11b38022bea7a59c
2021-08-23 17:14:21 +02:00
Pau Espin f45ede640b Drop duplicate log line
Same line (or similar if run_diag) is logged immediately below, showing
up twice in log which is confusing:

"""
20210726171543005 DTBF tbf.cpp:455 TBF(TFI=2 TLLI=0xfe563576 DIR=DL STATE=WAIT_RELEASE EGPRS) T3193 timeout expired, freeing TBF
20210726171543005 DTBF tbf.cpp:462 TBF(TFI=2 TLLI=0xfe563576 DIR=DL STATE=WAIT_RELEASE EGPRS) T3193 timeout expired, freeing TBF
"""

Change-Id: Ie171c458e670f8471ac93f78520a05926114c974
2021-08-23 17:14:21 +02:00
Pau Espin cfb61d9536 Move T3169 and T3195 to tbf_fsm
Change-Id: I599f4e7e82b0a8c0f5cf633c2d8b1975435f0b60
2021-08-23 17:14:21 +02:00
Pau Espin 55f600b702 Move RELEASING tbf_state transition to tbf_fsm
PdchUlcTest output changes because the original state NULL is not
expected when transactioning to RELEASING upon MAX N310* being hit. In
any case, none of those events should happen in NULL state, but we
don't really care about TBF states there so we are fine with whatever
the state is.

Related: OS#2709
Change-Id: I516b8d989a0d705e5664f8aeaf7d108e0105aa16
2021-08-23 17:14:21 +02:00
Pau Espin efcb046ce1 Move WAIT_RELEASE tbf_state transition to tbf_fsm
While at it, method maybe_start_new_window is renamed to
rcvd_dl_final_ack to make more sense out of the code.

Related: OS#2709
Change-Id: Iebd650c1036ef2d5132789778be7117ce3391c01
2021-08-23 17:14:21 +02:00
Pau Espin c32c4a3648 Move FINISHED tbf_state transition to tbf_fsm
Related: OS#2709
Change-Id: I81f507e3a2821254f03364a58ead02333e63099f
2021-08-23 17:14:21 +02:00
Pau Espin 65bba93afa tests: tbf: Fix dl_tbf polled for data without being in FLOW state
Prior code was wrong, as in it did stuff diferent to what is expected
and actually done in osmo-pcu. In osmo-pcu code, the function is guarded
and only called in FLOW or FINISHED state by the scheduler.

Change-Id: If8029bee90adceee128ebb20c033756efd50e90e
2021-08-23 17:14:21 +02:00
Pau Espin 720e19e7f3 Move FLOW tbf_state transition to tbf_fsm.
Related: OS#2709
Change-Id: Ia8c7de759c195d09263fb1f083fbf6cfa3087f8d
2021-08-23 17:14:21 +02:00
Pau Espin 33e8007100 Move NULL and ASSIGN tbf_state transition to tbf_fsm
At some point later in time the state_flags will most probably be split
into different variables, one ending up in a different FSM. It is moved
so far to the exsiting FSM from the C++ class since it's easier to
access it from C and C++ code, and anyway that kind of information
belongs to the FSM.

Related: OS#2709
Change-Id: I3c62e9e83965cb28065338733f182863e54d7474
2021-08-23 17:14:21 +02:00
Pau Espin 88f34812df Revert "Revert "Stop abusing T3169""
This reverts commit 112c63e9b4.

Change-Id: Ic18674ccd38f81ddd46e1ec733159df350991899
2021-08-23 17:14:19 +02:00
Pau Espin b5fece959f Revert "fix: handle NULL return of as_dl_tbf() and as_ul_tbf()"
This reverts commit d8e8ea9c8f.

Change-Id: I8000e78515b25b9be5c28a249bde330dac915dcb
2021-08-23 17:14:17 +02:00
Pau Espin bc139a4af4 Revert "coverity: fix null deref from recent UL TBF leak fix"
This reverts commit 3bd6488889.

Change-Id: I59c4ae726286216850ad9b53fee34ab4bda5630f
2021-08-23 17:14:15 +02:00
Oliver Smith e54f148ce9 tests: make update_exp: build check_PROGRAMS first
Make test development a bit more convenient.

Change-Id: Ic053aa3ab7485176b58eab6ebb4835cfbe6b8260
2021-08-23 14:56:27 +02:00
Oliver Smith 3f79470453 bts: delete pch_timer list in destructor
Run bts_pch_timer_remove() on each entry of the BTS specific pch_timer
list, so we don't have a memory leak and so the timer doesn't
potentially fire for a deallocated BTS.

Fixes: d3c7591 ("Add counters: pcu.bts.N.pch.requests.timeout")
Change-Id: Ia5e33d1894408e93a51c452002ef2f5758808269
2021-08-23 14:51:18 +02:00
Neels Hofmeyr 3bd6488889 coverity: fix null deref from recent UL TBF leak fix
Fix a possible NULL deref, introduced in recent patch
I8ce21be6836549b47a606c00b793d6f005964c5c /
d8e8ea9c8f

Related: OS#5205 SYS#5561 CID#239246
Change-Id: I603d4a5bc0fe5bd2e9f0dba171604c459e38aeaf
2021-08-18 18:23:44 +02:00
Neels Hofmeyr d8e8ea9c8f fix: handle NULL return of as_dl_tbf() and as_ul_tbf()
Go through all callers of as_dl_tbf() and as_ul_tbf(), and make sure
they can handle the possible NULL return value.

OS#5205 reports a NULL deref crash of osmo-pcu at pdch.cpp:525. The
immediate cause is that as_dl_tbf() may well return NULL, which this
caller does not handle and instead dereferences immediately.
This is a code path that apparently assumes that a DL-TBF should always
be present. The higher level cause for the NULL DL-TBF has not been
identified.

Related: OS#5205 SYS#5561
Change-Id: I8ce21be6836549b47a606c00b793d6f005964c5c
2021-08-17 12:17:13 +00:00
Neels Hofmeyr 112c63e9b4 Revert "Stop abusing T3169"
This reverts commit 846fd248dc.

The commit introduced a leak of UL-TBF, which do not time out and
accumulate indefinitely, leading to out-of-memory for the running
osmo-pcu process.

A proper fix for the leak is pending on a development branch pespin/fsm,
but that branch is not yet ready for merging. Hence let's re-introduce
timer T3169 to avoid the OOM due to lingering UL-TBF.

Related: OS#5209
Change-Id: I99a7d2ddf68a76739ce2db1d6a44967dd97667b0
2021-08-15 17:46:53 +00:00
Neels Hofmeyr 4163361906 T_defs_bts: remove unit from doc strings
The main reason to change this is that the unit for T3172 is wrong. It
is defined as ms but the doc string says "(s)".

The tdef implementation already includes the unit as defined for each T
in the doc string implicitly, so instead of fixing that string, just
remove the unit strings from all the doc strings.

Now it will show:

 OsmoPCU# show bts-timer
 BTS0:
  T3142 = 20 s	Wait Indication used in Imm Ass Reject during TBF Establishment (CCCH) (default: 20 s, range: [0 .. 255])
  T3169 = 5 s	Reuse of USF and TFI(s) after the MS uplink TBF assignment is invalid (default: 5 s)
  T3172 = 5000 ms	Wait Indication used in Imm Ass Reject during TBF Establishment (PACCH) (default: 5000 ms, range: [0 .. 255000])
  T3191 = 5 s	Reuse of TFI(s) after sending (1) last RLC Data Block on TBF(s), or (2) PACKET TBF RELEASE for an MBMS radio bearer (default: 5 s)
  T3193 = 1600 ms	Reuse of TFI(s) after reception of final PACKET DOWNLINK ACK/NACK from MS for TBF (default: 100 ms)
  T3195 = 5 s	Reuse of TFI(s) upon no response from the MS (radio failure or cell change) for TBF/MBMS radio bearer (default: 5 s)

Related: OS#5209
Change-Id: I140122bb10f750bf996272cc7f9c5b541c9bd364
2021-08-12 15:08:16 +02:00
Oliver Smith d3c7591304 Add counters: pcu.bts.N.pch.requests.timeout
Implement T3113 for paging over PCH with default value of 7s (same as
T3113 in OsmoBSC). Increase the new counter on timeout.

Related: SYS#4878
Change-Id: I97475c3dbe2cf00b9cbfec39e93a3c65cb7f749f
2021-08-11 13:42:30 +02:00
Oliver Smith 4df959d305 Add counters: pcu.bts.N.pch.requests
Count attempted paging requests over PCH.

Related: SYS#4878
Change-Id: I1026780ef8542f40060b961df2f37213e15c29d7
2021-08-10 10:35:18 +00:00
Oliver Smith 978396732b Add counters: pcu.sgsn.N.rx_paging_{cs,ps}
Related: SYS#4878
Change-Id: Iefba6f3d29c69fd4865c084bd9cf1a3a78f5c202
2021-08-10 10:35:18 +00:00
Oliver Smith 3f561bfbfe test: add 'make update_exp' target
Add convenience target to update the test output.

Change-Id: I225dd3746200cad748ea09b2c3e1c7f9d006d32f
2021-08-06 22:21:14 +02:00
Pau Espin 945be91032 tests/tbf: Fix null pointer access if slowly stepping with gdb
When slowly debugging test_tbf_dl_llc_loss, bssgp_tx_llc_discarded() may
trigger, submitting events to the libosmogb code. Since it didn't
properly set up the callback, it would end up in a null pointer
dereference when lib code tried to use backward-compatible API (which
was neither set up properly).

"""
TBF(TFI=0 TLLI=0xc0123456 DIR=DL STATE=ASSIGN) Discarding LLC PDU because lifetime limit reached, count=3 new_queue_size=0
BSSGP (BVCI=2234) Tx LLC-DISCARDED TLLI=0xc0123456, FRAMES=3, OCTETS=57
/git/libosmocore/src/gb/gprs_ns.c:271:2: runtime error: member access within null pointer of type 'struct gprs_ns_inst'
"""

"""
(gdb) bt
 #0  0x00007ffff729cac0 in gprs_active_nsvc_by_nsei (nsi=nsi@entry=0x0, nsei=2234, bvci=bvci@entry=0)
    at /git/libosmocore/src/gb/gprs_ns.c:271
 #1  0x00007ffff72b1fec in gprs_ns_sendmsg (nsi=0x0, msg=0x621000000160) at /git/libosmocore/src/gb/gprs_ns.c:1087
 #2  0x00007ffff72d1803 in _gprs_ns_sendmsg (ctx=<optimized out>, msg=<optimized out>) at /git/libosmocore/src/gb/gprs_bssgp.c:80
 #3  0x00007ffff730226f in bssgp_tx_llc_discarded (bctx=<optimized out>, tlli=<optimized out>, num_frames=<optimized out>, num_octets=<optimized out>)
    at /git/libosmocore/src/gb/gprs_bssgp_bss.c:249
 #4  0x000055555588243e in gprs_rlcmac_dl_tbf::llc_dequeue (this=0x7ffff1622860, bctx=<optimized out>)
    at /git/osmo-pcu/src/tbf_dl.cpp:413
"""

Change-Id: Iee5bcf21afc8980a14f90f5b1ead6d2460a244ea
2021-07-26 16:05:55 +02:00
Pau Espin 81db7334da tbf: Drop impossible paths in create_dl_ass()
create_dl_ass() is only called in gprs_rlcmac_sched.cpp on
tbf_cand->dl_ass pointer, which is always assigned under the guard
"!tbf->is_control_ts(pdch->ts_no)", since we only send CTRL messages for
a TBF on its control TS.
Hence, condition "!is_control_ts(ts)" in create_dl_ass will always be
false, and as a result poll_ass_dl will always be 1.
So we can drop different code paths.

Change-Id: Ibea4100a5dc8bd49303cb6a3d02417038c3d3887
2021-07-22 20:06:18 +02:00
Pau Espin 890de986ce Make gcc 11.1.0 false positivies happy
After my system's gcc was upgraded, I get false positivies like the one
below:
"""
/git/osmo-pcu/src/gprs_bssgp_pcu.c: In function ‘ns_configure_nse’:
/git/osmo-pcu/src/gprs_bssgp_pcu.c:1103:58: error: ‘%d’ directive output may be truncated writing between 1 and 11 bytes into a region of size 2 [-Werror=format-truncation=]
 1103 |                         snprintf(name, sizeof(name), "pcu%d", i);
      |                                                          ^~
/git/osmo-pcu/src/gprs_bssgp_pcu.c:1103:54: note: directive argument in the range [-2147483648, 1]
 1103 |                         snprintf(name, sizeof(name), "pcu%d", i);
      |                                                      ^~~~~~~
/git/osmo-pcu/src/gprs_bssgp_pcu.c:1103:25: note: ‘snprintf’ output between 5 and 15 bytes into a destination of size 5
 1103 |                         snprintf(name, sizeof(name), "pcu%d", i);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
"""
In this case, i can't never take a value with more than 1 digit, but gcc
seems to be unable to see that.

Let's increase the buffer size a few bytes to make gcc happy, and make
the variable unsigned since it never will get negative values.

Next change is also a false positive, since variables are always
initialized beforehand in the cod epaths where they are used:
"""
/git/osmo-pcu/src/bts.cpp: In function ‘int bts_rcv_rach(gprs_rlcmac_bts*, const rach_ind_params*)’:
/git/osmo-pcu/src/bts.cpp:859:25: error: ‘ts_no’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  859 |         uint8_t trx_no, ts_no;
      |                         ^~~~~
/git/osmo-pcu/src/bts.cpp:859:17: error: ‘trx_no’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  859 |         uint8_t trx_no, ts_no;
      |                 ^~~~~~
"""

Change-Id: I1362a335a0c761bde367dbc779de4afa88f13584
2021-07-15 14:39:39 +02:00
Pau Espin 4f67a9bf46 pdch: Fix heap-use-after-free in pdch->ulc
In existing previous code, pdch->ulc would be freed in
gprs_rlcmac_pdch::free_resources() when  it became disabled as per PCUIF
info_ind (for instance, when a DYN TS is switched PDCH->SDCCH8).
However, pdch->ulc was so far only allocated during pdch_init, which is
only called during bts_alloc() time.
Hence, after first info_ind disabling it, if it became again enabled
(again by info_ind re-enabling it after SDCCH8 was not longer in use),
the pdch->ulc would be used again but it would point to freed memory.

Let's rearrange how/when resources are freed to make it more logical.
With this patch, pdch internal resources are freed upon ->disable(), and
re-allocated upon ->enable().

Change-Id: Id51f5f6a54ac9f24b784c17bc360ac38f5726fc7
2021-07-01 13:09:10 +02:00
Pau Espin 1989a19066 Support proto IPAC_PROTO_EXT_PCU BSC<->PCU
Related: SYS#5303
Change-Id: I633db291107883c2e370a9b56606d562a990b714
2021-06-25 17:20:50 +02:00
Pau Espin 8c29236d35 pcuif_proto.h: Add new container message
Related: SYS#5303
Change-Id: Ib6c7bf5ca5a06186a71ec50cfc1a91a5c9b01d9c
2021-06-25 17:20:46 +02:00
Pau Espin ab178903d4 pdch: Fix null MS access gprs_rlcmac_pdch::rcv_control_ack
If bts_ms_by_tlli() at the start of the function fails, ms could be
NULL. As a result "ms->nacc" access at the end of the function would
crash.
Solution:
In the function, we get the related expected TBF from pdch_ulc, and we only
continue if a TBF is found. Since tbf objects are always expected to
have a GprsMs, simply gather it from there.

Change-Id: I666ed5d157f42e74956fa49fc9eea85d27e63d44
2021-06-23 13:50:35 +02:00
Vadim Yanitskiy b657213773 pcu_l1_if: ignore PDCH interference reports, do not log errors
Change-Id: I88e5c53131ee94bc3f3ff3f095077feb4ff272a7
Related: SYS#5313, OS#1569
2021-06-22 05:58:23 +00:00
Vadim Yanitskiy 94dc6a8b6c PCUIF protocol: add message definition for interference report
Change-Id: I4b5a4c25e984f9262f0afd30f9671f150edee20f
Related: SYS#5313, OS#1569
2021-06-22 05:58:23 +00:00
Vadim Yanitskiy 826576287e gprs_rlcmac_sched: fix incorrect length for CTR_RLC_DL_BYTES
msg->data_len is the total number of bytes available in the buffer,
while for CTR_RLC_DL_BYTES we need to count size of the actual
payload within the buffer.  A consequence of this bug: osmo-pcu
was counting more Downlink bytes than it's actually transmitted.

Change-Id: I6884d220f3d06a79b16c18ccc2d2a6cd047b8251
2021-06-21 02:33:07 +02:00
Pau Espin 86f4c093d1 pcuif: Support receiving System Information 2
OsmoPCU will need this SI2 in order to gain knowledge of the BCCH
Frequency List being broadcasted, in order to build a per-MS specific
Neighbour List using NC_FREQUENCY_LIST bits in Packet Measurement Order.

Related: SYS#5303
Change-Id: I4a9c4f70beac6805322a19835a0d30f7247780b4
2021-06-15 19:11:07 +02:00
Pau Espin c6e911cf22 pdch: Log pdch_ulc reason upon rx of pkt ctrl ack
Change-Id: I7c7a421b1e9189e2814e9a28698d66655fc9ba60
2021-06-07 18:16:55 +02:00
Pau Espin 9c1db1738f Use new stat item/ctr getter APIs
Patch mostly done with the help of several small spatch snippets.

Change-Id: I600c7a8725f5b229b1a2feb879da7c3b2dce4505
2021-06-04 17:14:32 +02:00
Pau Espin d65bd9d7b2 bts: Fix typo in field name
Change-Id: I5426ff4ccbc45464888e2246cceb8e861d1e477e
2021-06-01 16:43:41 +02:00
Pau Espin c9880b97cf csn1: Implement CSN_CALLBACK type in encoder
Picked code from the Decoder function. I gave it a try
callback_init_Cell_Selection_Params_FREQUENCY_DIFF and looks
like working fine.

Change-Id: Iac962ae3e9f52f417f394060b64fc4d0ebf3d0bf
2021-05-28 18:42:42 +02:00
Pau Espin 4c2387026a gsm_rlcmac.c: Fix arg list of 2 callbacks
Other callback functions are properly specified as per what's in
"typedef CSN_CallBackStatus_t". However, these two were wrong.

Change-Id: I280b51d4c8c38c76cc1ccd49656b6b7bbe769760
2021-05-28 18:42:42 +02:00
Pau Espin 2761e574de cosmetic: Fix typo s/TIMSI/TMSI/
Change-Id: I64231311633b64d898625c49fdbf3f816dfbb97a
2021-05-21 13:44:18 +02:00
Pau Espin dc2aaac29f tbf: Move existing tbf_state implementation to osmo_fsm
This is only an initial implementation, where all state changes are
still done outside the FSM itself.
The idea is to do the move in several commits so that they can be
digested better in logical steps and avoid major break up.

Related: OS#2709
Change-Id: I6bb4baea2dee191ba5bbcbec2ea9dcf681aa1237
2021-05-19 12:50:25 +02:00
Pau Espin 38f80be73b MsTest: Set up tbf talloc destructor
This is right now not an issue, but it will be whenever talloc
destructor contains extra steps like freeing an FSM.

Change-Id: I096ff56321c8ae5e66634537aae8b95804282c65
2021-05-19 12:50:25 +02:00