Bug #5916
closedMGW FSM code handle_rab_release() looks odd
100%
Description
Looking at this code:
/* The MSC may ask to release a specific RAB within a RAB-AssignmentRequest */ static int handle_rab_release(struct hnbgw_context_map *map, struct msgb *ranap_msg, ranap_message *message) { bool rab_release_req; struct osmo_fsm_inst *fi = map->mgw_fi; struct mgw_fsm_priv *mgw_fsm_priv = fi->priv; int rc; /* Check if the RAB that is handled by this FSM is addressed by the release request */ rab_release_req = ranap_rab_ass_req_ies_check_release(&message->msg.raB_AssignmentRequestIEs, mgw_fsm_priv->rab_id); if (!rab_release_req) return -EINVAL; LOGPFSML(map->mgw_fi, LOGL_NOTICE, "MSC asked to release RAB-ID %u\n", mgw_fsm_priv->rab_id); /* Forward the unmodifed RAB-AssignmentRequest to HNB, so that the HNB is informed about the RAB release as * well */ LOGPFSML(fi, LOGL_DEBUG, "forwarding unmodified RAB-AssignmentRequest to HNB\n"); rc = map_rua_dispatch(map, MAP_RUA_EV_TX_RANAP_MSG, ranap_msg); /* Release the FSM normally */ osmo_fsm_inst_state_chg(fi, MGW_ST_RELEASE, 0, 0); return rc; } int handle_rab_ass_req(struct hnbgw_context_map *map, struct msgb *ranap_msg, ranap_message *message) { [...] /* The RTP stream negotiation usually begins with a RAB-AssignmentRequest and ends with an IU-Release, however * it may also be thet the MSC decides to release the RAB with a dedicated RAB-AssignmentRequest that contains * a ReleaseList. In this case an FSM will already be present. */ if (map->mgw_fi) { /* A RAB Release might be in progress, handle it */ rc = handle_rab_release(map, ranap_msg, message); if (rc >= 0) return rc; LOGPFSML(map->mgw_fi, LOGL_ERROR, "mgw_fsm_alloc_and_handle_rab_ass_req() unable to handle RAB-AssignmentRequest!\n"); osmo_fsm_inst_state_chg(map->mgw_fi, MGW_ST_FAILURE, 0, 0); OSMO_ASSERT(map->mgw_fi == NULL); }
This code looks like it has these problems:
- The rc >= 0 is used to indicate that release IEs were present. But in handle_rab_release(), dispatching the RUA message may also return rc < 0, after a release IE was actually detected and should have returned rc == 0 to indicate so.
- If, upon receiving a RAB Assignment message, an mgw_fwm != NULL, we always fail the CS RAB:
- if rc >= 0, handle_rab_release() has changed the state to MGW_ST_RELEASE
- if rc < 0, we log an error and go to MGW_ST_FAILURE.
- Upon rc < 0, after error logging and state change, we do not return, but continue to evaluate the RAB Assignment, even though mgw_fi == NULL now.
IIUC HNBGW_Tests.TC_rab_release tests for this behavior, so it is somehow working, but the code seems very odd to me.
Could you please check this and see what you think?
Updated by dexter 2 months ago
- % Done changed from 0 to 90
- If there is already an MGW FSM and if there is a RAB-ReleaseItem that matches the RAB ID of that FSM then close this FSM and be done
- If there is already an FSM and the above is not applicable then we might have an abandoned MGW FSM and we should terminate it. We then continue normally since the message may still contain a normal RAB-Assignment.
In both cases the end result is that the MGW FSM is terminated. Either because it is somehow abandoned or the MSC has explicitly ordered to terminate it.
One must also keep in mind that when we implemented the code we have decided to simplify the implementation by only allowing one RAB-Assignment at the time. The idea behind this that it is unlikely that one subscriber will have more than one call at a time.
A patch with improved code is now in gerrit:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/31989 mgw_fsm: refactor helper function handle_rab_release()