[3/3] gtp: Handle gtpv1 in gtp_update_pdp_conf() correctly
diff mbox

Message ID 31daf439e34a6f4b1b6a323ab84e437371f972d5.1454521570.git.daniel@totalueberwachung.de
State Changes Requested
Headers show

Commit Message

Daniel Willmann Feb. 3, 2016, 5:53 p.m. UTC
From: Daniel Willmann <dwillmann@sysmocom.de>

libgtp cannot understand its own update pdp request (in gtp v1)
Only require the conditional and mandatory fields for gtpv1 and not
others.
Refer to 3GPP TS 29.060 Ch. 7.3.4
---
 gtp/gtp.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 112 insertions(+), 32 deletions(-)

Comments

Harald Welte Feb. 3, 2016, 6:12 p.m. UTC | #1
Hi Daniel,

On Wed, Feb 03, 2016 at 06:53:31PM +0100, Daniel Willmann wrote:
> +			if (gtpie_gettv0(ie, GTPIE_QOS_PROFILE0, 0,
> +					 &pdp->qos_neg0,
> +					 sizeof(pdp->qos_neg0))) {
> +				gsn->missing++;
> +				GTP_LOGPKG(LOGL_ERROR, peer,
> +					    pack, len,
> +					    "Missing conditional information field\n");
> +				if (gsn->cb_conf)
> +					gsn->cb_conf(type, EOF, pdp, cbp);
> +				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
> +				   pdp_freepdp(pdp); */
> +				return EOF;
> +			}
> +
> +			if (gtpie_gettv2(ie, GTPIE_FL_DI, 0, &pdp->flru)) {
> +				gsn->missing++;
> +				GTP_LOGPKG(LOGL_ERROR, peer,
> +					    pack, len,
> +					    "Missing conditional information field\n");
> +				if (gsn->cb_conf)
> +					gsn->cb_conf(type, EOF, pdp, cbp);
> +				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
> +				   pdp_freepdp(pdp); */
> +				return EOF;
> +			}

This looks like a bit too much of copy+paste to me.  Can't we put that
into a separate function, or if not, at least a macro that hides all the
verbosity and the copy+paste?  Also, why is ther a commented-out section
about cb_delete_context involved?
Daniel Willmann Feb. 4, 2016, 2:35 p.m. UTC | #2
Hi Harald,

On Wed, 2016-02-03 at 19:12 +0100, Harald Welte wrote:
> On Wed, Feb 03, 2016 at 06:53:31PM +0100, Daniel Willmann wrote:
> > +			if (gtpie_gettv0(ie, GTPIE_QOS_PROFILE0, 0,
> > +					 &pdp->qos_neg0,
> > +					 sizeof(pdp->qos_neg0))) {
> > +				gsn->missing++;
> > +				GTP_LOGPKG(LOGL_ERROR, peer,
> > +					    pack, len,
> > +					    "Missing conditional information field\n");
> > +				if (gsn->cb_conf)
> > +					gsn->cb_conf(type, EOF, pdp, cbp);
> > +				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
> > +				   pdp_freepdp(pdp); */
> > +				return EOF;
> > +			}
> > +
> > +			if (gtpie_gettv2(ie, GTPIE_FL_DI, 0, &pdp->flru)) {
> > +				gsn->missing++;
> > +				GTP_LOGPKG(LOGL_ERROR, peer,
> > +					    pack, len,
> > +					    "Missing conditional information field\n");
> > +				if (gsn->cb_conf)
> > +					gsn->cb_conf(type, EOF, pdp, cbp);
> > +				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
> > +				   pdp_freepdp(pdp); */
> > +				return EOF;
> > +			}
> 
> This looks like a bit too much of copy+paste to me.  Can't we put that

true. Unfortunately, the whole code looks like this and I tried to stick
with the "style".

> into a separate function, or if not, at least a macro that hides all the
> verbosity and the copy+paste?  Also, why is ther a commented-out section
> about cb_delete_context involved?

