neels has submitted this change. ( 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(-)

Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve pespin: Looks good to me, approved

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); }