[PATCH net-next 00/14] gtp: Additional feature support

[PATCH net-next 00/14] gtp: Additional feature support

[Tom Herbert]

This patch set builds upon the initial GTP implementation to make
support closer to that enjoyed by other encapsulation protocols.

The major items are:

  - IPv6 support
  - Configurable networking interfaces so that GTP kernel can be
    used and tested without needing GSN network emulation (i.e. no user
    space daemon needed).
  - GSO,GRO
  - Control of zero UDP checksums
  - Port numbers are configurable
  - Addition of a dst_cache in the GTP structure and other cleanup

Additionally, this patch set also includes a couple of general support
capabilities:

  - A facility that allows application specific GSO callbacks
  - Common functions to get a route fo for an IP tunnel

For IPv6 support, the mobile subscriber needs to allow IPv6 addresses,
and the remote enpoint can be IPv6.

For configurable interfaces, configuration is added to allow an
alterate means to configure a GTP and device. This follows the
typical UDP encapsulation model of specifying a listener port for
receive, and a remote address and port for transmit. 

GRO was straightfoward to implement following the model of other
UDP encapsulations.

Providing GSO support had one wrinkle-- the GTP header includes a
payload length field that needs to be set per GSO segment. In order
to address that in a general way, I create the concept of
application specific GSO.

To implement application layer GSO I reserved the top four bits of
shinfo(skb)->gso_type. The idea is that an application or encapsulation
protocol (like GTP in this case) can register a GSO segment callback.
The facility returns a gso_type with upper four bits set to a value
(index into a table). When the application sets up a packet it includes
the code in the gso_type for the skb. At some point (e.g. from UDP
segment) the gso_type is checked in the skb and if the application
specific GSO is indicated then the callback is called. The
registered callbacks include a set of other gso_types so that
an application callback can be matched to an appropriate instance.
FOr instance, the GTP callback checks for the UDP GSO flags.

Zero UDP checksum, port number configuration, and dst_cache are
straightforwad.

Configuration is performed by iproute2/ip. I will post that
in a subsequent patch set.

Tested:

Configured the matrix of IPv4/IPv6 mobile subscriber, IPv4/IPv6 remote
peer, and GTP version 0 and 1 (eight combinations). Observed
connectivity and proper GSO/GRO. Also, tested VXLAN for
regression.

Tom Herbert (14):
  iptunnel: Add common functions to get a tunnel route
  vxlan: Call common functions to get tunnel routes
  gtp: Call common functions to get tunnel routes and add dst_cache
  gtp: udp recv clean up
  gtp: Remove special mtu handling
  gtp: Eliminate pktinfo and add port configuration
  gtp: Support encapsulation of IPv6 packets
  gtp: Support encpasulating over IPv6
  gtp: Allow configuring GTP interface as standalone
  gtp: Add support for devnet
  net: Add a facility to support application defined GSO
  gtp: Configuration for zero UDP checksum
  gtp: Support for GRO
  gtp: GSO support

 drivers/net/gtp.c            | 1300 ++++++++++++++++++++++++++++++++----------
 drivers/net/vxlan.c          |   84 +--
 include/linux/netdevice.h    |   31 +
 include/linux/skbuff.h       |   25 +
 include/net/ip6_tunnel.h     |   33 ++
 include/net/ip_tunnels.h     |   33 ++
 include/uapi/linux/gtp.h     |    8 +
 include/uapi/linux/if_link.h |    6 +
 net/core/dev.c               |   47 ++
 net/ipv4/ip_tunnel.c         |   41 ++
 net/ipv4/ip_tunnel_core.c    |    6 +
 net/ipv4/udp_offload.c       |   20 +-
 net/ipv6/ip6_tunnel.c        |   43 ++
 13 files changed, 1306 insertions(+), 371 deletions(-)

-- 
2.11.0

[PATCH net-next 01/14] iptunnel: Add common functions to get a tunnel route

[Tom Herbert]

ip_tunnel_get_route and ip6_tnl_get_route are create to return
routes for a tunnel. These functions are derived from the VXLAN
functions.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/net/ip6_tunnel.h | 33 +++++++++++++++++++++++++++++++++
 include/net/ip_tunnels.h | 33 +++++++++++++++++++++++++++++++++
 net/ipv4/ip_tunnel.c     | 41 +++++++++++++++++++++++++++++++++++++++++
 net/ipv6/ip6_tunnel.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 150 insertions(+)

diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
index 08fbc7f7d8d7..233097bf07a2 100644
--- a/include/net/ip6_tunnel.h
+++ b/include/net/ip6_tunnel.h
@@ -142,6 +142,39 @@ __u32 ip6_tnl_get_cap(struct ip6_tnl *t, const struct in6_addr *laddr,
 struct net *ip6_tnl_get_link_net(const struct net_device *dev);
 int ip6_tnl_get_iflink(const struct net_device *dev);
 int ip6_tnl_change_mtu(struct net_device *dev, int new_mtu);
+struct dst_entry *__ip6_tnl_get_route(struct net_device *dev,
+				      struct sk_buff *skb, struct sock *sk,
+				      u8 proto, int oif, u8 tos, __be32 label,
+				      const struct in6_addr *daddr,
+				      struct in6_addr *saddr,
+				      __be16 dport, __be16 sport,
+				      struct dst_cache *dst_cache,
+				      const struct ip_tunnel_info *info,
+				      bool use_cache);
+
+static inline struct dst_entry *ip6_tnl_get_route(struct net_device *dev,
+			struct sk_buff *skb, struct sock *sk, u8 proto,
+			int oif, u8 tos, __be32 label,
+			const struct in6_addr *daddr,
+			struct in6_addr *saddr,
+			__be16 dport, __be16 sport,
+			struct dst_cache *dst_cache,
+			const struct ip_tunnel_info *info)
+{
+	 bool use_cache = (ip_tunnel_dst_cache_usable(skb, info) &&
+		(!tos || info));
+
+	if (use_cache) {
+		struct dst_entry *ndst = dst_cache_get_ip6(dst_cache, saddr);
+
+		if (ndst)
+			return ndst;
+	}
+
+	return __ip6_tnl_get_route(dev, skb, sk, proto, oif, tos, label,
+				   daddr, saddr, dport, sport, dst_cache,
+				   info, use_cache);
+}
 
 static inline void ip6tunnel_xmit(struct sock *sk, struct sk_buff *skb,
 				  struct net_device *dev)
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 992652856fe8..91d5150a1044 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -284,6 +284,39 @@ int ip_tunnel_newlink(struct net_device *dev, struct nlattr *tb[],
 		      struct ip_tunnel_parm *p, __u32 fwmark);
 void ip_tunnel_setup(struct net_device *dev, unsigned int net_id);
 
+struct rtable *__ip_tunnel_get_route(struct net_device *dev,
+				     struct sk_buff *skb, u8 proto,
+				     int oif, u8 tos,
+				     __be32 daddr, __be32 *saddr,
+				     __be16 dport, __be16 sport,
+				     struct dst_cache *dst_cache,
+				     const struct ip_tunnel_info *info,
+				     bool use_cache);
+
+static inline struct rtable *ip_tunnel_get_route(struct net_device *dev,
+				     struct sk_buff *skb, u8 proto,
+				     int oif, u8 tos,
+				     __be32 daddr, __be32 *saddr,
+				     __be16 dport, __be16 sport,
+				     struct dst_cache *dst_cache,
+				     const struct ip_tunnel_info *info)
+{
+	bool use_cache = (ip_tunnel_dst_cache_usable(skb, info) &&
+		(!tos || info));
+
+	if (use_cache) {
+		struct rtable *rt;
+
+		rt = dst_cache_get_ip4(dst_cache, saddr);
+		if (rt)
+			return rt;
+	}
+
+	return __ip_tunnel_get_route(dev, skb, proto, oif, tos,
+				     daddr, saddr, dport, sport,
+				     dst_cache, info, use_cache);
+}
+
 struct ip_tunnel_encap_ops {
 	size_t (*encap_hlen)(struct ip_tunnel_encap *e);
 	int (*build_header)(struct sk_buff *skb, struct ip_tunnel_encap *e,
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index e9805ad664ac..f0f35333febd 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -935,6 +935,47 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
 }
 EXPORT_SYMBOL_GPL(ip_tunnel_ioctl);
 
+struct rtable *__ip_tunnel_get_route(struct net_device *dev,
+				     struct sk_buff *skb, u8 proto,
+				    int oif, u8 tos,
+				     __be32 daddr, __be32 *saddr,
+				     __be16 dport, __be16 sport,
+				     struct dst_cache *dst_cache,
+				     const struct ip_tunnel_info *info,
+				     bool use_cache)
+{
+	struct rtable *rt = NULL;
+	struct flowi4 fl4;
+
+	memset(&fl4, 0, sizeof(fl4));
+	fl4.flowi4_oif = oif;
+	fl4.flowi4_tos = RT_TOS(tos);
+	fl4.flowi4_mark = skb->mark;
+	fl4.flowi4_proto = proto;
+	fl4.daddr = daddr;
+	fl4.saddr = *saddr;
+	fl4.fl4_dport = dport;
+	fl4.fl4_sport = sport;
+
+	rt = ip_route_output_key(dev_net(dev), &fl4);
+	if (likely(!IS_ERR(rt))) {
+		if (rt->dst.dev == dev) {
+			netdev_dbg(dev, "circular route to %pI4\n", &daddr);
+			ip_rt_put(rt);
+			return ERR_PTR(-ELOOP);
+		}
+
+		*saddr = fl4.saddr;
+		if (use_cache)
+			dst_cache_set_ip4(dst_cache, &rt->dst, fl4.saddr);
+	} else {
+		netdev_dbg(dev, "no route to %pI4\n", &daddr);
+		return ERR_PTR(-ENETUNREACH);
+	}
+	return rt;
+}
+EXPORT_SYMBOL_GPL(__ip_tunnel_get_route);
+
 int __ip_tunnel_change_mtu(struct net_device *dev, int new_mtu, bool strict)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index ae73164559d5..9a02b62c808b 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1663,6 +1663,49 @@ ip6_tnl_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	return err;
 }
 
+struct dst_entry *__ip6_tnl_get_route(struct net_device *dev,
+				      struct sk_buff *skb, struct sock *sk,
+				      u8 proto, int oif, u8 tos, __be32 label,
+				      const struct in6_addr *daddr,
+				      struct in6_addr *saddr,
+				      __be16 dport, __be16 sport,
+				      struct dst_cache *dst_cache,
+				      const struct ip_tunnel_info *info,
+				      bool use_cache)
+{
+	struct dst_entry *ndst;
+	struct flowi6 fl6;
+	int err;
+
+	memset(&fl6, 0, sizeof(fl6));
+	fl6.flowi6_oif = oif;
+	fl6.daddr = *daddr;
+	fl6.saddr = *saddr;
+	fl6.flowlabel = ip6_make_flowinfo(RT_TOS(tos), label);
+	fl6.flowi6_mark = skb->mark;
+	fl6.flowi6_proto = proto;
+	fl6.fl6_dport = dport;
+	fl6.fl6_sport = sport;
+
+	err = ipv6_stub->ipv6_dst_lookup(dev_net(dev), sk, &ndst, &fl6);
+	if (unlikely(err < 0)) {
+		netdev_dbg(dev, "no route to %pI6\n", daddr);
+		return ERR_PTR(-ENETUNREACH);
+	}
+
+	if (unlikely(ndst->dev == dev)) {
+		netdev_dbg(dev, "circular route to %pI6\n", daddr);
+		dst_release(ndst);
+		return ERR_PTR(-ELOOP);
+	}
+
+	*saddr = fl6.saddr;
+	if (use_cache)
+		dst_cache_set_ip6(dst_cache, ndst, saddr);
+	return ndst;
+}
+EXPORT_SYMBOL_GPL(__ip6_tnl_get_route);
+
 /**
  * ip6_tnl_change_mtu - change mtu manually for tunnel device
  *   @dev: virtual device associated with tunnel
-- 
2.11.0

Re: [PATCH net-next 01/14] iptunnel: Add common functions to get a tunnel route

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]

Hi Tom,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Tom-Herbert/gtp-Additional-feature-support/20170919-143920
config: i386-randconfig-x074-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from net/ipv4/ip_tunnel_core.c:40:0:
   include/net/ip6_tunnel.h: In function 'ip6_tnl_get_route':
>> include/net/ip6_tunnel.h:168:28: error: implicit declaration of function 'dst_cache_get_ip6' [-Werror=implicit-function-declaration]
      struct dst_entry *ndst = dst_cache_get_ip6(dst_cache, saddr);
                               ^~~~~~~~~~~~~~~~~
>> include/net/ip6_tunnel.h:168:28: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
   cc1: some warnings being treated as errors

vim +/dst_cache_get_ip6 +168 include/net/ip6_tunnel.h

   129	
   130	int ip6_tnl_rcv_ctl(struct ip6_tnl *t, const struct in6_addr *laddr,
   131			const struct in6_addr *raddr);
   132	int ip6_tnl_rcv(struct ip6_tnl *tunnel, struct sk_buff *skb,
   133			const struct tnl_ptk_info *tpi, struct metadata_dst *tun_dst,
   134			bool log_ecn_error);
   135	int ip6_tnl_xmit_ctl(struct ip6_tnl *t, const struct in6_addr *laddr,
   136			     const struct in6_addr *raddr);
   137	int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
   138			 struct flowi6 *fl6, int encap_limit, __u32 *pmtu, __u8 proto);
   139	__u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw);
   140	__u32 ip6_tnl_get_cap(struct ip6_tnl *t, const struct in6_addr *laddr,
   141				     const struct in6_addr *raddr);
   142	struct net *ip6_tnl_get_link_net(const struct net_device *dev);
   143	int ip6_tnl_get_iflink(const struct net_device *dev);
   144	int ip6_tnl_change_mtu(struct net_device *dev, int new_mtu);
   145	struct dst_entry *__ip6_tnl_get_route(struct net_device *dev,
   146					      struct sk_buff *skb, struct sock *sk,
   147					      u8 proto, int oif, u8 tos, __be32 label,
   148					      const struct in6_addr *daddr,
   149					      struct in6_addr *saddr,
   150					      __be16 dport, __be16 sport,
   151					      struct dst_cache *dst_cache,
   152					      const struct ip_tunnel_info *info,
   153					      bool use_cache);
   154	
   155	static inline struct dst_entry *ip6_tnl_get_route(struct net_device *dev,
   156				struct sk_buff *skb, struct sock *sk, u8 proto,
   157				int oif, u8 tos, __be32 label,
   158				const struct in6_addr *daddr,
   159				struct in6_addr *saddr,
   160				__be16 dport, __be16 sport,
   161				struct dst_cache *dst_cache,
   162				const struct ip_tunnel_info *info)
   163	{
   164		 bool use_cache = (ip_tunnel_dst_cache_usable(skb, info) &&
   165			(!tos || info));
   166	
   167		if (use_cache) {
 > 168			struct dst_entry *ndst = dst_cache_get_ip6(dst_cache, saddr);
   169	
   170			if (ndst)
   171				return ndst;
   172		}
   173	
   174		return __ip6_tnl_get_route(dev, skb, sk, proto, oif, tos, label,
   175					   daddr, saddr, dport, sport, dst_cache,
   176					   info, use_cache);
   177	}
   178	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30824 bytes --]

[PATCH net-next 02/14] vxlan: Call common functions to get tunnel routes

[Tom Herbert]

Call ip_tunnel_get_route and ip6_tnl_get_route to handle getting a route
and dealing with the dst_cache.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/vxlan.c | 84 ++++-------------------------------------------------
 1 file changed, 5 insertions(+), 79 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index d7c49cf1d5e9..810caa9adf37 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1867,47 +1867,11 @@ static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan, struct net_device
 				      struct dst_cache *dst_cache,
 				      const struct ip_tunnel_info *info)
 {
-	bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
-	struct rtable *rt = NULL;
-	struct flowi4 fl4;
-
 	if (!sock4)
 		return ERR_PTR(-EIO);
 
-	if (tos && !info)
-		use_cache = false;
-	if (use_cache) {
-		rt = dst_cache_get_ip4(dst_cache, saddr);
-		if (rt)
-			return rt;
-	}
-
-	memset(&fl4, 0, sizeof(fl4));
-	fl4.flowi4_oif = oif;
-	fl4.flowi4_tos = RT_TOS(tos);
-	fl4.flowi4_mark = skb->mark;
-	fl4.flowi4_proto = IPPROTO_UDP;
-	fl4.daddr = daddr;
-	fl4.saddr = *saddr;
-	fl4.fl4_dport = dport;
-	fl4.fl4_sport = sport;
-
-	rt = ip_route_output_key(vxlan->net, &fl4);
-	if (likely(!IS_ERR(rt))) {
-		if (rt->dst.dev == dev) {
-			netdev_dbg(dev, "circular route to %pI4\n", &daddr);
-			ip_rt_put(rt);
-			return ERR_PTR(-ELOOP);
-		}
-
-		*saddr = fl4.saddr;
-		if (use_cache)
-			dst_cache_set_ip4(dst_cache, &rt->dst, fl4.saddr);
-	} else {
-		netdev_dbg(dev, "no route to %pI4\n", &daddr);
-		return ERR_PTR(-ENETUNREACH);
-	}
-	return rt;
+	return ip_tunnel_get_route(dev, skb, IPPROTO_UDP, oif, tos, daddr,
+				   saddr, dport, sport, dst_cache, info);
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
@@ -1922,50 +1886,12 @@ static struct dst_entry *vxlan6_get_route(struct vxlan_dev *vxlan,
 					  struct dst_cache *dst_cache,
 					  const struct ip_tunnel_info *info)
 {
-	bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
-	struct dst_entry *ndst;
-	struct flowi6 fl6;
-	int err;
-
 	if (!sock6)
 		return ERR_PTR(-EIO);
 
-	if (tos && !info)
-		use_cache = false;
-	if (use_cache) {
-		ndst = dst_cache_get_ip6(dst_cache, saddr);
-		if (ndst)
-			return ndst;
-	}
-
-	memset(&fl6, 0, sizeof(fl6));
-	fl6.flowi6_oif = oif;
-	fl6.daddr = *daddr;
-	fl6.saddr = *saddr;
-	fl6.flowlabel = ip6_make_flowinfo(RT_TOS(tos), label);
-	fl6.flowi6_mark = skb->mark;
-	fl6.flowi6_proto = IPPROTO_UDP;
-	fl6.fl6_dport = dport;
-	fl6.fl6_sport = sport;
-
-	err = ipv6_stub->ipv6_dst_lookup(vxlan->net,
-					 sock6->sock->sk,
-					 &ndst, &fl6);
-	if (unlikely(err < 0)) {
-		netdev_dbg(dev, "no route to %pI6\n", daddr);
-		return ERR_PTR(-ENETUNREACH);
-	}
-
-	if (unlikely(ndst->dev == dev)) {
-		netdev_dbg(dev, "circular route to %pI6\n", daddr);
-		dst_release(ndst);
-		return ERR_PTR(-ELOOP);
-	}
-
-	*saddr = fl6.saddr;
-	if (use_cache)
-		dst_cache_set_ip6(dst_cache, ndst, saddr);
-	return ndst;
+	return ip6_tnl_get_route(dev, skb, sock6->sock->sk, IPPROTO_UDP, oif,
+				   tos, label, daddr, saddr, dport, sport,
+				   dst_cache, info);
 }
 #endif
 
-- 
2.11.0

[PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Tom Herbert]

Call ip_tunnel_get_route and dst_cache to pdp context which should
improve performance by obviating the need to perform a route lookup
on every packet.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c | 59 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index f38e32a7ec9c..95df3bcebbb2 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -63,6 +63,8 @@ struct pdp_ctx {
 
 	atomic_t		tx_seq;
 	struct rcu_head		rcu_head;
+
+	struct dst_cache	dst_cache;
 };
 
 /* One instance of the GTP device. */
@@ -379,20 +381,6 @@ static void gtp_dev_uninit(struct net_device *dev)
 	free_percpu(dev->tstats);
 }
 
-static struct rtable *ip4_route_output_gtp(struct flowi4 *fl4,
-					   const struct sock *sk,
-					   __be32 daddr)
-{
-	memset(fl4, 0, sizeof(*fl4));
-	fl4->flowi4_oif		= sk->sk_bound_dev_if;
-	fl4->daddr		= daddr;
-	fl4->saddr		= inet_sk(sk)->inet_saddr;
-	fl4->flowi4_tos		= RT_CONN_FLAGS(sk);
-	fl4->flowi4_proto	= sk->sk_protocol;
-
-	return ip_route_output_key(sock_net(sk), fl4);
-}
-
 static inline void gtp0_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)
 {
 	int payload_len = skb->len;
@@ -479,6 +467,8 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,
 	struct rtable *rt;
 	struct flowi4 fl4;
 	struct iphdr *iph;
+	struct sock *sk;
+	__be32 saddr;
 	__be16 df;
 	int mtu;
 
@@ -498,19 +488,27 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,
 	}
 	netdev_dbg(dev, "found PDP context %p\n", pctx);
 
-	rt = ip4_route_output_gtp(&fl4, pctx->sk, pctx->peer_addr_ip4.s_addr);
-	if (IS_ERR(rt)) {
-		netdev_dbg(dev, "no route to SSGN %pI4\n",
-			   &pctx->peer_addr_ip4.s_addr);
-		dev->stats.tx_carrier_errors++;
-		goto err;
-	}
+	sk = pctx->sk;
+	saddr = inet_sk(sk)->inet_saddr;
 
-	if (rt->dst.dev == dev) {
-		netdev_dbg(dev, "circular route to SSGN %pI4\n",
-			   &pctx->peer_addr_ip4.s_addr);
-		dev->stats.collisions++;
-		goto err_rt;
+	rt = ip_tunnel_get_route(dev, skb, sk->sk_protocol,
+				 sk->sk_bound_dev_if, RT_CONN_FLAGS(sk),
+				 pctx->peer_addr_ip4.s_addr, &saddr,
+				 pktinfo->gtph_port, pktinfo->gtph_port,
+				 &pctx->dst_cache, NULL);
+
+	if (IS_ERR(rt)) {
+		if (rt == ERR_PTR(-ELOOP)) {
+			netdev_dbg(dev, "circular route to SSGN %pI4\n",
+				   &pctx->peer_addr_ip4.s_addr);
+			dev->stats.collisions++;
+			goto err_rt;
+		} else {
+			netdev_dbg(dev, "no route to SSGN %pI4\n",
+				   &pctx->peer_addr_ip4.s_addr);
+			dev->stats.tx_carrier_errors++;
+			goto err;
+		}
 	}
 
 	skb_dst_drop(skb);
@@ -543,7 +541,7 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,
 		goto err_rt;
 	}
 
-	gtp_set_pktinfo_ipv4(pktinfo, pctx->sk, iph, pctx, rt, &fl4, dev);
+	gtp_set_pktinfo_ipv4(pktinfo, sk, iph, pctx, rt, &fl4, dev);
 	gtp_push_header(skb, pktinfo);
 
 	return 0;
@@ -917,6 +915,7 @@ static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock *sk,
 	struct pdp_ctx *pctx;
 	bool found = false;
 	__be32 ms_addr;
+	int err;
 
 	ms_addr = nla_get_be32(info->attrs[GTPA_MS_ADDRESS]);
 	hash_ms = ipv4_hashfn(ms_addr) % gtp->hash_size;
@@ -951,6 +950,12 @@ static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock *sk,
 	if (pctx == NULL)
 		return -ENOMEM;
 
+	err = dst_cache_init(&pctx->dst_cache, GFP_KERNEL);
+	if (err) {
+		kfree(pctx);
+		return err;
+	}
+
 	sock_hold(sk);
 	pctx->sk = sk;
 	pctx->dev = gtp->dev;
-- 
2.11.0

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[David Miller]

From: Tom Herbert <tom@quantonium.net>
Date: Mon, 18 Sep 2017 17:38:53 -0700

> Call ip_tunnel_get_route and dst_cache to pdp context which should
> improve performance by obviating the need to perform a route lookup
> on every packet.
> 
> Signed-off-by: Tom Herbert <tom@quantonium.net>

Not caused by your changes, but something to think about:

> -static struct rtable *ip4_route_output_gtp(struct flowi4 *fl4,
> -					   const struct sock *sk,
> -					   __be32 daddr)
> -{
> -	memset(fl4, 0, sizeof(*fl4));
> -	fl4->flowi4_oif		= sk->sk_bound_dev_if;
> -	fl4->daddr		= daddr;
> -	fl4->saddr		= inet_sk(sk)->inet_saddr;
> -	fl4->flowi4_tos		= RT_CONN_FLAGS(sk);
> -	fl4->flowi4_proto	= sk->sk_protocol;
> -
> -	return ip_route_output_key(sock_net(sk), fl4);
> -}

This and the new dst caching code ignores any source address selection
done by ip_route_output_key() or the new tunnel route lookup helpers.

Either source address selection should be respected, or if saddr will
never be modified by a route lookup for some specific reason here,
that should be documented.

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Harald Welte]

Hi Dave,

On Mon, Sep 18, 2017 at 09:17:51PM -0700, David Miller wrote:
> This and the new dst caching code ignores any source address selection
> done by ip_route_output_key() or the new tunnel route lookup helpers.
> 
> Either source address selection should be respected, or if saddr will
> never be modified by a route lookup for some specific reason here,
> that should be documented.

The IP source address is fixed by signaling on the GTP-C control plane
and nothing that the kernel can unilaterally decide to change.  Such a
change of address would have to be decided by and first be signaled on
GTP-C to the peer by the userspace daemon, which would then update the
PDP context in the kernel.

So I guess you're asking us to document that rationale as form of a
source code comment ?

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

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[David Miller]

From: Harald Welte <laforge@gnumonks.org>
Date: Tue, 19 Sep 2017 20:09:42 +0800

> So I guess you're asking us to document that rationale as form of a
> source code comment ?

Yes that would make ignoring the potential changing of the non-const
'saddr' argument at least be documented.

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Andreas Schultz]

On 19/09/17 14:09, Harald Welte wrote:
> Hi Dave,
> 
> On Mon, Sep 18, 2017 at 09:17:51PM -0700, David Miller wrote:
>> This and the new dst caching code ignores any source address selection
>> done by ip_route_output_key() or the new tunnel route lookup helpers.
>>
>> Either source address selection should be respected, or if saddr will
>> never be modified by a route lookup for some specific reason here,
>> that should be documented.
> 
> The IP source address is fixed by signaling on the GTP-C control plane
> and nothing that the kernel can unilaterally decide to change.  Such a
> change of address would have to be decided by and first be signaled on
> GTP-C to the peer by the userspace daemon, which would then update the
> PDP context in the kernel.

I think we had this discussion before. The sending IP and port are not 
part of the identity of the PDP context. So IMHO the sender is permitted
to change the source IP at random.

Regards
Andreas

> 
> So I guess you're asking us to document that rationale as form of a
> source code comment ?
> 

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Harald Welte]

Hi Andreas,

On Wed, Sep 20, 2017 at 05:37:52PM +0200, Andreas Schultz wrote:
> I think we had this discussion before. The sending IP and port are not part
> of the identity of the PDP context. So IMHO the sender is permitted
> to change the source IP at random.

Thanks for the reminder: You are correct, at least in the uplink case
(MS->GGSN) where there is mobility of the MS. In the downlink case
(GGSN->MS), which is the "sending" part for the kernel GTP code used at
a GGSN, I'm not sure if that theory holds true in reality.

Do you agree that the current behavior of not using automatic source
address selection for encapsulated GTP packets but rather using the
source address of the socket is intended?

Do you further agree that the dst_cache support patch by Tom retains
that intended behavior and it should be merged?

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

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Tom Herbert]

On Mon, Sep 18, 2017 at 9:17 PM, David Miller <davem@davemloft.net> wrote:
> From: Tom Herbert <tom@quantonium.net>
> Date: Mon, 18 Sep 2017 17:38:53 -0700
>
>> Call ip_tunnel_get_route and dst_cache to pdp context which should
>> improve performance by obviating the need to perform a route lookup
>> on every packet.
>>
>> Signed-off-by: Tom Herbert <tom@quantonium.net>
>
> Not caused by your changes, but something to think about:
>
>> -static struct rtable *ip4_route_output_gtp(struct flowi4 *fl4,
>> -                                        const struct sock *sk,
>> -                                        __be32 daddr)
>> -{
>> -     memset(fl4, 0, sizeof(*fl4));
>> -     fl4->flowi4_oif         = sk->sk_bound_dev_if;
>> -     fl4->daddr              = daddr;
>> -     fl4->saddr              = inet_sk(sk)->inet_saddr;
>> -     fl4->flowi4_tos         = RT_CONN_FLAGS(sk);
>> -     fl4->flowi4_proto       = sk->sk_protocol;
>> -
>> -     return ip_route_output_key(sock_net(sk), fl4);
>> -}
>
> This and the new dst caching code ignores any source address selection
> done by ip_route_output_key() or the new tunnel route lookup helpers.
>
> Either source address selection should be respected, or if saddr will
> never be modified by a route lookup for some specific reason here,
> that should be documented.

