Project

General

Profile

Actions

Bug #3499

closed

pcu is logging "Allocating DL TBF: MS_CLASS=0/0" ; TLV parsing code commented?

Added by keith over 5 years ago. Updated almost 4 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Target version:
-
Start date:
08/24/2018
Due date:
% Done:

100%

Spec Reference:

Description

This also relates to this error seen with EGPRS capable phone:
"Not accepting non-EGPRS phone in EGPRS-only mode"

However, in GPRS mode, this does not appear to make any immediately obvious difference to functionality, but all the same, MS_CLASS=0/0 is obviously wrong.

This is because the code to parse the MS RA Cap is commented, with a note:

"Do not rely on this IE."

As far as I can tell from analysis of NS trace, the BSSGP contains correct Radio Access Capability IE, that is to say, It is corresponding with what the MS sent in the Attach Request.

So why "do not rely?" Why is the code commented? Perhaps in 2016, this information from the BSSGP was not reliable?

See http://git.osmocom.org/osmo-pcu/commit/?id=741d25cb6f8c0c1522cf6d1be2fea49356ecd4bd
and the two previous commits.


Related issues

Related to OsmoPCU - Feature #1525: Use the Radio Access Capabilites that are being transferred via BSSGP (downlink)Resolvedpespin02/22/2016

Actions
Actions #1

Updated by laforge over 5 years ago

Actions #2

Updated by msuraev over 5 years ago

  • Related to Feature #1525: Use the Radio Access Capabilites that are being transferred via BSSGP (downlink) added
Actions #3

Updated by laforge almost 5 years ago

  • Assignee set to lynxis
Actions #4

Updated by laforge about 4 years ago

  • Assignee deleted (lynxis)
Actions #5

Updated by pespin about 4 years ago

This ticket is a duplicate (talking about same issue) of #1525

We need to add some TTCN3 test to check emulation-SGSN sending this data to PCU and make sure it's taken into account.

Related code: gprs_bssgp_pcu_rx_dl_ud() (src/gprs_bssgp_pcu.cpp).

Related commits (from older to newer, they are together in history):
0df80be95e3604656ff36024f793ef3c36455051
4a07a3be11e7366b557bab795aa23b42725ec23e
f4bb42459ca4f3e18f9ee3d3a27389b85c7692e8
741d25cb6f8c0c1522cf6d1be2fea49356ecd4bd

Actions #6

Updated by pespin about 4 years ago

  • Assignee set to pespin
Actions #7

Updated by keith about 4 years ago

pespin wrote:

This ticket is a duplicate (talking about same issue) of #1525

Yes, I did not see #1525 when I made this. Feel free to close this one. (maybe move the description to a comment in the other ticket if you feel it's helpful in any way)

Actions #8

Updated by pespin about 4 years ago

According to some TTCN3 tests I'm doing, decode_gsm_ra_cap() is broken an returns an error, that's probably why it was commented out (probably they thought messages sent by SGSN were wrong, but actually decoding in PCU seems broken.).
I'm working on fixing it now.

Actions #9

Updated by pespin about 4 years ago

I just saw that csn1.cpp/csn1.hpp files are actually coming initially from wireshark. In current master, they are named:
epan/dissectors/packet-csn1.c
epan/dissectors/packet-csn1.h

I think it may make sense to try updating our code to match the one in wireshark instead of fixing this exact issue, because wireshark is decoding it fine.

Actions #10

Updated by pespin about 4 years ago

I went over commits in wireshark.git/packet-csn1{.cpp,.h} and ported changes to our csn1.* files to have a closer code from there, since the one in wireshark is more prone to get fixes and better support.

