[PATCH 0/3] Fix handling PDP update messages

List overview All Threads
Download

newer

older

Cosmetic early RTP sending issue...

[PATCH 1/2] Expand bitvec interface

Daniel Willmann

3 Feb 2016 3 Feb '16

6:53 p.m.

From: Daniel Willmann daniel@totalueberwachung.de

Hello,

it seems PDP context updates was never really used/tested to an Update PDP context would not go through. The following patches fix that (at least for gtpv1).

Regards Daniel

Daniel Willmann (3): gtp: Pass pdp along when calling gtp_req() in gtp_update_context() gtp: Make gtp_update_pdp_conf() work for gtp0 and gtp1 connections gtp: Handle gtpv1 in gtp_update_pdp_conf() correctly

gtp/gtp.c | 162 +++++++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 118 insertions(+), 44 deletions(-)

-- 2.1.4 -- - Daniel Willmann dwillmann@sysmocom.de http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte

Show replies by date

Daniel Willmann

3 Feb 3 Feb

6:53 p.m.

New subject: [PATCH 1/3] gtp: Pass pdp along when calling gtp_req() in gtp_update_context()

From: Daniel Willmann dwillmann@sysmocom.de

With no pdp parameter gtp_req() will send the packet to TEID 0 which is not what we want. When trying to modify an established pdp context the correct TEID of that context must be used. --- gtp/gtp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gtp/gtp.c b/gtp/gtp.c index a3772ff..7cc2328 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -1871,7 +1871,7 @@ int gtp_update_context(struct gsn_t *gsn, struct pdp_t *pdp, void *cbp, gtpie_tlv(&packet, &length, GTP_MAX, GTPIE_OMC_ID, pdp->omcid.l, pdp->omcid.v);

- gtp_req(gsn, pdp->version, NULL, &packet, length, inetaddr, cbp); + gtp_req(gsn, pdp->version, pdp, &packet, length, inetaddr, cbp);

return 0; }

-- 2.1.4

Daniel Willmann

6:53 p.m.

New subject: [PATCH 2/3] gtp: Make gtp_update_pdp_conf() work for gtp0 and gtp1 connections

From: Daniel Willmann dwillmann@sysmocom.de

pdp_getgtp1(&pdp, get_tei(pack)) works like pdp_getgtp0 for gtp0 connections. Using get_hlen() for gtpie_decaps is used in other places to decode ies for both version 0 and 1. --- gtp/gtp.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/gtp/gtp.c b/gtp/gtp.c index 7cc2328..772ab08 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -2175,33 +2175,27 @@ int gtp_update_pdp_conf(struct gsn_t *gsn, int version, uint8_t cause, recovery; void *cbp = NULL; uint8_t type = 0; + int hlen = get_hlen(pack);

/* Remove packet from queue */ if (gtp_conf(gsn, 0, peer, pack, len, &type, &cbp)) return EOF;

- /* TODO This function is called from gtp_decaps1c() (for GTP v1) but - * uses gtp0.h.flow (GTP v0 data element) - */ /* Find the context in question */ - if (pdp_getgtp0(&pdp, ntoh16(((union gtp_packet *)pack)->gtp0.h.flow))) { + if (pdp_getgtp1(&pdp, get_tei(pack))) { gsn->err_unknownpdp++; GTP_LOGPKG(LOGL_ERROR, peer, pack, len, - "Unknown PDP context\n"); + "Unknown PDP context: %u\n", get_tei(pack)); if (gsn->cb_conf) - gsn->cb_conf(type, cause, NULL, cbp); + gsn->cb_conf(type, EOF, NULL, cbp); return EOF; }

/* Register that we have received a valid teic from GGSN */ pdp->teic_confirmed = 1;

- /* TODO This function is called from gtp_decaps1c() (for GTP v1) but - * explicitly passes version 0 and GTP0_HEADER_SIZE to gtpie_decaps() - */ /* Decode information elements */ - if (gtpie_decaps - (ie, 0, pack + GTP0_HEADER_SIZE, len - GTP0_HEADER_SIZE)) { + if (gtpie_decaps(ie, version, pack + hlen, len - hlen)) { gsn->invalid++; GTP_LOGPKG(LOGL_ERROR, peer, pack, len, "Invalid message format\n");

-- 2.1.4

Daniel Willmann

6:53 p.m.

New subject: [PATCH 3/3] gtp: Handle gtpv1 in gtp_update_pdp_conf() correctly

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(-)

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 */

-- 2.1.4

Harald Welte

7:12 p.m.

New subject: [PATCH 3/3] gtp: Handle gtpv1 in gtp_update_pdp_conf() correctly

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?

-- - Harald Welte laforge@gnumonks.org http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)

Daniel Willmann

4 Feb 4 Feb

3:35 p.m.

New subject: [PATCH 3/3] gtp: Handle gtpv1 in gtp_update_pdp_conf() correctly

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

-- - Daniel Willmann dwillmann@sysmocom.de http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte

Daniel Willmann

3:38 p.m.

New subject: [PATCH 1/1] gtp: Handle gtpv1 in gtp_update_pdp_conf() correctly

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 | 120 +++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 63 insertions(+), 57 deletions(-)

diff --git a/gtp/gtp.c b/gtp/gtp.c index 2a6ecd7..12cb492 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -2187,9 +2187,8 @@ int gtp_update_pdp_conf(struct gsn_t *gsn, int version, gsn->err_unknownpdp++; GTP_LOGPKG(LOGL_ERROR, peer, pack, len, "Unknown PDP context: %u\n", get_tei(pack)); - if (gsn->cb_conf) - gsn->cb_conf(type, EOF, NULL, cbp); - return EOF; + pdp = NULL; + goto err_out; }

/* Register that we have received a valid teic from GGSN */ @@ -2200,23 +2199,12 @@ int gtp_update_pdp_conf(struct gsn_t *gsn, int version, gsn->invalid++; GTP_LOGPKG(LOGL_ERROR, peer, pack, len, "Invalid message format\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; + goto err_out; }

/* Extract cause value (mandatory) */ if (gtpie_gettv1(ie, GTPIE_CAUSE, 0, &cause)) { - gsn->missing++; - GTP_LOGPKG(LOGL_ERROR, peer, pack, len, - "Missing mandatory 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; + goto err_missing; }

/* Extract recovery (optional) */ @@ -2226,51 +2214,69 @@ int gtp_update_pdp_conf(struct gsn_t *gsn, int version, }

- /* 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 (version == 1) { + if (gtpie_gettv4(ie, GTPIE_TEI_DI, 0, &pdp->teid_gn)) { + goto err_missing; + }

- if (gsn->cb_conf) - gsn->cb_conf(type, cause, pdp, cbp); - return 0; /* Succes */ + if (gtpie_gettv4(ie, GTPIE_TEI_C, 0, &pdp->teic_gn)) { + goto err_missing; + } + } + + if (gtpie_gettv4(ie, GTPIE_CHARGING_ID, 0, &pdp->cid)) { + goto err_missing; + } + + if (gtpie_gettlv(ie, GTPIE_GSN_ADDR, 0, &pdp->gsnrc.l, + &pdp->gsnrc.v, sizeof(pdp->gsnrc.v))) { + goto err_missing; + } + + if (gtpie_gettlv(ie, GTPIE_GSN_ADDR, 1, &pdp->gsnru.l, + &pdp->gsnru.v, sizeof(pdp->gsnru.v))) { + goto err_missing; + } + + if (version == 1) { + if (gtpie_gettlv + (ie, GTPIE_QOS_PROFILE, 0, &pdp->qos_neg.l, + &pdp->qos_neg.v, sizeof(pdp->qos_neg.v))) { + goto err_missing; + } + } } + + if (gsn->cb_conf) + gsn->cb_conf(type, cause, pdp, cbp); + return 0; /* Succes */ + +err_missing: + gsn->missing++; + GTP_LOGPKG(LOGL_ERROR, peer, pack, len, + "Missing information field\n"); +err_out: + if (gsn->cb_conf) + gsn->cb_conf(type, EOF, pdp, cbp); + return EOF; }

/* API: Send Delete PDP Context Request */

-- 2.1.4

3562

Age (days ago)

3563

Last active (days ago)

openbsc@lists.osmocom.org

6 comments

3 participants

tags (0)

participants (3)

Daniel Willmann
Daniel Willmann
Harald Welte