Yes, I noticed that. In this case the source address is intended to be
taken bound on the socket which would imply we aren't interested in
source address selection.

Tom

[PATCH net-next 04/14] gtp: udp recv clean up

[Tom Herbert]

Create separate UDP receive functions for GTP version 0 and version 1.
Set encap_rcv appropriately when configuring a socket. Also, convert to
using gro_cells.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c | 130 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 71 insertions(+), 59 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 95df3bcebbb2..1de2ea6217ea 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -80,6 +80,8 @@ struct gtp_dev {
 	unsigned int		hash_size;
 	struct hlist_head	*tid_hash;
 	struct hlist_head	*addr_hash;
+
+	struct gro_cells	gro_cells;
 };
 
 static unsigned int gtp_net_id __read_mostly;
@@ -217,55 +219,83 @@ static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
 	stats->rx_bytes += skb->len;
 	u64_stats_update_end(&stats->syncp);
 
-	netif_rx(skb);
+	gro_cells_receive(&gtp->gro_cells, skb);
+
 	return 0;
 }
 
-/* 1 means pass up to the stack, -1 means drop and 0 means decapsulated. */
-static int gtp0_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
+/* UDP encapsulation receive handler for GTPv0-U . See net/ipv4/udp.c.
+ * Return codes: 0: success, <0: error, >0: pass up to userspace UDP socket.
+ */
+static int gtp0_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
+	struct gtp_dev *gtp = rcu_dereference_sk_user_data(sk);
 	unsigned int hdrlen = sizeof(struct udphdr) +
 			      sizeof(struct gtp0_header);
 	struct gtp0_header *gtp0;
 	struct pdp_ctx *pctx;
 
+	if (!gtp)
+		goto pass;
+
 	if (!pskb_may_pull(skb, hdrlen))
-		return -1;
+		goto drop;
 
 	gtp0 = (struct gtp0_header *)(skb->data + sizeof(struct udphdr));
 
 	if ((gtp0->flags >> 5) != GTP_V0)
-		return 1;
+		goto pass;
 
 	if (gtp0->type != GTP_TPDU)
-		return 1;
+		goto pass;
+
+	netdev_dbg(gtp->dev, "received GTP0 packet\n");
 
 	pctx = gtp0_pdp_find(gtp, be64_to_cpu(gtp0->tid));
 	if (!pctx) {
 		netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb);
-		return 1;
+		goto pass;
+	}
+
+	if (!gtp_rx(pctx, skb, hdrlen, gtp->role)) {
+		/* Successfully received */
+		return 0;
 	}
 
-	return gtp_rx(pctx, skb, hdrlen, gtp->role);
+drop:
+	kfree_skb(skb);
+	return 0;
+
+pass:
+	return 1;
 }
 
-static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
+/* UDP encapsulation receive handler for GTPv0-U . See net/ipv4/udp.c.
+ * Return codes: 0: success, <0: error, >0: pass up to userspace UDP socket.
+ */
+static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
+	struct gtp_dev *gtp = rcu_dereference_sk_user_data(sk);
 	unsigned int hdrlen = sizeof(struct udphdr) +
 			      sizeof(struct gtp1_header);
 	struct gtp1_header *gtp1;
 	struct pdp_ctx *pctx;
 
+	if (!gtp)
+		goto pass;
+
 	if (!pskb_may_pull(skb, hdrlen))
-		return -1;
+		goto drop;
 
 	gtp1 = (struct gtp1_header *)(skb->data + sizeof(struct udphdr));
 
 	if ((gtp1->flags >> 5) != GTP_V1)
-		return 1;
+		goto pass;
 
 	if (gtp1->type != GTP_TPDU)
-		return 1;
+		goto pass;
+
+	netdev_dbg(gtp->dev, "received GTP1 packet\n");
 
 	/* From 29.060: "This field shall be present if and only if any one or
 	 * more of the S, PN and E flags are set.".
@@ -278,17 +308,27 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
 
 	/* Make sure the header is larger enough, including extensions. */
 	if (!pskb_may_pull(skb, hdrlen))
-		return -1;
+		goto drop;
 
 	gtp1 = (struct gtp1_header *)(skb->data + sizeof(struct udphdr));
 
 	pctx = gtp1_pdp_find(gtp, ntohl(gtp1->tid));
 	if (!pctx) {
 		netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb);
-		return 1;
+		goto pass;
+	}
+
+	if (!gtp_rx(pctx, skb, hdrlen, gtp->role)) {
+		/* Successfully received */
+		return 0;
 	}
 
-	return gtp_rx(pctx, skb, hdrlen, gtp->role);
+drop:
+	kfree_skb(skb);
+	return 0;
+
+pass:
+	return 1;
 }
 
 static void gtp_encap_destroy(struct sock *sk)
@@ -317,49 +357,6 @@ static void gtp_encap_disable(struct gtp_dev *gtp)
 	gtp_encap_disable_sock(gtp->sk1u);
 }
 
-/* UDP encapsulation receive handler. See net/ipv4/udp.c.
- * Return codes: 0: success, <0: error, >0: pass up to userspace UDP socket.
- */
-static int gtp_encap_recv(struct sock *sk, struct sk_buff *skb)
-{
-	struct gtp_dev *gtp;
-	int ret = 0;
-
-	gtp = rcu_dereference_sk_user_data(sk);
-	if (!gtp)
-		return 1;
-
-	netdev_dbg(gtp->dev, "encap_recv sk=%p\n", sk);
-
-	switch (udp_sk(sk)->encap_type) {
-	case UDP_ENCAP_GTP0:
-		netdev_dbg(gtp->dev, "received GTP0 packet\n");
-		ret = gtp0_udp_encap_recv(gtp, skb);
-		break;
-	case UDP_ENCAP_GTP1U:
-		netdev_dbg(gtp->dev, "received GTP1U packet\n");
-		ret = gtp1u_udp_encap_recv(gtp, skb);
-		break;
-	default:
-		ret = -1; /* Shouldn't happen. */
-	}
-
-	switch (ret) {
-	case 1:
-		netdev_dbg(gtp->dev, "pass up to the process\n");
-		break;
-	case 0:
-		break;
-	case -1:
-		netdev_dbg(gtp->dev, "GTP packet has been dropped\n");
-		kfree_skb(skb);
-		ret = 0;
-		break;
-	}
-
-	return ret;
-}
-
 static int gtp_dev_init(struct net_device *dev)
 {
 	struct gtp_dev *gtp = netdev_priv(dev);
@@ -627,6 +624,8 @@ static void gtp_link_setup(struct net_device *dev)
 				  sizeof(struct iphdr) +
 				  sizeof(struct udphdr) +
 				  sizeof(struct gtp0_header);
+
+	gro_cells_init(&gtp->gro_cells, dev);
 }
 
 static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize);
@@ -683,6 +682,7 @@ static void gtp_dellink(struct net_device *dev, struct list_head *head)
 {
 	struct gtp_dev *gtp = netdev_priv(dev);
 
+	gro_cells_destroy(&gtp->gro_cells);
 	gtp_encap_disable(gtp);
 	gtp_hashtable_free(gtp);
 	list_del_rcu(&gtp->list);
@@ -804,9 +804,21 @@ static struct sock *gtp_encap_enable_socket(int fd, int type,
 	sk = sock->sk;
 	sock_hold(sk);
 
+	switch (type) {
+	case UDP_ENCAP_GTP0:
+		tuncfg.encap_rcv = gtp0_udp_encap_recv;
+		break;
+	case UDP_ENCAP_GTP1U:
+		tuncfg.encap_rcv = gtp1u_udp_encap_recv;
+		break;
+	default:
+		pr_debug("Unknown encap type %u\n", type);
+		sk = ERR_PTR(-EINVAL);
+		goto out_sock;
+	}
+
 	tuncfg.sk_user_data = gtp;
 	tuncfg.encap_type = type;
-	tuncfg.encap_rcv = gtp_encap_recv;
 	tuncfg.encap_destroy = gtp_encap_destroy;
 
 	setup_udp_tunnel_sock(sock_net(sock->sk), sock, &tuncfg);
-- 
2.11.0

Re: [PATCH net-next 04/14] gtp: udp recv clean up

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

Hi Tom,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Tom-Herbert/gtp-Additional-feature-support/20170919-143920
config: i386-randconfig-x016-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: the linux-review/Tom-Herbert/gtp-Additional-feature-support/20170919-143920 HEAD 737a09b8f9cd56706d01703d17523b0fea907f41 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers//net/gtp.c: In function 'gtp_rx':
>> drivers//net/gtp.c:222:21: error: 'gtp' undeclared (first use in this function)
     gro_cells_receive(&gtp->gro_cells, skb);
                        ^~~
   drivers//net/gtp.c:222:21: note: each undeclared identifier is reported only once for each function it appears in
   drivers//net/gtp.c: In function 'gtp_link_setup':
   drivers//net/gtp.c:628:18: error: 'gtp' undeclared (first use in this function)
     gro_cells_init(&gtp->gro_cells, dev);
                     ^~~

vim +/gtp +222 drivers//net/gtp.c

   190	
   191	static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
   192				unsigned int hdrlen, unsigned int role)
   193	{
   194		struct pcpu_sw_netstats *stats;
   195	
   196		if (!gtp_check_ms(skb, pctx, hdrlen, role)) {
   197			netdev_dbg(pctx->dev, "No PDP ctx for this MS\n");
   198			return 1;
   199		}
   200	
   201		/* Get rid of the GTP + UDP headers. */
   202		if (iptunnel_pull_header(skb, hdrlen, skb->protocol,
   203					 !net_eq(sock_net(pctx->sk), dev_net(pctx->dev))))
   204			return -1;
   205	
   206		netdev_dbg(pctx->dev, "forwarding packet from GGSN to uplink\n");
   207	
   208		/* Now that the UDP and the GTP header have been removed, set up the
   209		 * new network header. This is required by the upper layer to
   210		 * calculate the transport header.
   211		 */
   212		skb_reset_network_header(skb);
   213	
   214		skb->dev = pctx->dev;
   215	
   216		stats = this_cpu_ptr(pctx->dev->tstats);
   217		u64_stats_update_begin(&stats->syncp);
   218		stats->rx_packets++;
   219		stats->rx_bytes += skb->len;
   220		u64_stats_update_end(&stats->syncp);
   221	
 > 222		gro_cells_receive(&gtp->gro_cells, skb);
   223	
   224		return 0;
   225	}
   226	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26821 bytes --]

Re: [PATCH net-next 04/14] gtp: udp recv clean up

[Harald Welte]

Hi Tom,

I think this patch does too many things at once:
* introduce separate rx functions
* convert from netif_rx to gro_cells_receive
* cosmetic changes like "return -1" to "goto drop"

In the context of reviewability and the "one patch per topic", I would
prefer to see those separated, thanks.

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

[PATCH net-next 05/14] gtp: Remove special mtu handling

[Tom Herbert]

Removes MTU handling in gtp_build_skb_ip4. This is non standard relative
to how other tunneling protocols handle MTU. The model espoused is that
the inner interface should set it's MTU to be less than the expected
path MTU on the overlay network. Path MTU discovery is not typically
used for modifying tunnel MTUs.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 1de2ea6217ea..f2089fa4f004 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -466,8 +466,6 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,
 	struct iphdr *iph;
 	struct sock *sk;
 	__be32 saddr;
-	__be16 df;
-	int mtu;
 
 	/* Read the IP destination address and resolve the PDP context.
 	 * Prepend PDP header with TEI/TID from PDP ctx.
@@ -510,34 +508,6 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,
 
 	skb_dst_drop(skb);
 
-	/* This is similar to tnl_update_pmtu(). */
-	df = iph->frag_off;
-	if (df) {
-		mtu = dst_mtu(&rt->dst) - dev->hard_header_len -
-			sizeof(struct iphdr) - sizeof(struct udphdr);
-		switch (pctx->gtp_version) {
-		case GTP_V0:
-			mtu -= sizeof(struct gtp0_header);
-			break;
-		case GTP_V1:
-			mtu -= sizeof(struct gtp1_header);
-			break;
-		}
-	} else {
-		mtu = dst_mtu(&rt->dst);
-	}
-
-	rt->dst.ops->update_pmtu(&rt->dst, NULL, skb, mtu);
-
-	if (!skb_is_gso(skb) && (iph->frag_off & htons(IP_DF)) &&
-	    mtu < ntohs(iph->tot_len)) {
-		netdev_dbg(dev, "packet too big, fragmentation needed\n");
-		memset(IPCB(skb), 0, sizeof(*IPCB(skb)));
-		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
-			  htonl(mtu));
-		goto err_rt;
-	}
-
 	gtp_set_pktinfo_ipv4(pktinfo, sk, iph, pctx, rt, &fl4, dev);
 	gtp_push_header(skb, pktinfo);
 
-- 
2.11.0

Re: [PATCH net-next 05/14] gtp: Remove special mtu handling

[Harald Welte]

Hi Tom,

On Mon, Sep 18, 2017 at 05:38:55PM -0700, Tom Herbert wrote:
> Removes MTU handling in gtp_build_skb_ip4. This is non standard relative
> to how other tunneling protocols handle MTU. The model espoused is that
> the inner interface should set it's MTU to be less than the expected
> path MTU on the overlay network. Path MTU discovery is not typically
> used for modifying tunnel MTUs.

The point of the kernel GTP module is to interoperate with existing
other GTP implementations and the practises established by cellular
operators when operating GTP in their networks.

While what you describe (chose interface MTU to be less than the
expected path MTU) is generally best practise in the Linux IP/networking
world, this is not generally reflected in the cellular
universe. You see quite a bit of GTP fragmentation due to the fact
that the transport network simply has to deal with the MTU that has
been established via the control plane between SGSN and MS/UE, without
the GGSN even being part of that negotiation.

Also, you may very well have one "gtp0" tunnel device at the GGSN,
but you are establishing individual GTP tunnels to dozesn to hundreds of
different SGSNs at operators all over the world.  You cannot reliably
set the "gtp0" interface MTU to "the path MTU of the overlay network",
as the overlay network is in fact different for each of the SGSNs you're
talking to - and each may have a different path MTU.

So unless I'm missing something, I would currently vote for staying with
the current code, which uses the path MTU to the specific destination IP
address (the SGSN).

Regards,
	Harald

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

Re: [PATCH net-next 05/14] gtp: Remove special mtu handling

[Tom Herbert]

On Tue, Sep 19, 2017 at 4:42 AM, Harald Welte <laforge@gnumonks.org> wrote:
> Hi Tom,
>
> On Mon, Sep 18, 2017 at 05:38:55PM -0700, Tom Herbert wrote:
>> Removes MTU handling in gtp_build_skb_ip4. This is non standard relative
>> to how other tunneling protocols handle MTU. The model espoused is that
>> the inner interface should set it's MTU to be less than the expected
>> path MTU on the overlay network. Path MTU discovery is not typically
>> used for modifying tunnel MTUs.
>
> The point of the kernel GTP module is to interoperate with existing
> other GTP implementations and the practises established by cellular
> operators when operating GTP in their networks.
>
> While what you describe (chose interface MTU to be less than the
> expected path MTU) is generally best practise in the Linux IP/networking
> world, this is not generally reflected in the cellular
> universe. You see quite a bit of GTP fragmentation due to the fact
> that the transport network simply has to deal with the MTU that has
> been established via the control plane between SGSN and MS/UE, without
> the GGSN even being part of that negotiation.
>
> Also, you may very well have one "gtp0" tunnel device at the GGSN,
> but you are establishing individual GTP tunnels to dozesn to hundreds of
> different SGSNs at operators all over the world.  You cannot reliably
> set the "gtp0" interface MTU to "the path MTU of the overlay network",
> as the overlay network is in fact different for each of the SGSNs you're
> talking to - and each may have a different path MTU.
>
> So unless I'm missing something, I would currently vote for staying with
> the current code, which uses the path MTU to the specific destination IP
> address (the SGSN).
>
Okay, I'll modify tnl_update_pmtu so we can call it from GTP and not
have to replicate that function. I suspect VXLAN might also what this
at some point.

Tom

> Regards,
>         Harald
>
> --
> - 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)

[PATCH net-next 06/14] gtp: Eliminate pktinfo and add port configuration

[Tom Herbert]

The gtp pktinfo structure is unnecessary and needs a lot of code to
manage it. Remove it. Also, add per pdp port configuration for transmit.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c        | 167 ++++++++++++++++++++---------------------------
 include/uapi/linux/gtp.h |   1 +
 2 files changed, 71 insertions(+), 97 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index f2089fa4f004..a928279c382c 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -53,6 +53,7 @@ struct pdp_ctx {
 		} v1;
 	} u;
 	u8			gtp_version;
+	__be16			gtp_port;
 	u16			af;
 
 	struct in_addr		ms_addr_ip4;
@@ -418,149 +419,112 @@ static inline void gtp1_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)
 	 */
 }
 
-struct gtp_pktinfo {
-	struct sock		*sk;
-	struct iphdr		*iph;
-	struct flowi4		fl4;
-	struct rtable		*rt;
-	struct pdp_ctx		*pctx;
-	struct net_device	*dev;
-	__be16			gtph_port;
-};
-
-static void gtp_push_header(struct sk_buff *skb, struct gtp_pktinfo *pktinfo)
+static void gtp_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)
 {
-	switch (pktinfo->pctx->gtp_version) {
+	switch (pctx->gtp_version) {
 	case GTP_V0:
-		pktinfo->gtph_port = htons(GTP0_PORT);
-		gtp0_push_header(skb, pktinfo->pctx);
+		gtp0_push_header(skb, pctx);
 		break;
 	case GTP_V1:
-		pktinfo->gtph_port = htons(GTP1U_PORT);
-		gtp1_push_header(skb, pktinfo->pctx);
+		gtp1_push_header(skb, pctx);
 		break;
 	}
 }
 
-static inline void gtp_set_pktinfo_ipv4(struct gtp_pktinfo *pktinfo,
-					struct sock *sk, struct iphdr *iph,
-					struct pdp_ctx *pctx, struct rtable *rt,
-					struct flowi4 *fl4,
-					struct net_device *dev)
-{
-	pktinfo->sk	= sk;
-	pktinfo->iph	= iph;
-	pktinfo->pctx	= pctx;
-	pktinfo->rt	= rt;
-	pktinfo->fl4	= *fl4;
-	pktinfo->dev	= dev;
-}
-
-static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,
-			     struct gtp_pktinfo *pktinfo)
+static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
+		    struct pdp_ctx *pctx)
 {
-	struct gtp_dev *gtp = netdev_priv(dev);
-	struct pdp_ctx *pctx;
+	struct sock *sk = pctx->sk;
+	__be32 saddr = inet_sk(sk)->inet_saddr;
 	struct rtable *rt;
-	struct flowi4 fl4;
-	struct iphdr *iph;
-	struct sock *sk;
-	__be32 saddr;
-
-	/* Read the IP destination address and resolve the PDP context.
-	 * Prepend PDP header with TEI/TID from PDP ctx.
-	 */
-	iph = ip_hdr(skb);
-	if (gtp->role == GTP_ROLE_SGSN)
-		pctx = ipv4_pdp_find(gtp, iph->saddr);
-	else
-		pctx = ipv4_pdp_find(gtp, iph->daddr);
+	int err = 0;
 
-	if (!pctx) {
-		netdev_dbg(dev, "no PDP ctx found for %pI4, skip\n",
-			   &iph->daddr);
-		return -ENOENT;
-	}
-	netdev_dbg(dev, "found PDP context %p\n", pctx);
+	/* Ensure there is sufficient headroom. */
+	err = skb_cow_head(skb, dev->needed_headroom);
+	if (unlikely(err))
+		goto out_err;
 
-	sk = pctx->sk;
-	saddr = inet_sk(sk)->inet_saddr;
+	skb_reset_inner_headers(skb);
 
 	rt = ip_tunnel_get_route(dev, skb, sk->sk_protocol,
 				 sk->sk_bound_dev_if, RT_CONN_FLAGS(sk),
 				 pctx->peer_addr_ip4.s_addr, &saddr,
-				 pktinfo->gtph_port, pktinfo->gtph_port,
+				 pctx->gtp_port, pctx->gtp_port,
 				 &pctx->dst_cache, NULL);
 
 	if (IS_ERR(rt)) {
-		if (rt == ERR_PTR(-ELOOP)) {
-			netdev_dbg(dev, "circular route to SSGN %pI4\n",
-				   &pctx->peer_addr_ip4.s_addr);
-			dev->stats.collisions++;
-			goto err_rt;
-		} else {
-			netdev_dbg(dev, "no route to SSGN %pI4\n",
-				   &pctx->peer_addr_ip4.s_addr);
-			dev->stats.tx_carrier_errors++;
-			goto err;
-		}
+		err = PTR_ERR(rt);
+		goto out_err;
 	}
 
 	skb_dst_drop(skb);
 
-	gtp_set_pktinfo_ipv4(pktinfo, sk, iph, pctx, rt, &fl4, dev);
-	gtp_push_header(skb, pktinfo);
+	gtp_push_header(skb, pctx);
+	udp_tunnel_xmit_skb(rt, sk, skb, saddr,
+			    pctx->peer_addr_ip4.s_addr,
+			    0, ip4_dst_hoplimit(&rt->dst), 0,
+			    pctx->gtp_port, pctx->gtp_port,
+			    false, false);
+
+	netdev_dbg(dev, "gtp -> IP src: %pI4 dst: %pI4\n",
+		   &saddr, &pctx->peer_addr_ip4.s_addr);
 
 	return 0;
-err_rt:
-	ip_rt_put(rt);
-err:
-	return -EBADMSG;
+
+out_err:
+	if (err == -ELOOP)
+		dev->stats.collisions++;
+	else
+		dev->stats.tx_carrier_errors++;
+
+	return err;
 }
 
 static netdev_tx_t gtp_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	unsigned int proto = ntohs(skb->protocol);
-	struct gtp_pktinfo pktinfo;
+	struct gtp_dev *gtp = netdev_priv(dev);
+	struct pdp_ctx *pctx;
 	int err;
 
-	/* Ensure there is sufficient headroom. */
-	if (skb_cow_head(skb, dev->needed_headroom))
-		goto tx_err;
-
-	skb_reset_inner_headers(skb);
-
 	/* PDP context lookups in gtp_build_skb_*() need rcu read-side lock. */
 	rcu_read_lock();
 	switch (proto) {
-	case ETH_P_IP:
-		err = gtp_build_skb_ip4(skb, dev, &pktinfo);
+	case ETH_P_IP: {
+		struct iphdr *iph = ip_hdr(skb);
+
+		if (gtp->role == GTP_ROLE_SGSN)
+			pctx = ipv4_pdp_find(gtp, iph->saddr);
+		else
+			pctx = ipv4_pdp_find(gtp, iph->daddr);
+
+		if (!pctx) {
+			netdev_dbg(dev, "no PDP ctx found for %pI4, skip\n",
+				   &iph->daddr);
+			err = -ENOENT;
+			goto tx_err;
+		}
+
 		break;
+	}
 	default:
 		err = -EOPNOTSUPP;
-		break;
+		goto tx_err;
 	}
-	rcu_read_unlock();
+
+	netdev_dbg(dev, "found PDP context %p\n", pctx);
+
+	err = gtp_xmit(skb, dev, pctx);
 
 	if (err < 0)
 		goto tx_err;
 
-	switch (proto) {
-	case ETH_P_IP:
-		netdev_dbg(pktinfo.dev, "gtp -> IP src: %pI4 dst: %pI4\n",
-			   &pktinfo.iph->saddr, &pktinfo.iph->daddr);
-		udp_tunnel_xmit_skb(pktinfo.rt, pktinfo.sk, skb,
-				    pktinfo.fl4.saddr, pktinfo.fl4.daddr,
-				    pktinfo.iph->tos,
-				    ip4_dst_hoplimit(&pktinfo.rt->dst),
-				    0,
-				    pktinfo.gtph_port, pktinfo.gtph_port,
-				    true, false);
-		break;
-	}
+	rcu_read_unlock();
 
 	return NETDEV_TX_OK;
+
 tx_err:
+	rcu_read_unlock();
 	dev->stats.tx_errors++;
 	dev_kfree_skb(skb);
 	return NETDEV_TX_OK;
@@ -864,6 +828,8 @@ static struct gtp_dev *gtp_find_dev(struct net *src_net, struct nlattr *nla[])
 
 static void ipv4_pdp_fill(struct pdp_ctx *pctx, struct genl_info *info)
 {
+	__be16 default_port = 0;
+
 	pctx->gtp_version = nla_get_u32(info->attrs[GTPA_VERSION]);
 	pctx->af = AF_INET;
 	pctx->peer_addr_ip4.s_addr =
@@ -879,14 +845,21 @@ static void ipv4_pdp_fill(struct pdp_ctx *pctx, struct genl_info *info)
 		 */
 		pctx->u.v0.tid = nla_get_u64(info->attrs[GTPA_TID]);
 		pctx->u.v0.flow = nla_get_u16(info->attrs[GTPA_FLOW]);
+		default_port = htons(GTP0_PORT);
 		break;
 	case GTP_V1:
 		pctx->u.v1.i_tei = nla_get_u32(info->attrs[GTPA_I_TEI]);
 		pctx->u.v1.o_tei = nla_get_u32(info->attrs[GTPA_O_TEI]);
+		default_port = htons(GTP1U_PORT);
 		break;
 	default:
 		break;
 	}
+
+	if (info->attrs[GTPA_PORT])
+		pctx->gtp_port = nla_get_u16(info->attrs[GTPA_PORT]);
+	else
+		pctx->gtp_port = default_port;
 }
 
 static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock *sk,
diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
index 57d1edb8efd9..b2283a5c6d7f 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -27,6 +27,7 @@ enum gtp_attrs {
 	GTPA_I_TEI,	/* for GTPv1 only */
 	GTPA_O_TEI,	/* for GTPv1 only */
 	GTPA_PAD,
+	GTPA_PORT,
 	__GTPA_MAX,
 };
 #define GTPA_MAX (__GTPA_MAX + 1)
-- 
2.11.0

[PATCH net-next 07/14] gtp: Support encapsulation of IPv6 packets

[Tom Herbert]

Allow IPv6 mobile subscriber packets. This entails adding an IPv6 mobile
subscriber address to pdp context and IPv6 specific variants to find pdp
contexts by address.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c        | 259 +++++++++++++++++++++++++++++++++++++----------
 include/uapi/linux/gtp.h |   1 +
 2 files changed, 209 insertions(+), 51 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index a928279c382c..62c0c968efa6 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -54,9 +54,13 @@ struct pdp_ctx {
 	} u;
 	u8			gtp_version;
 	__be16			gtp_port;
-	u16			af;
 
-	struct in_addr		ms_addr_ip4;
+	u16			ms_af;
+	union {
+		struct in_addr	ms_addr_ip4;
+		struct in6_addr	ms_addr_ip6;
+	};
+
 	struct in_addr		peer_addr_ip4;
 
 	struct sock		*sk;
@@ -80,7 +84,9 @@ struct gtp_dev {
 	unsigned int		role;
 	unsigned int		hash_size;
 	struct hlist_head	*tid_hash;
-	struct hlist_head	*addr_hash;
+
+	struct hlist_head	*addr4_hash;
+	struct hlist_head	*addr6_hash;
 
 	struct gro_cells	gro_cells;
 };
@@ -98,6 +104,7 @@ static void pdp_context_delete(struct pdp_ctx *pctx);
 static inline u32 gtp0_hashfn(u64 tid)
 {
 	u32 *tid32 = (u32 *) &tid;
+
 	return jhash_2words(tid32[0], tid32[1], gtp_h_initval);
 }
 
@@ -111,6 +118,11 @@ static inline u32 ipv4_hashfn(__be32 ip)
 	return jhash_1word((__force u32)ip, gtp_h_initval);
 }
 