A function or macro will need 7 variables passed to it which makes the
whole thing quite ugly (unless the macro makes assumptions about the
local variable names). I have now put the error handling code at the end
of the function and jump to it in case of an error.

We lose the ability to infer the missing element from the line number of
the log message, but I don't think it's a big issue. The message is
still logged and wireshark can be used for debugging as well.

The section about cb_delete_context was there before (and still is in
other parts of the code). I have removed it in that function now.


Regards
Daniel

Patch
diff mbox

diff --git a/gtp/gtp.c b/gtp/gtp.c
index 772ab08..0ef4a54 100644
--- a/gtp/gtp.c
+++ b/gtp/gtp.c
@@ -2225,22 +2225,87 @@  int gtp_update_pdp_conf(struct gsn_t *gsn, int version,
 	}
 
 	/* Check all conditional information elements */
-	if (GTPCAUSE_ACC_REQ != cause) {
-		if (gsn->cb_conf)
-			gsn->cb_conf(type, cause, pdp, cbp);
-		/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
-		   pdp_freepdp(pdp); */
-		return 0;
-	} else {
-		/* Check for missing conditionary information elements */
-		if (!(gtpie_exist(ie, GTPIE_QOS_PROFILE0, 0) &&
-		      gtpie_exist(ie, GTPIE_REORDER, 0) &&
-		      gtpie_exist(ie, GTPIE_FL_DI, 0) &&
-		      gtpie_exist(ie, GTPIE_FL_C, 0) &&
-		      gtpie_exist(ie, GTPIE_CHARGING_ID, 0) &&
-		      gtpie_exist(ie, GTPIE_EUA, 0) &&
-		      gtpie_exist(ie, GTPIE_GSN_ADDR, 0) &&
-		      gtpie_exist(ie, GTPIE_GSN_ADDR, 1))) {
+	/* TODO: This does not handle GGSN-initiated update responses */
+	if (GTPCAUSE_ACC_REQ == cause) {
+		if (version == 0) {
+			if (gtpie_gettv0(ie, GTPIE_QOS_PROFILE0, 0,
+					 &pdp->qos_neg0,
+					 sizeof(pdp->qos_neg0))) {
+				gsn->missing++;
+				GTP_LOGPKG(LOGL_ERROR, peer,
+					    pack, len,
+					    "Missing conditional information field\n");
+				if (gsn->cb_conf)
+					gsn->cb_conf(type, EOF, pdp, cbp);
+				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
+				   pdp_freepdp(pdp); */
+				return EOF;
+			}
+
+			if (gtpie_gettv2(ie, GTPIE_FL_DI, 0, &pdp->flru)) {
+				gsn->missing++;
+				GTP_LOGPKG(LOGL_ERROR, peer,
+					    pack, len,
+					    "Missing conditional information field\n");
+				if (gsn->cb_conf)
+					gsn->cb_conf(type, EOF, pdp, cbp);
+				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
+				   pdp_freepdp(pdp); */
+				return EOF;
+			}
+
+			if (gtpie_gettv2(ie, GTPIE_FL_C, 0, &pdp->flrc)) {
+				gsn->missing++;
+				GTP_LOGPKG(LOGL_ERROR, peer,
+					    pack, len,
+					    "Missing conditional information field\n");
+				if (gsn->cb_conf)
+					gsn->cb_conf(type, EOF, pdp, cbp);
+				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
+				   pdp_freepdp(pdp); */
+				return EOF;
+			}
+		}
+
+		if (version == 1) {
+			if (gtpie_gettv4(ie, GTPIE_TEI_DI, 0, &pdp->teid_gn)) {
+				gsn->missing++;
+				GTP_LOGPKG(LOGL_ERROR, peer,
+					    pack, len,
+					    "Missing conditional information field\n");
+				if (gsn->cb_conf)
+					gsn->cb_conf(type, EOF, pdp, cbp);
+				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
+				   pdp_freepdp(pdp); */
+				return EOF;
+			}
+
+			if (gtpie_gettv4(ie, GTPIE_TEI_C, 0, &pdp->teic_gn)) {
+				gsn->missing++;
+				GTP_LOGPKG(LOGL_ERROR, peer,
+					    pack, len,
+					    "Missing conditional information field\n");
+				if (gsn->cb_conf)
+					gsn->cb_conf(type, EOF, pdp, cbp);
+				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
+				   pdp_freepdp(pdp); */
+				return EOF;
+			}
+		}
+
+		if (gtpie_gettv4(ie, GTPIE_CHARGING_ID, 0, &pdp->cid)) {
+			gsn->missing++;
+			GTP_LOGPKG(LOGL_ERROR, peer, pack,
+				    len,
+				    "Missing conditional information field\n");
+			if (gsn->cb_conf)
+				gsn->cb_conf(type, EOF, pdp, cbp);
+			/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
+			   pdp_freepdp(pdp); */
+		}
+
+		if (gtpie_gettlv(ie, GTPIE_GSN_ADDR, 0, &pdp->gsnrc.l,
+				 &pdp->gsnrc.v, sizeof(pdp->gsnrc.v))) {
 			gsn->missing++;
 			GTP_LOGPKG(LOGL_ERROR, peer, pack,
 				    len,
@@ -2252,24 +2317,39 @@  int gtp_update_pdp_conf(struct gsn_t *gsn, int version,
 			return EOF;
 		}
 
-		/* Update pdp with new values */
-		gtpie_gettv0(ie, GTPIE_QOS_PROFILE0, 0,
-			     pdp->qos_neg0, sizeof(pdp->qos_neg0));
-		gtpie_gettv1(ie, GTPIE_REORDER, 0, &pdp->reorder);
-		gtpie_gettv2(ie, GTPIE_FL_DI, 0, &pdp->flru);
-		gtpie_gettv2(ie, GTPIE_FL_C, 0, &pdp->flrc);
-		gtpie_gettv4(ie, GTPIE_CHARGING_ID, 0, &pdp->cid);
-		gtpie_gettlv(ie, GTPIE_EUA, 0, &pdp->eua.l,
-			     &pdp->eua.v, sizeof(pdp->eua.v));
-		gtpie_gettlv(ie, GTPIE_GSN_ADDR, 0, &pdp->gsnrc.l,
-			     &pdp->gsnrc.v, sizeof(pdp->gsnrc.v));
-		gtpie_gettlv(ie, GTPIE_GSN_ADDR, 1, &pdp->gsnru.l,
-			     &pdp->gsnru.v, sizeof(pdp->gsnru.v));
+		if (gtpie_gettlv(ie, GTPIE_GSN_ADDR, 1, &pdp->gsnru.l,
+				 &pdp->gsnru.v, sizeof(pdp->gsnru.v))) {
+			gsn->missing++;
+			GTP_LOGPKG(LOGL_ERROR, peer, pack,
+				    len,
+				    "Missing conditional information field\n");
+			if (gsn->cb_conf)
+				gsn->cb_conf(type, EOF, pdp, cbp);
+			/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
+			   pdp_freepdp(pdp); */
+			return EOF;
+		}
 
-		if (gsn->cb_conf)
-			gsn->cb_conf(type, cause, pdp, cbp);
-		return 0;	/* Succes */
+		if (version == 1) {
+			if (gtpie_gettlv
+			    (ie, GTPIE_QOS_PROFILE, 0, &pdp->qos_neg.l,
+			     &pdp->qos_neg.v, sizeof(pdp->qos_neg.v))) {
+				gsn->missing++;
+				GTP_LOGPKG(LOGL_ERROR, peer,
+					    pack, len,
+					    "Missing conditional information field\n");
+				if (gsn->cb_conf)
+					gsn->cb_conf(type, EOF, pdp, cbp);
+				/*    if (gsn->cb_delete_context) gsn->cb_delete_context(pdp);
+				   pdp_freepdp(pdp); */
+				return EOF;
+			}
+		}
 	}
+
+	if (gsn->cb_conf)
+		gsn->cb_conf(type, cause, pdp, cbp);
+	return 0;	/* Succes */
 }
 
 /* API: Send Delete PDP Context Request */