GTPU Error indication #215

sergeymatov · 2022-02-03T07:14:40Z

When UPG receives a G-PDU for which no corresponding GTP-U tunnel
exists, it must discard the G-PDU and return a GTP-U Error Indication
to the sending node.
Integration test helps to capture and verify error indication packet and it's IEs.

RoadRunnr · 2022-02-03T08:01:22Z

upf/upf_gtpu_encap.c

+static u16
+encode_error_indication (vlib_buffer_t * b, gtp_error_ind_t * error, int is_ip4)
+{
+  u8 *p = vlib_buffer_get_current (b);
+  u16 length, nlength;
+
+  length = (is_ip4) ? 4 : 16;
+  nlength = clib_host_to_net_u16 (length);
+  /* IE TEID I (5 bytes) + IE GTP Peer address (3 bytes + IPaddr length) + IE Recovery (2 bytes) */
+  b->current_length = 5 + 2 + 3 + length;
+
+  /* Encode TEID I, TEID already in NET order */
+  *p = 16; p++;
+  memcpy (p, &error->teid, sizeof (u32));
+  p += 4;
+  /* Encode Recovery */
+  *p = 14; p++;
+  *p = 0; p++;
+  /* Encode GTP Peer address */
+  *p = 133; p++;
+  memcpy (p, &nlength, sizeof (u16));
+  p += 2;
+  memcpy (p, (length == 4) ? (void *) &error->addr.ip4 : (void *) &error->addr.ip6, length);
+
+  return b->current_length; //bytes encoded
+}


no magic constants, please. define the IEs id somewhere

no magic lengths for anything, I would define a GTP-U IE struct, with fields for id, length and a variable length array for the content. You can then calculate the length by sizeof(*ie + payload_size).

GTP extension header for UDP source port is missing, see TS 29.281 (be careful, extension headers are not GTP IEs)

Thanks for comments. I will fix constants for more readability and will operate language constructions rather then pure decimal representations.
About GTP extension header UDP Source Port, I did noticed that based on Clause 7.3.1 TS 29.281
GTP entities may include the "UDP Port" extension header (Type 0x40)
But I thought it's not mandatory to have in our implementation

I've added GTP Extension Header UDP Port encoding.
However, having this present in a packet confuses Scapy a little bit (GTP IE List is now considering as Raw data for the packet). Wireshark pcap looks OK for Error Indication message produced by UPF, so I can assume it's Scapy issue.

RoadRunnr · 2022-02-03T08:07:53Z

upf/upf_gtpu_encap.c

+      gtpu1->next_ext_type = 0;
+      gtpu1->ver_flags = GTPU_V1_VER | GTPU_PT_GTP;
+      gtpu1->type = GTPU_TYPE_ERROR_IND;
+      gtpu1->ver_flags |= GTPU_S_BIT;


this can be combined with init 2 lines prior

RoadRunnr · 2022-02-03T13:32:48Z

upf/upf_gtpu_encap.c

+
+      /* Swap udp ports */
+      udp1->src_port = udp0->dst_port;
+      udp1->dst_port = udp0->src_port;


from TS 29.281:

The UDP destination port for the Error Indication shall be the user plane UDP port (2152).

Encoded UDP port in a GTP Extension header UDP Port

RoadRunnr · 2022-02-04T12:39:35Z

upf/upf.h

