[PATCH net-next v3 0/2] GTP SGSN-side tunnels

[PATCH net-next v3 0/2] GTP SGSN-side tunnels

[Jonas Bonn]

Changes since v2:

* Move #define of legacy netlink attribute into enum
* Added note to commit message about load-testing use-case
* Ack from Pablo

Jonas Bonn (2):
  gtp: rename SGSN netlink attribute
  gtp: support SGSN-side tunnels

 drivers/net/gtp.c            | 63 +++++++++++++++++++++++++++++---------------
 include/uapi/linux/gtp.h     |  3 ++-
 include/uapi/linux/if_link.h |  7 +++++
 3 files changed, 51 insertions(+), 22 deletions(-)

-- 
2.9.3

[PATCH net-next v3 1/2] gtp: rename SGSN netlink attribute

[Jonas Bonn]

This is a mostly cosmetic rename of the SGSN netlink attribute to
the GTP link.  The justification for this is that we will be making
the module support decapsulation of "downstream" SGSN packets, in
which case the netlink parameter actually refers to the upstream GGSN
peer.  Renaming the parameter makes the relationship clearer.

The legacy name is maintained as a define in the header file in order
to not break existing code.

Signed-off-by: Jonas Bonn <jonas@southpole.se>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 drivers/net/gtp.c        | 22 +++++++++++-----------
 include/uapi/linux/gtp.h |  3 ++-
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 50349a9..3806be6 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -56,7 +56,7 @@ struct pdp_ctx {
 	u16			af;
 
 	struct in_addr		ms_addr_ip4;
-	struct in_addr		sgsn_addr_ip4;
+	struct in_addr		peer_addr_ip4;
 
 	atomic_t		tx_seq;
 	struct rcu_head		rcu_head;
@@ -522,17 +522,17 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,
 	}
 
 	rt = ip4_route_output_gtp(sock_net(sk), &fl4, gtp->sock0->sk,
-				  pctx->sgsn_addr_ip4.s_addr);
+				  pctx->peer_addr_ip4.s_addr);
 	if (IS_ERR(rt)) {
 		netdev_dbg(dev, "no route to SSGN %pI4\n",
-			   &pctx->sgsn_addr_ip4.s_addr);
+			   &pctx->peer_addr_ip4.s_addr);
 		dev->stats.tx_carrier_errors++;
 		goto err;
 	}
 
 	if (rt->dst.dev == dev) {
 		netdev_dbg(dev, "circular route to SSGN %pI4\n",
-			   &pctx->sgsn_addr_ip4.s_addr);
+			   &pctx->peer_addr_ip4.s_addr);
 		dev->stats.collisions++;
 		goto err_rt;
 	}
@@ -894,8 +894,8 @@ static void ipv4_pdp_fill(struct pdp_ctx *pctx, struct genl_info *info)
 {
 	pctx->gtp_version = nla_get_u32(info->attrs[GTPA_VERSION]);
 	pctx->af = AF_INET;
-	pctx->sgsn_addr_ip4.s_addr =
-		nla_get_be32(info->attrs[GTPA_SGSN_ADDRESS]);
+	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]);
 
@@ -981,13 +981,13 @@ static int ipv4_pdp_add(struct net_device *dev, struct genl_info *info)
 	switch (pctx->gtp_version) {
 	case GTP_V0:
 		netdev_dbg(dev, "GTPv0-U: new PDP ctx id=%llx ssgn=%pI4 ms=%pI4 (pdp=%p)\n",
-			   pctx->u.v0.tid, &pctx->sgsn_addr_ip4,
+			   pctx->u.v0.tid, &pctx->peer_addr_ip4,
 			   &pctx->ms_addr_ip4, pctx);
 		break;
 	case GTP_V1:
 		netdev_dbg(dev, "GTPv1-U: new PDP ctx id=%x/%x ssgn=%pI4 ms=%pI4 (pdp=%p)\n",
 			   pctx->u.v1.i_tei, pctx->u.v1.o_tei,
-			   &pctx->sgsn_addr_ip4, &pctx->ms_addr_ip4, pctx);
+			   &pctx->peer_addr_ip4, &pctx->ms_addr_ip4, pctx);
 		break;
 	}
 
@@ -1001,7 +1001,7 @@ static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info)
 
 	if (!info->attrs[GTPA_VERSION] ||
 	    !info->attrs[GTPA_LINK] ||
-	    !info->attrs[GTPA_SGSN_ADDRESS] ||
+	    !info->attrs[GTPA_PEER_ADDRESS] ||
 	    !info->attrs[GTPA_MS_ADDRESS])
 		return -EINVAL;
 
@@ -1114,7 +1114,7 @@ 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_SGSN_ADDRESS, pctx->sgsn_addr_ip4.s_addr) ||
+	    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))
 		goto nla_put_failure;
 
@@ -1267,7 +1267,7 @@ static struct nla_policy gtp_genl_policy[GTPA_MAX + 1] = {
 	[GTPA_LINK]		= { .type = NLA_U32, },
 	[GTPA_VERSION]		= { .type = NLA_U32, },
 	[GTPA_TID]		= { .type = NLA_U64, },
-	[GTPA_SGSN_ADDRESS]	= { .type = NLA_U32, },
+	[GTPA_PEER_ADDRESS]	= { .type = NLA_U32, },
 	[GTPA_MS_ADDRESS]	= { .type = NLA_U32, },
 	[GTPA_FLOW]		= { .type = NLA_U16, },
 	[GTPA_NET_NS_FD]	= { .type = NLA_U32, },
diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
index 72a04a0..57d1edb 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -19,7 +19,8 @@ enum gtp_attrs {
 	GTPA_LINK,
 	GTPA_VERSION,
 	GTPA_TID,	/* for GTPv0 only */
-	GTPA_SGSN_ADDRESS,
+	GTPA_PEER_ADDRESS,	/* Remote GSN peer, either SGSN or GGSN */
+#define GTPA_SGSN_ADDRESS GTPA_PEER_ADDRESS /* maintain legacy attr name */
 	GTPA_MS_ADDRESS,
 	GTPA_FLOW,
 	GTPA_NET_NS_FD,
-- 
2.9.3

[PATCH net-next v3 2/2] gtp: support SGSN-side tunnels

[Jonas Bonn]

The GTP-tunnel driver is explicitly GGSN-side as it searches for PDP
contexts based on the incoming packets _destination_ address.  If we
want to place ourselves on the SGSN side of the  tunnel, then we want
to be identifying PDP contexts based on _source_ address.

Let it be noted that in a "real" configuration this module would never
be used:  the SGSN normally does not see IP packets as input.  The
justification for this functionality is for PGW load-testing applications
where the input to the SGSN is locally generally IP traffic.

This patch adds a "role" argument at GTP-link creation time to specify
whether we are on the GGSN or SGSN side of the tunnel; this flag is then
used to determine which part of the IP packet to use in determining
the PDP context.

Signed-off-by: Jonas Bonn <jonas@southpole.se>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 drivers/net/gtp.c            | 41 +++++++++++++++++++++++++++++++----------
 include/uapi/linux/if_link.h |  7 +++++++
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 3806be6..b54d1a3 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -72,6 +72,7 @@ struct gtp_dev {
 	struct net		*net;
 	struct net_device	*dev;
 
+	unsigned int		role;
 	unsigned int		hash_size;
 	struct hlist_head	*tid_hash;
 	struct hlist_head	*addr_hash;
@@ -150,8 +151,8 @@ static struct pdp_ctx *ipv4_pdp_find(struct gtp_dev *gtp, __be32 ms_addr)
 	return NULL;
 }
 
-static bool gtp_check_src_ms_ipv4(struct sk_buff *skb, struct pdp_ctx *pctx,
-				  unsigned int hdrlen)
+static bool gtp_check_ms_ipv4(struct sk_buff *skb, struct pdp_ctx *pctx,
+				  unsigned int hdrlen, unsigned int role)
 {
 	struct iphdr *iph;
 
@@ -160,18 +161,22 @@ static bool gtp_check_src_ms_ipv4(struct sk_buff *skb, struct pdp_ctx *pctx,
 
 	iph = (struct iphdr *)(skb->data + hdrlen);
 
-	return iph->saddr == pctx->ms_addr_ip4.s_addr;
+	if (role == GTP_ROLE_SGSN) {
+		return iph->daddr == pctx->ms_addr_ip4.s_addr;
+	} else {
+		return iph->saddr == pctx->ms_addr_ip4.s_addr;
+	}
 }
 
-/* Check if the inner IP source address in this packet is assigned to any
+/* Check if the inner IP address in this packet is assigned to any
  * existing mobile subscriber.
  */
-static bool gtp_check_src_ms(struct sk_buff *skb, struct pdp_ctx *pctx,
-			     unsigned int hdrlen)
+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:
-		return gtp_check_src_ms_ipv4(skb, pctx, hdrlen);
+		return gtp_check_ms_ipv4(skb, pctx, hdrlen, role);
 	}
 	return false;
 }
@@ -205,7 +210,7 @@ static int gtp0_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb,
 		goto out_rcu;
 	}
 
