fixeria has submitted this change. ( https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/40993?usp=email )

Change subject: s1ap_proxy: erab_fsm_start_reg/2: check if already registered ......................................................................

s1ap_proxy: erab_fsm_start_reg/2: check if already registered

Before this patch, s1ap_proxy would spawn a new erab_fsm process with a duplicate UID and simply replace the old process in the registry. This would result in stale erab_fsm process, still linked but not present in the registry, and thus not reachable.

If there's already an E-RAB process for the given UID, simply return Pid of that existing process. The FSM will most likely reject an unexpected event, resulting in a negative verdict:

* 'reply' for duplicate E-RAB SETUP REQUEST * 'drop' for duplicate INITIAL CONTEXT SETUP REQUEST

Change-Id: I8e93492219100496fd8dc69e031310858ba9c7fb --- M src/s1ap_proxy.erl M test/s1ap_proxy_test.erl 2 files changed, 42 insertions(+), 2 deletions(-)

Approvals: Jenkins Builder: Verified pespin: Looks good to me, approved

diff --git a/src/s1ap_proxy.erl b/src/s1ap_proxy.erl index 8111ad6..0195e39 100644 --- a/src/s1ap_proxy.erl +++ b/src/s1ap_proxy.erl @@ -1104,8 +1104,14 @@ -spec erab_fsm_start_reg(erab_id(), proxy_state()) -> {pid(), proxy_state()}. erab_fsm_start_reg(RABId, #proxy_state{erabs = ERABs} = S) -> UID = erab_uid(RABId, S), - {ok, Pid} = erab_fsm:start_link(UID), - {Pid, S#proxy_state{erabs = dict:store(UID, Pid, ERABs)}}. + case dict:find(UID, ERABs) of + {ok, Pid} -> + ?LOG_ERROR("E-RAB ~p is already registered?!?", [UID]), + {Pid, S}; %% return Pid of the existing erab_fsm process + error -> + {ok, Pid} = erab_fsm:start_link(UID), + {Pid, S#proxy_state{erabs = dict:store(UID, Pid, ERABs)}} + end.

-spec erab_fsm_find(erab_id(), proxy_state()) -> {ok, pid()} | error. diff --git a/test/s1ap_proxy_test.erl b/test/s1ap_proxy_test.erl index 1145bc5..8f338c9 100644 --- a/test/s1ap_proxy_test.erl +++ b/test/s1ap_proxy_test.erl @@ -52,6 +52,8 @@ ?TC(fun test_e_rab_setup/1)}, {"E-RAB SETUP REQUEST (failure)", ?TC(fun test_e_rab_setup_req_fail/1)}, + {"E-RAB SETUP REQUEST (duplicate)", + ?TC(fun test_e_rab_setup_dup/1)}, {"E-RAB RELEASE COMMAND/RESPONSE", ?TC(fun test_e_rab_release_cmd/1)}, {"E-RAB RELEASE INDICATION", @@ -68,6 +70,8 @@ ?TC(fun test_e_rab_modify_ind_cnf_release/1)}, {"INITIAL CONTEXT SETUP REQUEST/RESPONSE", ?TC(fun test_initial_context_setup/1)}, + {"INITIAL CONTEXT SETUP REQUEST/RESPONSE (duplicate)", + ?TC(fun test_initial_context_setup_dup/1)}, {"UE CONTEXT RELEASE REQUEST", ?TC(fun test_ue_ctx_release_req/1)}, {"UE CONTEXT RELEASE COMMAND/COMPLETE", @@ -147,6 +151,23 @@ ?_assertEqual([], s1ap_proxy:fetch_erab_list(Pid))].

+test_e_rab_setup_dup(#{handler := Pid}) -> + %% [eNB <- MME] E-RAB SETUP REQUEST + SetupReqIn = e_rab_setup_req_pdu(?ADDR_U2C, ?TEID_U2C), + %% [eNB -> MME] E-RAB SETUP RESPONSE + SetupRspIn = e_rab_setup_rsp_pdu(?ADDR_U2A, ?TEID_U2A), + %% eNB <- [S1GW <- MME] E-RAB SETUP REQUEST (duplicate) + %% eNB -- [S1GW -> MME] E-RAB SETUP RESPONSE (failure) + SetupRspExp = e_rab_setup_rsp_fail_pdu(), + + [?_assertMatch({forward, _}, s1ap_proxy:process_pdu(Pid, SetupReqIn)), + ?_assertMatch({forward, _}, s1ap_proxy:process_pdu(Pid, SetupRspIn)), + %% duplicate E-RAB SETUP REQUEST triggers a reply + ?_assertEqual({reply, SetupRspExp}, s1ap_proxy:process_pdu(Pid, SetupReqIn)), + ?_assertMatch({ok, _}, s1ap_proxy:fetch_erab(Pid, {7, 9, 6})), + ?_assertMatch([_], s1ap_proxy:fetch_erab_list(Pid))]. + + test_e_rab_release_cmd(#{handler := Pid}) -> %% [eNB <- MME] E-RAB SETUP REQUEST SetupReq = e_rab_setup_req_pdu(?ADDR_U2C, ?TEID_U2C), @@ -357,6 +378,19 @@ ?_assertMatch([_], s1ap_proxy:fetch_erab_list(Pid))].

+test_initial_context_setup_dup(#{handler := Pid}) -> + %% [eNB <- MME] INITIAL CONTEXT SETUP REQUEST + InitCtxSetupReqIn = initial_context_setup_req_pdu(?ADDR_U2C, ?TEID_U2C), + %% [eNB -> MME] INITIAL CONTEXT SETUP RESPONSE + InitCtxSetupRspIn = initial_context_setup_rsp_pdu(?ADDR_U2A, ?TEID_U2A), + + [?_assertMatch({forward, _}, s1ap_proxy:process_pdu(Pid, InitCtxSetupReqIn)), + ?_assertMatch({forward, _}, s1ap_proxy:process_pdu(Pid, InitCtxSetupRspIn)), + %% duplicate INITIAL CONTEXT SETUP REQUEST results in the PDU being dropped + ?_assertEqual({drop, InitCtxSetupReqIn}, s1ap_proxy:process_pdu(Pid, InitCtxSetupReqIn)), + ?_assertMatch([_], s1ap_proxy:fetch_erab_list(Pid))]. + + test_ue_ctx_release_req(#{handler := Pid}) -> %% [eNB <- MME] INITIAL CONTEXT SETUP REQUEST InitCtxSetupReq = initial_context_setup_req_pdu(?ADDR_U2C, ?TEID_U2C),