From b4fea523b9d0ce0665e9a084b6fab08fce7ec1a8 Mon Sep 17 00:00:00 2001 From: Cedric Roux Date: Mon, 4 Sep 2023 11:00:23 +0200 Subject: [PATCH 1/6] nr rlc am: bugfix: discard a control PDU if ack-sn is invalid It is possible to receive an ACK that is not expected, in which case the whole control PDU has to be discarded. --- openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c index 033b436255..a0ed84657f 100644 --- a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c +++ b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c @@ -354,6 +354,17 @@ static void process_control_pdu(nr_rlc_entity_am_t *entity, nr_rlc_pdu_decoder_get_bits(&decoder, 7); R(decoder); } + /* discard the whole control PDU if ack_sn is invalid, that is + * if it does not satisfy tx_next_ack <= ack_sn <= tx_next + * (no need to test tx_next_ack <= ack_sn, this is always true since + * tx_next_ack is the modulus base) + */ + if (sn_compare_tx(entity, ack_sn, entity->tx_next) > 0) { + LOG_W(RLC, "ack_sn (%d) not valid (tx_next_ack %d tx_next %d), discard control PDU\n", + ack_sn, entity->tx_next_ack, entity->tx_next); + return; + } + /* 38.322 5.3.3.3 says to stop t_poll_retransmit if a ACK or NACK is * received for the SN 'poll_sn' - check ACK case (NACK done below) */ From e3dbc97723c0d9e0186576bddfe84a780f1bf9e0 Mon Sep 17 00:00:00 2001 From: Cedric Roux Date: Mon, 4 Sep 2023 11:09:06 +0200 Subject: [PATCH 2/6] nr rlc am: prepare variable to be reused --- openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c index a0ed84657f..9e16451352 100644 --- a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c +++ b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c @@ -327,6 +327,7 @@ static void process_control_pdu(nr_rlc_entity_am_t *entity, nr_rlc_sdu_segment_t *cur_retransmit_list; nr_rlc_sdu_segment_t *new_retransmit_list; nr_rlc_sdu_segment_t head_retransmit_list; + int cmp; head_wait_list.next = entity->wait_list; cur_wait_list = entity->wait_list; @@ -409,7 +410,7 @@ static void process_control_pdu(nr_rlc_entity_am_t *entity, * do not touch t_poll_retransmit */ if (prev_nack_sn != -1) { - int cmp = sn_compare_tx(entity, cur_nack_sn, prev_nack_sn); + cmp = sn_compare_tx(entity, cur_nack_sn, prev_nack_sn); if (cmp < 0 || (cmp == 0 && (prev_so_end == -1 From 43f5eecd19f7f604c637da99ae6d4165cfcac01d Mon Sep 17 00:00:00 2001 From: Cedric Roux Date: Mon, 4 Sep 2023 12:13:46 +0200 Subject: [PATCH 3/6] nr rlc am: bugfix: use sn_compare_tx() to compare SNs The use of < is not correct. SNs are cyclic. We must use sn_compare_tx(). --- openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c index 9e16451352..044572060b 100644 --- a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c +++ b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c @@ -437,9 +437,10 @@ process_next_pdu: goto process_retransmit_list_head; if (cur_retransmit_list == NULL) goto process_wait_list_head; - if (cur_wait_list->sdu->sn < cur_retransmit_list->sdu->sn - || (cur_wait_list->sdu->sn == cur_retransmit_list->sdu->sn && - cur_wait_list->so < cur_retransmit_list->so)) + cmp = sn_compare_tx(entity, cur_wait_list->sdu->sn, cur_retransmit_list->sdu->sn); + if (cmp < 0 + || (cmp == 0 + && cur_wait_list->so < cur_retransmit_list->so)) goto process_wait_list_head; goto process_retransmit_list_head; From f7fe3b448f1a2315b13b1be8c0fddc7ad2db1a16 Mon Sep 17 00:00:00 2001 From: Cedric Roux Date: Mon, 4 Sep 2023 12:26:17 +0200 Subject: [PATCH 4/6] nr rlc am: harmonize comparisons of SNs in process_control_pdu() Let's compare SNs in a similar way as done in previous commit, to ease readability. --- openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c index 044572060b..57713477ca 100644 --- a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c +++ b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c @@ -481,8 +481,9 @@ process_wait_list_head: } /* if current segment SN > current NACK, we can't classify it yet */ - if (sn_compare_tx(entity, cur_wait_list->sdu->sn, cur_nack_sn) > 0 - || (cur_wait_list->sdu->sn == cur_nack_sn + cmp = sn_compare_tx(entity, cur_wait_list->sdu->sn, cur_nack_sn); + if (cmp > 0 + || (cmp == 0 && cur_wait_list->so > cur_so_start)) goto done_nack; @@ -541,8 +542,9 @@ process_retransmit_list_head: } /* if current segment SN > current NACK, we can't classify it yet */ - if (sn_compare_tx(entity, cur_retransmit_list->sdu->sn, cur_nack_sn) > 0 - || (cur_retransmit_list->sdu->sn == cur_nack_sn + cmp = sn_compare_tx(entity, cur_retransmit_list->sdu->sn, cur_nack_sn); + if (cmp > 0 + || (cmp == 0 && cur_retransmit_list->so > cur_so_start)) goto done_nack; From 1272847feaae9320718b66889d58cda209243c5d Mon Sep 17 00:00:00 2001 From: Cedric Roux Date: Tue, 5 Sep 2023 21:52:30 +0200 Subject: [PATCH 5/6] nr rlc am: rewrite process_control_pdu() A first pass checks that the control PDU is valid. A second pass processes it for real. --- openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c | 105 ++++++++++++++++------ 1 file changed, 78 insertions(+), 27 deletions(-) diff --git a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c index 57713477ca..840e9177f5 100644 --- a/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c +++ b/openair2/LAYER2/nr_rlc/nr_rlc_entity_am.c @@ -329,21 +329,14 @@ static void process_control_pdu(nr_rlc_entity_am_t *entity, nr_rlc_sdu_segment_t head_retransmit_list; int cmp; - head_wait_list.next = entity->wait_list; - cur_wait_list = entity->wait_list; - prev_wait_list = &head_wait_list; - - head_retransmit_list.next = NULL; - cur_retransmit_list = entity->retransmit_list; - new_retransmit_list = &head_retransmit_list; + /* validate the control PDU: read it, check the values of ACK and NACKs */ nr_rlc_pdu_decoder_init(&decoder, buffer, size); nr_rlc_pdu_decoder_get_bits(&decoder, 1); R(decoder); /* dc */ cpt = nr_rlc_pdu_decoder_get_bits(&decoder, 3); R(decoder); if (cpt != 0) { - LOG_E(RLC, "%s:%d:%s: warning: discard PDU, CPT not 0 (%d)\n", - __FILE__, __LINE__, __FUNCTION__, cpt); + LOG_E(RLC, "discard PDU, CPT not 0 (%d)\n", cpt); goto err; } ack_sn = nr_rlc_pdu_decoder_get_bits(&decoder, entity->sn_field_length); R(decoder); @@ -366,12 +359,9 @@ static void process_control_pdu(nr_rlc_entity_am_t *entity, return; } - /* 38.322 5.3.3.3 says to stop t_poll_retransmit if a ACK or NACK is - * received for the SN 'poll_sn' - check ACK case (NACK done below) + /* discard the whole control PDU if NACKs are bad (not <= ack_sn, not in + * increasing order) */ - if (sn_compare_tx(entity, entity->poll_sn, ack_sn) < 0) - entity->t_poll_retransmit_start = 0; - while (e1) { nack_sn = nr_rlc_pdu_decoder_get_bits(&decoder, entity->sn_field_length); R(decoder); e1 = nr_rlc_pdu_decoder_get_bits(&decoder, 1); R(decoder); @@ -399,15 +389,13 @@ static void process_control_pdu(nr_rlc_entity_am_t *entity, if (so_end == 0xffff) so_end = -1; - /* process nacks */ for (i = 0; i < range; i++) { int cur_nack_sn = (nack_sn + i) % entity->sn_modulus; int cur_so_start = i == 0 ? so_start : 0; int cur_so_end = i == range - 1 ? so_end : -1; /* check that current nack is > previous nack and <= ack - * if not then skip it and all following nacks, and - * do not touch t_poll_retransmit + * if not then reject the control PDU */ if (prev_nack_sn != -1) { cmp = sn_compare_tx(entity, cur_nack_sn, prev_nack_sn); @@ -415,19 +403,84 @@ static void process_control_pdu(nr_rlc_entity_am_t *entity, || (cmp == 0 && (prev_so_end == -1 || cur_so_start <= prev_so_end))) { - LOG_E(RLC, "%s:%d:%s: bad NACK (nack sn %d so start/end %d/%d, previous nack sn %d so start/end %d/%d), skip it and all following NACKs\n", - __FILE__, __LINE__, __FUNCTION__, + LOG_E(RLC, "bad NACK, not bigger than previous NACK (nack sn %d so start/end %d/%d, previous nack sn %d so start/end %d/%d)\n", cur_nack_sn, cur_so_start, cur_so_end, prev_nack_sn, prev_so_start, prev_so_end); - goto nacks_done; + goto err; } } if (sn_compare_tx(entity, cur_nack_sn, ack_sn) > 0) { - LOG_E(RLC, "%s:%d:%s: bad NACK (nack %d ack %d), skip it and all following NACKs\n", - __FILE__, __LINE__, __FUNCTION__, - cur_nack_sn, ack_sn); - goto nacks_done; + LOG_E(RLC, "bad NACK, bigger than ACK (nack %d ack %d) (tx_next_ack %d tx_next %d)\n", + cur_nack_sn, ack_sn, entity->tx_next_ack, entity->tx_next); + goto err; } + } + } + + /* process the control PDU for real (checks not needed now, previous steps + * validated everything) + */ + + head_wait_list.next = entity->wait_list; + cur_wait_list = entity->wait_list; + prev_wait_list = &head_wait_list; + + head_retransmit_list.next = NULL; + cur_retransmit_list = entity->retransmit_list; + new_retransmit_list = &head_retransmit_list; + + nr_rlc_pdu_decoder_init(&decoder, buffer, size); + nr_rlc_pdu_decoder_get_bits(&decoder, 1); /* dc */ + + cpt = nr_rlc_pdu_decoder_get_bits(&decoder, 3); + ack_sn = nr_rlc_pdu_decoder_get_bits(&decoder, entity->sn_field_length); + e1 = nr_rlc_pdu_decoder_get_bits(&decoder, 1); + /* r bits */ + if (entity->sn_field_length == 18) { + nr_rlc_pdu_decoder_get_bits(&decoder, 1); + } else { + nr_rlc_pdu_decoder_get_bits(&decoder, 7); + } + + /* 38.322 5.3.3.3 says to stop t_poll_retransmit if a ACK or NACK is + * received for the SN 'poll_sn' - check ACK case (NACK done below) + */ + if (sn_compare_tx(entity, entity->poll_sn, ack_sn) < 0) + entity->t_poll_retransmit_start = 0; + + while (e1) { + nack_sn = nr_rlc_pdu_decoder_get_bits(&decoder, entity->sn_field_length); + e1 = nr_rlc_pdu_decoder_get_bits(&decoder, 1); + e2 = nr_rlc_pdu_decoder_get_bits(&decoder, 1); + e3 = nr_rlc_pdu_decoder_get_bits(&decoder, 1); + /* r bits */ + if (entity->sn_field_length == 18) { + nr_rlc_pdu_decoder_get_bits(&decoder, 3); + } else { + nr_rlc_pdu_decoder_get_bits(&decoder, 1); + } + if (e2) { + so_start = nr_rlc_pdu_decoder_get_bits(&decoder, 16); + so_end = nr_rlc_pdu_decoder_get_bits(&decoder, 16); + } else { + so_start = 0; + so_end = 0xffff; + } + if (e3) { + range = nr_rlc_pdu_decoder_get_bits(&decoder, 8); + } else { + range = 1; + } + /* special value 0xffff indicates 'all bytes to the end' */ + if (so_end == 0xffff) + so_end = -1; + + /* process nacks */ + for (i = 0; i < range; i++) { + int cur_nack_sn = (nack_sn + i) % entity->sn_modulus; + int cur_so_start = i == 0 ? so_start : 0; + int cur_so_end = i == range - 1 ? so_end : -1; + process_next_pdu: /* process smallest SN either from wait_list or retransmit list */ @@ -599,7 +652,6 @@ lists_over: entity->t_poll_retransmit_start = 0; } /* while (e1) */ -nacks_done: /* nacks done, finish with ack */ /* we may report successful delivery out of order, if it's a problem * then we can have a single loop and deal with the smallest sn of @@ -679,8 +731,7 @@ nacks_done: return; err: - LOG_E(RLC, "%s:%d:%s: error decoding PDU, NR RLC entity in inconsistent state\n", - __FILE__, __LINE__, __FUNCTION__); + LOG_E(RLC, "error decoding control PDU, discarding\n"); #undef R } From 282a20578f84485e44cb4180dc9c0dad401452c1 Mon Sep 17 00:00:00 2001 From: Cedric Roux Date: Tue, 5 Sep 2023 21:55:15 +0200 Subject: [PATCH 6/6] nr rlc test: minor: change the logging --- openair2/LAYER2/nr_rlc/tests/LOG/log.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openair2/LAYER2/nr_rlc/tests/LOG/log.h b/openair2/LAYER2/nr_rlc/tests/LOG/log.h index 8ebc7e1909..b9b8f3ce70 100644 --- a/openair2/LAYER2/nr_rlc/tests/LOG/log.h +++ b/openair2/LAYER2/nr_rlc/tests/LOG/log.h @@ -3,8 +3,8 @@ #include -#define LOG_E(x, ...) printf(__VA_ARGS__) -#define LOG_D(x, ...) printf(__VA_ARGS__) -#define LOG_W(x, ...) printf(__VA_ARGS__) +#define LOG_E(x, ...) do { printf("LOG_E: "); printf(__VA_ARGS__); } while (0) +#define LOG_D(x, ...) do { printf("LOG_D: "); printf(__VA_ARGS__); } while (0) +#define LOG_W(x, ...) do { printf("LOG_W: "); printf(__VA_ARGS__); } while (0) #endif /* _NR_RLC_TESTS_LOG_H_ */