From b157490cb3908be3f6bedcceaa255ea906f3ad78 Mon Sep 17 00:00:00 2001 From: Andre Puschmann Date: Tue, 19 Apr 2022 08:51:59 +0200 Subject: [PATCH] ue,nr,mac: fix race-condition when accesing temp RNTIs during RA procedure the race was in the prach_nr that stored the temp crnti without proper protection. the fix moves the logic to store the value to the MAC that uses the thread-safe RNTI object for this. --- srsue/hdr/stack/mac_nr/mac_nr.h | 3 ++ srsue/hdr/stack/mac_nr/mac_nr_interfaces.h | 2 + srsue/hdr/stack/mac_nr/proc_ra_nr.h | 3 +- srsue/src/stack/mac_nr/mac_nr.cc | 43 +++++++++++++------ srsue/src/stack/mac_nr/proc_ra_nr.cc | 36 ++++++---------- .../src/stack/mac_nr/test/proc_ra_nr_test.cc | 2 + 6 files changed, 50 insertions(+), 39 deletions(-) diff --git a/srsue/hdr/stack/mac_nr/mac_nr.h b/srsue/hdr/stack/mac_nr/mac_nr.h index 7727337e4..65c65035b 100644 --- a/srsue/hdr/stack/mac_nr/mac_nr.h +++ b/srsue/hdr/stack/mac_nr/mac_nr.h @@ -92,6 +92,8 @@ public: uint16_t get_crnti(); uint16_t get_temp_crnti(); uint16_t get_csrnti() { return SRSRAN_INVALID_RNTI; }; // SPS not supported + void set_temp_crnti(uint16_t temp_crnti); + void set_crnti_to_temp(); /// procedure sr nr interface void start_ra() { proc_ra.start_by_mac(); } @@ -130,6 +132,7 @@ private: bool is_paging_opportunity(); bool has_crnti(); + bool has_temp_crnti(); bool is_valid_crnti(const uint16_t crnti); std::vector logical_channels; // stores the raw configs provide by upper layers diff --git a/srsue/hdr/stack/mac_nr/mac_nr_interfaces.h b/srsue/hdr/stack/mac_nr/mac_nr_interfaces.h index 997b38d49..15f21b2dd 100644 --- a/srsue/hdr/stack/mac_nr/mac_nr_interfaces.h +++ b/srsue/hdr/stack/mac_nr/mac_nr_interfaces.h @@ -26,6 +26,8 @@ public: // Functions for identity handling, e.g., contention id and c-rnti virtual uint16_t get_crnti() = 0; virtual bool set_crnti(uint16_t c_rnti) = 0; + virtual void set_temp_crnti(uint16_t c_rnti) = 0; + virtual void set_crnti_to_temp() = 0; // Functions for msg3 manipulation which shall be transparent to the procedure virtual bool msg3_is_transmitted() = 0; diff --git a/srsue/hdr/stack/mac_nr/proc_ra_nr.h b/srsue/hdr/stack/mac_nr/proc_ra_nr.h index a08df5872..9807f73af 100644 --- a/srsue/hdr/stack/mac_nr/proc_ra_nr.h +++ b/srsue/hdr/stack/mac_nr/proc_ra_nr.h @@ -40,6 +40,7 @@ public: uint16_t get_rar_rnti(); bool has_temp_crnti(); uint16_t get_temp_crnti(); + void set_crnti_to_temp(); void received_contention_resolution(bool is_successful); // PHY interfaces @@ -64,8 +65,6 @@ private: int ra_window_length = -1, ra_window_start = -1; uint16_t rar_rnti = SRSRAN_INVALID_RNTI; - uint16_t temp_crnti = SRSRAN_INVALID_RNTI; - uint16_t transmitted_crnti = SRSRAN_INVALID_RNTI; std::mutex mutex; srsran::rach_cfg_nr_t rach_cfg = {}; diff --git a/srsue/src/stack/mac_nr/mac_nr.cc b/srsue/src/stack/mac_nr/mac_nr.cc index c529e9062..e9c4a3b23 100644 --- a/srsue/src/stack/mac_nr/mac_nr.cc +++ b/srsue/src/stack/mac_nr/mac_nr.cc @@ -163,9 +163,9 @@ void mac_nr::update_buffer_states() mac_interface_phy_nr::sched_rnti_t mac_nr::get_ul_sched_rnti_nr(const uint32_t tti) { - if (proc_ra.has_temp_crnti() && has_crnti() == false) { - logger.debug("SCHED: Searching temp C-RNTI=0x%x (proc_ra)", proc_ra.get_temp_crnti()); - return {proc_ra.get_temp_crnti(), srsran_rnti_type_c}; + if (has_temp_crnti() && has_crnti() == false) { + logger.debug("SCHED: Searching temp C-RNTI=0x%x (proc_ra)", rntis.get_temp_rnti()); + return {rntis.get_temp_rnti(), srsran_rnti_type_c}; } return {rntis.get_crnti(), srsran_rnti_type_c}; } @@ -195,9 +195,9 @@ mac_interface_phy_nr::sched_rnti_t mac_nr::get_dl_sched_rnti_nr(const uint32_t t return {proc_ra.get_rar_rnti(), srsran_rnti_type_ra}; } - if (proc_ra.has_temp_crnti() && has_crnti() == false) { - logger.debug("SCHED: Searching temp C-RNTI=0x%x (proc_ra)", proc_ra.get_temp_crnti()); - return {proc_ra.get_temp_crnti(), srsran_rnti_type_c}; + if (has_temp_crnti() && has_crnti() == false) { + logger.debug("SCHED: Searching temp C-RNTI=0x%x (proc_ra)", rntis.get_temp_rnti()); + return {rntis.get_temp_rnti(), srsran_rnti_type_c}; } if (has_crnti()) { @@ -209,6 +209,26 @@ mac_interface_phy_nr::sched_rnti_t mac_nr::get_dl_sched_rnti_nr(const uint32_t t return {SRSRAN_INVALID_RNTI, srsran_rnti_type_c}; } +bool mac_nr::has_temp_crnti() +{ + return rntis.get_temp_rnti() != SRSRAN_INVALID_RNTI; +} + +uint16_t mac_nr::get_temp_crnti() +{ + return rntis.get_temp_rnti(); +} + +void mac_nr::set_temp_crnti(uint16_t temp_crnti) +{ + rntis.set_temp_rnti(temp_crnti); +} + +void mac_nr::set_crnti_to_temp() +{ + rntis.set_crnti_to_temp(); +} + bool mac_nr::has_crnti() { return rntis.get_crnti() != SRSRAN_INVALID_RNTI; @@ -219,11 +239,6 @@ uint16_t mac_nr::get_crnti() return rntis.get_crnti(); } -uint16_t mac_nr::get_temp_crnti() -{ - return proc_ra.get_temp_crnti(); -} - srsran::mac_sch_subpdu_nr::lcg_bsr_t mac_nr::generate_sbsr() { return proc_bsr.generate_sbsr(); @@ -309,7 +324,7 @@ void mac_nr::new_grant_dl(const uint32_t cc_idx, const mac_nr_grant_dl_t& grant, void mac_nr::tb_decoded(const uint32_t cc_idx, const mac_nr_grant_dl_t& grant, tb_action_dl_result_t result) { - logger.debug("tb_decoded(): cc_idx=%d, tti=%d, rnti=%d, pid=%d, tbs=%d, ndi=%d, rv=%d, result=%s", + logger.debug("tb_decoded(): cc_idx=%d, tti=%d, rnti=0x%X, pid=%d, tbs=%d, ndi=%d, rv=%d, result=%s", cc_idx, grant.tti, grant.rnti, @@ -336,14 +351,14 @@ void mac_nr::tb_decoded(const uint32_t cc_idx, const mac_nr_grant_dl_t& grant, t } // If proc ra is in contention resolution (RA connection request procedure) - if (proc_ra.is_contention_resolution() && grant.rnti == get_temp_crnti()) { + if (proc_ra.is_contention_resolution() && grant.rnti == rntis.get_temp_rnti()) { proc_ra.received_contention_resolution(contention_res_successful); } } void mac_nr::new_grant_ul(const uint32_t cc_idx, const mac_nr_grant_ul_t& grant, tb_action_ul_t* action) { - logger.debug("new_grant_ul(): cc_idx=%d, tti=%d, rnti=%d, pid=%d, tbs=%d, ndi=%d, rv=%d, is_rar=%d", + logger.debug("new_grant_ul(): cc_idx=%d, tti=%d, rnti=0x%X, pid=%d, tbs=%d, ndi=%d, rv=%d, is_rar=%d", cc_idx, grant.tti, grant.rnti, diff --git a/srsue/src/stack/mac_nr/proc_ra_nr.cc b/srsue/src/stack/mac_nr/proc_ra_nr.cc index 200cc96cd..b08818571 100644 --- a/srsue/src/stack/mac_nr/proc_ra_nr.cc +++ b/srsue/src/stack/mac_nr/proc_ra_nr.cc @@ -115,18 +115,6 @@ bool proc_ra_nr::has_rar_rnti() return false; } -bool proc_ra_nr::has_temp_crnti() -{ - std::lock_guard lock(mutex); - return temp_crnti != SRSRAN_INVALID_RNTI; -} - -uint16_t proc_ra_nr::get_temp_crnti() -{ - std::lock_guard lock(mutex); - return temp_crnti; -} - void proc_ra_nr::received_contention_resolution(bool is_successful) { std::lock_guard lock(mutex); @@ -216,13 +204,10 @@ void proc_ra_nr::ra_response_reception(const mac_interface_phy_nr::tb_action_dl_ for (auto& subpdu : pdu.get_subpdus()) { if (subpdu.has_rapid() && subpdu.get_rapid() == preamble_index) { logger.debug("PROC RA NR: Setting UL grant and prepare Msg3"); - temp_crnti = subpdu.get_temp_crnti(); - - // Save transmitted C-RNTI (if any) - transmitted_crnti = mac.get_crnti(); + mac.set_temp_crnti(subpdu.get_temp_crnti()); // Set Temporary-C-RNTI if provided, otherwise C-RNTI is ok - phy->set_rar_grant(tb.rx_slot_idx, subpdu.get_ul_grant(), temp_crnti, srsran_rnti_type_ra); + phy->set_rar_grant(tb.rx_slot_idx, subpdu.get_ul_grant(), subpdu.get_temp_crnti(), srsran_rnti_type_ra); // Apply TA CMD current_ta = subpdu.get_ta(); @@ -280,12 +265,17 @@ void proc_ra_nr::ra_completion() srsran::enum_to_text(state_str_nr, (uint32_t)ra_state_t::MAX_RA_STATES, WAITING_FOR_COMPLETION)); return; } + + // Start looking for PDCCH CRNTI + if (!mac.get_crnti()) { + // promote temp RNTI to new C-RNTI + mac.set_crnti_to_temp(); + mac.set_temp_crnti(SRSRAN_INVALID_RNTI); + } + srsran::console("Random Access Complete. c-rnti=0x%x, ta=%d\n", mac.get_crnti(), current_ta); logger.info("Random Access Complete. c-rnti=0x%x, ta=%d", mac.get_crnti(), current_ta); - if (!transmitted_crnti) { - mac.set_crnti(temp_crnti); - } - temp_crnti = SRSRAN_INVALID_RNTI; + mac.rrc_ra_completed(); reset(); } @@ -293,9 +283,9 @@ void proc_ra_nr::ra_completion() void proc_ra_nr::ra_error() { std::lock_guard lock(mutex); - temp_crnti = SRSRAN_INVALID_RNTI; preamble_transmission_counter++; contention_resolution_timer.stop(); + mac.set_temp_crnti(SRSRAN_INVALID_RNTI); uint32_t backoff_wait; bool ra_procedure_completed = false; // true = (unsuccessfully) completed, false = uncompleted @@ -389,10 +379,10 @@ void proc_ra_nr::reset() { state = IDLE; started_by = initiators_t::initiators_t_NULLTYPE; + mac.set_temp_crnti(SRSRAN_INVALID_RNTI); prach_send_timer.stop(); rar_timeout_timer.stop(); contention_resolution_timer.stop(); - transmitted_crnti = SRSRAN_INVALID_RNTI; } } // namespace srsue diff --git a/srsue/src/stack/mac_nr/test/proc_ra_nr_test.cc b/srsue/src/stack/mac_nr/test/proc_ra_nr_test.cc index 0663bedb3..e63ff8f8a 100644 --- a/srsue/src/stack/mac_nr/test/proc_ra_nr_test.cc +++ b/srsue/src/stack/mac_nr/test/proc_ra_nr_test.cc @@ -66,6 +66,8 @@ public: crnti = c_rnti; return true; } + void set_temp_crnti(uint16_t c_rnti) {} + void set_crnti_to_temp() {} bool msg3_is_transmitted() { return true; } void msg3_flush() {}