@@ -186,7 +186,22 @@ typedef CLIB_PACKED (struct
 #define GTPU_TYPE_END_MARKER  254
 #define GTPU_TYPE_GTPU  255

+#define GTPU_UP_UDP_PORT 2152
+
+#define GTPU_EXT_HEADER_UPD_PORT_LENGTH 1


Suggested change

#define GTPU_EXT_HEADER_UPD_PORT_LENGTH 1

#define GTPU_EXT_HEADER_UDP_PORT_LENGTH 1

RoadRunnr · 2022-02-04T12:40:13Z

upf/upf.h

@@ -186,7 +186,22 @@ typedef CLIB_PACKED (struct
 #define GTPU_TYPE_END_MARKER  254
 #define GTPU_TYPE_GTPU  255

+#define GTPU_UP_UDP_PORT 2152


the _UP_ is not needed, the U is already in GTPU

Suggested change

#define GTPU_UP_UDP_PORT 2152

#define GTPU_UDP_PORT 2152

RoadRunnr · 2022-02-04T13:10:04Z

upf/upf_gtpu_encap.c

+static u16
+encode_error_indication (vlib_buffer_t * b, gtp_error_ind_t * error, int is_ip4)
+{
+  u8 *p = vlib_buffer_get_current (b);
+  u16 ip_length, nlength;
+  gtpu_ie_t teidi, recovery, gsn_address;
+
+  ip_length = (is_ip4) ? 4 : 16;
+  nlength = clib_host_to_net_u16 (ip_length);
+
+  teidi.id = GTPU_IE_TEID_I;
+  teidi.payload_len = sizeof (u32);
+
+  recovery.id = GTPU_IE_RECOVERY;
+  recovery.payload_len = sizeof (u8);
+
+  gsn_address.id = GTPU_IE_GSN_ADDRESS;
+  gsn_address.payload_len = sizeof (u16) + ip_length;
+
+  b->current_length = teidi.payload_len + gsn_address.payload_len + recovery.payload_len + (sizeof (u8) * 3);
+
+  *p = teidi.id; p++;
+  memcpy (p, &error->teid, teidi.payload_len); p += teidi.payload_len;
+
+  *p = recovery.id; p++; *p = 0; p += recovery.payload_len;
+
+  *p = gsn_address.id; p++;
+  memcpy (p, &nlength, sizeof (u16));
+  p += gsn_address.payload_len - ip_length;
+  memcpy (p, (is_ip4) ? (void *) &error->addr.ip4 : (void *) &error->addr.ip6, ip_length);
+
+  return b->current_length; //bytes encoded
+}


why so complicated?

Suggested change

static u16

encode_error_indication (vlib_buffer_t * b, gtp_error_ind_t * error, int is_ip4)

{

u8 *p = vlib_buffer_get_current (b);

u16 ip_length, nlength;

gtpu_ie_t teidi, recovery, gsn_address;

ip_length = (is_ip4) ? 4 : 16;

nlength = clib_host_to_net_u16 (ip_length);

teidi.id = GTPU_IE_TEID_I;

teidi.payload_len = sizeof (u32);

recovery.id = GTPU_IE_RECOVERY;

recovery.payload_len = sizeof (u8);

gsn_address.id = GTPU_IE_GSN_ADDRESS;

gsn_address.payload_len = sizeof (u16) + ip_length;

b->current_length = teidi.payload_len + gsn_address.payload_len + recovery.payload_len + (sizeof (u8) * 3);

*p = teidi.id; p++;

memcpy (p, &error->teid, teidi.payload_len); p += teidi.payload_len;

*p = recovery.id; p++; *p = 0; p += recovery.payload_len;

*p = gsn_address.id; p++;

memcpy (p, &nlength, sizeof (u16));

p += gsn_address.payload_len - ip_length;

memcpy (p, (is_ip4) ? (void *) &error->addr.ip4 : (void *) &error->addr.ip6, ip_length);

return b->current_length; //bytes encoded

}

typedef struct

{

u8 id;

u8 data[];

} gtpu_tv_ie_t;

typedef struct

{

u8 id;

u16 len;

u8 data[];

} gtpu_tlv_ie_t;

static u16

encode_error_indication (vlib_buffer_t * b, gtp_error_ind_t * error, int is_ip4)

{

u8 *p = vlib_buffer_get_current (b);

u16 ip_length, nlength;

gtpu_tv_ie_t *tv_ie;

gtpu_tlv_ie_t *tlv_ie;

tv_ie = (gtpu_tv_ie_t *)p;

tv_ie->id = GTPU_IE_TEID_I;

*((u32 *)&tv_ie->data) = error->teid;

p += sizeof(*tv_ie) + sizeof (u32);

tv_ie = (gtpu_tv_ie_t *)p;

tv_ie->id = GTPU_IE_RECOVERY;

*((u8 *)&tv_ie->data) = 0;

p += sizeof(*tv_ie) + sizeof (u8);

tlv_ie = (gtpu_tlv_ie_t *)p;

tlv_ie->id = GTPU_IE_GSN_ADDRESS;

if (is_ip4)

{

tlv_ie->len = clib_host_to_net_u16(sizeof (ip4_address_t));

clib_memcpy_fast (&tlv_ie->data, error->addr.ip4, sizeof (ip4_address_t));

}

else

{

tlv_ie->len = clib_host_to_net_u16(sizeof (ip6_address_t));

clib_memcpy_fast (&tlv_ie->data, error->addr.ip6, sizeof (ip6_address_t));

}

p += sizeof(*tlv_ie) + tlv_ie->len;

b->current_length = p - vlib_buffer_get_current (b);

return b->current_length; //bytes encoded```

Thanks for hints.
I based my implementation of encoders based on original decode_error_indication_ext_hdr and decode_error_indication funcs and tried to kept this logic to be compatible with existing implementation.
I've adjusted your proposals a bit.

RoadRunnr · 2022-02-04T13:10:44Z

upf/upf_gtpu_encap.c

+      len_p1 = encode_error_indication (p0, &error, is_ip4);
+      gtpu1->length = clib_host_to_net_u16 (len_p1 + ext_header_len + 4);
+
+      // Update checksums and length params for IP and UPD


Suggested change

// Update checksums and length params for IP and UPD

// Update checksums and length params for IP and UDP

RoadRunnr · 2022-02-04T13:13:13Z

upf/upf_gtpu_encap.c

+      vlib_buffer_advance (p0, sizeof (gtpu_header_t));
+      ext_header_len = encode_ext_header_udp_port (p0);


... and pass the pointer to next_ext_type into the header encode

Suggested change

vlib_buffer_advance (p0, sizeof (gtpu_header_t));

ext_header_len = encode_ext_header_udp_port (p0);

vlib_buffer_advance (p0, sizeof (gtpu_header_t) - sizeof (gtpu1->next_ext_type));

ext_header_len = encode_ext_header_udp_port (p0);

RoadRunnr · 2022-02-04T13:17:29Z

upf/upf_gtpu_encap.c

+      /* Fill GTPU info */
+      gtpu1->teid = gtpu0->teid;
+      gtpu1->pdu_number = 0;
+      gtpu1->next_ext_type = GTPU_EXT_HEADER_UDP_PORT;


remove that..

Fixed TEID in favor of Clause 5.1 of 29.281
– The Error Indication message where the Tunnel Endpoint Identifier shall be set to all zeros.

RoadRunnr · 2022-02-04T13:21:36Z

upf/upf_gtpu_encap.c

+static u16
+encode_ext_header_udp_port (vlib_buffer_t * b)
+{
+  u8 *p = vlib_buffer_get_current (b);
+  u16 port = clib_host_to_net_u16 (GTPU_UP_UDP_PORT);
+
+  /*
+   * TS 29.281 Clause 5.2.2.1, Length field valuse is 1. Therefore, length
+   *  of IE is 4 bytes.
+   */
+  b->current_length = (GTPU_EXT_HEADER_UPD_PORT_LENGTH * 4);
+  *p = GTPU_EXT_HEADER_UPD_PORT_LENGTH;
+  p++;
+
+  memcpy(p, &port, sizeof (u16));
+  p += sizeof (u16);
+
+  *p = GTPU_EXT_HEADER_NEXT_HEADER_NO_MORE;
+
+  return b->current_length;
+}


Suggested change

static u16

encode_ext_header_udp_port (vlib_buffer_t * b)

{

u8 *p = vlib_buffer_get_current (b);

u16 port = clib_host_to_net_u16 (GTPU_UP_UDP_PORT);

/*

* TS 29.281 Clause 5.2.2.1, Length field valuse is 1. Therefore, length

* of IE is 4 bytes.

*/

b->current_length = (GTPU_EXT_HEADER_UPD_PORT_LENGTH * 4);

*p = GTPU_EXT_HEADER_UPD_PORT_LENGTH;

p++;

memcpy(p, &port, sizeof (u16));

p += sizeof (u16);

*p = GTPU_EXT_HEADER_NEXT_HEADER_NO_MORE;

return b->current_length;

}

static u16

encode_ext_header_udp_port (vlib_buffer_t * b)

{

u8 *p = vlib_buffer_get_current (b);

gtpu_ext_header_t *hdr = (gtpu_ext_header_t *)p;

hdr->type = GTPU_EXT_HEADER_UDP_PORT;

hdr->len = 1; /* in 4 byte units */

*((u16 *)&hdr->pad) = clib_host_to_net_u16 (GTPU_UDP_PORT);

hdr++;

hdr->type = GTPU_EXT_HEADER_NEXT_HEADER_NO_MORE;

b->current_length = sizeof (hdr) + sizeof (hdr->type);

return b->current_length;

Fixed with slight adjustments. Thanks

RoadRunnr · 2022-02-08T15:54:34Z

upf/upf_gtpu_encap.c

+
+  hdr->type = GTPU_EXT_HEADER_UDP_PORT;
+  hdr->len = 1;   /* in 4 byte units */
+  *((u16 *)&hdr->pad) = clib_host_to_net_u16 (GTPU_UDP_PORT);


This needs to be the original source port, not the fixed GTP-U port.

Sorry for giving the wring sample in the suggested change.

RoadRunnr · 2022-02-08T15:55:30Z

upf/upf_gtpu_encap.c

+
+      /* Swap udp ports */
+      udp1->src_port = udp0->dst_port;
+      udp1->dst_port = udp0->src_port;


this is wrong, the destination port need to be 2152:

Suggested change

udp1->dst_port = udp0->src_port;

udp1->dst_port = clib_host_to_net_u16 (GTPU_UDP_PORT);

Fixed, thanks

When UPG receives a G-PDU for which no corresponding GTP-U tunnel exists, it must discard the G-PDU and return a GTP-U Error Indication to the sending node.

RoadRunnr

There are still code constructs that I'm not totally happy with, but I guess it is good enough...

sergeymatov added the enhancement New feature or request label Feb 3, 2022

sergeymatov requested review from ivan4th, RoadRunnr and korroot February 3, 2022 07:14

RoadRunnr requested changes Feb 3, 2022

View reviewed changes

RoadRunnr reviewed Feb 3, 2022

View reviewed changes

sergeymatov force-pushed the feature/gtpu-error-indication branch from d11d586 to 99ad550 Compare February 4, 2022 11:57

RoadRunnr requested changes Feb 4, 2022

View reviewed changes

sergeymatov force-pushed the feature/gtpu-error-indication branch from 99ad550 to 25bb989 Compare February 6, 2022 13:18

sergeymatov requested a review from RoadRunnr February 6, 2022 13:33

sergeymatov force-pushed the feature/gtpu-error-indication branch from 25bb989 to 5fd743e Compare February 8, 2022 12:18

RoadRunnr requested changes Feb 8, 2022

View reviewed changes

sergeymatov force-pushed the feature/gtpu-error-indication branch from 5fd743e to 0d69e20 Compare February 8, 2022 17:01

sergeymatov requested a review from RoadRunnr February 8, 2022 17:02

GTPU Error indication

2d048f8

When UPG receives a G-PDU for which no corresponding GTP-U tunnel exists, it must discard the G-PDU and return a GTP-U Error Indication to the sending node.

sergeymatov force-pushed the feature/gtpu-error-indication branch from 0d69e20 to 2d048f8 Compare February 14, 2022 14:27

RoadRunnr approved these changes Feb 16, 2022

View reviewed changes

sergeymatov merged commit 728da22 into master Feb 16, 2022

sergeymatov deleted the feature/gtpu-error-indication branch February 16, 2022 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GTPU Error indication #215

GTPU Error indication #215

sergeymatov commented Feb 3, 2022

RoadRunnr Feb 3, 2022

sergeymatov Feb 3, 2022

sergeymatov Feb 4, 2022

RoadRunnr Feb 3, 2022

sergeymatov Feb 4, 2022

RoadRunnr Feb 3, 2022

sergeymatov Feb 4, 2022

RoadRunnr Feb 4, 2022

sergeymatov Feb 8, 2022

RoadRunnr Feb 4, 2022

sergeymatov Feb 8, 2022

RoadRunnr Feb 4, 2022

sergeymatov Feb 6, 2022

RoadRunnr Feb 4, 2022

sergeymatov Feb 8, 2022

RoadRunnr Feb 4, 2022

RoadRunnr Feb 4, 2022

sergeymatov Feb 8, 2022

RoadRunnr Feb 4, 2022

sergeymatov Feb 8, 2022

RoadRunnr Feb 8, 2022

sergeymatov Feb 8, 2022

RoadRunnr Feb 8, 2022

sergeymatov Feb 8, 2022

RoadRunnr left a comment

	#define GTPU_EXT_HEADER_UPD_PORT_LENGTH 1
	#define GTPU_EXT_HEADER_UDP_PORT_LENGTH 1

	// Update checksums and length params for IP and UPD
	// Update checksums and length params for IP and UDP

		vlib_buffer_advance (p0, sizeof (gtpu_header_t));
		ext_header_len = encode_ext_header_udp_port (p0);

	udp1->dst_port = udp0->src_port;
	udp1->dst_port = clib_host_to_net_u16 (GTPU_UDP_PORT);

GTPU Error indication #215

GTPU Error indication #215

Conversation

sergeymatov commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoadRunnr left a comment

Choose a reason for hiding this comment