Attention is currently required from: wbokslag.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/33953 )
Change subject: Added calls to stub decryption functions for MAC resources
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
File src/tetra_upper_mac.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/33953/comment/b09ed4e4_47cac60e
PS3, Line 301: m
so you want to move the tail but not change the length (as msgb_get does). That's highly unusual, as it leaves the msgb in a somehwat inconsistent state.
--
To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/33953
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-tetra
Gerrit-Branch: master
Gerrit-Change-Id: I92d718789d6b7e84c1901d09165fce59cdf8c1ca
Gerrit-Change-Number: 33953
Gerrit-PatchSet: 3
Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Comment-Date: Thu, 27 Jul 2023 11:33:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: wbokslag.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/33943 )
Change subject: Added first steps towards tetra crypto support
......................................................................
Patch Set 3:
(9 comments)
Patchset:
PS3:
I feel a bit sorry for the large number of comments. Don't treat them as strict change requests, more like "these are my thoughts, do with them what you think'. The only ones I think are blockers is the talloc vs. calloc use and the license/copyright header.
File src/crypto/tetra_crypto.h:
https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/e26312ab_fed450a8
PS3, Line 47: mcc
it's been a long time since I read TETRA specs. However, in GSM there is a difference between MNC 02 and 002, and we made the early mistake to represent them as integers, where we cannot differentiate those two distinct values. We then had to invent work-arounds, etc.
So if TETRA also has similar differences (you tell me!) in terms of number of digits / leading zeroes, it might be best to avoid int.
https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/d9329000_417adb5d
PS3, Line 54: int is_gssi_range;
: int start;
: int end;
are there situations where those integers really can be negative?
https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/46c19233_b00a3a79
PS3, Line 61: mnc
is there a reeason to use int for mnc/mcc in 'struct network_info' but uint32_t here?
https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/db06bc10_6ecaf02e
PS3, Line 71: int num_keys;
I guess all of these could be unsigned as they simply count the number of list elements? Using the more constrained unsigned avoids situations where it's unclear what the meaning of a negative number could mean in this context, and it also helps to avoid static analysis tools to "think" about the question of negative values where none are desired/useful.
https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/318f3980_df2ccedc
PS3, Line 93: const char *tetra_get_key_type_name(int key_type);
didn't you define enums above for those types? Wondering why the argument type is int instead of the enum
File src/crypto/tetra_crypto.c:
https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/aa304436_7ca02f87
PS3, Line 14: struct tet
I'd generally suggest to a void global variables for state that might exist multiple times (think of decrypting multiple different calls/subscribers/... simultaneously).
This is why I'm using a context pointer (like 'struct tetra_mac_state *) that gets passed around to functions. So future software can have multiple of those around, rather than assuming there is only a singleton.
https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/989c662a_b8a769cc
PS3, Line 82: calloc
we generally use talloc in the existing osmo-tetra code base, and it would make sense to not mix different memory allocators. This way it's clear one always uses one type of free() function, and not another.
https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/f991f1b1_bf3f500d
PS3, Line 96: k->mcc, k->mnc, k->key_type);
we have something like OSMO_STRBUF_APPEND() in osmocom/core/utils.h to avoid the manual size-counting + pointer arithmetic in the snprintf calls here.
Not critical, up to you.
--
To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/33943
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-tetra
Gerrit-Branch: master
Gerrit-Change-Id: I63bc712630ae5dbaa049c129d456f7aef5bda863
Gerrit-Change-Number: 33943
Gerrit-PatchSet: 3
Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Comment-Date: Thu, 27 Jul 2023 11:31:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: wbokslag.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/33943 )
Change subject: Added first steps towards tetra crypto support
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
I know not all of the existing files adhere to that rule, but particularly as large portions of code are contributed by new authors, I think a comment section with copyright / author information and the license text and/or a SPDX line would be useful to have in all newly introduced files.
--
To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/33943
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-tetra
Gerrit-Branch: master
Gerrit-Change-Id: I63bc712630ae5dbaa049c129d456f7aef5bda863
Gerrit-Change-Number: 33943
Gerrit-PatchSet: 3
Gerrit-Owner: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: wbokslag <w.bokslag(a)midnightblue.nl>
Gerrit-Comment-Date: Thu, 27 Jul 2023 11:19:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33924 )
Change subject: gprs_rlc_ul_window: Mark received BSNs falling out of the V(N)/RBB when V(R) is raised
......................................................................
gprs_rlc_ul_window: Mark received BSNs falling out of the V(N)/RBB when V(R) is raised
This is a port of libosmo-gprs.git commit 49535bec37caddcea5718ed4e526c477c71f1b3a applied to osmo-pcu.
Problem this patch is fixing:
The current RLCMAC code is never invalidating the BSNs which have
been received after they are not needed.
This is somehow workaround by the following check when eg BSN wraps
around 127->0 and the BSN=0 is received:
""""
rcv_data_block_acknowledged()
m_window.m_v_n.is_received(rdbi->bsn)
/* Offset to the end of the received window */
offset_v_r = (m_v_r - 1 - bsn) & mod_sns();
return m_v_n.is_received(bsn) && offset_v_r < ws();
"""
The first m_v_n_is_received() would return true because the BSN has
never been invalidated, which is wrong, but fortunately the extra check
"offset_v_r < ws()" returns > ws() since:
v_r=0, bsn=0: "(0 - 1 - 0) & 127" ----> 0xffff & 0x007f ---> 0x007f > 64
So thanks to the above the BSN is considered not received, but looking
at the V(N) array would say the opposite. Hence, this commit aims at
fixing the V(N) array to contain proper values (marked "invalid") for
BSNs becoming out of the window when it rolls forward.
Explanation of the solution:
The V(N) array contains the status of the previous WS (64 in GPRS) data
blocks. This array is used to construct the RRB signaled to the peer
during PKT UL ACK/NACK messages together with the SSN (start sequence
number), which in our case is mainly V(R), aka one block higher than the
highest received block in the rx window.
Hence, whenever PKT UL ACK/NACK is transmitted, it contains an RRB
ranging [V(R)-1,...V(R)-WS)] mod SNS (SNS=128 in GPRS).
The receive window is basically [V(Q) <= BSN < V(R)] mod SNS, as is of
size 64.
The V(R) is increased whenever a highest new block arrives which is in the
receive window (guaranteeing it will be increased to at most V(Q)+64.
Since we are only announcing state of blocks from V(R)..V(R)-WS, and
blocks received which are before that BSN are dropped since don't fall
inside the rx window, we can securely mark as invalid those blocks
falling behind V(R)-WS whenever we increase V(R).
Related: OS#6102
Change-Id: I5ef4dcb0c5eac07a892114897f9e4f565b1dcc2c
---
M src/rlc_window_ul.cpp
M src/rlc_window_ul.h
M src/tbf_ul.cpp
3 files changed, 69 insertions(+), 3 deletions(-)
Approvals:
Jenkins Builder: Verified
laforge: Looks good to me, but someone else must approve
osmith: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
diff --git a/src/rlc_window_ul.cpp b/src/rlc_window_ul.cpp
index 568e126..32810d1 100644
--- a/src/rlc_window_ul.cpp
+++ b/src/rlc_window_ul.cpp
@@ -37,8 +37,14 @@
/* Positive offset, so raise. */
if (offset_v_r < (sns() >> 1)) {
while (offset_v_r--) {
- if (offset_v_r) /* all except the received block */
- m_v_n.mark_missing(v_r());
+ const uint16_t _v_r = v_r();
+ const uint16_t bsn_no_longer_in_ws = mod_sns(_v_r - ws());
+ LOGP(DRLCMACUL, LOGL_DEBUG, "- Mark BSN %u as INVALID\n", bsn_no_longer_in_ws);
+ m_v_n.mark_invalid(bsn_no_longer_in_ws);
+ if (offset_v_r) {/* all except the received block */
+ LOGP(DRLCMACUL, LOGL_DEBUG, "- Mark BSN %u as MISSING\n", _v_r);
+ m_v_n.mark_missing(_v_r);
+ }
raise_v_r_to(1);
}
LOGP(DRLCMACUL, LOGL_DEBUG, "- Raising V(R) to %d\n", v_r());
diff --git a/src/rlc_window_ul.h b/src/rlc_window_ul.h
index a2e60c0..8a48af9 100644
--- a/src/rlc_window_ul.h
+++ b/src/rlc_window_ul.h
@@ -32,6 +32,7 @@
void mark_received(int bsn);
void mark_missing(int bsn);
+ void mark_invalid(int bsn);
bool is_received(int bsn) const;
@@ -53,6 +54,11 @@
return mark(bsn, GPRS_RLC_UL_BSN_MISSING);
}
+inline void gprs_rlc_v_n::mark_invalid(int bsn)
+{
+ return mark(bsn, GPRS_RLC_UL_BSN_INVALID);
+}
+
inline bool gprs_rlc_v_n::is_received(int bsn) const
{
return is_state(bsn, GPRS_RLC_UL_BSN_RECEIVED);
diff --git a/src/tbf_ul.cpp b/src/tbf_ul.cpp
index 5ae0d49..3746130 100644
--- a/src/tbf_ul.cpp
+++ b/src/tbf_ul.cpp
@@ -303,7 +303,7 @@
rdbi->bsn,
m_window.v_q(), m_window.mod_sns(m_window.v_q() + ws - 1));
continue;
- } else if (m_window.is_received(rdbi->bsn)) {
+ } else if (m_window.m_v_n.is_received(rdbi->bsn)) {
LOGPTBFUL(this, LOGL_DEBUG,
"BSN %d already received\n", rdbi->bsn);
continue;
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33924
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I5ef4dcb0c5eac07a892114897f9e4f565b1dcc2c
Gerrit-Change-Number: 33924
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33924 )
Change subject: gprs_rlc_ul_window: Mark received BSNs falling out of the V(N)/RBB when V(R) is raised
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33924
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I5ef4dcb0c5eac07a892114897f9e4f565b1dcc2c
Gerrit-Change-Number: 33924
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 27 Jul 2023 11:17:52 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Hoernchen, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/33945 )
Change subject: ms/va: add test tool and data
......................................................................
Patch Set 8:
(1 comment)
Commit Message:
https://gerrit.osmocom.org/c/osmo-trx/+/33945/comment/3b3a75b9_21b6e57c
PS8, Line 10: demodulate bursts and prints the bits (differen), only useful for
differen? what do you mean?
--
To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/33945
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: Ie38ed4996134e84ad0481872a743299bb442b3db
Gerrit-Change-Number: 33945
Gerrit-PatchSet: 8
Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Thu, 27 Jul 2023 11:13:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment