[ovs-dev] [PATCHv4] userspace: Add GTP-U support.

[Ben Pfaff]

On Thu, Dec 05, 2019 at 12:43:31PM -0800, William Tu wrote:
> GTP, GPRS Tunneling Protocol, is a group of IP-based communications
> protocols used to carry general packet radio service (GPRS) within
> GSM, UMTS and LTE networks.  GTP protocol has two parts: Signalling
> (GTP-Control, GTP-C) and User data (GTP-User, GTP-U). GTP-C is used
> for setting up GTP-U protocol, which is an IP-in-UDP tunneling
> protocol. Usually GTP is used in connecting between base station for
> radio, Serving Gateway (S-GW), and PDN Gateway (P-GW).
> 
> This patch implements GTP-U protocol for userspace datapath,
> supporting only required header fields and G-PDU message type.
> See spec in:
> https://tools.ietf.org/html/draft-hmm-dmm-5g-uplane-analysis-00
> 
> Signed-off-by: Yi Yang <yangyi01 at inspur.com>
> Co-authored-by: Yi Yang <yangyi01 at inspur.com>
> Signed-off-by: William Tu <u9012063 at gmail.com>

Thanks a lot!  I made another pass and noticed a few more things.

For some "flags" fields, where it's important to understand the flags,
we break out the flags for user comprehension.  For example, we do this
with TCP flags.  Do you have an idea whether the GTP-U flags are
important enough for this?  If so, then we'd need some additional
changes for parsing and formatting GTP-U flags.  If not, I suggest
changing formatting for them in meta-flow.h from "decimal" to
"hexadecimal" because it's easier to understand bit-mappings from hex to
binary in your head (my suggested patch does that).

I think that flow_get_metadata() should copy the new fields.

These new fields are marked writable.  Is it actually safe to let the
user change the flags field?  It seems to me that all of the bits here
are either fixed or indicate whether some following field is present, so
that changing it could break the interpretation of the packet.  Maybe
the flags should be read-only.

I think that format_flow_tunnel() in lib/match.c should format the new
fields.

The use of "? :" as in netdev_gtpu_build_header() is a GCC extension.  I
think that MSVC will reject it.

Doesn't tun_key_to_attr() need to support the new fields?

I'm pretty suspicious of the treatment of PT_GTPU_MSG packets.  I don't
know what can be done with them.  I bet they have not been tested.

I'm appending a few more minor suggestions.

Thanks again!

Ben

-8<--------------------------cut here-------------------------->8--

diff --git a/Documentation/faq/configuration.rst b/Documentation/faq/configuration.rst
index a9ac9354d0dc..4a98740c5d4d 100644
--- a/Documentation/faq/configuration.rst
+++ b/Documentation/faq/configuration.rst
@@ -227,9 +227,9 @@ Q: Does Open vSwitch support IPv6 GRE?
 
 Q: Does Open vSwitch support GTP-U?
 
-    A: Yes. GTP-U (GPRS Tunnelling Protocol User Plane (GTPv1-U))
-    is supported in userspace datapath. TEID is set by using tunnel
-    key field.
+    A: Yes. Starting with version 2.13, the Open vSwitch userspace
+    datapath supports GTP-U (GPRS Tunnelling Protocol User Plane
+    (GTPv1-U)). TEID is set by using tunnel key field.
 
     ::
 
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index d57a36468e19..b3457f231f9a 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -512,7 +512,7 @@ enum OVS_PACKED_ENUM mf_field_id {
      *
      * Type: u8.
      * Maskable: bitwise.
-     * Formatting: decimal.
+     * Formatting: hexadecimal.
      * Prerequisites: none.
      * Access: read/write.
      * NXM: none.
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index 7ae554c87964..bbce5e1f55cb 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -738,7 +738,7 @@ netdev_gtpu_pop_header(struct dp_packet *packet)
 
     tnl->gtpu_flags = gtph->md.flags;
     tnl->gtpu_msgtype = gtph->md.msgtype;
-    tnl->tun_id = htonll(ntohl(get_16aligned_be32(&gtph->teid)));
+    tnl->tun_id = be32_to_be64(get_16aligned_be32(&gtph->teid));
 
     if (tnl->gtpu_msgtype == GTPU_MSGTYPE_GPDU) {
         struct ip_header *ip;
diff --git a/lib/packets.h b/lib/packets.h
index 6a4d3fb87392..405ea9325c21 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -1498,12 +1498,14 @@ struct gtpu_metadata {
     uint8_t flags;
     uint8_t msgtype;
 };
+BUILD_ASSERT_DECL(sizeof(struct gtpu_metadata) == 2);
 
 struct gtpuhdr {
     struct gtpu_metadata md;
     ovs_be16 len;
     ovs_16aligned_be32 teid;
 };
+BUILD_ASSERT_DECL(sizeof(struct gtpuhdr) == 8);
 
 /* VXLAN protocol header */
 struct vxlanhdr {