+static inline u32 ipv6_hashfn(const struct in6_addr *a)
+{
+	return __ipv6_addr_jhash(a, gtp_h_initval);
+}
+
 /* Resolve a PDP context structure based on the 64bit TID. */
 static struct pdp_ctx *gtp0_pdp_find(struct gtp_dev *gtp, u64 tid)
 {
@@ -149,10 +161,10 @@ static struct pdp_ctx *ipv4_pdp_find(struct gtp_dev *gtp, __be32 ms_addr)
 	struct hlist_head *head;
 	struct pdp_ctx *pdp;
 
-	head = &gtp->addr_hash[ipv4_hashfn(ms_addr) % gtp->hash_size];
+	head = &gtp->addr4_hash[ipv4_hashfn(ms_addr) % gtp->hash_size];
 
 	hlist_for_each_entry_rcu(pdp, head, hlist_addr) {
-		if (pdp->af == AF_INET &&
+		if (pdp->ms_af == AF_INET &&
 		    pdp->ms_addr_ip4.s_addr == ms_addr)
 			return pdp;
 	}
@@ -176,32 +188,95 @@ static bool gtp_check_ms_ipv4(struct sk_buff *skb, struct pdp_ctx *pctx,
 		return iph->saddr == pctx->ms_addr_ip4.s_addr;
 }
 
+/* Resolve a PDP context based on IPv6 address of MS. */
+static struct pdp_ctx *ipv6_pdp_find(struct gtp_dev *gtp,
+				     const struct in6_addr *ms_addr)
+{
+	struct hlist_head *head;
+	struct pdp_ctx *pdp;
+
+	head = &gtp->addr6_hash[ipv6_hashfn(ms_addr) % gtp->hash_size];
+
+	hlist_for_each_entry_rcu(pdp, head, hlist_addr) {
+		if (pdp->ms_af == AF_INET6 &&
+		    ipv6_addr_equal(&pdp->ms_addr_ip6, ms_addr))
+			return pdp;
+	}
+
+	return NULL;
+}
+
+static bool gtp_check_ms_ipv6(struct sk_buff *skb, struct pdp_ctx *pctx,
+			      unsigned int hdrlen, unsigned int role)
+{
+	struct ipv6hdr *ipv6h;
+
+	if (!pskb_may_pull(skb, hdrlen + sizeof(struct ipv6hdr)))
+		return false;
+
+	ipv6h = (struct ipv6hdr *)(skb->data + hdrlen);
+
+	if (role == GTP_ROLE_SGSN)
+		return ipv6_addr_equal(&ipv6h->daddr, &pctx->ms_addr_ip6);
+	else
+		return ipv6_addr_equal(&ipv6h->saddr, &pctx->ms_addr_ip6);
+}
+
 /* Check if the inner IP address in this packet is assigned to any
  * existing mobile subscriber.
  */
 static bool gtp_check_ms(struct sk_buff *skb, struct pdp_ctx *pctx,
 			     unsigned int hdrlen, unsigned int role)
 {
-	switch (ntohs(skb->protocol)) {
-	case ETH_P_IP:
+	struct iphdr *iph;
+
+	/* Minimally there needs to be an IPv4 header */
+	if (!pskb_may_pull(skb, hdrlen + sizeof(struct iphdr)))
+		return false;
+
+	iph = (struct iphdr *)(skb->data + hdrlen);
+
+	switch (iph->version) {
+	case 4:
 		return gtp_check_ms_ipv4(skb, pctx, hdrlen, role);
+	case 6:
+		return gtp_check_ms_ipv6(skb, pctx, hdrlen, role);
 	}
+
 	return false;
 }
 
+static u16 ipver_to_eth(struct iphdr *iph)
+{
+	switch (iph->version) {
+	case 4:
+		return htons(ETH_P_IP);
+	case 6:
+		return htons(ETH_P_IPV6);
+	default:
+		return 0;
+	}
+}
+
 static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
-			unsigned int hdrlen, unsigned int role)
+		  unsigned int hdrlen, unsigned int role)
 {
 	struct pcpu_sw_netstats *stats;
+	u16 inner_protocol;
 
 	if (!gtp_check_ms(skb, pctx, hdrlen, role)) {
 		netdev_dbg(pctx->dev, "No PDP ctx for this MS\n");
 		return 1;
 	}
 
+	inner_protocol = ipver_to_eth((struct iphdr *)(skb->data + hdrlen));
+	if (!inner_protocol)
+		return -1;
+
 	/* Get rid of the GTP + UDP headers. */
-	if (iptunnel_pull_header(skb, hdrlen, skb->protocol,
-				 !net_eq(sock_net(pctx->sk), dev_net(pctx->dev))))
+	if (iptunnel_pull_header(skb, hdrlen, inner_protocol,
+				 !net_eq(sock_net(pctx->sk),
+					 dev_net(pctx->dev))))
 		return -1;
 
 	netdev_dbg(pctx->dev, "forwarding packet from GGSN to uplink\n");
@@ -239,7 +314,8 @@ static int gtp0_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	if (!gtp)
 		goto pass;
 
-	if (!pskb_may_pull(skb, hdrlen))
+	/* Pull through IP header since gtp_rx looks at IP version */
+	if (!pskb_may_pull(skb, hdrlen + sizeof(struct iphdr)))
 		goto drop;
 
 	gtp0 = (struct gtp0_header *)(skb->data + sizeof(struct udphdr));
@@ -285,7 +361,8 @@ static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	if (!gtp)
 		goto pass;
 
-	if (!pskb_may_pull(skb, hdrlen))
+	/* Pull through IP header since gtp_rx looks at IP version */
+	if (!pskb_may_pull(skb, hdrlen + sizeof(struct iphdr)))
 		goto drop;
 
 	gtp1 = (struct gtp1_header *)(skb->data + sizeof(struct udphdr));
@@ -307,8 +384,10 @@ static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	if (gtp1->flags & GTP1_F_MASK)
 		hdrlen += 4;
 
-	/* Make sure the header is larger enough, including extensions. */
-	if (!pskb_may_pull(skb, hdrlen))
+	/* Make sure the header is larger enough, including extensions and
+	 * also an IP header since gtp_rx looks at IP version
+	 */
+	if (!pskb_may_pull(skb, hdrlen + sizeof(struct iphdr)))
 		goto drop;
 
 	gtp1 = (struct gtp1_header *)(skb->data + sizeof(struct udphdr));
@@ -389,7 +468,8 @@ static inline void gtp0_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)
 	gtp0->flags	= 0x1e; /* v0, GTP-non-prime. */
 	gtp0->type	= GTP_TPDU;
 	gtp0->length	= htons(payload_len);
-	gtp0->seq	= htons((atomic_inc_return(&pctx->tx_seq) - 1) % 0xffff);
+	gtp0->seq	= htons((atomic_inc_return(&pctx->tx_seq) - 1) %
+				0xffff);
 	gtp0->flow	= htons(pctx->u.v0.flow);
 	gtp0->number	= 0xff;
 	gtp0->spare[0]	= gtp0->spare[1] = gtp0->spare[2] = 0xff;
@@ -507,6 +587,23 @@ static netdev_tx_t gtp_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 
 		break;
 	}
+	case ETH_P_IPV6: {
+		struct ipv6hdr *ipv6h = ipv6_hdr(skb);
+
+		if (gtp->role == GTP_ROLE_SGSN)
+			pctx = ipv6_pdp_find(gtp, &ipv6h->saddr);
+		else
+			pctx = ipv6_pdp_find(gtp, &ipv6h->daddr);
+
+		if (!pctx) {
+			netdev_dbg(dev, "no PDP ctx found for %pI6, skip\n",
+				   &ipv6h->daddr);
+			err = -ENOENT;
+			goto tx_err;
+		}
+
+		break;
+	}
 	default:
 		err = -EOPNOTSUPP;
 		goto tx_err;
@@ -674,23 +771,32 @@ static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize)
 {
 	int i;
 
-	gtp->addr_hash = kmalloc(sizeof(struct hlist_head) * hsize, GFP_KERNEL);
-	if (gtp->addr_hash == NULL)
-		return -ENOMEM;
+	gtp->addr4_hash = kmalloc_array(hsize, sizeof(*gtp->addr4_hash),
+					GFP_KERNEL);
+	if (!gtp->addr4_hash)
+		goto err;
+
+	gtp->addr6_hash = kmalloc_array(hsize, sizeof(*gtp->addr6_hash),
+					GFP_KERNEL);
+	if (!gtp->addr6_hash)
+		goto err;
 
-	gtp->tid_hash = kmalloc(sizeof(struct hlist_head) * hsize, GFP_KERNEL);
-	if (gtp->tid_hash == NULL)
-		goto err1;
+	gtp->tid_hash = kmalloc_array(hsize, sizeof(struct hlist_head),
+				      GFP_KERNEL);
+	if (!gtp->tid_hash)
+		goto err;
 
 	gtp->hash_size = hsize;
 
 	for (i = 0; i < hsize; i++) {
-		INIT_HLIST_HEAD(&gtp->addr_hash[i]);
+		INIT_HLIST_HEAD(&gtp->addr4_hash[i]);
+		INIT_HLIST_HEAD(&gtp->addr6_hash[i]);
 		INIT_HLIST_HEAD(&gtp->tid_hash[i]);
 	}
 	return 0;
-err1:
-	kfree(gtp->addr_hash);
+err:
+	kfree(gtp->addr4_hash);
+	kfree(gtp->addr6_hash);
 	return -ENOMEM;
 }
 
@@ -704,7 +810,8 @@ static void gtp_hashtable_free(struct gtp_dev *gtp)
 			pdp_context_delete(pctx);
 
 	synchronize_rcu();
-	kfree(gtp->addr_hash);
+	kfree(gtp->addr4_hash);
+	kfree(gtp->addr6_hash);
 	kfree(gtp->tid_hash);
 }
 
@@ -826,16 +933,13 @@ static struct gtp_dev *gtp_find_dev(struct net *src_net, struct nlattr *nla[])
 	return gtp;
 }
 
-static void ipv4_pdp_fill(struct pdp_ctx *pctx, struct genl_info *info)
+static void pdp_fill(struct pdp_ctx *pctx, struct genl_info *info)
 {
 	__be16 default_port = 0;
 
 	pctx->gtp_version = nla_get_u32(info->attrs[GTPA_VERSION]);
-	pctx->af = AF_INET;
 	pctx->peer_addr_ip4.s_addr =
 		nla_get_be32(info->attrs[GTPA_PEER_ADDRESS]);
-	pctx->ms_addr_ip4.s_addr =
-		nla_get_be32(info->attrs[GTPA_MS_ADDRESS]);
 
 	switch (pctx->gtp_version) {
 	case GTP_V0:
@@ -862,33 +966,46 @@ static void ipv4_pdp_fill(struct pdp_ctx *pctx, struct genl_info *info)
 		pctx->gtp_port = default_port;
 }
 
-static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock *sk,
-			struct genl_info *info)
+static int gtp_pdp_add(struct gtp_dev *gtp, struct sock *sk,
+		       struct genl_info *info)
 {
 	struct net_device *dev = gtp->dev;
+	struct hlist_head *addr_list;
+	struct pdp_ctx *pctx = NULL;
 	u32 hash_ms, hash_tid = 0;
-	struct pdp_ctx *pctx;
-	bool found = false;
-	__be32 ms_addr;
+	struct in6_addr ms6_addr;
+	__be32 ms_addr = 0;
+	int ms_af;
 	int err;
 
-	ms_addr = nla_get_be32(info->attrs[GTPA_MS_ADDRESS]);
-	hash_ms = ipv4_hashfn(ms_addr) % gtp->hash_size;
+	/* Caller ensures we have either v4 or v6 mobile subscriber address */
+	if (info->attrs[GTPA_MS_ADDRESS]) {
+		/* IPv4 mobile subscriber */
 
-	hlist_for_each_entry_rcu(pctx, &gtp->addr_hash[hash_ms], hlist_addr) {
-		if (pctx->ms_addr_ip4.s_addr == ms_addr) {
-			found = true;
-			break;
-		}
+		ms_addr = nla_get_in_addr(info->attrs[GTPA_MS_ADDRESS]);
+		hash_ms = ipv4_hashfn(ms_addr) % gtp->hash_size;
+		addr_list = &gtp->addr4_hash[hash_ms];
+		ms_af = AF_INET;
+
+		pctx = ipv4_pdp_find(gtp, ms_addr);
+	} else {
+		/* IPv6 mobile subscriber */
+
+		ms6_addr = nla_get_in6_addr(info->attrs[GTPA_MS6_ADDRESS]);
+		hash_ms = ipv6_hashfn(&ms6_addr) % gtp->hash_size;
+		addr_list = &gtp->addr6_hash[hash_ms];
+		ms_af = AF_INET6;
+
+		pctx = ipv6_pdp_find(gtp, &ms6_addr);
 	}
 
-	if (found) {
+	if (pctx) {
 		if (info->nlhdr->nlmsg_flags & NLM_F_EXCL)
 			return -EEXIST;
 		if (info->nlhdr->nlmsg_flags & NLM_F_REPLACE)
 			return -EOPNOTSUPP;
 
-		ipv4_pdp_fill(pctx, info);
+		pdp_fill(pctx, info);
 
 		if (pctx->gtp_version == GTP_V0)
 			netdev_dbg(dev, "GTPv0-U: update tunnel id = %llx (pdp %p)\n",
@@ -914,7 +1031,18 @@ static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock *sk,
 	sock_hold(sk);
 	pctx->sk = sk;
 	pctx->dev = gtp->dev;
-	ipv4_pdp_fill(pctx, info);
+	pctx->ms_af = ms_af;
+
+	switch (ms_af) {
+	case AF_INET:
+		pctx->ms_addr_ip4.s_addr = ms_addr;
+		break;
+	case AF_INET6:
+		pctx->ms_addr_ip6 = ms6_addr;
+		break;
+	}
+
+	pdp_fill(pctx, info);
 	atomic_set(&pctx->tx_seq, 0);
 
 	switch (pctx->gtp_version) {
@@ -931,7 +1059,7 @@ static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock *sk,
 		break;
 	}
 
-	hlist_add_head_rcu(&pctx->hlist_addr, &gtp->addr_hash[hash_ms]);
+	hlist_add_head_rcu(&pctx->hlist_addr, addr_list);
 	hlist_add_head_rcu(&pctx->hlist_tid, &gtp->tid_hash[hash_tid]);
 
 	switch (pctx->gtp_version) {
@@ -973,11 +1101,17 @@ static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info)
 	int err;
 
 	if (!info->attrs[GTPA_VERSION] ||
-	    !info->attrs[GTPA_LINK] ||
-	    !info->attrs[GTPA_PEER_ADDRESS] ||
-	    !info->attrs[GTPA_MS_ADDRESS])
+	   !info->attrs[GTPA_LINK] ||
+	   !info->attrs[GTPA_PEER_ADDRESS])
 		return -EINVAL;
 
+	if (!(!!info->attrs[GTPA_MS_ADDRESS] ^
+	      !!info->attrs[GTPA_MS6_ADDRESS])) {
+		/* Either v4 or v6 mobile subscriber address must be set */
+
+		return -EINVAL;
+	}
+
 	version = nla_get_u32(info->attrs[GTPA_VERSION]);
 
 	switch (version) {
@@ -1016,7 +1150,7 @@ static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info)
 		goto out_unlock;
 	}
 
-	err = ipv4_pdp_add(gtp, sk, info);
+	err = gtp_pdp_add(gtp, sk, info);
 
 out_unlock:
 	rcu_read_unlock();
@@ -1036,6 +1170,11 @@ static struct pdp_ctx *gtp_find_pdp_by_link(struct net *net,
 		__be32 ip = nla_get_be32(nla[GTPA_MS_ADDRESS]);
 
 		return ipv4_pdp_find(gtp, ip);
+	} else if (nla[GTPA_MS6_ADDRESS]) {
+		struct in6_addr ip6 =
+		    nla_get_in6_addr(nla[GTPA_MS6_ADDRESS]);
+
+		return ipv6_pdp_find(gtp, &ip6);
 	} else if (nla[GTPA_VERSION]) {
 		u32 gtp_version = nla_get_u32(nla[GTPA_VERSION]);
 
@@ -1106,10 +1245,26 @@ static int gtp_genl_fill_info(struct sk_buff *skb, u32 snd_portid, u32 snd_seq,
 		goto nlmsg_failure;
 
 	if (nla_put_u32(skb, GTPA_VERSION, pctx->gtp_version) ||
-	    nla_put_be32(skb, GTPA_PEER_ADDRESS, pctx->peer_addr_ip4.s_addr) ||
-	    nla_put_be32(skb, GTPA_MS_ADDRESS, pctx->ms_addr_ip4.s_addr))
+	    nla_put_be32(skb, GTPA_PEER_ADDRESS, pctx->peer_addr_ip4.s_addr))
 		goto nla_put_failure;
 
+	switch (pctx->ms_af) {
+	case AF_INET:
+		if (nla_put_be32(skb, GTPA_MS_ADDRESS,
+				 pctx->ms_addr_ip4.s_addr))
+			goto nla_put_failure;
+
+		break;
+	case AF_INET6:
+		if (nla_put_in6_addr(skb, GTPA_MS6_ADDRESS,
+				     &pctx->ms_addr_ip6))
+			goto nla_put_failure;
+
+		break;
+	default:
+		goto nla_put_failure;
+	}
+
 	switch (pctx->gtp_version) {
 	case GTP_V0:
 		if (nla_put_u64_64bit(skb, GTPA_TID, pctx->u.v0.tid, GTPA_PAD) ||
@@ -1219,6 +1374,8 @@ static struct nla_policy gtp_genl_policy[GTPA_MAX + 1] = {
 	[GTPA_TID]		= { .type = NLA_U64, },
 	[GTPA_PEER_ADDRESS]	= { .type = NLA_U32, },
 	[GTPA_MS_ADDRESS]	= { .type = NLA_U32, },
+	[GTPA_MS6_ADDRESS]	= { .len = FIELD_SIZEOF(struct ipv6hdr,
+							daddr) },
 	[GTPA_FLOW]		= { .type = NLA_U16, },
 	[GTPA_NET_NS_FD]	= { .type = NLA_U32, },
 	[GTPA_I_TEI]		= { .type = NLA_U32, },
diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
index b2283a5c6d7f..ae4e632c0360 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -28,6 +28,7 @@ enum gtp_attrs {
 	GTPA_O_TEI,	/* for GTPv1 only */
 	GTPA_PAD,
 	GTPA_PORT,
+	GTPA_MS6_ADDRESS,
 	__GTPA_MAX,
 };
 #define GTPA_MAX (__GTPA_MAX + 1)
-- 
2.11.0

Re: [PATCH net-next 07/14] gtp: Support encapsulation of IPv6 packets

[David Miller]

From: Tom Herbert <tom@quantonium.net>
Date: Mon, 18 Sep 2017 17:38:57 -0700

> @@ -98,6 +104,7 @@ static void pdp_context_delete(struct pdp_ctx *pctx);
>  static inline u32 gtp0_hashfn(u64 tid)
>  {
>  	u32 *tid32 = (u32 *) &tid;
> +
>  	return jhash_2words(tid32[0], tid32[1], gtp_h_initval);
>  }
>  
> @@ -111,6 +118,11 @@ static inline u32 ipv4_hashfn(__be32 ip)
>  	return jhash_1word((__force u32)ip, gtp_h_initval);
>  }
>  
> +static inline u32 ipv6_hashfn(const struct in6_addr *a)
> +{
> +	return __ipv6_addr_jhash(a, gtp_h_initval);
> +}

I know you are just following the pattern of the existing "ipv4_hashfn()" here
but this kind of stuff is not very global namespace friendly.  Even simply
adding a "gtp_" prefix to these hash functions would be a lot better.

Re: [PATCH net-next 07/14] gtp: Support encapsulation of IPv6 packets

[Harald Welte]

Hi Dave,

On Mon, Sep 18, 2017 at 09:19:08PM -0700, David Miller wrote:

> > +static inline u32 ipv6_hashfn(const struct in6_addr *a)
> > +{
> > +	return __ipv6_addr_jhash(a, gtp_h_initval);
> > +}
> 
> I know you are just following the pattern of the existing "ipv4_hashfn()" here
> but this kind of stuff is not very global namespace friendly.  Even simply
> adding a "gtp_" prefix to these hash functions would be a lot better.

I would agree if this was an inline function defined in a header file or
a non-static function.  But where is the global namespace concern in
case of static inline functions defined and used in the same .c file?

If it makes you happy, I'm all for adding the prefix - I just would like
to understand the rationale better, thanks :)

Regards,
	Harald
-- 
- 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)

Re: [PATCH net-next 07/14] gtp: Support encapsulation of IPv6 packets

[David Miller]

From: Harald Welte <laforge@gnumonks.org>
Date: Tue, 19 Sep 2017 20:12:45 +0800

> Hi Dave,
> 
> On Mon, Sep 18, 2017 at 09:19:08PM -0700, David Miller wrote:
> 
>> > +static inline u32 ipv6_hashfn(const struct in6_addr *a)
>> > +{
>> > +	return __ipv6_addr_jhash(a, gtp_h_initval);
>> > +}
>> 
>> I know you are just following the pattern of the existing "ipv4_hashfn()" here
>> but this kind of stuff is not very global namespace friendly.  Even simply
>> adding a "gtp_" prefix to these hash functions would be a lot better.
> 
> I would agree if this was an inline function defined in a header file or
> a non-static function.  But where is the global namespace concern in
> case of static inline functions defined and used in the same .c file?

The problem is if we create a generic ipv6_hashfn() in linux/ipv6.h or
something like that, then this driver stops building.

Re: [PATCH net-next 07/14] gtp: Support encapsulation of IPv6 packets

[Tom Herbert]

On Tue, Sep 19, 2017 at 10:42 AM, David Miller <davem@davemloft.net> wrote:
> From: Harald Welte <laforge@gnumonks.org>
> Date: Tue, 19 Sep 2017 20:12:45 +0800
>
>> Hi Dave,
>>
>> On Mon, Sep 18, 2017 at 09:19:08PM -0700, David Miller wrote:
>>
>>> > +static inline u32 ipv6_hashfn(const struct in6_addr *a)
>>> > +{
>>> > +  return __ipv6_addr_jhash(a, gtp_h_initval);
>>> > +}
>>>
>>> I know you are just following the pattern of the existing "ipv4_hashfn()" here
>>> but this kind of stuff is not very global namespace friendly.  Even simply
>>> adding a "gtp_" prefix to these hash functions would be a lot better.
>>
>> I would agree if this was an inline function defined in a header file or
>> a non-static function.  But where is the global namespace concern in
>> case of static inline functions defined and used in the same .c file?
>
> The problem is if we create a generic ipv6_hashfn() in linux/ipv6.h or
> something like that, then this driver stops building.

It was a carry over since ipv4_hashfn was already defined in the file.
I will prefix both functions.

Re: [PATCH net-next 07/14] gtp: Support encapsulation of IPv6 packets

[Harald Welte]

On Mon, Sep 18, 2017 at 05:38:57PM -0700, Tom Herbert wrote:
> Allow IPv6 mobile subscriber packets. This entails adding an IPv6 mobile
> subscriber address to pdp context and IPv6 specific variants to find pdp
> contexts by address.

Please note that there are three different PDP contexts for IP:
* IPv4 only (what gtp.c implements so far)
* IPv6 only
* dual IPv4+IPv6 (called IPv46)

This information will have to be provisioned by the control plane
via netlink for each PDP context.  The kernel module then needs to
make sure that on a v4-only context no IPv6 packets are accepted
and vice-versa.

Your proposed patch is missing this kind of screening function and
I would imagine it could introduce all kinds of security problems :/

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

[PATCH net-next 08/14] gtp: Support encpasulating over IPv6

[Tom Herbert]

Allow peers to be specified by IPv6 addresses.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c            | 198 +++++++++++++++++++++++++++++++++----------
 include/uapi/linux/gtp.h     |   1 +
 include/uapi/linux/if_link.h |   3 +
 3 files changed, 158 insertions(+), 44 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 62c0c968efa6..121b41e7a901 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -28,6 +28,7 @@
 #include <net/net_namespace.h>
 #include <net/protocol.h>
 #include <net/ip.h>
+#include <net/ip6_tunnel.h>
 #include <net/udp.h>
 #include <net/udp_tunnel.h>
 #include <net/icmp.h>
@@ -61,7 +62,11 @@ struct pdp_ctx {
 		struct in6_addr	ms_addr_ip6;
 	};
 
-	struct in_addr		peer_addr_ip4;
+	u16			peer_af;
+	union {
+		struct in_addr	peer_addr_ip4;
+		struct in6_addr	peer_addr_ip6;
+	};
 
 	struct sock		*sk;
 	struct net_device       *dev;
@@ -76,6 +81,8 @@ struct pdp_ctx {
 struct gtp_dev {
 	struct list_head	list;
 
+	unsigned int		is_ipv6:1;
+
 	struct sock		*sk0;
 	struct sock		*sk1u;
 
@@ -515,8 +522,6 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 		    struct pdp_ctx *pctx)
 {
 	struct sock *sk = pctx->sk;
-	__be32 saddr = inet_sk(sk)->inet_saddr;
-	struct rtable *rt;
 	int err = 0;
 
 	/* Ensure there is sufficient headroom. */
@@ -526,28 +531,63 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 
 	skb_reset_inner_headers(skb);
 
-	rt = ip_tunnel_get_route(dev, skb, sk->sk_protocol,
-				 sk->sk_bound_dev_if, RT_CONN_FLAGS(sk),
-				 pctx->peer_addr_ip4.s_addr, &saddr,
-				 pctx->gtp_port, pctx->gtp_port,
-				 &pctx->dst_cache, NULL);
+	if (pctx->peer_af == AF_INET) {
+		__be32 saddr = inet_sk(sk)->inet_saddr;
+		struct rtable *rt;
 
-	if (IS_ERR(rt)) {
-		err = PTR_ERR(rt);
-		goto out_err;
-	}
+		rt = ip_tunnel_get_route(dev, skb, sk->sk_protocol,
+					 sk->sk_bound_dev_if, RT_CONN_FLAGS(sk),
+					 pctx->peer_addr_ip4.s_addr, &saddr,
+					 pctx->gtp_port, pctx->gtp_port,
+					 &pctx->dst_cache, NULL);
+
+		if (IS_ERR(rt)) {
+			err = PTR_ERR(rt);
+			goto out_err;
+		}
+
+		skb_dst_drop(skb);
+
+		gtp_push_header(skb, pctx);
+		udp_tunnel_xmit_skb(rt, sk, skb, saddr,
+				    pctx->peer_addr_ip4.s_addr,
+				    0, ip4_dst_hoplimit(&rt->dst), 0,
+				    pctx->gtp_port, pctx->gtp_port,
+				    false, false);
 
-	skb_dst_drop(skb);
+		netdev_dbg(dev, "gtp -> IP src: %pI4 dst: %pI4\n",
+			   &saddr, &pctx->peer_addr_ip4.s_addr);
 
-	gtp_push_header(skb, pctx);
-	udp_tunnel_xmit_skb(rt, sk, skb, saddr,
-			    pctx->peer_addr_ip4.s_addr,
-			    0, ip4_dst_hoplimit(&rt->dst), 0,
-			    pctx->gtp_port, pctx->gtp_port,
-			    false, false);
+#if IS_ENABLED(CONFIG_IPV6)
+	} else if (pctx->peer_af == AF_INET6) {
+		struct in6_addr saddr = inet6_sk(sk)->saddr;
+		struct dst_entry *dst;
 
-	netdev_dbg(dev, "gtp -> IP src: %pI4 dst: %pI4\n",
-		   &saddr, &pctx->peer_addr_ip4.s_addr);
+		dst = ip6_tnl_get_route(dev, skb, sk, sk->sk_protocol,
+					sk->sk_bound_dev_if, 0,
+					0, &pctx->peer_addr_ip6, &saddr,
+					pctx->gtp_port, pctx->gtp_port,
+					&pctx->dst_cache, NULL);
+
+		if (IS_ERR(dst)) {
+			err = PTR_ERR(dst);
+			goto out_err;
+		}
+
+		skb_dst_drop(skb);
+
+		gtp_push_header(skb, pctx);
+		udp_tunnel6_xmit_skb(dst, sk, skb, dev,
+				     &saddr, &pctx->peer_addr_ip6,
+				     0, ip6_dst_hoplimit(dst), 0,
+				     pctx->gtp_port, pctx->gtp_port,
+				     true);
+
+		netdev_dbg(dev, "gtp -> IP src: %pI6 dst: %pI6\n",
+			   &saddr, &pctx->peer_addr_ip6);
+
+#endif
+	}
 
 	return 0;
 
@@ -652,7 +692,8 @@ static void gtp_link_setup(struct net_device *dev)
 
 	/* Assume largest header, ie. GTPv0. */
 	dev->needed_headroom	= LL_MAX_HEADER +
-				  sizeof(struct iphdr) +
+				  max_t(int, sizeof(struct iphdr),
+					sizeof(struct ipv6hdr)) +
 				  sizeof(struct udphdr) +
 				  sizeof(struct gtp0_header);
 
@@ -661,12 +702,15 @@ static void gtp_link_setup(struct net_device *dev)
 
 static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize);
 static void gtp_hashtable_free(struct gtp_dev *gtp);
-static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[]);
+static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[],
+			    bool is_ipv6);
 
 static int gtp_newlink(struct net *src_net, struct net_device *dev,
 		       struct nlattr *tb[], struct nlattr *data[],
 		       struct netlink_ext_ack *extack)
 {
+	unsigned int role = GTP_ROLE_GGSN;
+	bool is_ipv6 = false;
 	struct gtp_dev *gtp;
 	struct gtp_net *gn;
 	int hashsize, err;
@@ -674,9 +718,30 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,
 	if (!data[IFLA_GTP_FD0] && !data[IFLA_GTP_FD1])
 		return -EINVAL;
 
+	if (data[IFLA_GTP_ROLE]) {
+		role = nla_get_u32(data[IFLA_GTP_ROLE]);
+		if (role > GTP_ROLE_SGSN)
+			return -EINVAL;
+	}
+
+	if (data[IFLA_GTP_AF]) {
+		u16 af = nla_get_u16(data[IFLA_GTP_AF]);
+
+		switch (af) {
+		case AF_INET:
+			is_ipv6 = false;
+			break;
+		case AF_INET6:
+			is_ipv6 = true;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
 	gtp = netdev_priv(dev);
 
-	err = gtp_encap_enable(gtp, data);
+	err = gtp_encap_enable(gtp, data, is_ipv6);
 	if (err < 0)
 		return err;
 
@@ -695,6 +760,9 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,
 		goto out_hashtable;
 	}
 
+	gtp->role = role;
+	gtp->is_ipv6 = is_ipv6;
+
 	gn = net_generic(dev_net(dev), gtp_net_id);
 	list_add_rcu(&gtp->list, &gn->gtp_dev_list);
 
@@ -816,7 +884,8 @@ static void gtp_hashtable_free(struct gtp_dev *gtp)
 }
 
 static struct sock *gtp_encap_enable_socket(int fd, int type,
-					    struct gtp_dev *gtp)
+					    struct gtp_dev *gtp,
+					    bool is_ipv6)
 {
 	struct udp_tunnel_sock_cfg tuncfg = {NULL};
 	struct socket *sock;
@@ -837,6 +906,12 @@ static struct sock *gtp_encap_enable_socket(int fd, int type,
 		goto out_sock;
 	}
 
+	if (sock->sk->sk_family != (is_ipv6 ? AF_INET6 : AF_INET)) {
+		pr_debug("socket fd=%d not right family\n", fd);
+		sk = ERR_PTR(-EINVAL);
+		goto out_sock;
+	}
+
 	if (rcu_dereference_sk_user_data(sock->sk)) {
 		sk = ERR_PTR(-EBUSY);
 		goto out_sock;
@@ -869,16 +944,16 @@ static struct sock *gtp_encap_enable_socket(int fd, int type,
 	return sk;
 }
 
-static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[])
+static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[],
+			    bool is_ipv6)
 {
-	struct sock *sk1u = NULL;
-	struct sock *sk0 = NULL;
-	unsigned int role = GTP_ROLE_GGSN;
+	struct sock *sk0 = NULL, *sk1u = NULL;
 
 	if (data[IFLA_GTP_FD0]) {
 		u32 fd0 = nla_get_u32(data[IFLA_GTP_FD0]);
 
-		sk0 = gtp_encap_enable_socket(fd0, UDP_ENCAP_GTP0, gtp);
+		sk0 = gtp_encap_enable_socket(fd0, UDP_ENCAP_GTP0, gtp,
+					      is_ipv6);
 		if (IS_ERR(sk0))
 			return PTR_ERR(sk0);
 	}
@@ -886,7 +961,8 @@ static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[])
 	if (data[IFLA_GTP_FD1]) {
 		u32 fd1 = nla_get_u32(data[IFLA_GTP_FD1]);
 
-		sk1u = gtp_encap_enable_socket(fd1, UDP_ENCAP_GTP1U, gtp);
+		sk1u = gtp_encap_enable_socket(fd1, UDP_ENCAP_GTP1U, gtp,
+					       is_ipv6);
 		if (IS_ERR(sk1u)) {
 			if (sk0)
 				gtp_encap_disable_sock(sk0);
@@ -894,15 +970,8 @@ static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[])
 		}
 	}
 
-	if (data[IFLA_GTP_ROLE]) {
-		role = nla_get_u32(data[IFLA_GTP_ROLE]);
-		if (role > GTP_ROLE_SGSN)
-			return -EINVAL;
-	}
-
 	gtp->sk0 = sk0;
 	gtp->sk1u = sk1u;
-	gtp->role = role;
 
 	return 0;
 }
@@ -938,8 +1007,16 @@ static void pdp_fill(struct pdp_ctx *pctx, struct genl_info *info)
 	__be16 default_port = 0;
 
 	pctx->gtp_version = nla_get_u32(info->attrs[GTPA_VERSION]);
-	pctx->peer_addr_ip4.s_addr =
-		nla_get_be32(info->attrs[GTPA_PEER_ADDRESS]);
+
+	if (info->attrs[GTPA_PEER_ADDRESS]) {
+		pctx->peer_af = AF_INET;
+		pctx->peer_addr_ip4.s_addr =
+			nla_get_in_addr(info->attrs[GTPA_PEER_ADDRESS]);
+	} else if (info->attrs[GTPA_PEER6_ADDRESS]) {
+		pctx->peer_af = AF_INET6;
+		pctx->peer_addr_ip6 = nla_get_in6_addr(
+					info->attrs[GTPA_PEER6_ADDRESS]);
+	}
 
 	switch (pctx->gtp_version) {
 	case GTP_V0:
@@ -1101,9 +1178,15 @@ static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info)
 	int err;
 
 	if (!info->attrs[GTPA_VERSION] ||
-	   !info->attrs[GTPA_LINK] ||
-	   !info->attrs[GTPA_PEER_ADDRESS])
+	    !info->attrs[GTPA_LINK])
+		return -EINVAL;
+
+	if (!(!!info->attrs[GTPA_PEER_ADDRESS] ^
+	      !!info->attrs[GTPA_PEER6_ADDRESS])) {
+		/* Either v4 or v6 peer address must be set */
+
 		return -EINVAL;
+	}
 
 	if (!(!!info->attrs[GTPA_MS_ADDRESS] ^
 	      !!info->attrs[GTPA_MS6_ADDRESS])) {
@@ -1138,6 +1221,12 @@ static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info)
 		goto out_unlock;
 	}
 