remote: https://gerrit.osmocom.org/c/osmo-pcu/+/16994 csn1: Update M_NULL CSN_DESCR to match wireshark
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/16995 csn1: packet-csn1.c:179: warning: 'pui8' may be used uninitialized in this ...
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/16996 csn1: shuffle decrements of remaining_bits_len
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/16997 csn1: Extend CSN_SERIALIZE to allow 0 bit of length
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/16998 csn1: Allow CHOICE elements to re-process the bits used for the choice
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/16999 csn1: Fix pedantic compiler warnings in csn.1 dissectors
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17000 csn1: Fix an infinite loop in CSN.1 dissector when having more than 255 ...
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17001 csn1: Fix warning with -Wmissing-prototypes
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17002 csn1: Don't cast away constness
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17003 csn1: Try to fix cast discards '__attribute__((const))' qualifier from ...
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17004 csn1: Drop format_p union from CSN_DESCR
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17005 csn1: fix this statement may fall through [-Werror=implicit-fallthrough=] ...

TBD:

  • Check commits in following wireshark files, which matches with the structs we have in src/gsm_rlcmac.cpp of osmo-pcu.git. In there there may be fixes which could help us with this task.
    packet-gsm_rlcmac.c
    packet-gsm_rlcmac.h
  • Fix #4375: enable logging messing with decoder test result
  • Improve logging in csn1.cpp
  • Find out what fails exactly during decode of RA cap.
Actions #11

Updated by pespin about 4 years ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 20

I submitted a bunch of commits ported from wireshark's current packet-gsm_rlcmac.cpp/h files (wireshark master=0d4e81e7c7392222446526370d93499fab7ee4f9)

remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17041 gsm_rlcmac.h: #if 0 unused stuff
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17042 gsm_rlcmac.h: Make sure we have a corresponding 'u' member to RlcMacDownlink_...
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17043 gsm_rlcmac.h: Remove Uplink messages from the RlcMacDownlink_t structure
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17044 gsm_rlcmac: Enhance dissection of PSI1
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17045 gsm_rlcmac.cpp: Fix trailing whitespace
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17046 gsm_rlcmac.cpp: hanged all M_BIT macros to M_UINT, as M_BIT does not use the ...
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17047 gsm_rlcmac.cpp: Do not skip too many lines of the CSN_DESCR when the field ...
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17048 gsm_rlcmac.cpp: fix an out of bounds access
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17049 gsm_rlcmac: add dissection of NAS container
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17050 gsm_rlcmac: improve dissection of Packet Resource Request message
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17051 gsm_rlcmac: Update : PACKET RESOURCE REQUEST to Release 14.0.0
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17052 gsm_rlcmac.cpp: fix global-buffer-overflow error reported by ASAN
remote: https://gerrit.osmocom.org/c/osmo-pcu/+/17053 gsm_rlcmac.cpp: fix another global-buffer-overflow error reported by ASAN

Notice one of the patches is breaking a unit test and I had it change, so I changed the expected output. It needs to be investigated before merging it.

Some patches are still remaining but are not critical to the task here, and have been left as a separate ticket #4382.

TBD:
  • Investigate one ported patch breaking unit test (may be related to #1640).
  • Fix #4375: enable logging messing with decoder test result
  • Improve logging in csn1.cpp
  • Find out what fails exactly during decode of RA cap.
Actions #12

Updated by pespin about 4 years ago

  • Status changed from In Progress to Feedback
  • % Done changed from 20 to 90

Thanks to all recent work done in osmo-pcu CSN1/RLCMAC decoder, I could investigate the issue further, and it should be fixed by https://gerrit.osmocom.org/c/osmo-pcu/+/17524

Tested with: https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/17526

This ticket can be closed once the patches are merged.

keith give it a try if you want.

PS: I also submitted a patch to wirershark to be able to decode non CS-1 (e)gprs data blocks, see #1542.

Actions #13

Updated by pespin almost 4 years ago

  • Status changed from Feedback to Resolved
  • % Done changed from 90 to 100

Should be definitely fixed after all changes merged today 81b40cbaf3070f70954663f68375100128bdc77e..e50ce6e45c4509805807d599cadf1a1b23d37f63

Actions

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)