fixeria submitted this change.
s1ap_proxy: drop PDUs that we failed to process
Previously, the strategy was to forward PDUs as-is when processing
failed - whether due to decoding errors or issues like referencing
an unknown E-RAB. This patch changes the behavior to drop such PDUs,
which is more logical. This prevents forwarding PDUs with unexpected
contents, like unknown TEIDs or unreachable IP addresses.
Change-Id: I6345b4ce63e8fc5588de02fe921fa61807d97d91
Related: SYS#7352
---
M include/s1gw_metrics.hrl
M src/s1ap_proxy.erl
M src/s1gw_metrics.erl
M src/sctp_proxy.erl
M test/s1ap_proxy_test.erl
5 files changed, 67 insertions(+), 25 deletions(-)
diff --git a/include/s1gw_metrics.hrl b/include/s1gw_metrics.hrl
index 83ab46d..db087c5 100644
--- a/include/s1gw_metrics.hrl
+++ b/include/s1gw_metrics.hrl
@@ -14,6 +14,7 @@
-define(S1GW_CTR_S1AP_PROXY_UPLINK_PACKETS_QUEUED, [ctr, s1ap, proxy, uplink_packets_queued]).
-define(S1GW_CTR_S1AP_PROXY_EXCEPTION, [ctr, s1ap, proxy, exception]).
-define(S1GW_CTR_S1AP_PROXY_IN_PKT_ALL, [ctr, s1ap, proxy, in_pkt, all]).
+-define(S1GW_CTR_S1AP_PROXY_IN_PKT_DROP_ALL, [ctr, s1ap, proxy, in_pkt, drop, all]).
-define(S1GW_CTR_S1AP_PROXY_IN_PKT_DECODE_ERROR, [ctr, s1ap, proxy, in_pkt, decode_error]).
-define(S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, [ctr, s1ap, proxy, in_pkt, proc_error]).
-define(S1GW_CTR_S1AP_PROXY_IN_PKT_ERAB_SETUP_REQ, [ctr, s1ap, proxy, in_pkt, erab_setup_req]).
diff --git a/src/s1ap_proxy.erl b/src/s1ap_proxy.erl
index 953cf66..06e5302 100644
--- a/src/s1ap_proxy.erl
+++ b/src/s1ap_proxy.erl
@@ -82,7 +82,7 @@
}).
-type proxy_state() :: #proxy_state{}.
--type proxy_action() :: forward | reply.
+-type proxy_action() :: forward | reply | drop.
-export_type([proxy_action/0]).
@@ -324,33 +324,40 @@
?LOG_DEBUG("Tx (forward) S1AP PDU unmodified"),
ctr_inc(?S1GW_CTR_S1AP_PROXY_OUT_PKT_FWD_ALL, S1),
ctr_inc(?S1GW_CTR_S1AP_PROXY_OUT_PKT_FWD_UNMODIFIED, S1),
- {{forward, OrigData}, S1}
+ {{forward, OrigData}, S1};
+ {drop, S1} ->
+ ?LOG_NOTICE("Drop received S1AP PDU: ~p", [PDU]),
+ ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_DROP_ALL, S1),
+ {{drop, OrigData}, S1}
catch
Exception:Reason:StackTrace ->
?LOG_ERROR("An exception occurred: ~p, ~p, ~p", [Exception, Reason, StackTrace]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_EXCEPTION, S0),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, S0),
- ctr_inc(?S1GW_CTR_S1AP_PROXY_OUT_PKT_FWD_UNMODIFIED, S0),
- {{forward, OrigData}, S0} %% XXX: proxy as-is or drop?
+ ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_DROP_ALL, S0),
+ {{drop, OrigData}, S0}
end;
{error, {asn1, Error}} ->
?LOG_ERROR("S1AP PDU decoding failed: ~p", [Error]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_DECODE_ERROR, S0),
- ctr_inc(?S1GW_CTR_S1AP_PROXY_OUT_PKT_FWD_UNMODIFIED, S0),
- {{forward, OrigData}, S0} %% XXX: forward as-is or drop?
+ ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_DROP_ALL, S0),
+ {{drop, OrigData}, S0}
catch
Exception:Reason:StackTrace ->
?LOG_ERROR("An exception occurred: ~p, ~p, ~p", [Exception, Reason, StackTrace]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_EXCEPTION, S0),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_DECODE_ERROR, S0),
- ctr_inc(?S1GW_CTR_S1AP_PROXY_OUT_PKT_FWD_UNMODIFIED, S0),
- {{forward, OrigData}, S0} %% XXX: proxy as-is or drop?
+ ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_DROP_ALL, S0),
+ {{drop, OrigData}, S0}
end.
%% Process an S1AP PDU (decoded)
--spec handle_pdu(s1ap_pdu(), proxy_state()) -> {{proxy_action(), s1ap_pdu()}, proxy_state()} |
- {forward, proxy_state()}.
+-spec handle_pdu(PDU, S0) -> {Result, S1}
+ when PDU :: s1ap_pdu(),
+ S0 :: proxy_state(),
+ S1 :: proxy_state(),
+ Result :: {forward | reply, s1ap_pdu()} | forward | drop.
%% 9.1.8.4 S1 SETUP REQUEST
handle_pdu({initiatingMessage,
@@ -407,7 +414,7 @@
{{error, Reason}, S1} ->
?LOG_NOTICE("Failed to process E-RAB SETUP RESPONSE: ~p", [Reason]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, S1),
- {forward, S1} %% XXX: forward as-is or drop?
+ {drop, S1} %% drop this PDU
end;
%% 9.1.3.3 E-RAB MODIFY REQUEST
@@ -425,7 +432,7 @@
{{error, Reason}, S1} ->
?LOG_NOTICE("Failed to process E-RAB MODIFY REQUEST: ~p", [Reason]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, S1),
- {forward, S1} %% XXX: forward as-is or drop?
+ {drop, S1} %% drop this PDU
end;
%% 9.1.3.4 E-RAB MODIFY RESPONSE
@@ -448,15 +455,15 @@
value = C0}}, S0) ->
?LOG_DEBUG("Processing E-RAB RELEASE COMMAND"),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_ERAB_RELEASE_CMD, S0),
+ %% there's nothing to patch in this PDU, so we forward it as-is
case handle_ies(?'id-E-RABToBeReleasedList',
C0#'E-RABReleaseCommand'.protocolIEs, S0) of
{{ok, _}, S1} ->
- %% there's nothing to patch in this PDU, so we forward it as-is
- {forward, S1}; %% forward patched PDU
+ {forward, S1};
{{error, Reason}, S1} ->
?LOG_NOTICE("Failed to process E-RAB RELEASE COMMAND: ~p", [Reason]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, S1),
- {forward, S1} %% XXX: forward as-is or drop?
+ {forward, S1}
end;
%% 9.1.3.6 E-RAB RELEASE RESPONSE
@@ -465,15 +472,15 @@
value = C0}}, S0) ->
?LOG_DEBUG("Processing E-RAB RELEASE RESPONSE"),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_ERAB_RELEASE_RSP, S0),
+ %% there's nothing to patch in this PDU, so we forward it as-is
case handle_ies(?'id-E-RABReleaseListBearerRelComp',
C0#'E-RABReleaseResponse'.protocolIEs, S0) of
{{ok, _}, S1} ->
- %% there's nothing to patch in this PDU, so we forward it as-is
- {forward, S1}; %% forward patched PDU
+ {forward, S1};
{{error, Reason}, S1} ->
?LOG_NOTICE("Failed to process E-RAB RELEASE RESPONSE: ~p", [Reason]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, S1),
- {forward, S1} %% XXX: forward as-is or drop?
+ {forward, S1}
end;
%% 9.1.3.7 E-RAB RELEASE INDICATION
@@ -482,15 +489,15 @@
value = C0}}, S0) ->
?LOG_DEBUG("Processing E-RAB RELEASE INDICATION"),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_ERAB_RELEASE_IND, S0),
+ %% there's nothing to patch in this PDU, so we forward it as-is
case handle_ies(?'id-E-RABReleasedList',
C0#'E-RABReleaseIndication'.protocolIEs, S0) of
{{ok, _}, S1} ->
- %% there's nothing to patch in this PDU, so we forward it as-is
{forward, S1};
{{error, Reason}, S1} ->
?LOG_NOTICE("Failed to process E-RAB RELEASE INDICATION: ~p", [Reason]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, S1),
- {forward, S1} %% XXX: forward as-is or drop?
+ {forward, S1}
end;
%% 9.1.3.8 E-RAB MODIFICATION INDICATION
@@ -544,7 +551,7 @@
{{error, Reason}, S1} ->
?LOG_NOTICE("Failed to process INITIAL CONTEXT SETUP REQUEST: ~p", [Reason]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, S1),
- {forward, S1} %% XXX: forward as-is or drop?
+ {drop, S1} %% drop this PDU
end;
%% 9.1.4.3 INITIAL CONTEXT SETUP RESPONSE
@@ -563,7 +570,7 @@
{{error, Reason}, S1} ->
?LOG_NOTICE("Failed to process INITIAL CONTEXT SETUP RESPONSE: ~p", [Reason]),
ctr_inc(?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, S1),
- {forward, S1} %% XXX: forward as-is or drop?
+ {drop, S1} %% drop this PDU
end;
%% 9.1.4.5 UE CONTEXT RELEASE REQUEST
diff --git a/src/s1gw_metrics.erl b/src/s1gw_metrics.erl
index 9272033..1c3d76e 100644
--- a/src/s1gw_metrics.erl
+++ b/src/s1gw_metrics.erl
@@ -69,6 +69,7 @@
?S1GW_CTR_S1AP_PROXY_EXCEPTION, %% exception(s) occurred
%% s1ap_proxy: INcoming PDU counters
?S1GW_CTR_S1AP_PROXY_IN_PKT_ALL, %% received total
+ ?S1GW_CTR_S1AP_PROXY_IN_PKT_DROP_ALL, %% dropped total
?S1GW_CTR_S1AP_PROXY_IN_PKT_DECODE_ERROR, %% failed to decode
?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, %% failed to process
?S1GW_CTR_S1AP_PROXY_IN_PKT_ERAB_SETUP_REQ, %% E-RAB SETUP.req PDUs
diff --git a/src/sctp_proxy.erl b/src/sctp_proxy.erl
index d475b20..e8460a4 100644
--- a/src/sctp_proxy.erl
+++ b/src/sctp_proxy.erl
@@ -186,7 +186,9 @@
{forward, FwdData} ->
sctp_server:send_data(EnbAid, FwdData);
{reply, ReData} ->
- ok = sctp_common:send_data({Sock, Aid}, ReData)
+ ok = sctp_common:send_data({Sock, Aid}, ReData);
+ {drop, Data} ->
+ ok %% no-op
end,
{keep_state, S};
@@ -251,7 +253,9 @@
{forward, FwdData} ->
ok = sctp_common:send_data({Sock, Aid}, FwdData);
{reply, ReData} ->
- sctp_server:send_data(EnbAid, ReData)
+ sctp_server:send_data(EnbAid, ReData);
+ {drop, Data} ->
+ ok %% no-op
end.
diff --git a/test/s1ap_proxy_test.erl b/test/s1ap_proxy_test.erl
index 61f4511..1145bc5 100644
--- a/test/s1ap_proxy_test.erl
+++ b/test/s1ap_proxy_test.erl
@@ -71,7 +71,11 @@
{"UE CONTEXT RELEASE REQUEST",
?TC(fun test_ue_ctx_release_req/1)},
{"UE CONTEXT RELEASE COMMAND/COMPLETE",
- ?TC(fun test_ue_ctx_release_cmd/1)}].
+ ?TC(fun test_ue_ctx_release_cmd/1)},
+ {"ASN.1 parsing error (drop)",
+ ?TC(fun test_drop_asn1_error/1)},
+ {"PDU processing error (drop)",
+ ?TC(fun test_drop_proc_error/1)}].
%% ------------------------------------------------------------------
@@ -395,6 +399,31 @@
?_assertMatch([], s1ap_proxy:fetch_erab_list(Pid))].
+test_drop_asn1_error(#{handler := Pid}) ->
+ PDU = << 16#de, 16#ad, 16#be, 16#ef >>,
+ [?_assertEqual({drop, PDU}, s1ap_proxy:process_pdu(Pid, PDU)),
+ ?_assertMetric(?S1GW_CTR_S1AP_PROXY_IN_PKT_ALL, 1),
+ ?_assertMetric(?S1GW_CTR_S1AP_PROXY_IN_PKT_DROP_ALL, 1)].
+
+
+test_drop_proc_error(#{handler := Pid}) ->
+ %% [eNB <- MME] INITIAL CONTEXT SETUP REQUEST
+ InitCtxSetupReq = initial_context_setup_req_pdu(?ADDR_U2C, ?TEID_U2C),
+ %% [eNB -> MME] INITIAL CONTEXT SETUP RESPONSE
+ InitCtxSetupRsp = initial_context_setup_rsp_pdu(?ADDR_U2A, ?TEID_U2A),
+ %% [eNB <- MME] UE CONTEXT RELEASE COMMAND
+ UeCtxReleaseCmd = ue_ctx_release_cmd_pdu(),
+
+ [?_assertMatch({forward, _}, s1ap_proxy:process_pdu(Pid, InitCtxSetupReq)),
+ %% eNB does not respond in time, so the MME releases the UE context
+ ?_assertEqual({forward, UeCtxReleaseCmd}, s1ap_proxy:process_pdu(Pid, UeCtxReleaseCmd)),
+ %% eNB is late to the party, perhaps because of increased link latency?
+ ?_assertEqual({drop, InitCtxSetupRsp}, s1ap_proxy:process_pdu(Pid, InitCtxSetupRsp)),
+ ?_assertMetric(?S1GW_CTR_S1AP_PROXY_IN_PKT_ALL, 3),
+ ?_assertMetric(?S1GW_CTR_S1AP_PROXY_IN_PKT_PROC_ERROR, 1),
+ ?_assertMetric(?S1GW_CTR_S1AP_PROXY_IN_PKT_DROP_ALL, 1)].
+
+
%% ------------------------------------------------------------------
%% S1AP PDU templates
%% ------------------------------------------------------------------
To view, visit change 40767. To unsubscribe, or for help writing mail filters, visit settings.