+	if ((info->attrs[GTPA_PEER_ADDRESS] && gtp->is_ipv6) ||
+	    (info->attrs[GTPA_PEER6_ADDRESS] && !gtp->is_ipv6)) {
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
 	if (version == GTP_V0)
 		sk = gtp->sk0;
 	else if (version == GTP_V1)
@@ -1244,9 +1333,28 @@ static int gtp_genl_fill_info(struct sk_buff *skb, u32 snd_portid, u32 snd_seq,
 	if (genlh == NULL)
 		goto nlmsg_failure;
 
-	if (nla_put_u32(skb, GTPA_VERSION, pctx->gtp_version) ||
-	    nla_put_be32(skb, GTPA_PEER_ADDRESS, pctx->peer_addr_ip4.s_addr))
+	if (nla_put_u32(skb, GTPA_VERSION, pctx->gtp_version))
+		goto nla_put_failure;
+
+	if (nla_put_u32(skb, GTPA_LINK, pctx->dev->ifindex))
+		goto nla_put_failure;
+
+	switch (pctx->peer_af) {
+	case AF_INET:
+		if (nla_put_be32(skb, GTPA_PEER_ADDRESS,
+				 pctx->peer_addr_ip4.s_addr))
+			goto nla_put_failure;
+
+		break;
+	case AF_INET6:
+		if (nla_put_in6_addr(skb, GTPA_PEER6_ADDRESS,
+				     &pctx->peer_addr_ip6))
+			goto nla_put_failure;
+
+		break;
+	default:
 		goto nla_put_failure;
+	}
 
 	switch (pctx->ms_af) {
 	case AF_INET:
@@ -1373,6 +1481,8 @@ static struct nla_policy gtp_genl_policy[GTPA_MAX + 1] = {
 	[GTPA_VERSION]		= { .type = NLA_U32, },
 	[GTPA_TID]		= { .type = NLA_U64, },
 	[GTPA_PEER_ADDRESS]	= { .type = NLA_U32, },
+	[GTPA_PEER6_ADDRESS]	= { .len = FIELD_SIZEOF(struct ipv6hdr,
+							daddr) },
 	[GTPA_MS_ADDRESS]	= { .type = NLA_U32, },
 	[GTPA_MS6_ADDRESS]	= { .len = FIELD_SIZEOF(struct ipv6hdr,
 							daddr) },
diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
index ae4e632c0360..8eec519fa754 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -29,6 +29,7 @@ enum gtp_attrs {
 	GTPA_PAD,
 	GTPA_PORT,
 	GTPA_MS6_ADDRESS,
+	GTPA_PEER6_ADDRESS,
 	__GTPA_MAX,
 };
 #define GTPA_MAX (__GTPA_MAX + 1)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 8d062c58d5cb..81c26864abeb 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -552,6 +552,9 @@ enum {
 	IFLA_GTP_FD1,
 	IFLA_GTP_PDP_HASHSIZE,
 	IFLA_GTP_ROLE,
+	IFLA_GTP_AF,
+	IFLA_GTP_PORT0,
+	IFLA_GTP_PORT1,
 	__IFLA_GTP_MAX,
 };
 #define IFLA_GTP_MAX (__IFLA_GTP_MAX - 1)
-- 
2.11.0

Re: [PATCH net-next 08/14] gtp: Support encpasulating over IPv6

[David Miller]

From: Tom Herbert <tom@quantonium.net>
Date: Mon, 18 Sep 2017 17:38:58 -0700

> Allow peers to be specified by IPv6 addresses.
> 
> Signed-off-by: Tom Herbert <tom@quantonium.net>

Hmmm, can you just check the socket family or something like that?

Re: [PATCH net-next 08/14] gtp: Support encpasulating over IPv6

[Tom Herbert]

On Mon, Sep 18, 2017 at 9:19 PM, David Miller <davem@davemloft.net> wrote:
> From: Tom Herbert <tom@quantonium.net>
> Date: Mon, 18 Sep 2017 17:38:58 -0700
>
>> Allow peers to be specified by IPv6 addresses.
>>
>> Signed-off-by: Tom Herbert <tom@quantonium.net>
>
> Hmmm, can you just check the socket family or something like that?

I'm not sure what code you're referring to.

Thanks

Re: [PATCH net-next 08/14] gtp: Support encpasulating over IPv6

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Wed, 20 Sep 2017 11:03:52 -0700

> On Mon, Sep 18, 2017 at 9:19 PM, David Miller <davem@davemloft.net> wrote:
>> From: Tom Herbert <tom@quantonium.net>
>> Date: Mon, 18 Sep 2017 17:38:58 -0700
>>
>>> Allow peers to be specified by IPv6 addresses.
>>>
>>> Signed-off-by: Tom Herbert <tom@quantonium.net>
>>
>> Hmmm, can you just check the socket family or something like that?
> 
> I'm not sure what code you're referring to.

There is a socket associated with the tunnel to do the encapsulation
and it has an address family, right?

Re: [PATCH net-next 08/14] gtp: Support encpasulating over IPv6

[Tom Herbert]

On Wed, Sep 20, 2017 at 12:45 PM, David Miller <davem@davemloft.net> wrote:
> From: Tom Herbert <tom@herbertland.com>
> Date: Wed, 20 Sep 2017 11:03:52 -0700
>
>> On Mon, Sep 18, 2017 at 9:19 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Tom Herbert <tom@quantonium.net>
>>> Date: Mon, 18 Sep 2017 17:38:58 -0700
>>>
>>>> Allow peers to be specified by IPv6 addresses.
>>>>
>>>> Signed-off-by: Tom Herbert <tom@quantonium.net>
>>>
>>> Hmmm, can you just check the socket family or something like that?
>>
>> I'm not sure what code you're referring to.
>
> There is a socket associated with the tunnel to do the encapsulation
> and it has an address family, right?

If fd's are set from userspace for the sockets then we could derive
the address family from them. I'll change that. Although, looking at
now I am wondering why were passing fds into GTP instead of just
having the kernel create the UDP port like is done for other encaps.

Tom

Re: [PATCH net-next 08/14] gtp: Support encpasulating over IPv6

[Harald Welte]

Hi Tom,

On Wed, Sep 20, 2017 at 01:40:54PM -0700, Tom Herbert wrote:
> On Wed, Sep 20, 2017 at 12:45 PM, David Miller <davem@davemloft.net> wrote:
> > There is a socket associated with the tunnel to do the encapsulation
> > and it has an address family, right?
> 
> If fd's are set from userspace for the sockets then we could derive
> the address family from them. I'll change that. Although, looking at
> now I am wondering why were passing fds into GTP instead of just
> having the kernel create the UDP port like is done for other encaps.

because the userspace process has to take care of those bits of GTP-U
that the kernel doesn't, such as responding to GTP ECHO requests with
GTP echo responses.  Only the "GTP Message type G-PDU" is handled in the
kernel, as only those frames contain user plane.  See table 1 of Section
7.1 of 3GPP TS 29.060.

If you create the socket in the kernel, how would you hand the socket to
the userspace process later on?

IMHO, it feels more natural to simply create it in userspace (like you
would do in the non-kernel-accelerated case) and then simply handle the
G-PDU messages in the kernel while doing the rest in userspace.

But if there's another method that feels more usual to the kernel
community, I'm not against any changes - but given kernel policies, we'd
have to keep userspace compatbility, right?

Regards,
	Harald
-- 
- 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)

Re: [PATCH net-next 08/14] gtp: Support encpasulating over IPv6

[Tom Herbert]

On Wed, Sep 20, 2017 at 5:04 PM, Harald Welte <laforge@gnumonks.org> wrote:
> Hi Tom,
>
> On Wed, Sep 20, 2017 at 01:40:54PM -0700, Tom Herbert wrote:
>> On Wed, Sep 20, 2017 at 12:45 PM, David Miller <davem@davemloft.net> wrote:
>> > There is a socket associated with the tunnel to do the encapsulation
>> > and it has an address family, right?
>>
>> If fd's are set from userspace for the sockets then we could derive
>> the address family from them. I'll change that. Although, looking at
>> now I am wondering why were passing fds into GTP instead of just
>> having the kernel create the UDP port like is done for other encaps.
>
> because the userspace process has to take care of those bits of GTP-U
> that the kernel doesn't, such as responding to GTP ECHO requests with
> GTP echo responses.  Only the "GTP Message type G-PDU" is handled in the
> kernel, as only those frames contain user plane.  See table 1 of Section
> 7.1 of 3GPP TS 29.060.
>
Okay, thanks for the explanation.

Tom

Re: [PATCH net-next 08/14] gtp: Support encpasulating over IPv6

[Harald Welte]

Hi Tom,

On Mon, Sep 18, 2017 at 05:38:58PM -0700, Tom Herbert wrote:
> Allow peers to be specified by IPv6 addresses.

> +	u16			peer_af;
> +	union {
> +		struct in_addr	peer_addr_ip4;
> +		struct in6_addr	peer_addr_ip6;
> +	};

this will not really work, as an union means that a PDP context
will be either IPv4-only or IPV6-only, while in reality there
are three types, see my other mail.  So you have to  deal
with v4-only, v6-only or v4v6.

The v6-only is legacy by now, and all modern phones I've tested in
recent years can do v4v6 rather than having a v4-only and a v6-only
PDP context in parallel.

>From the operator point of view, v4v6 is very desirable, as it basically
halves the amount of PDP contexts compared to the old approach, which
significantly reduces signalling load across your network, as well as
the amount of memory (and thus capacity) in your core network elements.

I've recently implemented v6 + v4v6 support in osmo-ggsn (see
http://git.osmocom.org/osmo-ggsn/) in case you would like to see another
FOSS implementation for v6 + v4v6 - though in userspace, of course.

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

[PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Tom Herbert]

Add new configuration of GTP interfaces that allow specifying a port to
listen on (as opposed to having to get sockets from a userspace control
plane). This allows GTP interfaces to be configured and the data path
tested without requiring a GTP-C daemon.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c        | 212 +++++++++++++++++++++++++++++++++++------------
 include/uapi/linux/gtp.h |   5 ++
 2 files changed, 166 insertions(+), 51 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 121b41e7a901..1870469a4982 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -86,6 +86,9 @@ struct gtp_dev {
 	struct sock		*sk0;
 	struct sock		*sk1u;
 
+	struct socket		*sock0;
+	struct socket		*sock1u;
+
 	struct net_device	*dev;
 
 	unsigned int		role;
@@ -430,26 +433,33 @@ static void gtp_encap_destroy(struct sock *sk)
 	}
 }
 
-static void gtp_encap_disable_sock(struct sock *sk)
+static void gtp_encap_release(struct gtp_dev *gtp)
 {
-	if (!sk)
-		return;
+	if (gtp->sk0) {
+		if (gtp->sock0) {
+			udp_tunnel_sock_release(gtp->sock0);
+			gtp->sock0 = NULL;
+		} else {
+			gtp_encap_destroy(gtp->sk0);
+		}
 
-	gtp_encap_destroy(sk);
-}
+		gtp->sk0 = NULL;
+	}
 
-static void gtp_encap_disable(struct gtp_dev *gtp)
-{
-	gtp_encap_disable_sock(gtp->sk0);
-	gtp_encap_disable_sock(gtp->sk1u);
+	if (gtp->sk1u) {
+		if (gtp->sock1u) {
+			udp_tunnel_sock_release(gtp->sock1u);
+			gtp->sock1u = NULL;
+		} else {
+			gtp_encap_destroy(gtp->sk1u);
+		}
+
+		gtp->sk1u = NULL;
+	}
 }
 
 static int gtp_dev_init(struct net_device *dev)
 {
-	struct gtp_dev *gtp = netdev_priv(dev);
-
-	gtp->dev = dev;
-
 	dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
 	if (!dev->tstats)
 		return -ENOMEM;
@@ -461,7 +471,8 @@ static void gtp_dev_uninit(struct net_device *dev)
 {
 	struct gtp_dev *gtp = netdev_priv(dev);
 
-	gtp_encap_disable(gtp);
+	gtp_encap_release(gtp);
+
 	free_percpu(dev->tstats);
 }
 
@@ -676,6 +687,7 @@ static const struct net_device_ops gtp_netdev_ops = {
 
 static void gtp_link_setup(struct net_device *dev)
 {
+	struct gtp_dev *gtp = netdev_priv(dev);
 	dev->netdev_ops		= &gtp_netdev_ops;
 	dev->needs_free_netdev	= true;
 
@@ -697,6 +709,8 @@ static void gtp_link_setup(struct net_device *dev)
 				  sizeof(struct udphdr) +
 				  sizeof(struct gtp0_header);
 
+	gtp->dev = dev;
+
 	gro_cells_init(&gtp->gro_cells, dev);
 }
 
@@ -710,13 +724,19 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,
 		       struct netlink_ext_ack *extack)
 {
 	unsigned int role = GTP_ROLE_GGSN;
+	bool have_fd, have_ports;
 	bool is_ipv6 = false;
 	struct gtp_dev *gtp;
 	struct gtp_net *gn;
 	int hashsize, err;
 
-	if (!data[IFLA_GTP_FD0] && !data[IFLA_GTP_FD1])
+	have_fd = !!data[IFLA_GTP_FD0] || !!data[IFLA_GTP_FD1];
+	have_ports = !!data[IFLA_GTP_PORT0] || !!data[IFLA_GTP_PORT1];
+
+	if (!(have_fd ^ have_ports)) {
+		/* Either got fd(s) or port(s) */
 		return -EINVAL;
+	}
 
 	if (data[IFLA_GTP_ROLE]) {
 		role = nla_get_u32(data[IFLA_GTP_ROLE]);
@@ -773,7 +793,7 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,
 out_hashtable:
 	gtp_hashtable_free(gtp);
 out_encap:
-	gtp_encap_disable(gtp);
+	gtp_encap_release(gtp);
 	return err;
 }
 
@@ -782,7 +802,7 @@ static void gtp_dellink(struct net_device *dev, struct list_head *head)
 	struct gtp_dev *gtp = netdev_priv(dev);
 
 	gro_cells_destroy(&gtp->gro_cells);
-	gtp_encap_disable(gtp);
+	gtp_encap_release(gtp);
 	gtp_hashtable_free(gtp);
 	list_del_rcu(&gtp->list);
 	unregister_netdevice_queue(dev, head);
@@ -793,6 +813,8 @@ static const struct nla_policy gtp_policy[IFLA_GTP_MAX + 1] = {
 	[IFLA_GTP_FD1]			= { .type = NLA_U32 },
 	[IFLA_GTP_PDP_HASHSIZE]		= { .type = NLA_U32 },
 	[IFLA_GTP_ROLE]			= { .type = NLA_U32 },
+	[IFLA_GTP_PORT0]		= { .type = NLA_U16 },
+	[IFLA_GTP_PORT1]		= { .type = NLA_U16 },
 };
 
 static int gtp_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -883,11 +905,35 @@ static void gtp_hashtable_free(struct gtp_dev *gtp)
 	kfree(gtp->tid_hash);
 }
 
-static struct sock *gtp_encap_enable_socket(int fd, int type,
-					    struct gtp_dev *gtp,
-					    bool is_ipv6)
+static int gtp_encap_enable_sock(struct socket *sock, int type,
+				 struct gtp_dev *gtp)
 {
 	struct udp_tunnel_sock_cfg tuncfg = {NULL};
+
+	switch (type) {
+	case UDP_ENCAP_GTP0:
+		tuncfg.encap_rcv = gtp0_udp_encap_recv;
+		break;
+	case UDP_ENCAP_GTP1U:
+		tuncfg.encap_rcv = gtp1u_udp_encap_recv;
+		break;
+	default:
+		pr_debug("Unknown encap type %u\n", type);
+		return -EINVAL;
+	}
+
+	tuncfg.sk_user_data = gtp;
+	tuncfg.encap_type = type;
+	tuncfg.encap_destroy = gtp_encap_destroy;
+
+	setup_udp_tunnel_sock(sock_net(sock->sk), sock, &tuncfg);
+
+	return 0;
+}
+
+static struct sock *gtp_encap_enable_fd(int fd, int type, struct gtp_dev *gtp,
+					bool is_ipv6)
+{
 	struct socket *sock;
 	struct sock *sk;
 	int err;
@@ -920,60 +966,124 @@ static struct sock *gtp_encap_enable_socket(int fd, int type,
 	sk = sock->sk;
 	sock_hold(sk);
 
-	switch (type) {
-	case UDP_ENCAP_GTP0:
-		tuncfg.encap_rcv = gtp0_udp_encap_recv;
-		break;
-	case UDP_ENCAP_GTP1U:
-		tuncfg.encap_rcv = gtp1u_udp_encap_recv;
-		break;
-	default:
-		pr_debug("Unknown encap type %u\n", type);
-		sk = ERR_PTR(-EINVAL);
-		goto out_sock;
-	}
-
-	tuncfg.sk_user_data = gtp;
-	tuncfg.encap_type = type;
-	tuncfg.encap_destroy = gtp_encap_destroy;
-
-	setup_udp_tunnel_sock(sock_net(sock->sk), sock, &tuncfg);
+	err = gtp_encap_enable_sock(sock, type, gtp);
+	if (err < 0)
+		sk = ERR_PTR(err);
 
 out_sock:
 	sockfd_put(sock);
 	return sk;
 }
 
+static struct socket *gtp_create_sock(struct net *net, bool ipv6,
+				      __be16 port, u32 flags)
+{
+	struct socket *sock;
+	struct udp_port_cfg udp_conf;
+	int err;
+
+	memset(&udp_conf, 0, sizeof(udp_conf));
+
+	if (ipv6) {
+		udp_conf.family = AF_INET6;
+		udp_conf.ipv6_v6only = 1;
+	} else {
+		udp_conf.family = AF_INET;
+	}
+
+	udp_conf.local_udp_port = port;
+
+	/* Open UDP socket */
+	err = udp_sock_create(net, &udp_conf, &sock);
+	if (err)
+		return ERR_PTR(err);
+
+	return sock;
+}
+
 static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[],
 			    bool is_ipv6)
 {
+	int err;
+
+	struct socket *sock0 = NULL, *sock1u = NULL;
 	struct sock *sk0 = NULL, *sk1u = NULL;
 
 	if (data[IFLA_GTP_FD0]) {
 		u32 fd0 = nla_get_u32(data[IFLA_GTP_FD0]);
 
-		sk0 = gtp_encap_enable_socket(fd0, UDP_ENCAP_GTP0, gtp,
-					      is_ipv6);
-		if (IS_ERR(sk0))
-			return PTR_ERR(sk0);
+		sk0 = gtp_encap_enable_fd(fd0, UDP_ENCAP_GTP0, gtp, is_ipv6);
+		if (IS_ERR(sk0)) {
+			err = PTR_ERR(sk0);
+			sk0 = NULL;
+			goto out_err;
+		}
+	} else if (data[IFLA_GTP_PORT0]) {
+		__be16 port = nla_get_u16(data[IFLA_GTP_PORT0]);
+
+		sock0 = gtp_create_sock(dev_net(gtp->dev), is_ipv6, port, 0);
+		if (IS_ERR(sock0)) {
+			err = PTR_ERR(sock0);
+			sock0 = NULL;
+			goto out_err;
+		}
+
+		err = gtp_encap_enable_sock(sock0, UDP_ENCAP_GTP0, gtp);
+		if (err)
+			goto out_err;
 	}
 
 	if (data[IFLA_GTP_FD1]) {
 		u32 fd1 = nla_get_u32(data[IFLA_GTP_FD1]);
 
-		sk1u = gtp_encap_enable_socket(fd1, UDP_ENCAP_GTP1U, gtp,
-					       is_ipv6);
+		sk1u = gtp_encap_enable_fd(fd1, UDP_ENCAP_GTP1U, gtp, is_ipv6);
 		if (IS_ERR(sk1u)) {
-			if (sk0)
-				gtp_encap_disable_sock(sk0);
-			return PTR_ERR(sk1u);
+			err = PTR_ERR(sk1u);
+			sk1u = NULL;
+			goto out_err;
+		}
+	} else if (data[IFLA_GTP_PORT1]) {
+		__be16 port = nla_get_u16(data[IFLA_GTP_PORT1]);
+
+		sock1u = gtp_create_sock(dev_net(gtp->dev), is_ipv6, port, 0);
+		if (IS_ERR(sock1u)) {
+			err = PTR_ERR(sock1u);
+			sock1u = NULL;
+			goto out_err;
 		}
+
+		err = gtp_encap_enable_sock(sock1u, UDP_ENCAP_GTP1U, gtp);
+		if (err)
+			goto out_err;
+	}
+
+	if (sock0) {
+		gtp->sock0 = sock0;
+		gtp->sk0 = sock0->sk;
+	} else {
+		gtp->sk0 = sk0;
 	}
 
-	gtp->sk0 = sk0;
-	gtp->sk1u = sk1u;
+	if (sock1u) {
+		gtp->sock1u = sock1u;
+		gtp->sk1u = sock1u->sk;
+	} else {
+		gtp->sk1u = sk1u;
+	}
 
 	return 0;
+
+out_err:
+	if (sk0)
+		gtp_encap_destroy(sk0);
+	if (sk1u)
+		gtp_encap_destroy(sk1u);
+	if (sock0)
+		udp_tunnel_sock_release(sock0);
+	if (sock1u)
+		udp_tunnel_sock_release(sock1u);
+
+	return err;
 }
 
 static struct gtp_dev *gtp_find_dev(struct net *src_net, struct nlattr *nla[])
@@ -1515,8 +1625,8 @@ static const struct genl_ops gtp_genl_ops[] = {
 };
 
 static struct genl_family gtp_genl_family __ro_after_init = {
-	.name		= "gtp",
-	.version	= 0,
+	.name		= GTP_GENL_NAME,
+	.version	= GTP_GENL_VERSION,
 	.hdrsize	= 0,
 	.maxattr	= GTPA_MAX,
 	.netnsok	= true,
diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
index 8eec519fa754..0da18aa88be8 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -9,6 +9,11 @@ enum gtp_genl_cmds {
 	GTP_CMD_MAX,
 };
 
+/* NETLINK_GENERIC related info
+ */
+#define GTP_GENL_NAME		"gtp"
+#define GTP_GENL_VERSION	0
+
 enum gtp_version {
 	GTP_V0 = 0,
 	GTP_V1,
-- 
2.11.0

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Andreas Schultz]

On 19/09/17 02:38, Tom Herbert wrote:
> Add new configuration of GTP interfaces that allow specifying a port to
> listen on (as opposed to having to get sockets from a userspace control
> plane). This allows GTP interfaces to be configured and the data path
> tested without requiring a GTP-C daemon.

This would imply that you can have multiple independent GTP sockets on 
the same IP address.That is not permitted by the GTP specifications. 
3GPP TS 29.281, section 4.3 states clearly that there is "only" one GTP 
entity per IP address.A PDP context is defined by the destination IP and 
the TEID. The destination port is not part of the identity of a PDP context.

Even the source IP and source port are not part of the tunnel identity. 
This makes is possible to send traffic from a new SGSN/SGW during 
handover before the control protocol has announced the handover.

At this point the usual response is: THAT IS NOT SAFE. Yes, GTP has been 
designed for cooperative networks only and should not be used on 
hostile/unsecured networks.

On the sending side, using multiple ports is permitted as long as the 
default GTP port is always able to receive incoming messages.

Andreas

[...]

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Tom Herbert]

On Wed, Sep 20, 2017 at 8:27 AM, Andreas Schultz <aschultz@tpip.net> wrote:
> On 19/09/17 02:38, Tom Herbert wrote:
>>
>> Add new configuration of GTP interfaces that allow specifying a port to
>> listen on (as opposed to having to get sockets from a userspace control
>> plane). This allows GTP interfaces to be configured and the data path
>> tested without requiring a GTP-C daemon.
>
>
> This would imply that you can have multiple independent GTP sockets on the
> same IP address.That is not permitted by the GTP specifications. 3GPP TS
> 29.281, section 4.3 states clearly that there is "only" one GTP entity per
> IP address.A PDP context is defined by the destination IP and the TEID. The
> destination port is not part of the identity of a PDP context.
>
We are in no way trying change GTP, if someone runs this in a real GTP
network then they need to abide by the specification. However, there
is nothing inconsistent and it breaks nothing if someone wishes to use
different port numbers in their own private network for testing or
development purposes. Every other UDP application that has assigned
port number allows configurable ports, I don't see that GTP is so
special that it should be an exception.

Tom

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Andreas Schultz]

On 20/09/17 17:57, Tom Herbert wrote:
> On Wed, Sep 20, 2017 at 8:27 AM, Andreas Schultz <aschultz@tpip.net> wrote:
>> On 19/09/17 02:38, Tom Herbert wrote:
>>>
>>> Add new configuration of GTP interfaces that allow specifying a port to
>>> listen on (as opposed to having to get sockets from a userspace control
>>> plane). This allows GTP interfaces to be configured and the data path
>>> tested without requiring a GTP-C daemon.
>>
>>
>> This would imply that you can have multiple independent GTP sockets on the
>> same IP address.That is not permitted by the GTP specifications. 3GPP TS
>> 29.281, section 4.3 states clearly that there is "only" one GTP entity per
>> IP address.A PDP context is defined by the destination IP and the TEID. The
>> destination port is not part of the identity of a PDP context.
>>
> We are in no way trying change GTP, if someone runs this in a real GTP
> network then they need to abide by the specification. However, there
> is nothing inconsistent and it breaks nothing if someone wishes to use
> different port numbers in their own private network for testing or
> development purposes. Every other UDP application that has assigned
> port number allows configurable ports, I don't see that GTP is so
> special that it should be an exception.

GTP isn't special, I just don't like to have testing only features in 
there when the same goal can be reached without having to add extra 
stuff. Adding code that is not going to be useful in real production 
setups (or in this case would even break production setups when enabled 
accidentally) makes the implementation more complex than it needs to be.

You can always add multiple IP's to your test system and have the same 
effect without having to change the ports.

Regards
Andreas

> 
> Tom
> 

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Tom Herbert]

