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