-	if (!gtp_check_src_ms(skb, pctx, hdrlen)) {
+	if (!gtp_check_ms(skb, pctx, hdrlen, gtp->role)) {
 		netdev_dbg(gtp->dev, "No PDP ctx for this MS\n");
 		ret = -1;
 		goto out_rcu;
@@ -262,7 +267,7 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb,
 		goto out_rcu;
 	}
 
-	if (!gtp_check_src_ms(skb, pctx, hdrlen)) {
+	if (!gtp_check_ms(skb, pctx, hdrlen, gtp->role)) {
 		netdev_dbg(gtp->dev, "No PDP ctx for this MS\n");
 		ret = -1;
 		goto out_rcu;
@@ -491,7 +496,11 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,
 	 * Prepend PDP header with TEI/TID from PDP ctx.
 	 */
 	iph = ip_hdr(skb);
-	pctx = ipv4_pdp_find(gtp, iph->daddr);
+	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);
@@ -666,12 +675,23 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,
 	int hashsize, err, fd0, fd1;
 	struct gtp_dev *gtp;
 	struct gtp_net *gn;
+	unsigned int role;
+
+	if (data[IFLA_GTP_ROLE]) {
+		role = nla_get_u32(data[IFLA_GTP_ROLE]);
+		if (role > GTP_ROLE_SGSN)
+			return -EINVAL;
+	} else {
+		role = GTP_ROLE_GGSN;
+	}
 
 	if (!data[IFLA_GTP_FD0] || !data[IFLA_GTP_FD1])
 		return -EINVAL;
 
 	gtp = netdev_priv(dev);
 
+	gtp->role = role;
+
 	fd0 = nla_get_u32(data[IFLA_GTP_FD0]);
 	fd1 = nla_get_u32(data[IFLA_GTP_FD1]);
 
@@ -723,6 +743,7 @@ static const struct nla_policy gtp_policy[IFLA_GTP_MAX + 1] = {
 	[IFLA_GTP_FD0]			= { .type = NLA_U32 },
 	[IFLA_GTP_FD1]			= { .type = NLA_U32 },
 	[IFLA_GTP_PDP_HASHSIZE]		= { .type = NLA_U32 },
+	[IFLA_GTP_ROLE]			= { .type = NLA_U32 },
 };
 
 static int gtp_validate(struct nlattr *tb[], struct nlattr *data[])
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index ccde456..da62d8c 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -533,11 +533,18 @@ enum {
 #define IFLA_PPP_MAX (__IFLA_PPP_MAX - 1)
 
 /* GTP section */
+
+enum ifla_gtp_role {
+	GTP_ROLE_GGSN = 0,
+	GTP_ROLE_SGSN,
+};
+
 enum {
 	IFLA_GTP_UNSPEC,
 	IFLA_GTP_FD0,
 	IFLA_GTP_FD1,
 	IFLA_GTP_PDP_HASHSIZE,
+	IFLA_GTP_ROLE,
 	__IFLA_GTP_MAX,
 };
 #define IFLA_GTP_MAX (__IFLA_GTP_MAX - 1)
-- 
2.9.3

Re: [PATCH net-next v3 0/2] GTP SGSN-side tunnels

[Harald Welte]

Hi Jonas,

looks fine to me, but I haven't tested it.  Did you manually test it
using the extended libgtpnl + tools?

Also, in code like this:

+       if (gtp->role == GTP_ROLE_SGSN) {
+               pctx = ipv4_pdp_find(gtp, iph->saddr);
+       } else {

I think general Linux kernel coding style is to not have curly-brackets
around single-line blocks. See "Do not unnecessarily use braces where a
single statement will do." in line 169 of
Documentation/process/coding-style.rst

I won't mind your current style, and it is not a blocker issue to me,
but still it would be nice for general consistency.

Acked-by: Harald Welte <laforge@gnumonks.org>

-- 
- 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 v3 0/2] GTP SGSN-side tunnels

[Pablo Neira Ayuso]

On Fri, Mar 24, 2017 at 10:33:56AM +0100, Jonas Bonn wrote:
> Changes since v2:
> 
> * Move #define of legacy netlink attribute into enum
> * Added note to commit message about load-testing use-case
> * Ack from Pablo

Thanks a lot Jonas!

David, please apply. Thanks!

Re: [PATCH net-next v3 0/2] GTP SGSN-side tunnels

[Pablo Neira Ayuso]

On Fri, Mar 24, 2017 at 11:15:25AM +0100, Harald Welte wrote:
> Hi Jonas,
> 
> looks fine to me, but I haven't tested it.  Did you manually test it
> using the extended libgtpnl + tools?
> 
> Also, in code like this:
> 
> +       if (gtp->role == GTP_ROLE_SGSN) {
> +               pctx = ipv4_pdp_find(gtp, iph->saddr);
> +       } else {
> 
> I think general Linux kernel coding style is to not have curly-brackets
> around single-line blocks. See "Do not unnecessarily use braces where a
> single statement will do." in line 169 of
> Documentation/process/coding-style.rst
> 
> I won't mind your current style, and it is not a blocker issue to me,
> but still it would be nice for general consistency.

Harald is right, I overlook this comestic thing.

Re: [PATCH net-next v3 0/2] GTP SGSN-side tunnels

[Sergei Shtylyov]

On 03/24/2017 01:15 PM, Harald Welte wrote:

> looks fine to me, but I haven't tested it.  Did you manually test it
> using the extended libgtpnl + tools?
>
> Also, in code like this:
>
> +       if (gtp->role == GTP_ROLE_SGSN) {
> +               pctx = ipv4_pdp_find(gtp, iph->saddr);
> +       } else {
>
> I think general Linux kernel coding style is to not have curly-brackets
> around single-line blocks. See "Do not unnecessarily use braces where a
> single statement will do." in line 169 of
> Documentation/process/coding-style.rst

    See line 191 in the same file for the exception. :-)

[...]

MBR, Sergei

Re: [PATCH net-next v3 0/2] GTP SGSN-side tunnels

[Jonas Bonn]

Hi Harald,

On 03/24/2017 11:15 AM, Harald Welte wrote:
> Hi Jonas,
>
> looks fine to me, but I haven't tested it.  Did you manually test it
> using the extended libgtpnl + tools?

I have tested it against your libgtpnl branch laforge/sgsn-role. 
Creating an interface with tools/gtp-link works fine.

We have been running the v1 patches in a production load-testing system 
for the last month.  That all works and gives good performance.  I 
haven't been able to actually run the v2 and v3 patches in that 
environment as access to it is a bit limited... that said, the changes 
from v1 to v3 are really only cosmetic and don't even touch the "parts 
that matter" with respect to encapsulation. Really the patch is pretty 
trivial... mucking about with the netlink interface is the bulk of it 
and that bit has been tested and works.

>
> Also, in code like this:
> in
> +       if (gtp->role == GTP_ROLE_SGSN) {
> +               pctx = ipv4_pdp_find(gtp, iph->saddr);
> +       } else {
>
> I think general Linux kernel coding style is to not have curly-brackets
> around single-line blocks. See "Do not unnecessarily use braces where a
> single statement will do." in line 169 of
> Documentation/process/coding-style.rst
>
> I won't mind your current style, and it is not a blocker issue to me,
> but still it would be nice for general consistency.
OK, I'll fix that up just for the sake of correctness and send a v4.

>
> Acked-by: Harald Welte <laforge@gnumonks.org>
>
Thanks,

Jonas

Re: [PATCH net-next v3 0/2] GTP SGSN-side tunnels

[Jonas Bonn]

On 03/24/2017 11:41 AM, Pablo Neira Ayuso wrote:
> On Fri, Mar 24, 2017 at 10:33:56AM +0100, Jonas Bonn wrote:
>> Changes since v2:
>>
>> * Move #define of legacy netlink attribute into enum
>> * Added note to commit message about load-testing use-case
>> * Ack from Pablo
> Thanks a lot Jonas!
>
> David, please apply. Thanks!

David, please apply v4 instead as it has a coding-style fixup requested 
by Harald.

Thanks,

Jonas

Re: [PATCH net-next v3 0/2] GTP SGSN-side tunnels

[Pablo Neira Ayuso]

On Fri, Mar 24, 2017 at 02:57:41PM +0100, Jonas Bonn wrote:
> On 03/24/2017 11:41 AM, Pablo Neira Ayuso wrote:
> >On Fri, Mar 24, 2017 at 10:33:56AM +0100, Jonas Bonn wrote:
> >>Changes since v2:
> >>
> >>* Move #define of legacy netlink attribute into enum
> >>* Added note to commit message about load-testing use-case
> >>* Ack from Pablo
> >Thanks a lot Jonas!
> >
> >David, please apply. Thanks!
> 
> David, please apply v4 instead as it has a coding-style fixup requested by
> Harald.

Right, please apply v4, thanks!