Change in osmo-upf[master]: fix some PFCP peer,session error handling paths

[neels]

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 <nhofm...@sysmocom.de>
Gerrit-MessageType: newchange