pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-sigtran/+/41396?usp=email )
Change subject: ipa: Clarify msgb_free in ipa_rx_msg() ......................................................................
ipa: Clarify msgb_free in ipa_rx_msg()
First of all, clean up and clarify the handling of msgb in ipa_rx_msg_sccp() by always freeing the patched msgb copy, and letting parent function free the original received msgb.
This in turn allows moving the msgb_free() of the received msg to ipa_cli_read_cb() and ss7_asp_ipa_srv_conn_rx_cb(), which is where the msgb is allocated, and makes the msgb lifecycle far more easy to understand. This is also what's already done in the xua_cli_read_cb()/ss7_asp_xua_srv_conn_rx_cb(). This way we have similar code paths in xua and ipa.
Related: OS#6876 Change-Id: Id29955182d9da47165ee9a2d7777b92fb87844b9 --- M src/ipa.c M src/ss7_asp.c 2 files changed, 26 insertions(+), 25 deletions(-)
Approvals: pespin: Looks good to me, approved Jenkins Builder: Verified fixeria: Looks good to me, but someone else must approve osmith: Looks good to me, but someone else must approve
diff --git a/src/ipa.c b/src/ipa.c index 3b2b6da..c748f22 100644 --- a/src/ipa.c +++ b/src/ipa.c @@ -149,8 +149,6 @@ rc = -1; }
- msgb_free(msg); - return rc; }
@@ -171,7 +169,7 @@ }
/* Patch a SCCP message and add point codes to Called/Calling Party (if missing) */ -static struct msgb *patch_sccp_with_pc(struct osmo_ss7_asp *asp, struct msgb *sccp_msg_in, +static struct msgb *patch_sccp_with_pc(const struct osmo_ss7_asp *asp, const struct msgb *sccp_msg_in, uint32_t opc, uint32_t dpc) { struct osmo_sccp_addr addr; @@ -184,11 +182,8 @@ if (!sua) { LOGPASP(asp, DLSS7, LOGL_ERROR, "Couldn't convert SCCP to SUA: %s\n", msgb_hexdump(sccp_msg_in)); - msgb_free(sccp_msg_in); return NULL; } - /* free the input message and work with SUA version instead */ - msgb_free(sccp_msg_in);
rc = sua_addr_parse(&addr, sua, SUA_IEI_DEST_ADDR); switch (rc) { @@ -238,13 +233,12 @@ { int rc; struct m3ua_data_hdr data_hdr; - struct xua_msg *xua; + struct xua_msg *xua = NULL; struct osmo_ss7_as *as = ipa_find_as_for_asp(asp); uint32_t opc, dpc;
if (!as) { LOGPASP(asp, DLSS7, LOGL_ERROR, "Rx message for IPA ASP without AS?!\n"); - msgb_free(msg); return -1; }
@@ -302,25 +296,27 @@ dpc = as->cfg.pc_override.dpc; }
- /* Second, patch this into the SCCP message */ - if (as->cfg.pc_override.sccp_mode == OSMO_SS7_PATCH_BOTH) { - msg = patch_sccp_with_pc(asp, msg, opc, dpc); - if (!msg) { - LOGPASP(asp, DLSS7, LOGL_ERROR, "Unable to patch PC into SCCP message; dropping\n"); - return -1; - } - } - - /* Third, create a MTP3/M3UA label with those point codes */ + /* Second, create a MTP3/M3UA label with those point codes */ memset(&data_hdr, 0, sizeof(data_hdr)); data_hdr.si = MTP_SI_SCCP; data_hdr.opc = osmo_htonl(opc); data_hdr.dpc = osmo_htonl(dpc); data_hdr.sls = sls; data_hdr.ni = as->inst->cfg.network_indicator; - /* Create M3UA message in XUA structure */ - xua = m3ua_xfer_from_data(&data_hdr, msgb_l2(msg), msgb_l2len(msg)); - msgb_free(msg); + + /* Third, patch this into the SCCP message and create M3UA message in XUA structure */ + if (as->cfg.pc_override.sccp_mode == OSMO_SS7_PATCH_BOTH) { + struct msgb *msg_patched = patch_sccp_with_pc(asp, msg, opc, dpc); + if (!msg_patched) { + LOGPASP(asp, DLSS7, LOGL_ERROR, "Unable to patch PC into SCCP message; dropping\n"); + return -1; + } + xua = m3ua_xfer_from_data(&data_hdr, msgb_l2(msg_patched), msgb_l2len(msg_patched)); + msgb_free(msg_patched); + } else { + xua = m3ua_xfer_from_data(&data_hdr, msgb_l2(msg), msgb_l2len(msg)); + } + /* Update xua->mtp with values from data_hdr */ m3ua_dh_to_xfer_param(&xua->mtp, &data_hdr);
@@ -332,7 +328,7 @@
/*! \brief process M3UA message received from socket * \param[in] asp Application Server Process receiving \a msg - * \param[in] msg received message buffer. Callee takes ownership! + * \param[in] msg received message buffer. It is kept owned by the caller. * \param[in] sls The SLS (signaling link selector) field to use in the generated M3UA header * \returns 0 on success; negative on error */ int ipa_rx_msg(struct osmo_ss7_asp *asp, struct msgb *msg, uint8_t sls) @@ -355,7 +351,6 @@ break; default: rc = ss7_asp_rx_unknown(asp, hh->proto, msg); - msgb_free(msg); }
return rc; diff --git a/src/ss7_asp.c b/src/ss7_asp.c index 7b819de..23c0737 100644 --- a/src/ss7_asp.c +++ b/src/ss7_asp.c @@ -1031,6 +1031,7 @@ { struct osmo_ss7_asp *asp = osmo_stream_srv_get_data(conn); struct osmo_stream_srv_link *link = osmo_stream_srv_get_master(conn); + int rc;
/* Reparent msg to srv_link, to avoid "msg" being automatically freed if * "conn" is teared down during msg handling (or if its associated @@ -1051,7 +1052,9 @@
msg->dst = asp; rate_ctr_inc2(asp->ctrg, SS7_ASP_CTR_PKT_RX_TOTAL); - return ipa_rx_msg(asp, msg, asp->ipa.sls); + rc = ipa_rx_msg(asp, msg, asp->ipa.sls); + msgb_free(msg); + return rc; }
/* netif code tells us we can read something from the socket */ @@ -1239,6 +1242,7 @@ static int ipa_cli_read_cb(struct osmo_stream_cli *conn, int res, struct msgb *msg) { struct osmo_ss7_asp *asp = osmo_stream_cli_get_data(conn); + int rc;
if (res <= 0) { if (res == -EAGAIN) { @@ -1252,7 +1256,9 @@
msg->dst = asp; rate_ctr_inc2(asp->ctrg, SS7_ASP_CTR_PKT_RX_TOTAL); - return ipa_rx_msg(asp, msg, asp->ipa.sls); + rc = ipa_rx_msg(asp, msg, asp->ipa.sls); + msgb_free(msg); + return rc; }
/* read call-back for M3UA-over-TCP socket */