On Wed, Sep 20, 2017 at 9:07 AM, Andreas Schultz <aschultz@tpip.net> wrote:
>
>
> On 20/09/17 17:57, Tom Herbert wrote:
>>
>> On Wed, Sep 20, 2017 at 8:27 AM, Andreas Schultz <aschultz@tpip.net>
>> wrote:
>>>
>>> On 19/09/17 02:38, Tom Herbert wrote:
>>>>
>>>>
>>>> Add new configuration of GTP interfaces that allow specifying a port to
>>>> listen on (as opposed to having to get sockets from a userspace control
>>>> plane). This allows GTP interfaces to be configured and the data path
>>>> tested without requiring a GTP-C daemon.
>>>
>>>
>>>
>>> This would imply that you can have multiple independent GTP sockets on
>>> the
>>> same IP address.That is not permitted by the GTP specifications. 3GPP TS
>>> 29.281, section 4.3 states clearly that there is "only" one GTP entity
>>> per
>>> IP address.A PDP context is defined by the destination IP and the TEID.
>>> The
>>> destination port is not part of the identity of a PDP context.
>>>
>> We are in no way trying change GTP, if someone runs this in a real GTP
>> network then they need to abide by the specification. However, there
>> is nothing inconsistent and it breaks nothing if someone wishes to use
>> different port numbers in their own private network for testing or
>> development purposes. Every other UDP application that has assigned
>> port number allows configurable ports, I don't see that GTP is so
>> special that it should be an exception.
>
>
> GTP isn't special, I just don't like to have testing only features in there
> when the same goal can be reached without having to add extra stuff. Adding
> code that is not going to be useful in real production setups (or in this
> case would even break production setups when enabled accidentally) makes the
> implementation more complex than it needs to be.

Well, you could make the same argument that allowing GTP to configured
as standalone interface is a problem since GTP is only allowed to be
with used with GTP-C. But, then we have something in the kernel that
the community is expected to support, but requires jumping through a
whole bunch of hoops just to run a simple netperf. The more that
patches and features look like other things in the kernel that are
already well established, the better the chances we can accept them
and support them. It's probably a natural consequence of any large
open source project, so sometimes it's worth the effort to add a few
lines of complexity to get the benefits of community contribution and
support.

Tom

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Harald Welte]

Hi Tom,

On Wed, Sep 20, 2017 at 09:24:07AM -0700, Tom Herbert wrote:
> On Wed, Sep 20, 2017 at 9:07 AM, Andreas Schultz <aschultz@tpip.net> wrote:
> > GTP isn't special, I just don't like to have testing only features in there
> > when the same goal can be reached without having to add extra stuff. Adding
> > code that is not going to be useful in real production setups (or in this
> > case would even break production setups when enabled accidentally) makes the
> > implementation more complex than it needs to be.
> 
> Well, you could make the same argument that allowing GTP to configured
> as standalone interface is a problem since GTP is only allowed to be
> with used with GTP-C. But, then we have something in the kernel that
> the community is expected to support, but requires jumping through a
> whole bunch of hoops just to run a simple netperf. 

"A whole bunch of hoops" without your new interface would consist of
running a single command-line program that is supplied with libgtpnl.
This is not a complete 3GPP network, but a simple libmnl-based helper
library with no other depenencies.

I'm not neccessarily against introducing features like the 'standalone
interface configuration'.  However, we must make sure that any
significant new feature contributions like IPv6 are tested in a
"realistic setup" and not just using those 'interfaces added for easy
development'.  Also, I would argue those 'interfaces added for easy
deveopment/benchmarking' should probably be clearly marked as such to
avoid raising the impression that this is what leads to a
standard-conforming / production-type setup.

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

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Tom Herbert]

On Wed, Sep 20, 2017 at 5:13 PM, Harald Welte <laforge@gnumonks.org> wrote:
> Hi Tom,
>
> On Wed, Sep 20, 2017 at 09:24:07AM -0700, Tom Herbert wrote:
>> On Wed, Sep 20, 2017 at 9:07 AM, Andreas Schultz <aschultz@tpip.net> wrote:
>> > GTP isn't special, I just don't like to have testing only features in there
>> > when the same goal can be reached without having to add extra stuff. Adding
>> > code that is not going to be useful in real production setups (or in this
>> > case would even break production setups when enabled accidentally) makes the
>> > implementation more complex than it needs to be.
>>
>> Well, you could make the same argument that allowing GTP to configured
>> as standalone interface is a problem since GTP is only allowed to be
>> with used with GTP-C. But, then we have something in the kernel that
>> the community is expected to support, but requires jumping through a
>> whole bunch of hoops just to run a simple netperf.
>
> "A whole bunch of hoops" without your new interface would consist of
> running a single command-line program that is supplied with libgtpnl.
> This is not a complete 3GPP network, but a simple libmnl-based helper
> library with no other depenencies.
>
You have the point of view of someone who has a lot of experience
dealing with this protocol. Try to imagine if you were some random
kernel network programmer with no experience in the area. If they
happen to find a one-off bug and want to do the right thing by running
a test, you want to make that as easy as possible. From that
perspective, building protocol specific libraries and finding the
right cmd line to run is significant hoops (I can attest to this).
There are other examples in the kernel of systems bigger than GTP that
require a whole lot of effort just to run a simple test; you'll notice
for those it's rare that best developers ever bother to look at them
unless they're making a global change that affects the code. We don't
want GTP to take be like that!

> I'm not neccessarily against introducing features like the 'standalone
> interface configuration'.  However, we must make sure that any
> significant new feature contributions like IPv6 are tested in a
> "realistic setup" and not just using those 'interfaces added for easy
> development'.  Also, I would argue those 'interfaces added for easy
> deveopment/benchmarking' should probably be clearly marked as such to
> avoid raising the impression that this is what leads to a
> standard-conforming / production-type setup.
>
Given the obvious complexity of running a real GTP stack, I don't
think we have to worry about this. In order to test a "realistic
setup" a whole bunch of other support is needed. So the forward
looking question now is how to get to be able to run a "realistic
setup"?

Tom

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Harald Welte]

Hi Tom,

On Wed, Sep 20, 2017 at 05:55:01PM -0700, Tom Herbert wrote:
> You have the point of view of someone who has a lot of experience
> dealing with this protocol. Try to imagine if you were some random
> kernel network programmer with no experience in the area. If they
> happen to find a one-off bug and want to do the right thing by running
> a test, you want to make that as easy as possible. 

Agreed.  But we're not talking abut fixing a random bug in your patch
series, but we're talking about adding significant new features - and
those features need to be tested in real use caes, not just in an
artificial test setup that holds assumptions that are not true.

To improve performance, or to fix simple bugs that only affect the
processing of the GTP-U G-PDU, a much more limited and hence
"unrealistic" test scenario is probably sufficient/acceptable.

> From that perspective, building protocol specific libraries and
> finding the right cmd line to run is significant hoops (I can attest
> to this).

I understand your argument.  But then, there is actually quite some
tools to help you (see further below), as well as the wiki page at
http://osmocom.org/projects/linux-kernel-gtp-u/wiki/Basic_Testing

Of course, existing tools and existing wiki pages also only document
existing features of the kernel code :)

Yes, the documentation could be better.  But then, how much more can you
expect from somebody who's doing this mostly for fun and who - despite
working in his dayjob on FOSS cellular projects - has no single
commercial project/context that uses the kernel GTP code.

In any case, working on a specific protocol or technology will require
that you understand that technology to some extent, including the
available tools.  There's always a learning curve involved.

> There are other examples in the kernel of systems bigger than GTP that
> require a whole lot of effort just to run a simple test; you'll notice
> for those it's rare that best developers ever bother to look at them
> unless they're making a global change that affects the code. We don't
> want GTP to take be like that!

I'm all for following your argument.  My point is simply: You cannot
develop code solely based on mock-ups without any 'realistic' test
scenarios.  Otherwise you will end up with something that works only in
your artificial lab setup, and follows all the best practises of the way
how the Linux kernel traditionally approaches tunneling implementations,
but it will never work/interop in the real world.

And I'm very strongly opposed to merging code where we have not been
able to show that it will inter-operate in at least one realistic
scenario.  This would raise wrong expectations with users and all sorts
of downstream problems.

So let's say we merge your IPv6 support as-is, and kernels get released
+ shipped with it.  Later on, we find that in order to turn it into a
standards-compliant implementation together with all the required bits
in userspace and on the control plane, we need to change some parts of
it, particularly those parts that affect the netlink or any other
exposed userspace interface.  At that point, we cannot change the
interface as the kernel has a strict rule of never breaking userspace
ABI.  But we must change it in order to make it work in the real world.
So what do we do?  Add lots of cruft in order to emulate backwards
compatibility?

> So the forward looking question now is how to get to be able to run a
> "realistic setup"?

You can run this realistic setup entirely using Osmocom components.

For running a 2G network: OsmoBTS+OsmoNITB+OsmoSGSN
For running a 3G network: OsmoHNBGW+OsmoMSC+OsmoHLR+OsmoSGSN

Both above stacks/combinations will provide you with GTP-C and GTP-U
against a GGSN.  As GGSN, you can then use either OpenGGSN, or OsmoGGSN,
or ergw.  For OpenGGSN and ergw, this will work with kernel GTP today,
for those features present in kernel GTP (i.e. IPv4-only).

In both cases you need some RF hardware.  I'm happy to contribute
related hardware (and support getting it set up) free of charge from my
company sysmocom to anyone who has a realistic prospect of either
* integrating your IPv6 support or other significant feature patches with
  libgtpnl + OsmoGGSN (at which point you can run a complete setup with
  real phones to verify it works end-to-end)
* building and documenting or operating a continuous integration setup
  that would run tests on each new kernel version (or net-next, or
  whatever tree makes sense) to help us catch any regressions as the
  code proceeds

In order to have a smaller, but still realistic test scenario, I
implemented a series of GTP tests in
http://git.osmocom.org/osmo-ttcn3-hacks/tree/ggsn_tests/GGSN_Tests.ttcn

This code basically emulates the combination of everything from Phone to
SGSN, so that you have only two entities:
* the implementation under test = IUT (a GGSN implementing GTP-C + GTP-U)
* the test itself (GGSN_Test) executing against the IUT

The code so far implements PDP context activation + address allocation
for IPv4-only and IPv6-only cases and can be run against a GGSN
implementing those.  The IPv6-only PDP context unit tests include the
convoluted two-phase address assignment including sending the router
solicitation from the simulated phone as well as verifying the router
advertisement sent in response from the GGSN.

Yes, I know they're written in an unknown niche programming language
called TTCN-3, but this was the best tool at hand for the job I could
find, and the tests are open source as is the Eclipse Titan toolchain
for compiling it.

We even have a Dockerfile that will build you a docker container
containing the compiled GGSN_test at
http://git.osmocom.org/docker-playground/tree/ggsn-test

That docker container is used by a jenkins build test job to test
current OsmoGGSN master every night against the test suite.

I was stupid enough to break the testsuite with an accidential commit,
so tests between August 27th and today failed.  I've just reverted that
accident - don't let that mistake of mine mislead you.

I'm happy to contribute further to this by adding actual user-IP GTP-U
functional testing beyond the router soliciatation/advertisement to it.

So my suggestion in terms of a "realistic testbed without having to
configure + run dozens of programs and using real RF + phones" is to use
that testsuite.

What one cannot get around is having to implement support for new
features added on the kernel side such as IPv6 in libgtpnl and at least
one of the GGSN's using it, sorry.  Without that, there is no way to
know if that code would do anything useful.  You simply cannot
realistically test GTP-U alone without GTP-C.

I'd love to offer help on this, but it's really impossible right now.
I'm on holidays on a motorbike tour through rural Taiwan's mountains,
had to deal with a flat tire today, have limited connectivity and am
already cutting down hard on sleep every night to be able to respond to
the absolute minimally required work e-mails.  And review/follow-up to
your much appreciated patch series the last couple of days has also used
a lot of (unexpected not scheduled for) time.  I'm not complaining, I'm
just saying I am really not able to contribute more to this effort right
now beyond my review, the offer of free hardware for a real cellular
network, and the extension of the test cases for GTP-U beyond the
already implemented very important IPv6 address allocation/assignment
which I believe your current code would not pass.

Regards,
	Harald

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

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Tom Herbert]

On Thu, Sep 21, 2017 at 8:12 AM, Harald Welte <laforge@netfilter.org> wrote:
> Hi Tom,
>
> On Wed, Sep 20, 2017 at 05:55:01PM -0700, Tom Herbert wrote:
>> You have the point of view of someone who has a lot of experience
>> dealing with this protocol. Try to imagine if you were some random
>> kernel network programmer with no experience in the area. If they
>> happen to find a one-off bug and want to do the right thing by running
>> a test, you want to make that as easy as possible.
>
> Agreed.  But we're not talking abut fixing a random bug in your patch
> series, but we're talking about adding significant new features - and
> those features need to be tested in real use caes, not just in an
> artificial test setup that holds assumptions that are not true.
>
> To improve performance, or to fix simple bugs that only affect the
> processing of the GTP-U G-PDU, a much more limited and hence
> "unrealistic" test scenario is probably sufficient/acceptable.
>
>> From that perspective, building protocol specific libraries and
>> finding the right cmd line to run is significant hoops (I can attest
>> to this).
>
> I understand your argument.  But then, there is actually quite some
> tools to help you (see further below), as well as the wiki page at
> http://osmocom.org/projects/linux-kernel-gtp-u/wiki/Basic_Testing
>
> Of course, existing tools and existing wiki pages also only document
> existing features of the kernel code :)
>
> Yes, the documentation could be better.  But then, how much more can you
> expect from somebody who's doing this mostly for fun and who - despite
> working in his dayjob on FOSS cellular projects - has no single
> commercial project/context that uses the kernel GTP code.
>
> In any case, working on a specific protocol or technology will require
> that you understand that technology to some extent, including the
> available tools.  There's always a learning curve involved.
>
>> There are other examples in the kernel of systems bigger than GTP that
>> require a whole lot of effort just to run a simple test; you'll notice
>> for those it's rare that best developers ever bother to look at them
>> unless they're making a global change that affects the code. We don't
>> want GTP to take be like that!
>
> I'm all for following your argument.  My point is simply: You cannot
> develop code solely based on mock-ups without any 'realistic' test
> scenarios.  Otherwise you will end up with something that works only in
> your artificial lab setup, and follows all the best practises of the way
> how the Linux kernel traditionally approaches tunneling implementations,
> but it will never work/interop in the real world.
>

> And I'm very strongly opposed to merging code where we have not been
> able to show that it will inter-operate in at least one realistic
> scenario.  This would raise wrong expectations with users and all sorts
> of downstream problems.
>
Harald,

Please see the cover letter for the original GTP kernel patches dated
May 10, 2016. My first question on those was "Is there a timeline for
adding IPv6 support?". To which Pablo replied that there was a
preliminary patch for it that has not been released. That was almost a
year and half ago and we have not heard anything since. If you don't
like my patches or don't think that can be adapted to fully support
the GTP specification, that's fine. But then you need to provide a
viable alternative. We are at the point where a kernel networking
feature that only supports IPv4 when it could support IPv6 must be
considered incomplete.

Thanks,
Tom

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Harald Welte]

Hi Tom,

On Thu, Sep 21, 2017 at 09:43:02AM -0700, Tom Herbert wrote:
> Please see the cover letter for the original GTP kernel patches dated
> May 10, 2016. My first question on those was "Is there a timeline for
> adding IPv6 support?". To which Pablo replied that there was a
> preliminary patch for it that has not been released. 

I'll suggest Pablo to comment on that.  I don't recall the details at
that time, I was only involved in the earliest development of the module
and then handed over.

> If you don't like my patches or don't think that can be adapted to
> fully support the GTP specification, that's fine. 

It's not about "not liking".  I'm very happy about contributions,
including (of course) yours.  It's about making sure that code we merge
into the kernel GTP driver will actually be usable to create a
standards-compliant GTP application or not.

There's no use in merging an IPv6 support patch if already by code
review it can be shown that it's impossible to create a spec-compliant
implementation using that patch.  To me, that would be "merging IPv6
support so we can check off a box on a management form or marketing
sheet", but not for any practical value.

> But then you need to provide a viable alternative.

Why do *I* have to provide a viable alternative?  Who says that *I* have
an obligation to do so?  A (co-)maintainer of a given driver doesn't
have the obligation of implementing any feature as requested.

Community based collaborative development only gets those things done
that people contribute.  I have already contributed almost a decade of
my life to creating Free Software implementations of cellular protocol
stacks, and it continues to be the center of my work and spare time.
GTP is only one protocol layer on one of those stacks.

Pablo, Andreas and I have contributed a Linux kernel implementation that
currently only implements IPv4.  This implementation can by anyone
extended to support IPv6, and as you see from this e-mail thread, there
is interest in helping this along by
* providing code review (even at times when it's personally difficult
  for me)
* providing free hardware for setting up a "private cellular network"
  to test interoperability
* providing testing tools for validation in absence of such a cellular
  network

> We are at the point where a kernel networking feature that only
> supports IPv4 when it could support IPv6 must be considered
> incomplete.

I agree it is incomplete.  There's no doubt about that.  But then,
even the current "incomplete" implementation is working and can be used
to operate an interoperable, spec-compatible IPv4 GGSN.  So it serves a
practical purpose.  All I'm asking is that any IPv6 support patches are
developed with that same practical purpose in mind.

Going through the cover letter of your series again:

>  - IPv6 support

Cannot be merged as-is, see lengthy review discussion

> - Configurable networking interfaces so that GTP kernel can be
>   used and tested without needing GSN network emulation (i.e. no user
>   space daemon needed).
> - Port numbers are configurable

As I indicated, I'm not fundamentally opposed to it, but I'm wondering
how much value they bring in reality.  Andreas has raised the valid
concern that we're adding code that is not used in production setups or
by any of the userspace implementations using this tunneling module.
The code gets more complex and gets code paths that will not be
exercised/tested.

Nevertheless, if it helps you to work on GTP, we can merge them from my
point of view - unless Pablo and/or Andreas object more strongly.

> - GSO,GRO
> - A facility that allows application specific GSO callbacks

Fine with me, but I think you need to convince other folks about the
"application specific GSO" and the usage of the upper bits of
shinfo(skb)->gso_type.

>  - Control of zero UDP checksums

Same as above, Dave was raising some question about it, not sure if
his concern remains.

>  - Addition of a dst_cache in the GTP structure

Fine with me.


As for the patches touching gtp.c:

* 04/14 udp recv clean up:
	fine with me, but kbuild robot complaint?
	On a minor note, I think you're mixing two unrelated topics:
	Separating the UDP receive functions and conversion to gro_cells,
	which violates the "one patch per feature" rule.  I'd still
	merge it, but would prefer two separate patches

* 05/14 Remove special mtu handling
	Pending your rework

* 06/14 Eliminate pktinfo and add port configuration
	I don't like the combination of a non-functional "cosmetic"
	refactoring of removing a data structure with the introduction
	of a new feature.  Makes it harder to review, impossible to
	merge only one of the two.  For the rationale of introducing the
	gtp_pktinfo struct, see
	http://git.osmocom.org/osmo-gtp-kernel/commit/?id=3bc7019c7afd06b5c7d94e5621728d092b82bb85
	it was actually intended to make IPv6 support easier, but the
	partial IPv6 support was removed before mainline submission.

* 07/14 Support encapsulation of IPv6 packets
	Not acceptable in its current form, see extensive review

* 08/14 Support encpasulating over IPv6
	No concerns in principle. Pending you making it dependent on AF
	of socket

* 09/14 Allow configuring GTP interface as standalone
	Can be merged unless strong objection from Pablo/Andreas (see above)

* 10/14 Add support for devnet
	No concerns from my side

* 12/14 Configuration for zero UDP checksum
	Up to Dave, he raised a question on it

* 13/14 Support for GRO
	No concerns from my side

* 14/14 GSO support
	No concerns from my side

BTW: Where have the iproute2/ip patches been posted, which you mention
in your cover page of the patch series?

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

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Tom Herbert]

> It's not about "not liking".  I'm very happy about contributions,
> including (of course) yours.  It's about making sure that code we merge
> into the kernel GTP driver will actually be usable to create a
> standards-compliant GTP application or not.
>
Harald,

Do you believe that these patches are not at all on the right track,
that they can't be built upon to get to a standards-compliant
implementation, and that we are going to have to throw all of this and
start from scratch to provide IPv6 support?

> There's no use in merging an IPv6 support patch if already by code
> review it can be shown that it's impossible to create a spec-compliant
> implementation using that patch.  To me, that would be "merging IPv6
> support so we can check off a box on a management form or marketing
> sheet", but not for any practical value.
>

To be clear, these patches are not done because to be a bullet point
on a marketing sheet. IPv6 is becoming _the_ Internet protocol. It
continues to exhibit exponential growth (~20% of Internet, per Google
stats), I believe least two of the largest datacenter operators are
running everything over IPv6, and there are already proposals to start
official deprecation of IPv4. In the mobile space IPv6 is going to be
a critical enabler of IoT and security in technologies like 5G. If we
want Linux to be at the forefront of the next technology wave then we
need to focus on IPv6 now! We should be far past the days of vendors
only providing IPv4 in the kernel support because "that's what our
customers use" and they'll get to IPv6 support at their leisure. IMO,
davem has every right to unilaterally NAK patches that only support
IPv4 or only test IPv4 with not even a path or timeline for IPv6
support.

Thanks,
Tom

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Harald Welte]

Hi Tom,

On Sun, Sep 24, 2017 at 08:55:49AM -0700, Tom Herbert wrote:
> Do you believe that these patches are not at all on the right track,
> that they can't be built upon to get to a standards-compliant
> implementation, and that we are going to have to throw all of this and
> start from scratch to provide IPv6 support?

I believe I have pointed out where the problem areas are, several times
by now.  I see no reason why things would have to be started from
scratch.  However, the issues pointed out in the IPv6 support patch[es]
have to be resolved *before* any merge to mainline.

I don't mind merging "incomplete" code that doesn't cover all parts of a
spec but provides basic interoperability.  I also am not arguing that
code must be bug-free at the time it is merged (which is impossible
anyway).  But I am arguing that we cannot merge something that is
a wrong implementation as per the spec, and hence it must be brought
in-line with the spec before it can be merged.

> > There's no use in merging an IPv6 support patch if already by code
> > review it can be shown that it's impossible to create a spec-compliant
> > implementation using that patch.  To me, that would be "merging IPv6
> > support so we can check off a box on a management form or marketing
> > sheet", but not for any practical value.
> 
> To be clear, these patches are not done because to be a bullet point
> on a marketing sheet. 

Great.

> IPv6 is becoming _the_ Internet protocol.

I'm all aware of that, and I've been a very early adopter, since the
1990ies with 6bone.

My argument is not against IPv6 support.  My argument is against merging
something that introdues IPv6 in a way that's not in-line with the GTP
protocol specifications, as such a way is of no use to anyone (except
marketing sheets).

> We should be far past the days of vendors only providing IPv4 in the
> kernel support because "that's what our customers use" and they'll get
> to IPv6 support at their leisure. 

I'm not sure where a "vendor" is involved with the GTP patches so far.  I
think we have to draw a distinction between what you expect from
professional, corporate "vendors" with a commercial interest in mind
(such as supporting their hardware) and what you can expect from people
doing things in their spare time, out of enthusiasm to finally bring
some Free Software into the closed world of telecommunications.

The Telecom world should have implemented something like a GTP kernel
module a decade to 15 years ago.  They could have saved significant
investments in proprietary hardware by running open source GGSNs with an
accelerated user plane in the kernel.  Nobody seemed to have an interest
in that, until today - as you can see from Pablo and me working on this
in our spare time, whenever we have a couple of spare cycles next to
many other projects.  You can see from the osmo-gtp-kernel commit log it
took years of being a ultra-low-priority on-and-off project  to ever get
to a point where we thought it was worth submitting it mainline.
Andreas deserves the praise for finally pushing it ahead.

I'm looking forward to reviewing the next version of the patch series.
-- 
- 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)

Re: [PATCH net-next 09/14] gtp: Allow configuring GTP interface as standalone

[Tom Herbert]

