SGSN Crash Report

[Holger Hans Peter Freyther]

On Thu, Aug 01, 2013 at 05:06:37PM +0200, Holger Hans Peter Freyther wrote:
> On Thu, Aug 01, 2013 at 08:48:42PM +0800, Harald Welte wrote:
> > 
> > I _think_ the following (untested) patch should do the trick.
> 
> nope, the whole ownership model is wrong. BSSGP will not delete
> in most casess, NS will delete them.
> 
> 
> a.) we use the trick i mentioned (e.g. talloc_steal)
> b.) we review the entire stack and fix and enforce the ownershop
> 

E.g. I started to do b.) in OpenBSC and libosmocore.. but it requires
quite some work.. to descend all the way down to sendmsg/write and then
all the way up to all callers... in my changes I am still on BSSGP
and have not decended down... to NS.. specially in the FlowControl
code it is not too clear what will out_cb handlers. :)

openbsc:
diff --git a/openbsc/src/gprs/gprs_llc.c b/openbsc/src/gprs/gprs_llc.c
index e2d3160..6f209d4 100644
--- a/openbsc/src/gprs/gprs_llc.c
+++ b/openbsc/src/gprs/gprs_llc.c
@@ -422,6 +422,7 @@ int gprs_llc_tx_ui(struct msgb *msg, uint8_t sapi, int command,
 	if (msg->len > lle->params.n201_u) {
 		LOGP(DLLC, LOGL_ERROR, "Cannot Tx %u bytes (N201-U=%u)\n",
 			msg->len, lle->params.n201_u);
+		msgb_free(msg);
 		return -EFBIG;
 	}
 
@@ -479,6 +480,7 @@ int gprs_llc_tx_ui(struct msgb *msg, uint8_t sapi, int command,
 				     kc, iv, GPRS_CIPH_SGSN2MS);
 		if (rc < 0) {
 			LOGP(DLLC, LOGL_ERROR, "Error crypting UI frame: %d\n", rc);
+			msgb_free(msg);
 			return rc;
 		}
 
diff --git a/openbsc/src/gprs/gprs_sndcp.c b/openbsc/src/gprs/gprs_sndcp.c
index 853f8db..ffbfab5 100644
--- a/openbsc/src/gprs/gprs_sndcp.c
+++ b/openbsc/src/gprs/gprs_sndcp.c
@@ -420,7 +420,6 @@ static int sndcp_send_ud_frag(struct sndcp_frag_state *fs)
 	rc = gprs_llc_tx_ui(fmsg, lle->sapi, 0, fs->mmcontext);
 	if (rc < 0) {
 		/* abort in case of error, do not advance frag_nr / next_byte */
-		msgb_free(fmsg);
 		return rc;
 	}
 

libosmocore:
diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c
index 5ef1887..325e35c 100644
--- a/src/gb/gprs_bssgp.c
+++ b/src/gb/gprs_bssgp.c
@@ -609,17 +609,21 @@ static int fc_queue_timer_cfg(struct bssgp_flow_control *fc)
 }
 
 /* Enqueue a PDU in the flow control queue for delayed transmission */
-static int fc_enqueue(struct bssgp_flow_control *fc, struct msgb *msg,
+static int fc_enqueue(struct bssgp_flow_control *fc, struct msgb OWNS(*msg),
 		      uint32_t llc_pdu_len, void *priv)
 {
 	struct bssgp_fc_queue_element *fcqe;
 
-	if (fc->queue_depth >= fc->max_queue_depth)
+	if (fc->queue_depth >= fc->max_queue_depth) {
+		msgb_free(msg);
 		return -ENOSPC;
+	}
 
 	fcqe = talloc_zero(fc, struct bssgp_fc_queue_element);
-	if (!fcqe)
+	if (!fcqe) {
+		msgb_free(msg);
 		return -ENOMEM;
+	}
 	fcqe->msg = msg;
 	fcqe->llc_pdu_len = llc_pdu_len;
 	fcqe->priv = priv;
@@ -687,7 +691,7 @@ static int _bssgp_tx_dl_ud(struct bssgp_flow_control *fc, struct msgb *msg,
 /* input function of the flow control implementation, called first
  * for the MM flow control, and then as the MM flow control output
  * callback in order to perform BVC flow control */
-int bssgp_fc_in(struct bssgp_flow_control *fc, struct msgb *msg,
+int bssgp_fc_in(struct bssgp_flow_control *fc, struct msgb OWNS(*msg),
 		uint32_t llc_pdu_len, void *priv)
 {
 	struct timeval time_now;
@@ -696,6 +700,7 @@ int bssgp_fc_in(struct bssgp_flow_control *fc, struct msgb *msg,
 		LOGP(DBSSGP, LOGL_NOTICE, "Single PDU (size=%u) is larger "
 		     "than maximum bucket size (%u)!\n", llc_pdu_len,
 		     fc->bucket_size_max);
+		msgb_free(msg)
 		return -EIO;
 	}
 
@@ -1018,7 +1023,7 @@ int bssgp_rcvmsg(struct msgb *msg)
 	return rc;
 }
 
-int bssgp_tx_dl_ud(struct msgb *msg, uint16_t pdu_lifetime,
+int bssgp_tx_dl_ud(struct msgb OWNS(*msg), uint16_t pdu_lifetime,
 		   struct bssgp_dl_ud_par *dup)
 {
 	struct bssgp_bvc_ctx *bctx;
@@ -1035,6 +1040,7 @@ int bssgp_tx_dl_ud(struct msgb *msg, uint16_t pdu_lifetime,
 	if (bvci <= BVCI_PTM ) {
 		LOGP(DBSSGP, LOGL_ERROR, "Cannot send DL-UD to BVCI %u\n",
 			bvci);
+		msgb_free(msg);
 		return -EINVAL;
 	}
 
@@ -1042,6 +1048,7 @@ int bssgp_tx_dl_ud(struct msgb *msg, uint16_t pdu_lifetime,
 	if (!bctx) {
 		LOGP(DBSSGP, LOGL_ERROR, "Cannot send DL-UD to unknown BVCI %u\n",
 			bvci);
+		msgb_free(msg)
 		return -ENODEV;
 	}