This was actually discovered by the following compiler warning in
gcc-6.2.0:
CC gprs_bssgp_bss.lo
gprs_bssgp_bss.c: In function ‘bssgp_rx_paging’:
gprs_bssgp_bss.c:544:2: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
if (TLVP_PRESENT(&tp, BSSGP_IE_TMSI) &&
^~
gprs_bssgp_bss.c:548:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
*(pinfo->ptmsi) = ntohl(*(uint32_t *)
^
This is an actual bug. If we recevied a BSSGP Paging Request without
P-TMSI, we might crash or report some random memory as P-TMSI to the
caller in the output data structure.
Change-Id: Ib4f307827cd7cccc91c1415a6fb5428d7cf8416d
gprs_bssgp_vty.c:48:34: warning: ‘gprs_bssgp_timer_strs’ defined but not used [-Wunused-const-variable=]
static const struct value_string gprs_bssgp_timer_strs[] = {
^~~~~~~~~~~~~~~~~~~~~
Change-Id: Ia41ccb7b227c41996cdef51dc6779bfc5b5a8d48
gprs_bssgp.c:461:9: runtime error: load of misaligned address
0x62100001a66b for type 'uint32_t', which requires 4 byte alignment
gprs_ns.c:937:16: runtime error: load of misaligned address
0x61d00002a97f for type 'uint16_t', which requires 2 byte alignment
This function originates from openbsc/src/gprs but is just specific
to BSSGP/Gb on the same level like bssgp_msgb_alloc.
This commit puts the former gprs_msgb_copy function beside
bssgp_msgb_alloc.
Renamed function:
gprs_msgb_copy -> bssgp_msgb_copy
Sponsored-by: On-Waves ehf
Currently the state is assumed to remain the same while the reset
procedure is active. While this works correctly in general, a single
unexpected BLOCK_ACK or UNBLOCK_ACK can change the state but will not
stop the reset procedure. The leads to repeated RESET messages, where
the corresponding RESET_ACK is ignored. This is a stable state which
can only be left by manual intervention or by reception of a RESET
message from the peer.
This commit changes the RESET timeout handler to set the state to
BLOCKED/RESET when sending the new NS RESET message.
Note that this should ensure a clean restart even if the state has been
screwed up. It does not fix the handling of BLOCK_ACK or UNBLOCK_ACK
in abnormal cases.
Addresses:
gprs_ns.c:349 NSEI=8895 Tx NS RESET (NSVCI=8895, cause=O&M intervention)
gprs_ns.c:878 NSVCI=8895 Rx NS RESET ACK (NSEI=8895, NSVCI=8895)
gprs_ns.c:887 NS RESET ACK Discarding unexpected message for NS-VCI 8895 from SGSN NSEI=8895
Ticket: OW#1551
Sponsored-by: On-Waves ehf
The stat item group is not removed by gprs_nsvc_delete which will
corrupt the group list.
Addresses:
valgrind tests/gbproxy/gbproxy_test
[...]
==4541== Invalid write of size 4
==4541== at 0x4071ACA: __llist_add (linuxlist.h:65)
==4541== by 0x4071ACA: llist_add (linuxlist.h:81)
==4541== by 0x4071ACA: osmo_stat_item_group_alloc
(stat_item.c:112)
==4541== by 0x407EDFD: gprs_nsvc_create (gprs_ns.c:244)
==4541== by 0x408109D: gprs_ns_instantiate (gprs_ns.c:1388)
==4541== by 0x804CFD3: test_gbproxy_ident_changes
(gbproxy_test.c:1501)
==4541== by 0x805FBD3: main (gbproxy_test.c:5803)
Sponsored-by: On-Waves ehf
Shipping our own private copy of talloc was a good idea in 2008,
when it was not readily available on most target platforms. Today,
the situation is quite different, as it is a standard library on
major Linux distributions.
The NS object is created with an unknown identity and only after
the reset procedure has progressed (completed?) we know the real
ID for this peer. Before nobody has looked at the idx values (this
could have been seen with the CTRL interface) but with statsd the
wrong NSVCI becomes obvious.
Add routines to update the idx and I don't know if the change of
idx is causing any issues but we will find that out soon.
Currently the class_id is not set which effectively puts these groups
into the 'subscriber' class.
This commit adds the missing initialisation value.
Sponsored-by: On-Waves ehf
This commit adds class_id fields to the rate_ctr and stat_item group
descriptions. The stats reporter code is extended to only process
groups whose class_id does not exceed a per reporter max_class level.
If the class_id is not set, the code assumes 'global' for groups with
idx == 0 and 'subscriber' otherwise.
The following vty command is added to config-stats:
level (global|peer|subscriber) Set the maximum group level
Sponsored-by: On-Waves ehf
Since the the stat_item and stats functions and data types are meant
to be exported, they get an osmo_ prefix.
Sponsored-by: On-Waves ehf
[hfreyther: Prepended the enum values too. This was requested by
Jacob]
The following counters are added to the ns.nsvc counter group:
lost.alive The number of missing ALIVE ACK messages
lost.reset The number of missing RESET ACK messages
The following items are added to the ns.nsvc stat item group:
alive.delay The time in ms between sending ALIVE and
receiving the next ALIVE ACK
Sponsored-by: On-Waves ehf
Currently the size of the IMSI pointer is used instead of the size of
the talloc'ed buffer.
This commit changes the call to gsm48_mi_to_string to use the same
value that has been used with talloc_zero_size(). The length is
changed to 17 since that value is used for GSM_IMSI_LENGTH in
openbsc.
Fixes: Coverity CID 1040663
Sponsored-by: On-Waves ehf
Currently large values for Bmax default MS get sliced since a uint16_t is
used as the type of the corresponding parameter of bssgp_tx_fc_bvc.
GSM 48.018, 11.3.2 which in turn refers to 11.3.5 specifies a maximum
of 6MB (0xffff * 100).
This commit changes the type to uint32_t to cover the full value
range.
Sponsored-by: On-Waves ehf
Currently the DL sometimes hangs and sometimes a lot of messages
(still not able to send PDU) are logged. This is caused by an invalid
timer delay computation, setting msecs either to 0 or to some big value.
This is due to an '&' operator at the wrong place, accessing some
parts in fc instead of the first element of the list.
This commit fixes that issue.
Sponsored-by: On-Waves ehf
Currently all 2 byte IE of the message are transmitted in the little
endian byte ordering.
This commit adds htons to the encoding expressions.
Sponsored-by: On-Waves ehf
Currently out-of-memory is not handled by bssgp_msgb_alloc, leading
to SEGV failures if msgb_alloc_headroom returns NULL.
This commit adds an OSMO_ASSERT to catch this case, which improves
the situation only slightly. But bssgp_msgb_alloc is used in many
places without checking the return value, so just adding a
conditional early NULL return would not fix the issue either.
Fixes: Coverity CID 1293377
Sponsored-by: On-Waves ehf
Currently the return value of the gprs_ns_tx family of functions is
often ignored. This is not a serious issue, since the successful
delivery of the messages is neither guaranteed nor acknowledged by
the network layer anyway.
Nevertheless this commit adds logging (level INFO) to gprs_ns_tx and
gprs_ns_msgb_alloc. The definition of the latter has been moved from
the header file to gprs_ns.c.
Fixes: Coverity CID 1040678, 1040679, 1040680, 1040681, 1040682,
1040683, 1040684, 1040686, 1040687, 1040688, 1111545,
1240203, 1240204
Sponsored-by: On-Waves ehf
Currently bssgp_rx_ptp might be called with bctx being NULL, when the
NS BVCI is neither BVCI_SIGNALLING nor BVCI_PTM, but the message is
a BVC_RESET or it contains an BVCI IE != BVCI_SIGNALLING where the
BVCI is not known.
This patch ensures that bssgp_rx_ptp will only be called with a
non-NULL bctx. A log message will be issued, if the bctx is NULL when
this was not expected.
Fixes: Coverity CID 1040674
Sponsored-by: On-Waves ehf
Currently the implementation of bssgp_tx_dl_ud conditionally adds
some optional IE if dup != NULL. Later on is dereferences dup to
access qos_profile and fc, but this without checking dup in advance.
This may lead to an segmentation violation fault.
This commit changes the value range of the function to only accept
dup != NULL. An assertion will fail otherwise. All other explicit
checks for non-NULL are removed.
Fixes: Coverity CID 1040673
Sponsored-by: On-Waves ehf
Currently incoming BSSGP STATUS messages are just logged and no other
action is taken. This makes it impossible for higher layers to react
to failures which are indicated by corresponding STATUS messages
unless a timeout is triggered as a result of that failure later on.
This commit adds a bssgp_rx_status() function and calls it on
incoming STATUS messages. That function logs a message, increments the
new BSSGP_CTR_STATUS counter if the bctx context exists and invokes
an NM_STATUS status indication. The latter will allow the application
to handle failures immediately. Since all STATUS messages should be
handled, the function is already called in bssgp_rcvmsg and the
message is no longer handled in (and will not reach) bssgp_rx_sign
and bssgp_rx_ptp.
Ticket: OW#1414
Sponsored-by: On-Waves ehf
Currently each incoming PtP BSSGP STATUS message is handled as 'not
yet implemented' and a BSSGP STATUS message (cause
BSSGP_CAUSE_PROTO_ERR_UNSPEC) is sent back to the peer. This will
cause endless messages loops if both peers use this BSSGP stack
implementation. This does not apply to signalling messages.
This commit changes the implementation of bssgp_rx_ptp() to just do
logging in this case.
Sponsored-by: On-Waves ehf
Currently this command segfaults (at least when ASAN is enabled),
because when getting the NSEI the index to argv is wrong and out of
bounds.
This patch fixes the offset.
Sponsored-by: On-Waves ehf
The copyright holders Harald Welte, Holger Freyther, Andreas Eversberg
and sysmocom - s.f.m.c. GmbH (represented by Holger and Harald) agree
that the license of libosmogb should be GPLv2+ and not AGPLv3+.
The reason the source files stated AGPLv3+ is due to the history, as
they were moved from OpenBSC to libosmocore at the time we needed to use
them from osmo-pcu. It was an oversight back then to not re-license
them accordingly.
Currently sending SUSPEND/RESUME messages to this function (like it
is done in the osmo-sgsn) results in STATUS messages complaining
about an unknown BVCI. The reason is, that these messages rely on a
TLLI/RAI pair to identify the context and do not contain an explicit
BVCI.
This patch modifies bssgp_rcvmsg() to only complain about and unknown
BVCI if one is given but a matching context is not found (except for
RESET messages). The ctx argument is removed from the functions
handling SUSPEND and RESUME since it will always be NULL then.
Sponsored-by: On-Waves ehf
In the OsmoSGSN we have a crash with a DEAD/BLOCKED GPRS-NS and
segmented SN-UNITDATA. For the caller it is not easy to know if
the passed msg buffer has been freed or not. The most easy solution
is to always take the ownership and either pass it on or free it
in case of an error.
Adjust indirect and direct callers of gprs_ns_sendmsg. I found
the following call-chains with an external msgb parameter.
gprs_ns_sendmsg
<- _bssgp_tx_dl_ud
<- bssgp_fc_in
<- bssgp_tx_dl_ud
Update the test to allocate a real msgb because for the test with
'1000' we will msgb_free it right away.
Sponsored-by: On-Waves ehf
Currently the NS-VC's state is updated from within gprs_ns_tx_reset,
which can lead to an inconsistent state when the RESET_ACK is lost.
In this state, the NSE_S_RESET bit is set but the Tns-reset timer is
not started.
This patch moves the state update into gprs_nsvc_reset. This way, the
state flags are consistent with the timer.
Addresses:
SGSN -> BSS NS_ALIVE
BSS -> SGSN NS_ALIVE_ACK
BSS -> SGSN BVC_RESET
SGSN -> BSS NS_STATUS, Cause: NS-VC blocked, NS VCI: 0x65
and there is no BSS->SGSN NS_ALIVE
Ticket: OW#1213
Sponsored-by: On-Waves ehf
Currently gprs_nsvc_reset does not return any value.
This patch changes the function to return an integer, where a value
less than zero indicates an error. The value is taken from the
gprs_ns_tx_reset function. In case of failure, an error message is logged.
Sponsored-by: On-Waves ehf
the bits/crc files are not inside the sourcedirectory but will end
in the build directory. Go and look there as well. This somehow
doesn't fail with make distcheck but when building for Yocto Dizzy.
Currently the bssgph field is not set when using the bssgp_tx_*
functions. This hinders unit testing of generated messages.
This patch initializes the bssgph field directly after allocation a
new bssgp msgb in bssgp_msgb_alloc() so that it is set by default.
Sponsored-by: On-Waves ehf
Currently BSSGP messages with an NS BVCI of 0 (signalling) are
discarded if they aren't RESET messages. Thus valid signalling
messages (e.g. BLOCK) are not handled properly, because the BVCI IE
is ignored if it present. Instead a STATUS message referring to BVCI
0 (instead of the BVCI used in the BLOCK message) is returned.
This patch changes the implementation to use the BVCI contained in
the BVCI IE if that is present in a signalling message.
It fixes BSSGP BLOCK/UNBLOCK for the osmo-sgsn.
Note that signalling messages without an BVCI IE (e.g.
SUSPEND/RESUME) are still rejected.
Ticket: OW#1205
Sponsored-by: On-Waves ehf
Currently the BVCI is not set in all invocations to bssgp_tx_status()
when the cause is UNKNOWN_BVCI.
This patch adds the argument where it is missing.
It also adds a check for compliance (GSM 08.18, 10.4.14.1) to
bssgp_tx_status() to emit errors when the following requirement is
not fulfilled: The BVCI must be included if (and only if) the cause
is either "BVCI blocked" or "BVCI unknown".
Sponsored-by: On-Waves ehf
Fix re-initialize issue of the GPRS NS. This was found while
working on the GB Proxy tests.
==27800== Invalid write of size 4
==27800== at 0x403C263: rate_ctr_group_alloc (linuxlist.h:65)
==27800== by 0x4050974: gprs_nsvc_create (gprs_ns.c:209)
==27800== by 0x405320D: gprs_ns_instantiate (gprs_ns.c:1330)
==27800== by 0x804B212: main (gbproxy_test.c:797)
==27800== Address 0x434173c is 52 bytes inside a block of size 784 free'd
==27800== at 0x4029DA8: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==27800== by 0x4041BBD: _talloc_free (talloc.c:609)
==27800== by 0x40432B2: talloc_free (talloc.c:578)
==27800== by 0x40532D3: gprs_ns_destroy (gprs_ns.c:1363)
==27800== by 0x804B1FE: main (gbproxy_test.c:791)
Okay. this comment thing has not worked (I totally forgot about it).
I think either we try to find ABI breakages with tools or we use
#error macros and check for a version...
Currently the field nsvci_is_valid is set to 0 in the NSVC object
returned by gprs_nsvc_create(). This was a semantic change probably
introduced by commit 5e6d679d. As a result, NSVC created via the VTY
have this flag set to 0 causing RESET_ACK messages to be rejected.
This patch changes the default behaviour of gprs_nsvc_create() to
always set this flag. So it must be set to 0 explicitely if needed
which is more intuitive and thus less error prone.
It fixes breaking connections from the Gbproxy to the SGSN.
Ticket: OW#874
Sponsored-by: On-Waves ehf
Currently those messages are answered by a NS_STATUS message when
received on an uninitialised (no NS_RESET hat been seen yet) NS_VC
which violates GPP TS 08.16 7.3.1 and 7.4.1.
This patch adds checks to gprs_ns_vc_create() and eventually returns
before the error NS_STATUS message is generated.
Sponsored-by: On-Waves ehf
Harald Welte
Neels Hofmeyr
Max