> I'm not sure where a "vendor" is involved with the GTP patches so far.  I
> think we have to draw a distinction between what you expect from
> professional, corporate "vendors" with a commercial interest in mind
> (such as supporting their hardware) and what you can expect from people
> doing things in their spare time, out of enthusiasm to finally bring
> some Free Software into the closed world of telecommunications.
>
If it makes you feel any better I am not getting paid for this work either :-)

> The Telecom world should have implemented something like a GTP kernel
> module a decade to 15 years ago.  They could have saved significant
> investments in proprietary hardware by running open source GGSNs with an
> accelerated user plane in the kernel.  Nobody seemed to have an interest
> in that, until today - as you can see from Pablo and me working on this
> in our spare time, whenever we have a couple of spare cycles next to
> many other projects.  You can see from the osmo-gtp-kernel commit log it
> took years of being a ultra-low-priority on-and-off project  to ever get
> to a point where we thought it was worth submitting it mainline.
> Andreas deserves the praise for finally pushing it ahead.
>
I completely agree, and your work is well appreciated! But I don't
believe it is to late to steer the ship away from proprietary
solutions. In fact, given the direction of the rest of the industry
direction, now is our best opportunity to try. That is a major reason
for these patches. We need to bring GTP into the limelight and get a
lot more people thinking about. This might even be the world's most
important tunneling protocol. If nothing else, a discussion like this
is good if it inspires others in the community to start to look at it.

Tom

[PATCH net-next 10/14] gtp: Add support for devnet

[Tom Herbert]

Add a net field to gtp that is derived from src_net. Use net_eq to make
cross net argument for transmit functions.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 1870469a4982..393f63cb2576 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -89,6 +89,7 @@ struct gtp_dev {
 	struct socket		*sock0;
 	struct socket		*sock1u;
 
+	struct net		*net;
 	struct net_device	*dev;
 
 	unsigned int		role;
@@ -271,6 +272,7 @@ static u16 ipver_to_eth(struct iphdr *iph)
 static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
 		  unsigned int hdrlen, unsigned int role)
 {
+	struct gtp_dev *gtp = netdev_priv(pctx->dev);
 	struct pcpu_sw_netstats *stats;
 	u16 inner_protocol;
 
@@ -285,8 +287,7 @@ static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
 
 	/* Get rid of the GTP + UDP headers. */
 	if (iptunnel_pull_header(skb, hdrlen, inner_protocol,
-				 !net_eq(sock_net(pctx->sk),
-					 dev_net(pctx->dev))))
+				 !net_eq(gtp->net, dev_net(pctx->dev))))
 		return -1;
 
 	netdev_dbg(pctx->dev, "forwarding packet from GGSN to uplink\n");
@@ -532,6 +533,8 @@ static void gtp_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)
 static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 		    struct pdp_ctx *pctx)
 {
+	struct gtp_dev *gtp = netdev_priv(dev);
+	bool xnet = !net_eq(gtp->net, dev_net(gtp->dev));
 	struct sock *sk = pctx->sk;
 	int err = 0;
 
@@ -564,7 +567,7 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 				    pctx->peer_addr_ip4.s_addr,
 				    0, ip4_dst_hoplimit(&rt->dst), 0,
 				    pctx->gtp_port, pctx->gtp_port,
-				    false, false);
+				    xnet, false);
 
 		netdev_dbg(dev, "gtp -> IP src: %pI4 dst: %pI4\n",
 			   &saddr, &pctx->peer_addr_ip4.s_addr);
@@ -782,6 +785,7 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,
 
 	gtp->role = role;
 	gtp->is_ipv6 = is_ipv6;
+	gtp->net = src_net;
 
 	gn = net_generic(dev_net(dev), gtp_net_id);
 	list_add_rcu(&gtp->list, &gn->gtp_dev_list);
-- 
2.11.0

[PATCH net-next 11/14] net: Add a facility to support application defined GSO

[Tom Herbert]

Allow applications or encapsulation protocols to register a GSO segment
function to their specific protocol. To faciliate this I reserved the
upper four bits in the gso_type to indicate the application specific GSO
type. Zero in these bits indicates no application GSO, so there are
fifteen instance that can be defined.

An application registers a a gso_segment using the skb_gso_app_register
this takes a struct skb_gso_app that indicates a callback function as
well as a set of GSO types for which at least one must be matched before
calling he segment function. GSO returns one of the application GSO
types described above (not a fixed value for the applications).
Subsequently, when the application sends a GSO packet the application
gso_type is set in the skb gso_type along with any other types.

skb_gso_app_segment is the function called from another GSO segment
function to handle segmentation of the application or encapsulation
protocol. This function includes check flags that provides context for
the appropriate GSO instance to match. For instance, in order to handle
a protocol encapsulated in UDP (GTP for instance) skb_gso_app_segment is
call from udp_tunnel_segment and check flags would be
SKB_GSO_UDP_TUNNEL_CSUM | SKB_GSO_UDP_TUNNEL.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/linux/netdevice.h | 31 +++++++++++++++++++++++++++++++
 include/linux/skbuff.h    | 25 +++++++++++++++++++++++++
 net/core/dev.c            | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/ip_tunnel_core.c |  6 ++++++
 net/ipv4/udp_offload.c    | 20 +++++++++++++++-----
 5 files changed, 124 insertions(+), 5 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f535779d9dc1..f3bed4f8ba83 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3932,6 +3932,37 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
 struct sk_buff *skb_mac_gso_segment(struct sk_buff *skb,
 				    netdev_features_t features);
 
+struct skb_gso_app {
+	unsigned int check_flags;
+	struct sk_buff *(*gso_segment)(struct sk_buff *skb,
+				       netdev_features_t features);
+};
+
+extern struct skb_gso_app *skb_gso_apps[];
+int skb_gso_app_register(const struct skb_gso_app *app);
+void skb_gso_app_unregister(int num, const struct skb_gso_app *app);
+
+/* rcu_read_lock() must be held */
+static inline struct skb_gso_app *skb_gso_app_lookup(struct sk_buff *skb,
+						     netdev_features_t features,
+						     unsigned int check_flags)
+{
+	struct skb_gso_app *app;
+	int type;
+
+	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_APP_MASK))
+		return false;
+
+	type = skb_gso_app_to_index(skb_shinfo(skb)->gso_type);
+
+	app = rcu_dereference(skb_gso_apps[type]);
+	if (app && app->gso_segment &&
+	    (check_flags & app->check_flags))
+		return app;
+
+	return NULL;
+}
+
 struct netdev_bonding_info {
 	ifslave	slave;
 	ifbond	master;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 72299ef00061..ea45fb93897c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -535,6 +535,9 @@ enum {
 	SKB_FCLONE_CLONE,	/* companion fclone skb (from fclone_cache) */
 };
 
+#define SKB_GSO_APP_LOW_SHIFT	28
+#define SKB_GSO_APP_HIGH_SHIFT	31
+
 enum {
 	SKB_GSO_TCPV4 = 1 << 0,
 
@@ -569,8 +572,30 @@ enum {
 	SKB_GSO_SCTP = 1 << 14,
 
 	SKB_GSO_ESP = 1 << 15,
+
+	/* UDP encapsulation specific GSO consumes bits 28 through 31 */
+
+	SKB_GSO_APP_LOW = 1 << SKB_GSO_APP_LOW_SHIFT,
+
+	SKB_GSO_APP_HIGH = 1 << SKB_GSO_APP_HIGH_SHIFT,
 };
 
+#define SKB_GSO_APP_MASK ((-1U << SKB_GSO_APP_LOW_SHIFT) & \
+			  (-1U >> (8*sizeof(u32) - SKB_GSO_APP_HIGH_SHIFT - 1)))
+#define SKB_GSO_APP_NUM (SKB_GSO_APP_MASK >> SKB_GSO_APP_LOW_SHIFT)
+
+static inline int skb_gso_app_to_index(unsigned int x)
+{
+	/* Caller should check that app bits are non-zero */
+
+	return ((SKB_GSO_APP_MASK & x) >> SKB_GSO_APP_LOW_SHIFT) - 1;
+}
+
+static inline int skb_gso_app_to_gso_type(unsigned int x)
+{
+	return (x + 1) << SKB_GSO_APP_LOW_SHIFT;
+}
+
 #if BITS_PER_LONG > 32
 #define NET_SKBUFF_DATA_USES_OFFSET 1
 #endif
diff --git a/net/core/dev.c b/net/core/dev.c
index fb766d906148..c77fca112e67 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -156,6 +156,7 @@
 
 static DEFINE_SPINLOCK(ptype_lock);
 static DEFINE_SPINLOCK(offload_lock);
+static DEFINE_SPINLOCK(skb_gso_app_lock);
 struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;
 struct list_head ptype_all __read_mostly;	/* Taps */
 static struct list_head offload_base __read_mostly;
@@ -2725,6 +2726,52 @@ struct sk_buff *skb_mac_gso_segment(struct sk_buff *skb,
 }
 EXPORT_SYMBOL(skb_mac_gso_segment);
 
+struct skb_gso_app *skb_gso_apps[SKB_GSO_APP_NUM];
+EXPORT_SYMBOL(skb_gso_apps);
+
+int skb_gso_app_register(const struct skb_gso_app *app)
+{
+	int i, ret = 0;
+
+	spin_lock(&skb_gso_app_lock);
+
+	for (i = 0; i < SKB_GSO_APP_NUM; i++) {
+		if (!rcu_dereference_protected(skb_gso_apps[i],
+				lockdep_is_held(&skb_gso_app_lock))) {
+			/* Found an empty slot */
+			rcu_assign_pointer(skb_gso_apps[i], app);
+
+			ret = skb_gso_app_to_gso_type(i);
+
+			break;
+		}
+	}
+
+	spin_unlock(&skb_gso_app_lock);
+
+	return ret;
+return 0;
+}
+EXPORT_SYMBOL(skb_gso_app_register);
+
+void skb_gso_app_unregister(int num, const struct skb_gso_app *app)
+{
+	if (!num)
+		return;
+
+	num = skb_gso_app_to_index(num);
+
+	spin_lock(&skb_gso_app_lock);
+
+	if (app == rcu_dereference_protected(skb_gso_apps[num],
+				lockdep_is_held(&skb_gso_app_lock))) {
+		/* Matched entry */
+		rcu_assign_pointer(skb_gso_apps[num], NULL);
+	}
+
+	spin_unlock(&skb_gso_app_lock);
+}
+EXPORT_SYMBOL(skb_gso_app_unregister);
 
 /* openvswitch calls this on rx path, so we need a different check.
  */
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index 2f39479be92f..f2fd96d55c4e 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -171,6 +171,12 @@ int iptunnel_handle_offloads(struct sk_buff *skb,
 		err = skb_header_unclone(skb, GFP_ATOMIC);
 		if (unlikely(err))
 			return err;
+		if (!!(gso_type_mask & SKB_GSO_APP_MASK) &&
+		    !!(skb_shinfo(skb)->gso_type & SKB_GSO_APP_MASK)) {
+			/* Only allow one GSO app per packet */
+			return -EALREADY;
+		}
+
 		skb_shinfo(skb)->gso_type |= gso_type_mask;
 		return 0;
 	}
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 97658bfc1b58..ba58b36b35b2 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -152,19 +152,29 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 				       netdev_features_t features,
 				       bool is_ipv6)
 {
-	__be16 protocol = skb->protocol;
-	const struct net_offload **offloads;
-	const struct net_offload *ops;
-	struct sk_buff *segs = ERR_PTR(-EINVAL);
 	struct sk_buff *(*gso_inner_segment)(struct sk_buff *skb,
 					     netdev_features_t features);
+	const struct net_offload **offloads;
+	__be16 protocol = skb->protocol;
+	struct skb_gso_app *gso_app;
+	const struct net_offload *ops;
+	struct sk_buff *segs;
+
+	segs = ERR_PTR(-EINVAL);
 
 	rcu_read_lock();
 
+	gso_app = skb_gso_app_lookup(skb, features,
+				     SKB_GSO_UDP_TUNNEL_CSUM |
+				     SKB_GSO_UDP_TUNNEL);
+
 	switch (skb->inner_protocol_type) {
 	case ENCAP_TYPE_ETHER:
 		protocol = skb->inner_protocol;
-		gso_inner_segment = skb_mac_gso_segment;
+		if (gso_app && gso_app->gso_segment)
+			gso_inner_segment = gso_app->gso_segment;
+		else
+			gso_inner_segment = skb_mac_gso_segment;
 		break;
 	case ENCAP_TYPE_IPPROTO:
 		offloads = is_ipv6 ? inet6_offloads : inet_offloads;
-- 
2.11.0

Re: [PATCH net-next 11/14] net: Add a facility to support application defined GSO

[David Miller]

From: Tom Herbert <tom@quantonium.net>
Date: Mon, 18 Sep 2017 17:39:01 -0700

> Allow applications or encapsulation protocols to register a GSO segment
> function to their specific protocol. To faciliate this I reserved the
> upper four bits in the gso_type to indicate the application specific GSO
> type. Zero in these bits indicates no application GSO, so there are
> fifteen instance that can be defined.
> 
> An application registers a a gso_segment using the skb_gso_app_register
> this takes a struct skb_gso_app that indicates a callback function as
> well as a set of GSO types for which at least one must be matched before
> calling he segment function. GSO returns one of the application GSO
> types described above (not a fixed value for the applications).
> Subsequently, when the application sends a GSO packet the application
> gso_type is set in the skb gso_type along with any other types.
> 
> skb_gso_app_segment is the function called from another GSO segment
> function to handle segmentation of the application or encapsulation
> protocol. This function includes check flags that provides context for
> the appropriate GSO instance to match. For instance, in order to handle
> a protocol encapsulated in UDP (GTP for instance) skb_gso_app_segment is
> call from udp_tunnel_segment and check flags would be
> SKB_GSO_UDP_TUNNEL_CSUM | SKB_GSO_UDP_TUNNEL.
> 
> Signed-off-by: Tom Herbert <tom@quantonium.net>

What happens on cards that can offload existing arbitrary UDP tunnel
encapsulations?

Will something about the state of the GSO type bits you are adding
prevent that?  Or do we need to add some new checks somewhere?

[PATCH net-next 12/14] gtp: Configuration for zero UDP checksum

[Tom Herbert]

Add configuration to control use of zero checksums on transmit for both
IPv4 and IPv6, and control over accepting zero IPv6 checksums on
receive.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c            | 35 +++++++++++++++++++++++++++++++++--
 include/uapi/linux/if_link.h |  4 ++++
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 393f63cb2576..b53946f8b10b 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -75,6 +75,13 @@ struct pdp_ctx {
 	struct rcu_head		rcu_head;
 
 	struct dst_cache	dst_cache;
+
+	unsigned int		cfg_flags;
+
+#define GTP_F_UDP_ZERO_CSUM_TX		0x1
+#define GTP_F_UDP_ZERO_CSUM6_TX		0x2
+#define GTP_F_UDP_ZERO_CSUM6_RX		0x4
+
 };
 
 /* One instance of the GTP device. */
@@ -536,6 +543,7 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 	struct gtp_dev *gtp = netdev_priv(dev);
 	bool xnet = !net_eq(gtp->net, dev_net(gtp->dev));
 	struct sock *sk = pctx->sk;
+	bool udp_csum;
 	int err = 0;
 
 	/* Ensure there is sufficient headroom. */
@@ -563,11 +571,12 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 		skb_dst_drop(skb);
 
 		gtp_push_header(skb, pctx);
+		udp_csum = !(pctx->cfg_flags & GTP_F_UDP_ZERO_CSUM_TX);
 		udp_tunnel_xmit_skb(rt, sk, skb, saddr,
 				    pctx->peer_addr_ip4.s_addr,
 				    0, ip4_dst_hoplimit(&rt->dst), 0,
 				    pctx->gtp_port, pctx->gtp_port,
-				    xnet, false);
+				    xnet, !udp_csum);
 
 		netdev_dbg(dev, "gtp -> IP src: %pI4 dst: %pI4\n",
 			   &saddr, &pctx->peer_addr_ip4.s_addr);
@@ -591,11 +600,12 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 		skb_dst_drop(skb);
 
 		gtp_push_header(skb, pctx);
+		udp_csum = !(pctx->cfg_flags & GTP_F_UDP_ZERO_CSUM6_TX);
 		udp_tunnel6_xmit_skb(dst, sk, skb, dev,
 				     &saddr, &pctx->peer_addr_ip6,
 				     0, ip6_dst_hoplimit(dst), 0,
 				     pctx->gtp_port, pctx->gtp_port,
-				     true);
+				     !udp_csum);
 
 		netdev_dbg(dev, "gtp -> IP src: %pI6 dst: %pI6\n",
 			   &saddr, &pctx->peer_addr_ip6);
@@ -728,6 +738,7 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,
 {
 	unsigned int role = GTP_ROLE_GGSN;
 	bool have_fd, have_ports;
+	unsigned int flags = 0;
 	bool is_ipv6 = false;
 	struct gtp_dev *gtp;
 	struct gtp_net *gn;
@@ -747,6 +758,21 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,
 			return -EINVAL;
 	}
 
+	if (data[IFLA_GTP_UDP_CSUM]) {
+		if (!nla_get_u8(data[IFLA_GTP_UDP_CSUM]))
+			flags |= GTP_F_UDP_ZERO_CSUM_TX;
+	}
+
+	if (data[IFLA_GTP_UDP_ZERO_CSUM6_TX]) {
+		if (nla_get_u8(data[IFLA_GTP_UDP_ZERO_CSUM6_TX]))
+			flags |= GTP_F_UDP_ZERO_CSUM6_TX;
+	}
+
+	if (data[IFLA_GTP_UDP_ZERO_CSUM6_RX]) {
+		if (nla_get_u8(data[IFLA_GTP_UDP_ZERO_CSUM6_RX]))
+			flags |= GTP_F_UDP_ZERO_CSUM6_RX;
+	}
+
 	if (data[IFLA_GTP_AF]) {
 		u16 af = nla_get_u16(data[IFLA_GTP_AF]);
 
@@ -819,6 +845,9 @@ static const struct nla_policy gtp_policy[IFLA_GTP_MAX + 1] = {
 	[IFLA_GTP_ROLE]			= { .type = NLA_U32 },
 	[IFLA_GTP_PORT0]		= { .type = NLA_U16 },
 	[IFLA_GTP_PORT1]		= { .type = NLA_U16 },
+	[IFLA_GTP_UDP_CSUM]		= { .type = NLA_U8 },
+	[IFLA_GTP_UDP_ZERO_CSUM6_TX]	= { .type = NLA_U8 },
+	[IFLA_GTP_UDP_ZERO_CSUM6_RX]	= { .type = NLA_U8 },
 };
 
 static int gtp_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -990,6 +1019,8 @@ static struct socket *gtp_create_sock(struct net *net, bool ipv6,
 
 	if (ipv6) {
 		udp_conf.family = AF_INET6;
+		udp_conf.use_udp6_rx_checksums =
+		    !(flags & GTP_F_UDP_ZERO_CSUM6_RX);
 		udp_conf.ipv6_v6only = 1;
 	} else {
 		udp_conf.family = AF_INET;
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 81c26864abeb..14a32d745e24 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -555,6 +555,10 @@ enum {
 	IFLA_GTP_AF,
 	IFLA_GTP_PORT0,
 	IFLA_GTP_PORT1,
+	IFLA_GTP_UDP_CSUM,
+	IFLA_GTP_UDP_ZERO_CSUM6_TX,
+	IFLA_GTP_UDP_ZERO_CSUM6_RX,
+
 	__IFLA_GTP_MAX,
 };
 #define IFLA_GTP_MAX (__IFLA_GTP_MAX - 1)
-- 
2.11.0

Re: [PATCH net-next 12/14] gtp: Configuration for zero UDP checksum

[David Miller]

From: Tom Herbert <tom@quantonium.net>
Date: Mon, 18 Sep 2017 17:39:02 -0700

> Add configuration to control use of zero checksums on transmit for both
> IPv4 and IPv6, and control over accepting zero IPv6 checksums on
> receive.
> 
> Signed-off-by: Tom Herbert <tom@quantonium.net>

I thought we were trying to move away from this special case of allowing
zero UDP checksums with tunnels, especially for ipv6.

Re: [PATCH net-next 12/14] gtp: Configuration for zero UDP checksum

[Tom Herbert]

On Mon, Sep 18, 2017 at 9:24 PM, David Miller <davem@davemloft.net> wrote:
> From: Tom Herbert <tom@quantonium.net>
> Date: Mon, 18 Sep 2017 17:39:02 -0700
>
>> Add configuration to control use of zero checksums on transmit for both
>> IPv4 and IPv6, and control over accepting zero IPv6 checksums on
>> receive.
>>
>> Signed-off-by: Tom Herbert <tom@quantonium.net>
>
> I thought we were trying to move away from this special case of allowing
> zero UDP checksums with tunnels, especially for ipv6.

I don't have a strong preference either way. I like consistency with
VXLAN and foo/UDP, but I guess it's not required. Interestingly, since
GTP only carries IP, IPv6 zero checksums are actually safer here than
VXLAN or GRE/UDP.

Tom

Re: [PATCH net-next 12/14] gtp: Configuration for zero UDP checksum

[Harald Welte]

Hi Tom,

On Wed, Sep 20, 2017 at 11:09:29AM -0700, Tom Herbert wrote:
> On Mon, Sep 18, 2017 at 9:24 PM, David Miller <davem@davemloft.net> wrote:
> > From: Tom Herbert <tom@quantonium.net>
> >> Add configuration to control use of zero checksums on transmit for both
> >> IPv4 and IPv6, and control over accepting zero IPv6 checksums on
> >> receive.
> >
> > I thought we were trying to move away from this special case of allowing
> > zero UDP checksums with tunnels, especially for ipv6.
> 
> I don't have a strong preference either way. I like consistency with
> VXLAN and foo/UDP, but I guess it's not required. Interestingly, since
> GTP only carries IP, IPv6 zero checksums are actually safer here than
> VXLAN or GRE/UDP.

Just for the record: I don't care either way and I defer to the kernel
networking developers to decide if they want to have zero UDP checksum
in GTP or not.

The 3GPP specs don't say anything about UDP checksums.  So there's no
requirement to use them, and hence operation without UDP checksums
should be compliant.  Cisco GTP implementation has udp checksumming
configurable, so other implementations also seem to provide both ways.

In general, I would argue one wants UDP checksumming of GTP in all
setups, as while the inner IP packet might be protected, the GTP header
itself is not, and that's what contains important data suhc as the TEID
(Tunnel Endpoint ID).  But that's of course just my personal opinion,
and I'm not saying we should prevent people from using lower protection
if that's what they want.

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

Re: [PATCH net-next 12/14] gtp: Configuration for zero UDP checksum

[Tom Herbert]

On Wed, Sep 20, 2017 at 6:55 PM, Harald Welte <laforge@gnumonks.org> wrote:
> Hi Tom,
>
> On Wed, Sep 20, 2017 at 11:09:29AM -0700, Tom Herbert wrote:
>> On Mon, Sep 18, 2017 at 9:24 PM, David Miller <davem@davemloft.net> wrote:
>> > From: Tom Herbert <tom@quantonium.net>
>> >> Add configuration to control use of zero checksums on transmit for both
>> >> IPv4 and IPv6, and control over accepting zero IPv6 checksums on
>> >> receive.
>> >
>> > I thought we were trying to move away from this special case of allowing
>> > zero UDP checksums with tunnels, especially for ipv6.
>>
>> I don't have a strong preference either way. I like consistency with
>> VXLAN and foo/UDP, but I guess it's not required. Interestingly, since
>> GTP only carries IP, IPv6 zero checksums are actually safer here than
>> VXLAN or GRE/UDP.
>
> Just for the record: I don't care either way and I defer to the kernel
> networking developers to decide if they want to have zero UDP checksum
> in GTP or not.
>
> The 3GPP specs don't say anything about UDP checksums.  So there's no
> requirement to use them, and hence operation without UDP checksums
> should be compliant.  Cisco GTP implementation has udp checksumming
> configurable, so other implementations also seem to provide both ways.
>
> In general, I would argue one wants UDP checksumming of GTP in all
> setups, as while the inner IP packet might be protected, the GTP header
> itself is not, and that's what contains important data suhc as the TEID
> (Tunnel Endpoint ID).  But that's of course just my personal opinion,
> and I'm not saying we should prevent people from using lower protection
> if that's what they want.
>
The tradeoffs and requirements of zero UDP6 checksums are discussed at
length in RFC6935 and RFC6936. Given other implementations make it
configurable it should also be here.

Tom

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

[PATCH net-next 13/14] gtp: Support for GRO

[Tom Herbert]

Populate GRO receive and GRO complete functions for GTP-Uv0 and v1.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c | 204 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 204 insertions(+)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index b53946f8b10b..2f9d810cf19f 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -22,6 +22,7 @@
 #include <linux/jhash.h>
 #include <linux/if_tunnel.h>
 #include <linux/net.h>
+#include <linux/netdevice.h>
 #include <linux/file.h>
 #include <linux/gtp.h>
 
@@ -429,6 +430,205 @@ static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	return 1;
 }
 
+static struct sk_buff **gtp_gro_receive_finish(struct sock *sk,
+					       struct sk_buff **head,
+					       struct sk_buff *skb,
+					       void *hdr, size_t hdrlen)
+{
+	const struct packet_offload *ptype;
+	struct sk_buff **pp;
+	__be16 type;
+
+	type = ipver_to_eth((struct iphdr *)((void *)hdr + hdrlen));
+	if (!type)
+		goto out_err;
+
+	rcu_read_lock();
+
+	ptype = gro_find_receive_by_type(type);
+	if (!ptype)
+		goto out_unlock_err;
+
+	skb_gro_pull(skb, hdrlen);
+	skb_gro_postpull_rcsum(skb, hdr, hdrlen);
+	pp = call_gro_receive(ptype->callbacks.gro_receive, head, skb);
+
+	rcu_read_unlock();
+
+	return pp;
+
+out_unlock_err:
+	rcu_read_unlock();
+out_err:
+	NAPI_GRO_CB(skb)->flush |= 1;
+	return NULL;
+}
+
+static struct sk_buff **gtp0_gro_receive(struct sock *sk,
+					 struct sk_buff **head,
+					 struct sk_buff *skb)
+{
+	struct gtp0_header *gtp0;
+	size_t len, hdrlen, off;
+	struct sk_buff *p;
+
+	off = skb_gro_offset(skb);
+	len = off + sizeof(*gtp0);
+	hdrlen = sizeof(*gtp0);
+
+	gtp0 = skb_gro_header_fast(skb, off);
+	if (skb_gro_header_hard(skb, len)) {
+		gtp0 = skb_gro_header_slow(skb, len, off);
+		if (unlikely(!gtp0))
+			goto out;
+	}
+
+	if ((gtp0->flags >> 5) != GTP_V0 || gtp0->type != GTP_TPDU)
+		goto out;
+
+	hdrlen += sizeof(*gtp0);
+
+	/* To get IP version */
+	len += sizeof(struct iphdr);
+
+	/* Now get header with GTP header an IPv4 header (for version) */
+	if (skb_gro_header_hard(skb, len)) {
+		gtp0 = skb_gro_header_slow(skb, len, off);
+		if (unlikely(!gtp0))
+			goto out;
+	}
+
+	for (p = *head; p; p = p->next) {
+		const struct gtp0_header *gtp0_t;
+
+		if (!NAPI_GRO_CB(p)->same_flow)
+			continue;
+
+		gtp0_t = (struct gtp0_header *)(p->data + off);
+
+		if (gtp0->flags != gtp0_t->flags ||
+		    gtp0->type != gtp0_t->type ||
+		    gtp0->flow != gtp0_t->flow ||
+		    gtp0->tid != gtp0_t->tid) {
+			NAPI_GRO_CB(p)->same_flow = 0;
+			continue;
+		}
+	}
+
+	return gtp_gro_receive_finish(sk, head, skb, gtp0, hdrlen);
+
+out:
+	NAPI_GRO_CB(skb)->flush |= 1;
+
+	return NULL;
+}
+
+static struct sk_buff **gtp1u_gro_receive(struct sock *sk,
+					  struct sk_buff **head,
+					  struct sk_buff *skb)
+{
+	struct gtp1_header *gtp1;
+	size_t len, hdrlen, off;
+	struct sk_buff *p;
+
+	off = skb_gro_offset(skb);
+	len = off + sizeof(*gtp1);
+	hdrlen = sizeof(*gtp1);
+
+	gtp1 = skb_gro_header_fast(skb, off);
+	if (skb_gro_header_hard(skb, len)) {
+		gtp1 = skb_gro_header_slow(skb, len, off);
+		if (unlikely(!gtp1))
+			goto out;
+	}
+
+	if ((gtp1->flags >> 5) != GTP_V1 || gtp1->type != GTP_TPDU)
+		goto out;
+
+	if (gtp1->flags & GTP1_F_MASK) {
+		hdrlen += 4;
+		len += 4;
+	}
+
+	len += sizeof(struct iphdr);
+
+	/* Now get header with GTP header an IPv4 header (for version) */
+	if (skb_gro_header_hard(skb, len)) {
+		gtp1 = skb_gro_header_slow(skb, len, off);
+		if (unlikely(!gtp1))
+			goto out;
+	}
+
+	for (p = *head; p; p = p->next) {
+		const struct gtp1_header *gtp1_t;
+
+		if (!NAPI_GRO_CB(p)->same_flow)
+			continue;
+
+		gtp1_t = (struct gtp1_header *)(p->data + off);
+
+		if (gtp1->flags != gtp1_t->flags ||
+		    gtp1->type != gtp1_t->type ||
+		    gtp1->tid != gtp1_t->tid) {
+			NAPI_GRO_CB(p)->same_flow = 0;
+			continue;
+		}
+	}
+
+	return gtp_gro_receive_finish(sk, head, skb, gtp1, hdrlen);
+
+out:
+	NAPI_GRO_CB(skb)->flush = 1;
+
+	return NULL;
+}
+
+static int gtp_gro_complete_finish(struct sock *sk, struct sk_buff *skb,
+				   int nhoff, size_t hdrlen)
+{
+	struct packet_offload *ptype;
+	int err = -EINVAL;
+	__be16 type;
+
+	type = ipver_to_eth((struct iphdr *)(skb->data + nhoff + hdrlen));
+	if (!type)
+		return err;
+
+	rcu_read_lock();
+	ptype = gro_find_complete_by_type(type);
+	if (ptype)
+		err = ptype->callbacks.gro_complete(skb, nhoff + hdrlen);
+
+	rcu_read_unlock();
+
+	skb_set_inner_mac_header(skb, nhoff + hdrlen);
+
+	return err;
+}
+
+static int gtp0_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff)
+{
+	struct gtp0_header *gtp0 = (struct gtp0_header *)(skb->data + nhoff);
+	size_t hdrlen = sizeof(struct gtp0_header);
+
+	gtp0->length = htons(skb->len - nhoff - hdrlen);
+
+	return gtp_gro_complete_finish(sk, skb, nhoff, hdrlen);
+}
+
+static int gtp1u_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff)
+{
+	struct gtp1_header *gtp1 = (struct gtp1_header *)(skb->data + nhoff);
+	size_t hdrlen = sizeof(struct gtp1_header);
+
+	if (gtp1->flags & GTP1_F_MASK)
+		hdrlen += 4;
+
+	gtp1->length = htons(skb->len - nhoff - hdrlen);
+
+	return gtp_gro_complete_finish(sk, skb, nhoff, hdrlen);
+}
+
 static void gtp_encap_destroy(struct sock *sk)
 {
 	struct gtp_dev *gtp;
@@ -946,9 +1146,13 @@ static int gtp_encap_enable_sock(struct socket *sock, int type,
 	switch (type) {
 	case UDP_ENCAP_GTP0:
 		tuncfg.encap_rcv = gtp0_udp_encap_recv;
+		tuncfg.gro_receive = gtp0_gro_receive;
+		tuncfg.gro_complete = gtp0_gro_complete;
 		break;
 	case UDP_ENCAP_GTP1U:
 		tuncfg.encap_rcv = gtp1u_udp_encap_recv;
+		tuncfg.gro_receive = gtp1u_gro_receive;
+		tuncfg.gro_complete = gtp1u_gro_complete;
 		break;
 	default:
 		pr_debug("Unknown encap type %u\n", type);
-- 
2.11.0

Re: [PATCH net-next 13/14] gtp: Support for GRO

[kbuild test robot]

Hi Tom,

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Tom-Herbert/gtp-Additional-feature-support/20170919-143920
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)


vim +3958 include/linux/netdevice.h

5b33bc6e Tom Herbert 2017-09-18  3944  
5b33bc6e Tom Herbert 2017-09-18  3945  /* rcu_read_lock() must be held */
5b33bc6e Tom Herbert 2017-09-18  3946  static inline struct skb_gso_app *skb_gso_app_lookup(struct sk_buff *skb,
5b33bc6e Tom Herbert 2017-09-18  3947  						     netdev_features_t features,
5b33bc6e Tom Herbert 2017-09-18  3948  						     unsigned int check_flags)
5b33bc6e Tom Herbert 2017-09-18  3949  {
5b33bc6e Tom Herbert 2017-09-18  3950  	struct skb_gso_app *app;
5b33bc6e Tom Herbert 2017-09-18  3951  	int type;
5b33bc6e Tom Herbert 2017-09-18  3952  
5b33bc6e Tom Herbert 2017-09-18  3953  	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_APP_MASK))
5b33bc6e Tom Herbert 2017-09-18  3954  		return false;
5b33bc6e Tom Herbert 2017-09-18  3955  
5b33bc6e Tom Herbert 2017-09-18  3956  	type = skb_gso_app_to_index(skb_shinfo(skb)->gso_type);
5b33bc6e Tom Herbert 2017-09-18  3957  
5b33bc6e Tom Herbert 2017-09-18 @3958  	app = rcu_dereference(skb_gso_apps[type]);
5b33bc6e Tom Herbert 2017-09-18  3959  	if (app && app->gso_segment &&
5b33bc6e Tom Herbert 2017-09-18  3960  	    (check_flags & app->check_flags))
5b33bc6e Tom Herbert 2017-09-18  3961  		return app;
5b33bc6e Tom Herbert 2017-09-18  3962  
5b33bc6e Tom Herbert 2017-09-18  3963  	return NULL;
5b33bc6e Tom Herbert 2017-09-18  3964  }
5b33bc6e Tom Herbert 2017-09-18  3965  

