neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-upf/+/31273 ) Change subject: fix some PFCP peer,session error handling paths ...................................................................... fix some PFCP peer,session error handling paths Fix various failures to return and/or discard a session on PFCP message errors. Change-Id: I12650037c7c74d98e1f33e0379cf91edcbd02d1a --- M src/osmo-upf/up_peer.c M src/osmo-upf/up_session.c 2 files changed, 17 insertions(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-upf refs/changes/73/31273/1 diff --git a/src/osmo-upf/up_peer.c b/src/osmo-upf/up_peer.c index ec50674..8499632 100644 --- a/src/osmo-upf/up_peer.c +++ b/src/osmo-upf/up_peer.c @@ -327,6 +327,8 @@ .cause = cause, }; osmo_pfcp_endpoint_tx(peer->up_endpoint->pfcp_ep, resp); + if (session) + up_session_discard(session); } static void up_peer_not_associated_action(struct osmo_fsm_inst *fi, uint32_t event, void *data) diff --git a/src/osmo-upf/up_session.c b/src/osmo-upf/up_session.c index 0c2f103..59d45c0 100644 --- a/src/osmo-upf/up_session.c +++ b/src/osmo-upf/up_session.c @@ -631,14 +631,20 @@ resp->up_f_seid_present = true; rc = osmo_pfcp_endpoint_tx(peer->up_endpoint->pfcp_ep, tx); - if (rc) + if (rc) { + /* sending ACK failed, discard session. It might seem like a good idea to keep the session around, + * because the creation succeeded, only the ACK failed. But in the greater scheme of things, if we + * cannot ACK to the PFCP peer, all is lost. Rather not keep stale sessions around. */ up_session_fsm_state_chg(UP_SESSION_ST_WAIT_USE_COUNT); + return; + } up_session_fsm_state_chg(UP_SESSION_ST_ESTABLISHED); return; nack_response: resp->created_pdr_count = 0; osmo_pfcp_endpoint_tx(peer->up_endpoint->pfcp_ep, tx); + /* No matter if sending the NACK succeeded or not, discard the session. */ up_session_fsm_state_chg(UP_SESSION_ST_WAIT_USE_COUNT); } @@ -719,8 +725,13 @@ goto nack_response; /* Success, send ACK */ - if (osmo_pfcp_endpoint_tx(peer->up_endpoint->pfcp_ep, tx)) + if (osmo_pfcp_endpoint_tx(peer->up_endpoint->pfcp_ep, tx)) { + /* sending ACK failed, discard session. It might seem like a good idea to keep the session around, + * because the modification succeeded, only the ACK failed. But in the greater scheme of things, if we + * cannot ACK to the PFCP peer, all is lost. Rather not keep stale sessions around. */ up_session_fsm_state_chg(UP_SESSION_ST_WAIT_USE_COUNT); + return; + } LOGPFSML(fi, LOGL_NOTICE, "Session modified: %s\n", up_session_gtp_status(session)); return; @@ -728,6 +739,7 @@ nack_response: resp->created_pdr_count = 0; osmo_pfcp_endpoint_tx(peer->up_endpoint->pfcp_ep, tx); + /* No matter if sending the NACK succeeded or not, discard the session. */ up_session_fsm_state_chg(UP_SESSION_ST_WAIT_USE_COUNT); } @@ -742,6 +754,7 @@ .cause = OSMO_PFCP_CAUSE_REQUEST_ACCEPTED }; osmo_pfcp_endpoint_tx(peer->up_endpoint->pfcp_ep, tx); + /* No matter if sending the deletion ACK succeeded or not, discard the session. */ up_session_fsm_state_chg(UP_SESSION_ST_WAIT_USE_COUNT); } -- To view, visit https://gerrit.osmocom.org/c/osmo-upf/+/31273 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-upf Gerrit-Branch: master Gerrit-Change-Id: I12650037c7c74d98e1f33e0379cf91edcbd02d1a Gerrit-Change-Number: 31273 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: newchange