:::::: The code at line 3958 was first introduced by commit
:::::: 5b33bc6e4fcae1113167c651a3d3a218c7e277c6 net: Add a facility to support application defined GSO

:::::: TO: Tom Herbert <tom@quantonium.net>
:::::: CC: 0day robot <fengguang.wu@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH net-next 13/14] gtp: Support for GRO

[Harald Welte]

On Mon, Sep 18, 2017 at 05:39:03PM -0700, Tom Herbert wrote:
> Populate GRO receive and GRO complete functions for GTP-Uv0 and v1.

looks fine to me, though I'm not the GRO expert here.  Let's say what
the netdev gurus have to say in their review.

If you say it is tested with GRO-capable and non-GRO capable device
drivers, I'm fine with the patch.

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

[PATCH net-next 14/14] gtp: GSO support

[Tom Herbert]

Need to define a gtp_gso_segment since the GTP header includes a length
field that must be set per packet. Also, GPv0 header includes a sequence
number that is incremented per packet.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c            | 176 +++++++++++++++++++++++++++++++++++++++----
 include/uapi/linux/if_link.h |   1 -
 2 files changed, 163 insertions(+), 14 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 2f9d810cf19f..a2c4d9804a8f 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -120,6 +120,8 @@ static u32 gtp_h_initval;
 
 static void pdp_context_delete(struct pdp_ctx *pctx);
 
+static int gtp_gso_type;
+
 static inline u32 gtp0_hashfn(u64 tid)
 {
 	u32 *tid32 = (u32 *) &tid;
@@ -430,6 +432,69 @@ static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	return 1;
 }
 
+static struct sk_buff *gtp_gso_segment(struct sk_buff *skb,
+				       netdev_features_t features)
+{
+	struct sk_buff *segs = ERR_PTR(-EINVAL);
+	int tnl_hlen = skb->mac_len;
+	struct gtp0_header *gtp0;
+
+	if (unlikely(!pskb_may_pull(skb, tnl_hlen)))
+		return ERR_PTR(-EINVAL);
+
+	/* Make sure we have a mininal GTP header */
+	if (unlikely(tnl_hlen < min_t(size_t, sizeof(struct gtp0_header),
+				      sizeof(struct gtp1_header))))
+		return ERR_PTR(-EINVAL);
+
+	/* Determine version */
+	gtp0 = (struct gtp0_header *)skb->data;
+	switch (gtp0->flags >> 5) {
+	case GTP_V0: {
+		u16 tx_seq;
+
+		if (unlikely(tnl_hlen != sizeof(struct gtp0_header)))
+			return ERR_PTR(-EINVAL);
+
+		tx_seq = ntohs(gtp0->seq);
+
+		/* segment inner packet. */
+		segs = skb_mac_gso_segment(skb, features);
+		if (!IS_ERR_OR_NULL(segs)) {
+			skb = segs;
+			do {
+				gtp0 = (struct gtp0_header *)
+						skb_mac_header(skb);
+				gtp0->length = ntohs(skb->len - tnl_hlen);
+				gtp0->seq = htons(tx_seq);
+				tx_seq++;
+			} while ((skb = skb->next));
+		}
+		break;
+	}
+	case GTP_V1: {
+		struct gtp1_header *gtp1;
+
+		if (unlikely(tnl_hlen != sizeof(struct gtp1_header)))
+			return ERR_PTR(-EINVAL);
+
+		/* segment inner packet. */
+		segs = skb_mac_gso_segment(skb, features);
+		if (!IS_ERR_OR_NULL(segs)) {
+			skb = segs;
+			do {
+				gtp1 = (struct gtp1_header *)
+						skb_mac_header(skb);
+				gtp1->length = ntohs(skb->len - tnl_hlen);
+			} while ((skb = skb->next));
+		}
+		break;
+	}
+	}
+
+	return segs;
+}
+
 static struct sk_buff **gtp_gro_receive_finish(struct sock *sk,
 					       struct sk_buff **head,
 					       struct sk_buff *skb,
@@ -688,18 +753,25 @@ static inline void gtp0_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)
 {
 	int payload_len = skb->len;
 	struct gtp0_header *gtp0;
+	u32 tx_seq;
 
 	gtp0 = skb_push(skb, sizeof(*gtp0));
 
 	gtp0->flags	= 0x1e; /* v0, GTP-non-prime. */
 	gtp0->type	= GTP_TPDU;
 	gtp0->length	= htons(payload_len);
-	gtp0->seq	= htons((atomic_inc_return(&pctx->tx_seq) - 1) %
-				0xffff);
 	gtp0->flow	= htons(pctx->u.v0.flow);
 	gtp0->number	= 0xff;
 	gtp0->spare[0]	= gtp0->spare[1] = gtp0->spare[2] = 0xff;
 	gtp0->tid	= cpu_to_be64(pctx->u.v0.tid);
+
+	/* If skb is GSO allocate sequence numbers for all the segments */
+	tx_seq = skb_shinfo(skb)->gso_segs ?
+			atomic_add_return(skb_shinfo(skb)->gso_segs,
+					  &pctx->tx_seq) :
+			atomic_inc_return(&pctx->tx_seq);
+
+	gtp0->seq	= (htons((u16)tx_seq) - 1) & 0xffff;
 }
 
 static inline void gtp1_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)
@@ -737,6 +809,59 @@ static void gtp_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)
 	}
 }
 
+static size_t gtp_max_header_len(int version)
+
+{
+	switch (version) {
+	case GTP_V0:
+		return sizeof(struct gtp0_header);
+	case GTP_V1:
+		return sizeof(struct gtp1_header) + 4;
+	}
+
+	/* Should not happen */
+	return 0;
+}
+
+static int gtp_build_skb(struct sk_buff *skb, struct dst_entry *dst,
+			 struct pdp_ctx *pctx, bool xnet, int ip_hdr_len,
+			 bool udp_sum)
+{
+	int type = (udp_sum ? SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL) |
+		   gtp_gso_type;
+	int min_headroom;
+	u16 protocol;
+	int err;
+
+	skb_scrub_packet(skb, xnet);
+
+	min_headroom = LL_RESERVED_SPACE(dst->dev) + dst->header_len +
+		       gtp_max_header_len(pctx->gtp_version) + ip_hdr_len;
+
+	err = skb_cow_head(skb, min_headroom);
+	if (unlikely(err))
+		goto free_dst;
+
+	err = iptunnel_handle_offloads(skb, type);
+	if (err)
+		goto free_dst;
+
+	protocol = ipver_to_eth(ip_hdr(skb));
+
+	gtp_push_header(skb, pctx);
+
+	/* GTP header is treated as inner MAC header */
+	skb_reset_inner_mac_header(skb);
+
+	skb_set_inner_protocol(skb, protocol);
+
+	return 0;
+
+free_dst:
+	dst_release(dst);
+	return err;
+}
+
 static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 		    struct pdp_ctx *pctx)
 {
@@ -746,13 +871,6 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 	bool udp_csum;
 	int err = 0;
 
-	/* Ensure there is sufficient headroom. */
-	err = skb_cow_head(skb, dev->needed_headroom);
-	if (unlikely(err))
-		goto out_err;
-
-	skb_reset_inner_headers(skb);
-
 	if (pctx->peer_af == AF_INET) {
 		__be32 saddr = inet_sk(sk)->inet_saddr;
 		struct rtable *rt;
@@ -768,9 +886,13 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 			goto out_err;
 		}
 
-		skb_dst_drop(skb);
+		err = gtp_build_skb(skb, &rt->dst, pctx, xnet,
+				    sizeof(struct iphdr),
+				    !(pctx->cfg_flags &
+				      GTP_F_UDP_ZERO_CSUM_TX));
+		if (err)
+			goto out_err;
 
-		gtp_push_header(skb, pctx);
 		udp_csum = !(pctx->cfg_flags & GTP_F_UDP_ZERO_CSUM_TX);
 		udp_tunnel_xmit_skb(rt, sk, skb, saddr,
 				    pctx->peer_addr_ip4.s_addr,
@@ -797,9 +919,13 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,
 			goto out_err;
 		}
 
-		skb_dst_drop(skb);
+		err = gtp_build_skb(skb, dst, pctx, xnet,
+				    sizeof(struct ipv6hdr),
+				    !(pctx->cfg_flags &
+				      GTP_F_UDP_ZERO_CSUM6_TX));
+		if (err)
+			goto out_err;
 
-		gtp_push_header(skb, pctx);
 		udp_csum = !(pctx->cfg_flags & GTP_F_UDP_ZERO_CSUM6_TX);
 		udp_tunnel6_xmit_skb(dst, sk, skb, dev,
 				     &saddr, &pctx->peer_addr_ip6,
@@ -898,6 +1024,12 @@ static const struct net_device_ops gtp_netdev_ops = {
 	.ndo_get_stats64	= ip_tunnel_get_stats64,
 };
 
+#define GTP_FEATURES (NETIF_F_SG |		\
+		      NETIF_F_FRAGLIST |	\
+		      NETIF_F_HIGHDMA |		\
+		      NETIF_F_GSO_SOFTWARE |	\
+		      NETIF_F_HW_CSUM)
+
 static void gtp_link_setup(struct net_device *dev)
 {
 	struct gtp_dev *gtp = netdev_priv(dev);
@@ -912,7 +1044,13 @@ static void gtp_link_setup(struct net_device *dev)
 	dev->flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST;
 
 	dev->priv_flags	|= IFF_NO_QUEUE;
+
 	dev->features	|= NETIF_F_LLTX;
+	dev->features	|= GTP_FEATURES;
+
+	dev->hw_features |= GTP_FEATURES;
+	dev->hw_features |= NETIF_F_GSO_SOFTWARE;
+
 	netif_keep_dst(dev);
 
 	/* Assume largest header, ie. GTPv0. */
@@ -1903,6 +2041,11 @@ static struct pernet_operations gtp_net_ops = {
 	.size	= sizeof(struct gtp_net),
 };
 
+static const struct skb_gso_app gtp_gso_app = {
+	.check_flags = SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM,
+	.gso_segment = gtp_gso_segment,
+};
+
 static int __init gtp_init(void)
 {
 	int err;
@@ -1921,6 +2064,10 @@ static int __init gtp_init(void)
 	if (err < 0)
 		goto unreg_genl_family;
 
+	gtp_gso_type = skb_gso_app_register(&gtp_gso_app);
+	if (!gtp_gso_type)
+		pr_warn("GTP unable to create UDP app gso type");
+
 	pr_info("GTP module loaded (pdp ctx size %zd bytes)\n",
 		sizeof(struct pdp_ctx));
 	return 0;
@@ -1937,6 +2084,9 @@ late_initcall(gtp_init);
 
 static void __exit gtp_fini(void)
 {
+	if (gtp_gso_type)
+		skb_gso_app_unregister(gtp_gso_type, &gtp_gso_app);
+
 	unregister_pernet_subsys(&gtp_net_ops);
 	genl_unregister_family(&gtp_genl_family);
 	rtnl_link_unregister(&gtp_link_ops);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 14a32d745e24..7c15db44eab3 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -558,7 +558,6 @@ enum {
 	IFLA_GTP_UDP_CSUM,
 	IFLA_GTP_UDP_ZERO_CSUM6_TX,
 	IFLA_GTP_UDP_ZERO_CSUM6_RX,
-
 	__IFLA_GTP_MAX,
 };
 #define IFLA_GTP_MAX (__IFLA_GTP_MAX - 1)
-- 
2.11.0

Re: [PATCH net-next 00/14] gtp: Additional feature support

[Harald Welte]

Hi Tom,

first of all, thanks a lot for your patch series.  It makes me happy to
see contributions on the GTP code :)

On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
>   - IPv6 support

see my detailed comments in other mails.  It's unfortunately only
support for the already "deprecated" IPv6-only PDP contexts, not the
more modern v4v6 type.  In order to interoperate with old and new
approach, all three cases (v4, v6 and v4v6) should be supported from one
code base.

>   - Configurable networking interfaces so that GTP kernel can be used
>   and tested without needing GSN network emulation (i.e. no user space
>   daemon needed).

We have some pretty decent userspace utilities for configuring the GTP
interfaces and tunnels in the libgtpnl repository, but if it helps
people to have another way of configuration, I won't be against it.

What we have to keep in mind is that the current model of 1:1 mapping of
a "UDP socket' to a GTP netdevice is conceptually broken and needs to be
refactored soon (without breaking backwards compatibility).  See related
earlier discussions with patches submitted by Andreas Schultz.

Summary:

In real-world GGSNs you often want to host multiple virtual GGSNs on a
single GGSN (= UDP socket).  Each virtual GGSN terminates into one
external PDN (packet data network), which can be a private corporate vpn
or any other IP network, with no routing between those networks.

Naively one would assume you "simply" run another virtual GGSN
instance on another IP address, and then differentiate like that.

However, the problem is that adding a new GGSN IP address will require
manual configuration changes at each of your roaming partners (easily
hundreds of operators!) and hence it is avoided at all cost due to the
related long schedule, requirement for interop testing with each of them,
etc.

So what you do in reality at operators is that you operate many of those
virtual GGSNs on the same IP:Port combination (and hence UDP socket),
which means you have PDP contexts for vGGSN A which terminate on e.g.
gtp0 and PDP contexts for vGGSN B on gtp1, and so on.  The decision
which gtp-device a given PDP context is a member is made by the GTP-C
instance.  In the kenel we'll have to decouple net-devices from sockets.

So whatever new configuration mechanism or architectural changes we
introduce, we need to make sure that those will accomodate the "new
model" rather than introducing further dependencies for which we will
have to maintain backwards compatibility workaronds later on.

>   - Port numbers are configurable

I'm not sure if this is a useful feature.  GTP is used only in
operator-controlled networks and only on standard ports.  It's not
possible to negotiate any non-standard ports on the signaling plane
either.

>   - Addition of a dst_cache in the GTP structure and other cleanup

looks fine to me.

>   - GSO,GRO
>   - Control of zero UDP checksums

[...]

> Additionally, this patch set also includes a couple of general support
> capabilities:
> 
>   - A facility that allows application specific GSO callbacks
>   - Common functions to get a route fo for an IP tunnel

This is where the "core netdev" folks will have to comment.  I'm too
remote from mainline kernel development these days and will focus on
reviewing the GTP specific bits of your patch series.

> For IPv6 support, the mobile subscriber needs to allow IPv6 addresses,
> and the remote enpoint can be IPv6.

Minor correction: The mobile subscriber specifically requests a PDP Type
when establishing the PDP context via Session Management related
signaling from MS/UE to SGSN.  The SGSN simply translates this to GTP
and then forwards it to the GGSN.  So it's acutally not "allow" but
"specifically request".

> Configured the matrix of IPv4/IPv6 mobile subscriber, IPv4/IPv6 remote
> peer, and GTP version 0 and 1 (eight combinations). Observed
> connectivity and proper GSO/GRO. Also, tested VXLAN for
> regression.

I presume those tests were done with manually configured GTP-devices and
PDP contexts to the (patched) kernel GTP module?  If so, I would like to
strongly suggest interop testing with a different implementation, such
as real phones on the MS/UE side and e.g. OsmoSGSN.  That would,
however, of course mean that the netlink related bits would have to be
added to libgtpnl and OsmoGGSN (or ergw) so that you have a daemon for
the control plane.

For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
related to the way how router solicitation/advertisements are modified
in the 3GPP world.

The address allocation in v4 is simple:
* MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
  activation
* GGSN responds with IPv4 address in EUA of Activate PDP context
  response (and then uses netlink to tell the kernel about that
  IPv4 address)

In v6 or the v6 portion of v4v6 it works differently:
* MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
  activation
* GGSN responds with an IPv6 address, but that address is *not* used
  for communication, but simply used as an "interface identifier" to
  build a link-local address.
* MS then uses router solicitation using that link-local address
* GGSN responds with router advertisement, allocating a single /64
  prefix, from which the MS then generates a fully-qualified IPv6
  source address for communication.

How did you envision this to be done with the v6 support you just added?
At the very least, the /64 prefix matching would have to be implemented
so that in fact all addresses within that /64 prefix are matched +
encapsulated for a given PDP context in the downlink (to phone)
direction.

Also, I think the responsibility for the router advertisements would be
in the kernel, too.  Otherwise, a GTP-C userspace implementation would
have to inject packets into the user plane (which is otherwise handled
completely inside the kernel).  Injecting packets would mean that in caes
GTP sequence numbers are used, that userspace implementation would have
to alter the sequence numbers of the kernel gtp.ko code using netlink,
but therre would  be race conditions, ...

The router advertisements and neighbor advertisements basically have the
semantics of one link per PDP context.  Each of them is a point-to-point
link, and it's not one router advertisement that's sent to all of the
PDP contexts on that gtp-device.

I know it all sucks.  I'm still happy to see somebody tackling v6
support in gtp.c :)

Regards,
	Harald

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

Re: [PATCH net-next 00/14] gtp: Additional feature support

[Tom Herbert]

On Tue, Sep 19, 2017 at 5:43 AM, Harald Welte <laforge@gnumonks.org> wrote:
> Hi Tom,
>
> first of all, thanks a lot for your patch series.  It makes me happy to
> see contributions on the GTP code :)
>
> On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
>>   - IPv6 support
>
> see my detailed comments in other mails.  It's unfortunately only
> support for the already "deprecated" IPv6-only PDP contexts, not the
> more modern v4v6 type.  In order to interoperate with old and new
> approach, all three cases (v4, v6 and v4v6) should be supported from one
> code base.
>
It sounds like something that can be subsequently added. Do you have a
reference to the spec?

>>   - Configurable networking interfaces so that GTP kernel can be used
>>   and tested without needing GSN network emulation (i.e. no user space
>>   daemon needed).
>
> We have some pretty decent userspace utilities for configuring the GTP
> interfaces and tunnels in the libgtpnl repository, but if it helps
> people to have another way of configuration, I won't be against it.
>
AFAIK those userspace utilities don't support IPv6. Being able to
configure GTP like any other encapsulation will facilitate development
of IPv6 and other features.

> What we have to keep in mind is that the current model of 1:1 mapping of
> a "UDP socket' to a GTP netdevice is conceptually broken and needs to be
> refactored soon (without breaking backwards compatibility).  See related
> earlier discussions with patches submitted by Andreas Schultz.
>
I don't think I changed the model, so this can evolve.

> Summary:
>
> In real-world GGSNs you often want to host multiple virtual GGSNs on a
> single GGSN (= UDP socket).  Each virtual GGSN terminates into one
> external PDN (packet data network), which can be a private corporate vpn
> or any other IP network, with no routing between those networks.
>
Sounds like network virtualization and VNIs.

> Naively one would assume you "simply" run another virtual GGSN
> instance on another IP address, and then differentiate like that.
>
> However, the problem is that adding a new GGSN IP address will require
> manual configuration changes at each of your roaming partners (easily
> hundreds of operators!) and hence it is avoided at all cost due to the
> related long schedule, requirement for interop testing with each of them,
> etc.
>
> So what you do in reality at operators is that you operate many of those
> virtual GGSNs on the same IP:Port combination (and hence UDP socket),
> which means you have PDP contexts for vGGSN A which terminate on e.g.
> gtp0 and PDP contexts for vGGSN B on gtp1, and so on.  The decision
> which gtp-device a given PDP context is a member is made by the GTP-C
> instance.  In the kenel we'll have to decouple net-devices from sockets.
>
> So whatever new configuration mechanism or architectural changes we
> introduce, we need to make sure that those will accomodate the "new
> model" rather than introducing further dependencies for which we will
> have to maintain backwards compatibility workaronds later on.
>
>>   - Port numbers are configurable
>
> I'm not sure if this is a useful feature.  GTP is used only in
> operator-controlled networks and only on standard ports.  It's not
> possible to negotiate any non-standard ports on the signaling plane
> either.
>
Bear in mind that we're not required to do everything the GTP spec
says. Adding port configuration is another one of those things that
gives us flexibility and and better capability to test without needing
a full blown GSN network. One feature I didn't implement was UDP
source for flow entropy-- as we've seen with other encapsulation
protocols this helps significantly to get good ECMP in the network. My
impression is GTP designers probably didn't think in terms of getting
best performance. But we can ;-)

>>   - Addition of a dst_cache in the GTP structure and other cleanup
>
> looks fine to me.
>
>>   - GSO,GRO
>>   - Control of zero UDP checksums
>
> [...]
>
>> Additionally, this patch set also includes a couple of general support
>> capabilities:
>>
>>   - A facility that allows application specific GSO callbacks
>>   - Common functions to get a route fo for an IP tunnel
>
> This is where the "core netdev" folks will have to comment.  I'm too
> remote from mainline kernel development these days and will focus on
> reviewing the GTP specific bits of your patch series.
>
Thanks. Obviously, I and many on this list have more expertise on the
core networking side than GTP, so your review is quite welcome.

>> For IPv6 support, the mobile subscriber needs to allow IPv6 addresses,
>> and the remote enpoint can be IPv6.
>
> Minor correction: The mobile subscriber specifically requests a PDP Type
> when establishing the PDP context via Session Management related
> signaling from MS/UE to SGSN.  The SGSN simply translates this to GTP
> and then forwards it to the GGSN.  So it's acutally not "allow" but
> "specifically request".
>
Okay.

>> Configured the matrix of IPv4/IPv6 mobile subscriber, IPv4/IPv6 remote
>> peer, and GTP version 0 and 1 (eight combinations). Observed
>> connectivity and proper GSO/GRO. Also, tested VXLAN for
>> regression.
>
> I presume those tests were done with manually configured GTP-devices and
> PDP contexts to the (patched) kernel GTP module?  If so, I would like to
> strongly suggest interop testing with a different implementation, such
> as real phones on the MS/UE side and e.g. OsmoSGSN.  That would,
> however, of course mean that the netlink related bits would have to be
> added to libgtpnl and OsmoGGSN (or ergw) so that you have a daemon for
> the control plane.
>
I also brought up open_ggsn. ggsn to sgsn.

> For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
> related to the way how router solicitation/advertisements are modified
> in the 3GPP world.
>
> The address allocation in v4 is simple:
> * MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
>   activation
> * GGSN responds with IPv4 address in EUA of Activate PDP context
>   response (and then uses netlink to tell the kernel about that
>   IPv4 address)
>
> In v6 or the v6 portion of v4v6 it works differently:
> * MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
>   activation
> * GGSN responds with an IPv6 address, but that address is *not* used
>   for communication, but simply used as an "interface identifier" to
>   build a link-local address.
> * MS then uses router solicitation using that link-local address
> * GGSN responds with router advertisement, allocating a single /64
>   prefix, from which the MS then generates a fully-qualified IPv6
>   source address for communication.
>
> How did you envision this to be done with the v6 support you just added?
> At the very least, the /64 prefix matching would have to be implemented
> so that in fact all addresses within that /64 prefix are matched +
> encapsulated for a given PDP context in the downlink (to phone)
> direction.
>
> Also, I think the responsibility for the router advertisements would be
> in the kernel, too.  Otherwise, a GTP-C userspace implementation would
> have to inject packets into the user plane (which is otherwise handled
> completely inside the kernel).  Injecting packets would mean that in caes
> GTP sequence numbers are used, that userspace implementation would have
> to alter the sequence numbers of the kernel gtp.ko code using netlink,
> but therre would  be race conditions, ...
>
> The router advertisements and neighbor advertisements basically have the
> semantics of one link per PDP context.  Each of them is a point-to-point
> link, and it's not one router advertisement that's sent to all of the
> PDP contexts on that gtp-device.
>
> I know it all sucks.  I'm still happy to see somebody tackling v6
> support in gtp.c :)
>
I would hope all the above you're describing is mostly control plane
matters. At least a good design decouples data palne and control
plane. I know that GTP is a bit convoluted in this regard.

Tom

Re: [PATCH net-next 00/14] gtp: Additional feature support

[Harald Welte]

Hi Tom,

On Tue, Sep 19, 2017 at 08:59:28AM -0700, Tom Herbert wrote:
> On Tue, Sep 19, 2017 at 5:43 AM, Harald Welte <laforge@gnumonks.org>
> wrote:
> > On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
> >>   - IPv6 support
> >
> > see my detailed comments in other mails.  It's unfortunately only
> > support for the already "deprecated" IPv6-only PDP contexts, not the
> > more modern v4v6 type.  In order to interoperate with old and new
> > approach, all three cases (v4, v6 and v4v6) should be supported from
> > one code base.
> >
> It sounds like something that can be subsequently added. 

Not entirely, at least on the netlink (and any other configuration
interface) you will have to reflect this from the very beginning.  You
have to have an explicit PDP type and cannot rely on the address type to
specify the type of PDP context.  Whatever interfaces are introduced
now will have to remain compatible to any future change.

My strategy to avoid any such possible 'road blocks' from being
introduced would be to simply add v4v6 and v6 support in one go.  The
differences are marginal (having both an IPv6 prefix and a v4 address in
parallel, rather than mutually exclusive only).

> Do you have a reference to the spec?

See http://osmocom.org/issues/2418#note-7 which lists Section 11.2.1.3.2
of 3GPP TS 29.061 in combination with RFC3314, RFC7066, RFC6459 and
3GPP TS 23.060 9.2.1 as well as a summary of my understanding of it some
months ago.

> >>   - Configurable networking interfaces so that GTP kernel can be
> >>   used and tested without needing GSN network emulation (i.e. no
> >>   user space daemon needed).
> >
> > We have some pretty decent userspace utilities for configuring the
> > GTP interfaces and tunnels in the libgtpnl repository, but if it
> > helps people to have another way of configuration, I won't be
> > against it.
> >
> AFAIK those userspace utilities don't support IPv6. 

Of course not [yet]. libgtpnl and the command line tools have been
implemented specifically for the in-kernel GTP driver, and you have to
make sure to add related support on both the kernel and the userspace
side (libgtpnl). So there's little point in adding features on either
side before the other side.  There would be no way to test...

> Being able to configure GTP like any other encapsulation will
> facilitate development of IPv6 and other features.

That may very well be the case, but adding "IPv6 support" to kernel GTP
in a way that is not in line with the existing userspace libraries and
control-plane implementations means that you're developing those
features in an artificial environment that doesn't resemble real 3GPP
interoperable networks out there.

As indicated, I'm not against adding additional interfaces, but we have
to make sure that we add IPv6 support (or any new feature support) to at
least libgtpnl, and to make sure we test interoperability with existing
3GPP network equipment such as real IPv6 capable phones and SGSNs.

> > I'm not sure if this is a useful feature.  GTP is used only in
> > operator-controlled networks and only on standard ports.  It's not
> > possible to negotiate any non-standard ports on the signaling plane
> > either.
> >
> Bear in mind that we're not required to do everything the GTP spec
> says. 

Yes, we are, at least as long as it affects interoperability with other
implemetations out there.

GTP uses well-known port numbers on *both* sides of the tunnel, and you
cannot deviate from that.

There's no point in having all kinds of feetures in the GTP user plane
which are not interoperable with other implementations, and which are
completely outside of the information model / architecture of GTP.

In the real world, GTP-U is only used in combination with GTP-C.  And in
GTP-C you can only negotiate the IP address of both sides of GTP-U, and
not the port number information.  As a result, the port numbers are
static on both sides.

> My impression is GTP designers probably didn't think in terms of
> getting best performance. But we can ;-)

I think it's wasted efforts if it's about "random udp ports" as no
standards-compliant implementation out there with which you will have to
interoperate will be able to support it.

GTP is used between home and roaming operator.  If you want to introduce
changes to how it works, you will have to have control over both sides
of the implementation of both the GTP-C and the GTP-u plane, which is
very unlikely and rather the exception in the hundreds of operators you
interoperate with.  Also keep in mind that there often are various
"middleboxes" that will suddenly have to reflect your changes.  That
starts from packet filters at various locations in the operator networks
and/or roaming hubs, down to GTP hubs and others.

My opinion is: Non-standard GTP ports are not going to happen.

> I also brought up open_ggsn. ggsn to sgsn.

That's good to hear.  For both v4 and v6 PDP contexts?  Whcih phones
did you use for testing?  Particularly given how convolved the address
allocation is (see below), I'm surprised it would work.

> > For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
> > related to the way how router solicitation/advertisements are modified
> > in the 3GPP world.
> >
> > The address allocation in v4 is simple:
> > * MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
> >   activation
> > * GGSN responds with IPv4 address in EUA of Activate PDP context
> >   response (and then uses netlink to tell the kernel about that
> >   IPv4 address)
> >
> > In v6 or the v6 portion of v4v6 it works differently:
> > * MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
> >   activation
> > * GGSN responds with an IPv6 address, but that address is *not* used
> >   for communication, but simply used as an "interface identifier" to
> >   build a link-local address.
> > * MS then uses router solicitation using that link-local address
> > * GGSN responds with router advertisement, allocating a single /64
> >   prefix, from which the MS then generates a fully-qualified IPv6
> >   source address for communication.
> >
> > How did you envision this to be done with the v6 support you just added?
> > At the very least, the /64 prefix matching would have to be implemented
> > so that in fact all addresses within that /64 prefix are matched +
> > encapsulated for a given PDP context in the downlink (to phone)
> > direction.
> > 
> > [...]
> I would hope all the above you're describing is mostly control plane
> matters. 

It is not.  The control plane is GTP-C and runs on different UDP ports
(at least for GTPv1/v2).  The user plane is GTP-U and is what's done in
the kernel.  And by its very nature, IPv6 router
solicitations/advertisements (as well as neighbor
solicitations/advertisements) are part of the user plane and thus
handled in GTP-U.

> At least a good design decouples data palne and control
> plane. I know that GTP is a bit convoluted in this regard.

The problem is that IPv6 has never been specified properly for
point-to-point links.  There's no decent PPP specs for IPv6.  So the
3GPP folks had to try to be as close as possible to the existing
(broadcast) link layer model to facilitate existing IPv6 implemetations
to work over 3GPP bearers.  That's why they kept whatever possible to
re-use in terms of neighbor/router discovery.

So the problem is now: Unless you handle GTP-U *entirely* in the kernel
(including router + neighbor advertisement/solicitation), you will have
a "split GTP-U" plane between kernel and userspace.  And in that context
the question is who owns the sequence numbers, how will you avoid race
conditions, ... - my simple suggestion is thus to keep with the current
split and do everything GTP-U related inside the kernel and everything
GTP-C related in userspace.

I think there has to be a clear plan/architecture on how to implement
those bits in terms of the kernel/userspace split, and at least a proof
of concept implementation that we can show works with some real phones
out there - otherwise there's no point in having IPv6 support that works
well with some custom tools.

Regards,
	Harald
-- 
- 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)

Re: [PATCH net-next 00/14] gtp: Additional feature support

[Tom Herbert]

On Tue, Sep 19, 2017 at 4:19 PM, Harald Welte <laforge@gnumonks.org> wrote:
> Hi Tom,
>
> On Tue, Sep 19, 2017 at 08:59:28AM -0700, Tom Herbert wrote:
>> On Tue, Sep 19, 2017 at 5:43 AM, Harald Welte <laforge@gnumonks.org>
>> wrote:
>> > On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
>> >>   - IPv6 support
>> >
>> > see my detailed comments in other mails.  It's unfortunately only
>> > support for the already "deprecated" IPv6-only PDP contexts, not the
>> > more modern v4v6 type.  In order to interoperate with old and new
>> > approach, all three cases (v4, v6 and v4v6) should be supported from
>> > one code base.
>> >
>> It sounds like something that can be subsequently added.
>
> Not entirely, at least on the netlink (and any other configuration
> interface) you will have to reflect this from the very beginning.  You
> have to have an explicit PDP type and cannot rely on the address type to
> specify the type of PDP context.  Whatever interfaces are introduced
> now will have to remain compatible to any future change.
>
> My strategy to avoid any such possible 'road blocks' from being
> introduced would be to simply add v4v6 and v6 support in one go.  The
> differences are marginal (having both an IPv6 prefix and a v4 address in
> parallel, rather than mutually exclusive only).
>
>> Do you have a reference to the spec?
>
> See http://osmocom.org/issues/2418#note-7 which lists Section 11.2.1.3.2
> of 3GPP TS 29.061 in combination with RFC3314, RFC7066, RFC6459 and
> 3GPP TS 23.060 9.2.1 as well as a summary of my understanding of it some
> months ago.
>
>> >>   - Configurable networking interfaces so that GTP kernel can be
>> >>   used and tested without needing GSN network emulation (i.e. no
>> >>   user space daemon needed).
>> >
>> > We have some pretty decent userspace utilities for configuring the
>> > GTP interfaces and tunnels in the libgtpnl repository, but if it
>> > helps people to have another way of configuration, I won't be
>> > against it.
>> >
>> AFAIK those userspace utilities don't support IPv6.
>
> Of course not [yet]. libgtpnl and the command line tools have been
> implemented specifically for the in-kernel GTP driver, and you have to
> make sure to add related support on both the kernel and the userspace
> side (libgtpnl). So there's little point in adding features on either
> side before the other side.  There would be no way to test...
>
>> Being able to configure GTP like any other encapsulation will
>> facilitate development of IPv6 and other features.
>
> That may very well be the case, but adding "IPv6 support" to kernel GTP
> in a way that is not in line with the existing userspace libraries and
> control-plane implementations means that you're developing those
> features in an artificial environment that doesn't resemble real 3GPP
> interoperable networks out there.
>
> As indicated, I'm not against adding additional interfaces, but we have
> to make sure that we add IPv6 support (or any new feature support) to at
> least libgtpnl, and to make sure we test interoperability with existing
> 3GPP network equipment such as real IPv6 capable phones and SGSNs.
>
>> > I'm not sure if this is a useful feature.  GTP is used only in
>> > operator-controlled networks and only on standard ports.  It's not
>> > possible to negotiate any non-standard ports on the signaling plane
>> > either.
>> >
>> Bear in mind that we're not required to do everything the GTP spec
>> says.
>
> Yes, we are, at least as long as it affects interoperability with other
> implemetations out there.
>
> GTP uses well-known port numbers on *both* sides of the tunnel, and you
> cannot deviate from that.
>
> There's no point in having all kinds of feetures in the GTP user plane
> which are not interoperable with other implementations, and which are
> completely outside of the information model / architecture of GTP.
>
> In the real world, GTP-U is only used in combination with GTP-C.  And in
> GTP-C you can only negotiate the IP address of both sides of GTP-U, and
> not the port number information.  As a result, the port numbers are
> static on both sides.
>
>> My impression is GTP designers probably didn't think in terms of
>> getting best performance. But we can ;-)
>
> I think it's wasted efforts if it's about "random udp ports" as no
> standards-compliant implementation out there with which you will have to
> interoperate will be able to support it.
>
> GTP is used between home and roaming operator.  If you want to introduce
> changes to how it works, you will have to have control over both sides
> of the implementation of both the GTP-C and the GTP-u plane, which is
> very unlikely and rather the exception in the hundreds of operators you
> interoperate with.  Also keep in mind that there often are various
> "middleboxes" that will suddenly have to reflect your changes.  That
> starts from packet filters at various locations in the operator networks
> and/or roaming hubs, down to GTP hubs and others.
>
> My opinion is: Non-standard GTP ports are not going to happen.
>
>> I also brought up open_ggsn. ggsn to sgsn.
>
> That's good to hear.  For both v4 and v6 PDP contexts?  Whcih phones
> did you use for testing?  Particularly given how convolved the address
> allocation is (see below), I'm surprised it would work.
>
>> > For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
>> > related to the way how router solicitation/advertisements are modified
>> > in the 3GPP world.
>> >
>> > The address allocation in v4 is simple:
>> > * MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
>> >   activation
>> > * GGSN responds with IPv4 address in EUA of Activate PDP context
>> >   response (and then uses netlink to tell the kernel about that
>> >   IPv4 address)
>> >
>> > In v6 or the v6 portion of v4v6 it works differently:
>> > * MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
>> >   activation
>> > * GGSN responds with an IPv6 address, but that address is *not* used
>> >   for communication, but simply used as an "interface identifier" to
>> >   build a link-local address.
>> > * MS then uses router solicitation using that link-local address
>> > * GGSN responds with router advertisement, allocating a single /64
>> >   prefix, from which the MS then generates a fully-qualified IPv6
>> >   source address for communication.
>> >
>> > How did you envision this to be done with the v6 support you just added?
>> > At the very least, the /64 prefix matching would have to be implemented
>> > so that in fact all addresses within that /64 prefix are matched +
>> > encapsulated for a given PDP context in the downlink (to phone)
>> > direction.
>> >
>> > [...]
>> I would hope all the above you're describing is mostly control plane
>> matters.
>
> It is not.  The control plane is GTP-C and runs on different UDP ports
> (at least for GTPv1/v2).  The user plane is GTP-U and is what's done in
> the kernel.  And by its very nature, IPv6 router
> solicitations/advertisements (as well as neighbor
> solicitations/advertisements) are part of the user plane and thus
> handled in GTP-U.
>
>> At least a good design decouples data palne and control
>> plane. I know that GTP is a bit convoluted in this regard.
>
> The problem is that IPv6 has never been specified properly for
> point-to-point links.  There's no decent PPP specs for IPv6.  So the
> 3GPP folks had to try to be as close as possible to the existing
> (broadcast) link layer model to facilitate existing IPv6 implemetations
> to work over 3GPP bearers.  That's why they kept whatever possible to
> re-use in terms of neighbor/router discovery.
>
> So the problem is now: Unless you handle GTP-U *entirely* in the kernel
> (including router + neighbor advertisement/solicitation), you will have
> a "split GTP-U" plane between kernel and userspace.  And in that context
> the question is who owns the sequence numbers, how will you avoid race
> conditions, ... - my simple suggestion is thus to keep with the current
> split and do everything GTP-U related inside the kernel and everything
> GTP-C related in userspace.
>
> I think there has to be a clear plan/architecture on how to implement
> those bits in terms of the kernel/userspace split, and at least a proof
> of concept implementation that we can show works with some real phones
> out there - otherwise there's no point in having IPv6 support that works
> well with some custom tools.
>
OTOH, I will argue that the GTP patches should never have been allowed
in the kernel in the first place without IPv6 support! ;-) I think the
best plan forward is to get the IPv6 data path running that so can
demonstrate a functional GTP/IPv6 datapath (my primary purpose here to
have something to compare against with ILA). Since "real"
configuration path doesn't use the path to set up a standalone
interface, I would presume that that will be fleshed when someone has
cycles and expertise to work on both sides of the problem. Even if
this requires structural changes to how IPv6 is managed in GTP, I
doubt that the fundamental TX/RX, GRO/GSO data paths will change much.
In other words, please consider this to be a step on an evolutionary
path. More work is required to reach the ultimate deployable solution.

As for testing on real phones, that is cannot be a requirement for a
kernel feature. If you expect Linux community to support this, then we
need a way to be develop and test on commodity PC hardware. That is
one of the major values of creating a standalone interface
configuration-- we can test the datapath just like any other
encapsulation supported by the kernel.

Tom

Re: [PATCH net-next 00/14] gtp: Additional feature support

[Harald Welte]

Hi Tom,

On Tue, Sep 19, 2017 at 04:47:11PM -0700, Tom Herbert wrote:
> On Tue, Sep 19, 2017 at 4:19 PM, Harald Welte <laforge@gnumonks.org> wrote:
>
> > I think there has to be a clear plan/architecture on how to implement
> > those bits in terms of the kernel/userspace split, and at least a proof
> > of concept implementation that we can show works with some real phones
> > out there - otherwise there's no point in having IPv6 support that works
> > well with some custom tools.
> >
> OTOH, I will argue that the GTP patches should never have been allowed
> in the kernel in the first place without IPv6 support! ;-) 

Well, it could be shown that the code works with integration to OpenGGSN
(and later ergw, and now OsmoGGSN) and works within a complete 3GPP
network.  So we were not merging something that we hypothesized it would
work once the rest would be implemented, but we could actually show it
was working before it got merged.

> I think the best plan forward is to get the IPv6 data path running
> that so can demonstrate a functional GTP/IPv6 datapath 

Yes, but a functional "datapath" (= GTP-U) unfortunately includes the
way how address allocation/assignment is done.  Putting something into
the mainline kernel that we know will for sure not work/interop in a
real scenario is not a good idea.  I think what's worse than not
supporting a feature is to implying support for it while actually
doing it in an incomplete/incompliant way.

So from my point of view, what's needed is
* making sure router advertisement/solicitation are covered in some
  way, either by doing it in the kernel or having a clear strategy how
  it could be done from userspace while not a) introducing races regarding
  who owns the sequence numbers
* making sure the implementation covers entire /64 prefixes for each
  PDP context and not single addresses.  That is non-negotiable and
  mandatory by 3GPP specs.

> Since "real" configuration path doesn't use the path to set up a
> standalone interface, I would presume that that will be fleshed when
> someone has cycles and expertise to work on both sides of the problem.

The configuration will be different, yes. but we need to ensure that the
actual *implementation* of the data path does what it is expected to do,
no matter who configures it via which interface.

> Even if this requires structural changes to how IPv6 is managed in
> GTP, I doubt that the fundamental TX/RX, GRO/GSO data paths will
> change much.  In other words, please consider this to be a step on an
> evolutionary path. More work is required to reach the ultimate
> deployable solution.

Agreed. But then I'm still against merging something that we know for
sure will not be compatible with real-world use case.  It should be kept
out of mainline until we are sure of that, at the very least
theoretically, but even better which we can prove in practise will do
what it claims to do.

> As for testing on real phones, that is cannot be a requirement for a
> kernel feature. 

GTP is not implemented on phones.  GTP is implemented only inside the
fixed (land-side) of the cellular network.  However, the inner IP data
originates from phones, and large parts of what you see on GTP
originates from phones in a different format.  The inner IP data inside
GTP-U originates from phones.  And that's where address configuration
for IPv6 works.

> If you expect Linux community to support this, then we
> need a way to be develop and test on commodity PC hardware. 

I'm not arguing you need to run any of the code on a different
architecture such as a phone.  I'm arguing for you to run a cellular
network including the kernel GTP code, and then use that cellular
network from a real phone.  This is the only way to know for sure you
interoperate.  See my other mail related to the Open Soruce based
configurations for both 2G and 3G that can be used for this.

As a replacement, one can e.g. look at protocol traces of real phones
and then simulate the behavior of one or several different phones and
implement that as test caess.  This is what I did in the GGSN_Test
pointed out in my other mail in this thread.  And btw, all I've asked is
for showing it works with *one* phone model at all.  I'm not talking
about the various different implementation specifics, such as whether or
not the phone will insist on using neighbor solicitation and mandate
neighbor advertisement (on a point-to-point link, how absurd!) after
the (mandatory) router discovery.

> That is one of the major values of creating a standalone interface
> configuration-- we can test the datapath just like any other
> encapsulation supported by the kernel.

Well, you cannot.  You might be able to do some benchmarking to compare
if an old version of the kernel gtp driver will perform better or worse
than some optimizations introduced.  But you can *not* have a realistic
functional test that will tell you if your implementation is 'valid'.
I'm not even talking about being 'complete' here, but simply about being
broken or not.  Or test whether it will interoperate.  Particularly for
IPv6 this is impossible, due to the conflated way of involving both
GTP-C and GTP-U with router advertisement+solicitation for PDP context
activation, as outlined several times in this thread.

I wish it was simpler, but I haven't created GTP, sorry :)

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

Re: [PATCH net-next 00/14] gtp: Additional feature support

[Andreas Schultz]

Hi Harald,

On 20/09/17 01:19, Harald Welte wrote:
> Hi Tom,
> 
> On Tue, Sep 19, 2017 at 08:59:28AM -0700, Tom Herbert wrote:
>> On Tue, Sep 19, 2017 at 5:43 AM, Harald Welte <laforge@gnumonks.org>
>> wrote:
>>> On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
>>>>    - IPv6 support
>>>
>>> see my detailed comments in other mails.  It's unfortunately only
>>> support for the already "deprecated" IPv6-only PDP contexts, not the
>>> more modern v4v6 type.  In order to interoperate with old and new
>>> approach, all three cases (v4, v6 and v4v6) should be supported from
>>> one code base.
>>>
>> It sounds like something that can be subsequently added.
> 
> Not entirely, at least on the netlink (and any other configuration
> interface) you will have to reflect this from the very beginning.  You
> have to have an explicit PDP type and cannot rely on the address type to
> specify the type of PDP context.  Whatever interfaces are introduced
> now will have to remain compatible to any future change.
> 
> My strategy to avoid any such possible 'road blocks' from being
> introduced would be to simply add v4v6 and v6 support in one go.  The
> differences are marginal (having both an IPv6 prefix and a v4 address in
> parallel, rather than mutually exclusive only).
> 
>> Do you have a reference to the spec?
> 
> See http://osmocom.org/issues/2418#note-7 which lists Section 11.2.1.3.2
> of 3GPP TS 29.061 in combination with RFC3314, RFC7066, RFC6459 and
> 3GPP TS 23.060 9.2.1 as well as a summary of my understanding of it some
> months ago.
> 
>>>>    - Configurable networking interfaces so that GTP kernel can be
>>>>    used and tested without needing GSN network emulation (i.e. no
>>>>    user space daemon needed).
>>>
>>> We have some pretty decent userspace utilities for configuring the
>>> GTP interfaces and tunnels in the libgtpnl repository, but if it
>>> helps people to have another way of configuration, I won't be
>>> against it.
>>>
>> AFAIK those userspace utilities don't support IPv6.
> 
> Of course not [yet]. libgtpnl and the command line tools have been
> implemented specifically for the in-kernel GTP driver, and you have to
> make sure to add related support on both the kernel and the userspace
> side (libgtpnl). So there's little point in adding features on either
> side before the other side.  There would be no way to test...
> 
>> Being able to configure GTP like any other encapsulation will
>> facilitate development of IPv6 and other features.
> 
> That may very well be the case, but adding "IPv6 support" to kernel GTP
> in a way that is not in line with the existing userspace libraries and
> control-plane implementations means that you're developing those
> features in an artificial environment that doesn't resemble real 3GPP
> interoperable networks out there.
> 
> As indicated, I'm not against adding additional interfaces, but we have
> to make sure that we add IPv6 support (or any new feature support) to at
> least libgtpnl, and to make sure we test interoperability with existing
> 3GPP network equipment such as real IPv6 capable phones and SGSNs.
> 
>>> I'm not sure if this is a useful feature.  GTP is used only in
>>> operator-controlled networks and only on standard ports.  It's not
>>> possible to negotiate any non-standard ports on the signaling plane
>>> either.
>>>
>> Bear in mind that we're not required to do everything the GTP spec
>> says.
> 
> Yes, we are, at least as long as it affects interoperability with other
> implemetations out there.
> 
> GTP uses well-known port numbers on *both* sides of the tunnel, and you
> cannot deviate from that.

Actually, the well-known port is only mandatory for the receiving side.
The sending side can choose any port it wishes as long as it is prepared
to receive possible error indication on the well-known port.

Of course, it makes the implementation simple to use only one port, but 
for scalability it might be a good idea to support per PDP context 
sending ports.

Regards
Andreas

> There's no point in having all kinds of feetures in the GTP user plane
> which are not interoperable with other implementations, and which are
> completely outside of the information model / architecture of GTP.
> 
> In the real world, GTP-U is only used in combination with GTP-C.  And in
> GTP-C you can only negotiate the IP address of both sides of GTP-U, and
> not the port number information.  As a result, the port numbers are
> static on both sides.
> 
>> My impression is GTP designers probably didn't think in terms of
>> getting best performance. But we can ;-)
> 
> I think it's wasted efforts if it's about "random udp ports" as no
> standards-compliant implementation out there with which you will have to
> interoperate will be able to support it.
> 
> GTP is used between home and roaming operator.  If you want to introduce
> changes to how it works, you will have to have control over both sides
> of the implementation of both the GTP-C and the GTP-u plane, which is
> very unlikely and rather the exception in the hundreds of operators you
> interoperate with.  Also keep in mind that there often are various
> "middleboxes" that will suddenly have to reflect your changes.  That
> starts from packet filters at various locations in the operator networks
> and/or roaming hubs, down to GTP hubs and others.
> 
> My opinion is: Non-standard GTP ports are not going to happen.
> 
>> I also brought up open_ggsn. ggsn to sgsn.
> 
> That's good to hear.  For both v4 and v6 PDP contexts?  Whcih phones
> did you use for testing?  Particularly given how convolved the address
> allocation is (see below), I'm surprised it would work.
> 
>>> For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
>>> related to the way how router solicitation/advertisements are modified
>>> in the 3GPP world.
>>>
>>> The address allocation in v4 is simple:
>>> * MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
>>>    activation
>>> * GGSN responds with IPv4 address in EUA of Activate PDP context
>>>    response (and then uses netlink to tell the kernel about that
>>>    IPv4 address)
>>>
>>> In v6 or the v6 portion of v4v6 it works differently:
>>> * MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
>>>    activation
>>> * GGSN responds with an IPv6 address, but that address is *not* used
>>>    for communication, but simply used as an "interface identifier" to
>>>    build a link-local address.
>>> * MS then uses router solicitation using that link-local address
>>> * GGSN responds with router advertisement, allocating a single /64
>>>    prefix, from which the MS then generates a fully-qualified IPv6
>>>    source address for communication.
>>>
>>> How did you envision this to be done with the v6 support you just added?
>>> At the very least, the /64 prefix matching would have to be implemented
>>> so that in fact all addresses within that /64 prefix are matched +
>>> encapsulated for a given PDP context in the downlink (to phone)
>>> direction.
>>>
>>> [...]
>> I would hope all the above you're describing is mostly control plane
>> matters.
> 
> It is not.  The control plane is GTP-C and runs on different UDP ports
> (at least for GTPv1/v2).  The user plane is GTP-U and is what's done in
> the kernel.  And by its very nature, IPv6 router
> solicitations/advertisements (as well as neighbor
> solicitations/advertisements) are part of the user plane and thus
> handled in GTP-U.
> 
>> At least a good design decouples data palne and control
>> plane. I know that GTP is a bit convoluted in this regard.
> 
> The problem is that IPv6 has never been specified properly for
> point-to-point links.  There's no decent PPP specs for IPv6.  So the
> 3GPP folks had to try to be as close as possible to the existing
> (broadcast) link layer model to facilitate existing IPv6 implemetations
> to work over 3GPP bearers.  That's why they kept whatever possible to
> re-use in terms of neighbor/router discovery.
> 
> So the problem is now: Unless you handle GTP-U *entirely* in the kernel
> (including router + neighbor advertisement/solicitation), you will have
> a "split GTP-U" plane between kernel and userspace.  And in that context
> the question is who owns the sequence numbers, how will you avoid race
> conditions, ... - my simple suggestion is thus to keep with the current
> split and do everything GTP-U related inside the kernel and everything
> GTP-C related in userspace.
> 
> I think there has to be a clear plan/architecture on how to implement
> those bits in terms of the kernel/userspace split, and at least a proof
> of concept implementation that we can show works with some real phones
> out there - otherwise there's no point in having IPv6 support that works
> well with some custom tools.
> 
> Regards,
> 	Harald
>