[PATCH 1/1] gtp: support SGSN-side tunnels

[PATCH 1/1] 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 write an SGSN, then we want to be idenityfing PDP contexts
based on _source_ address.

This patch adds a "flags" 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>
---

 drivers/net/gtp.c            | 43 ++++++++++++++++++++++++++++++++-----------
 include/uapi/linux/gtp.h     |  2 +-
 include/uapi/linux/if_link.h |  5 +++++
 3 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 50349a9..1bbac69 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		flags;
 	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 flags)
 {
 	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 (flags & GTP_FLAGS_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 flags)
 {
 	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, flags);
 	}
 	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->flags)) {
 		netdev_dbg(gtp->dev, "No PDP ctx for this MS\n");
 		ret = -1;
 		goto out_rcu;
@@ -248,7 +253,7 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb,
 	if (gtp1->flags & GTP1_F_MASK)
 		hdrlen += 4;
 
-	/* Make sure the header is larger enough, including extensions. */
+	/* Make sure the header is large enough, including extensions. */
 	if (!pskb_may_pull(skb, hdrlen))
 		return -1;
 
@@ -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->flags)) {
 		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->flags & GTP_FLAGS_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 flags;
+
+	if (data[IFLA_GTP_FLAGS]) {
+		flags = nla_get_u32(data[IFLA_GTP_FLAGS]);
+		if (flags & ~GTP_FLAGS_MASK)
+			return -EINVAL;
+	} else {
+		flags = 0;
+	}
 
 	if (!data[IFLA_GTP_FD0] || !data[IFLA_GTP_FD1])
 		return -EINVAL;
 
 	gtp = netdev_priv(dev);
 
+	gtp->flags = flags;
+
 	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_FLAGS]		= { .type = NLA_U32 },
 };
 
 static int gtp_validate(struct nlattr *tb[], struct nlattr *data[])
diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
index 72a04a0..79037cc 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -19,7 +19,7 @@ enum gtp_attrs {
 	GTPA_LINK,
 	GTPA_VERSION,
 	GTPA_TID,	/* for GTPv0 only */
-	GTPA_SGSN_ADDRESS,
+	GTPA_SGSN_ADDRESS, /* Remote GSN, either SGSN or GGSN */
 	GTPA_MS_ADDRESS,
 	GTPA_FLOW,
 	GTPA_NET_NS_FD,
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index ccde456..a446e7b 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -533,11 +533,16 @@ enum {
 #define IFLA_PPP_MAX (__IFLA_PPP_MAX - 1)
 
 /* GTP section */
+
+#define GTP_FLAGS_SGSN		(1U << 0)
+#define GTP_FLAGS_MASK		(GTP_FLAGS_SGSN)
+
 enum {
 	IFLA_GTP_UNSPEC,
 	IFLA_GTP_FD0,
 	IFLA_GTP_FD1,
 	IFLA_GTP_PDP_HASHSIZE,
+	IFLA_GTP_FLAGS,
 	__IFLA_GTP_MAX,
 };
 #define IFLA_GTP_MAX (__IFLA_GTP_MAX - 1)
-- 
2.9.3

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Pablo Neira Ayuso]

Hi Jonas,

On Fri, Feb 03, 2017 at 10:12:31AM +0100, Jonas Bonn wrote:
> 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 write an SGSN, then we want to be idenityfing PDP contexts
> based on _source_ address.
> 
> This patch adds a "flags" 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.

So far the implementation that I saw in osmocom relies on userspace code
to tunnel data from ME to the SSGN/SGW running on the base station.

The data we get from GGSN -> SGSN needs to be places into a SN-PDU (via
SNDCP) when sending it to the BTS, right? So I wonder how this can be
useful given that we would need to see real IP packets coming to the
SSGN that we tunnel into GTP.

Thanks!

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Jonas Bonn]

Hi Pablo,

On 02/06/2017 12:08 PM, Pablo Neira Ayuso wrote:
> Hi Jonas,
>
> On Fri, Feb 03, 2017 at 10:12:31AM +0100, Jonas Bonn wrote:
>> 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 write an SGSN, then we want to be idenityfing PDP contexts
>> based on _source_ address.
>>
>> This patch adds a "flags" 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.
> So far the implementation that I saw in osmocom relies on userspace code
> to tunnel data from ME to the SSGN/SGW running on the base station.
>
> The data we get from GGSN -> SGSN needs to be places into a SN-PDU (via
> SNDCP) when sending it to the BTS, right? So I wonder how this can be
> useful given that we would need to see real IP packets coming to the
> SSGN that we tunnel into GTP.

Fair enough.  The use-case I am looking at involves PGW load-testing 
where the simulated load is generated locally on the SGSN so it _is_ 
seeing IP packets and the SNDCP is left out altogether.  Perhaps this is 
too pathological to warrant messing with the upstream driver... I don't 
know: the symmetry does not cost much even if it's of limited use.

Couldn't the SNDCP theoretically be a separate node and push IP packets 
to the SGSN, thus making this useful?  Perhaps it's a stretch...

/Jonas

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Harald Welte]

Dear Jonas,

On Fri, Feb 03, 2017 at 10:12:31AM +0100, Jonas Bonn wrote:
> 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 write an SGSN, then we want to be idenityfing PDP contexts
> based on _source_ address.

A SGSN should never see the "native" User-IP payload.  It either has a
Gb interface towards the BSS which has a BSSGP/NS/UDP/IP protocol
stacking (for GERAN) or an IuUP or GTP stacking (for UTRAN).

The user-ip tunnel (PDP context) exists between the mobile station and
the GGSN.  Any intermediate nodes (BTS, BSC, NodeB, RNC, SGSN, ...) do
not appear as intermediate IP Layer nodes in that User IP tunnel, but
only serve to transparently pass the user-ip inside that tunnel between
the two tunnel end-points.

As such, I am very surprised by your patch.  Exposing the User IP to the
Linux network stack in the SGSN seems to be a severe layering violation
and contradict everything I know about 3GPP network architecture.  But
maybe I'm missing something here? Please explain.

The only SGSN-level user plane acceleration that I can think of is
quite different:

For an UMTS SGSN that only supports IuPS, and only supports IuPS over
IP (which is a sub-class of a sub-class of all use cases), what would
make sense is some Kernel-level support to map from one GTP
socket/tunnel to another GTP socket/tunnel based on the TEIDs.  So
basically you have a GTP tunnel on the RAN side and another GTP tunnel
on the CN side, without any decapsultaion.

The TEIDs on both sides *could* be identical, or *cold* be separate, as
a matter of implementation policy.  

The IP addresses /port numbers on both sides will in almost all
non-laboratory use cases be separate, as an operator typically doesn't
want a transparent/routed IP network between the RAN and Core Network (CN).

So the GTP tunnels between RNC/hNodeB/heNodeB on the RAN side get mapped
1:1 to GTP tunnels between SGSN and GGSN on the CN side.  However, as no
encapsulation/decapsulation is performed, this is outside of the scope
of the current kernel GTP tunneling module.  Rather, it's more something
similar to static NAT between two pairs of addresses.

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 1/1] gtp: support SGSN-side tunnels

[Harald Welte]

Hi Jonas,

On Mon, Feb 06, 2017 at 02:33:07PM +0100, Jonas Bonn wrote:
> Fair enough.  The use-case I am looking at involves PGW load-testing where
> the simulated load is generated locally on the SGSN so it _is_ seeing IP
> packets and the SNDCP is left out altogether.  

Ok, it would have been useful to document that test-only feature in the
changelog and/or code.  Like "support simulated RAN-side tunnels" or
"support SGSN/S-GW simulation".

> Perhaps this is too pathological to warrant messing with the upstream
> driver... I don't know: the symmetry does not cost much even if it's
> of limited use.

There are plenty of features in the mainline kernel related to testing,
see pktgen for example.  So I think if it doesn't impose complexity,
performance issues or stretches the existing architecture, I think
there's no reason to keep it out.

Looking at the code, I think the one conditional on the flags is not
going to kill significant performance of the "normal" use case.  But
that's of course just guessing, without any benchmark to back that up.

Semantically, I'm not sure if the FLAGS and the re-use of the
SGSN_ADDRESS TLV is the best choice.  If suddenly the meaning of the TLV
is "Peer GSN Address" then it should be called that way.  We could have
a #define SGSN_ADDRESS to GSN_PEER_ADDRESS to make old code compile.
I'll let Pablo respond to this as he came up with the netlink interface,
as far as I can remember :)

Also, like with any changes to the kernel and netlink interface code, I
think we should always mandate similar changes to be made to libgtpnl so
the feature can actually be used/tested with the standard
tools/utilities available to anyone.

> Couldn't the SNDCP theoretically be a separate node and push IP packets to
> the SGSN, thus making this useful?  Perhaps it's a stretch...

No, because you would introduce an hop (or even two!) at the IP level,
breaking
* the notion of who the remote IP address is (remote poin-to-point address)
  of the PDP context
* packets get modified (TTL decrement, ...) where they are not supposed to
* you suddenly might get TTL exceeded, dest unreachable, ...) out of
  nowhere into your user IP
* you introduce serious security issues by having the kernel IP routing
  code between the outer IP (the operator RAN/core network) and the
  inner user IP payload.

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 1/1] gtp: support SGSN-side tunnels

[Andreas Schultz]

----- On Feb 6, 2017, at 12:08 PM, pablo pablo@netfilter.org wrote:

> Hi Jonas,
> 
> On Fri, Feb 03, 2017 at 10:12:31AM +0100, Jonas Bonn wrote:
>> 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 write an SGSN, then we want to be idenityfing PDP contexts
>> based on _source_ address.
>> 
>> This patch adds a "flags" 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.
> 
> So far the implementation that I saw in osmocom relies on userspace code
> to tunnel data from ME to the SSGN/SGW running on the base station.
> 
> The data we get from GGSN -> SGSN needs to be places into a SN-PDU (via
> SNDCP) when sending it to the BTS, right? So I wonder how this can be
> useful given that we would need to see real IP packets coming to the
> SSGN that we tunnel into GTP.

Uhm, no, absolutely not. The SGSN is not seeing IP packets, it's seeing
stuff that is supposed to put into GTP tunnels. The only instance in an
EPC (apart from a UE), that knows about the payload content of a GTP tunnel
is the GGSN/PGW. Even with Rel 13 Non IP bearers and CIoT optimization,
the interpretation of the content of an bearer is only done at the PGW.

Andreas
> 
> Thanks!

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Andreas Schultz]

Hi Jonas,

Sorry, for later reply, I'm currently on vacation with almost no
internet access.

----- On Feb 6, 2017, at 2:33 PM, Jonas Bonn jonas@southpole.se wrote:

> Hi Pablo,
> 
> On 02/06/2017 12:08 PM, Pablo Neira Ayuso wrote:
>> Hi Jonas,
>>
>> On Fri, Feb 03, 2017 at 10:12:31AM +0100, Jonas Bonn wrote:
>>> 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 write an SGSN, then we want to be idenityfing PDP contexts
>>> based on _source_ address.
>>>
>>> This patch adds a "flags" 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.
>> So far the implementation that I saw in osmocom relies on userspace code
>> to tunnel data from ME to the SSGN/SGW running on the base station.
>>
>> The data we get from GGSN -> SGSN needs to be places into a SN-PDU (via
>> SNDCP) when sending it to the BTS, right? So I wonder how this can be
>> useful given that we would need to see real IP packets coming to the
>> SSGN that we tunnel into GTP.
> 
> Fair enough.  The use-case I am looking at involves PGW load-testing
> where the simulated load is generated locally on the SGSN so it _is_
> seeing IP packets and the SNDCP is left out altogether.  Perhaps this is
> too pathological to warrant messing with the upstream driver... I don't
> know: the symmetry does not cost much even if it's of limited use.

Sounds reasonable. I'll review change with that in mind next week.

Andreas

> Couldn't the SNDCP theoretically be a separate node and push IP packets
> to the SGSN, thus making this useful?  Perhaps it's a stretch...
> 
> /Jonas

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Pablo Neira Ayuso]

Hi Jonas,

On Mon, Feb 06, 2017 at 02:33:07PM +0100, Jonas Bonn wrote:
> Hi Pablo,
> 
> On 02/06/2017 12:08 PM, Pablo Neira Ayuso wrote:
> >Hi Jonas,
> >
> >On Fri, Feb 03, 2017 at 10:12:31AM +0100, Jonas Bonn wrote:
> >>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 write an SGSN, then we want to be idenityfing PDP contexts
> >>based on _source_ address.
> >>
> >>This patch adds a "flags" 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.
> >So far the implementation that I saw in osmocom relies on userspace code
> >to tunnel data from ME to the SSGN/SGW running on the base station.
> >
> >The data we get from GGSN -> SGSN needs to be places into a SN-PDU (via
> >SNDCP) when sending it to the BTS, right? So I wonder how this can be
> >useful given that we would need to see real IP packets coming to the
> >SSGN that we tunnel into GTP.
> 
> Fair enough.  The use-case I am looking at involves PGW load-testing where
> the simulated load is generated locally on the SGSN so it _is_ seeing IP
> packets and the SNDCP is left out altogether.  Perhaps this is too
> pathological to warrant messing with the upstream driver... I don't know:
> the symmetry does not cost much even if it's of limited use.

Thanks for explaining your use-case.

If some basic form of this load-testing tool ends up serving everyone,
ie. landing some code in the libgtpnl library that we can all use to
benchmark/stress test this driver, then I would be glad to take this.

Thanks!

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Pablo Neira Ayuso]

On Mon, Feb 06, 2017 at 03:16:22PM +0100, Harald Welte wrote:
> Hi Jonas,
> 
> On Mon, Feb 06, 2017 at 02:33:07PM +0100, Jonas Bonn wrote:
> > Fair enough.  The use-case I am looking at involves PGW load-testing where
> > the simulated load is generated locally on the SGSN so it _is_ seeing IP
> > packets and the SNDCP is left out altogether.  
> 
> Ok, it would have been useful to document that test-only feature in the
> changelog and/or code.  Like "support simulated RAN-side tunnels" or
> "support SGSN/S-GW simulation".

Right. Please, include this in your follow up v2 patch description.
BTW, please also indicate [PATCH net-next] for new features.

> > Perhaps this is too pathological to warrant messing with the upstream
> > driver... I don't know: the symmetry does not cost much even if it's
> > of limited use.
> 
> There are plenty of features in the mainline kernel related to testing,
> see pktgen for example.  So I think if it doesn't impose complexity,
> performance issues or stretches the existing architecture, I think
> there's no reason to keep it out.
> 
> Looking at the code, I think the one conditional on the flags is not
> going to kill significant performance of the "normal" use case.  But
> that's of course just guessing, without any benchmark to back that up.
> 
> Semantically, I'm not sure if the FLAGS and the re-use of the
> SGSN_ADDRESS TLV is the best choice.  If suddenly the meaning of the TLV
> is "Peer GSN Address" then it should be called that way.  We could have
> a #define SGSN_ADDRESS to GSN_PEER_ADDRESS to make old code compile.
> I'll let Pablo respond to this as he came up with the netlink interface,
> as far as I can remember :)

I agree with Harald that a new netlink TLV, ie. IFLA_GTP_MODE, to
indicate if this is expecting to operate on the GGSN or SGSN side
would be better. See include/uapi/linux/if_link.h.

Flags allows us to combine different features, in this case we won't
combine anything since these two modes are mutually exclusive.

> Also, like with any changes to the kernel and netlink interface code, I
> think we should always mandate similar changes to be made to libgtpnl so
> the feature can actually be used/tested with the standard
> tools/utilities available to anyone.

Yes, at least some scripts and short text file example would suffice.

Thanks!

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Andreas Schultz]

Hi,

I'm a bit late to comment, but maybe you can consider an additional
change for v2...

----- On Feb 3, 2017, at 10:12 AM, Jonas Bonn jonas@southpole.se wrote:

> 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 write an SGSN, then we want to be idenityfing PDP contexts
> based on _source_ address.
> 
> This patch adds a "flags" 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>
> ---
> 
> drivers/net/gtp.c            | 43 ++++++++++++++++++++++++++++++++-----------
> include/uapi/linux/gtp.h     |  2 +-
> include/uapi/linux/if_link.h |  5 +++++
> 3 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
> index 50349a9..1bbac69 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		flags;

This should IMHO not go into the gtp_dev, the right place
is the PDP context.

In the current design the netdevice might seem like the logical
place, but the relation between tunnels and netdevices is actually
wrong. This leaves the PDP context the only correct place.

I currently have an ongoing discussion with Pablo about this. Harald
seems already convinced (http://marc.info/?l=linux-netdev&m=148638992010344&w=2).

> 	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 flags)
> {
> 	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 (flags & GTP_FLAGS_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 flags)
> {
> 	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, flags);
> 	}
> 	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->flags)) {
> 		netdev_dbg(gtp->dev, "No PDP ctx for this MS\n");
> 		ret = -1;
> 		goto out_rcu;
> @@ -248,7 +253,7 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct
> sk_buff *skb,
> 	if (gtp1->flags & GTP1_F_MASK)
> 		hdrlen += 4;
> 
> -	/* Make sure the header is larger enough, including extensions. */
> +	/* Make sure the header is large enough, including extensions. */
> 	if (!pskb_may_pull(skb, hdrlen))
> 		return -1;
> 
> @@ -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->flags)) {
> 		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->flags & GTP_FLAGS_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 flags;
> +
> +	if (data[IFLA_GTP_FLAGS]) {
> +		flags = nla_get_u32(data[IFLA_GTP_FLAGS]);
> +		if (flags & ~GTP_FLAGS_MASK)
> +			return -EINVAL;
> +	} else {
> +		flags = 0;
> +	}
> 
> 	if (!data[IFLA_GTP_FD0] || !data[IFLA_GTP_FD1])
> 		return -EINVAL;
> 
> 	gtp = netdev_priv(dev);
> 
> +	gtp->flags = flags;
> +
> 	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_FLAGS]		= { .type = NLA_U32 },
> };
> 
> static int gtp_validate(struct nlattr *tb[], struct nlattr *data[])
> diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
> index 72a04a0..79037cc 100644
> --- a/include/uapi/linux/gtp.h
> +++ b/include/uapi/linux/gtp.h
> @@ -19,7 +19,7 @@ enum gtp_attrs {
> 	GTPA_LINK,
> 	GTPA_VERSION,
> 	GTPA_TID,	/* for GTPv0 only */
> -	GTPA_SGSN_ADDRESS,
> +	GTPA_SGSN_ADDRESS, /* Remote GSN, either SGSN or GGSN */
> 	GTPA_MS_ADDRESS,
> 	GTPA_FLOW,
> 	GTPA_NET_NS_FD,
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index ccde456..a446e7b 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -533,11 +533,16 @@ enum {
> #define IFLA_PPP_MAX (__IFLA_PPP_MAX - 1)
> 
> /* GTP section */
> +
> +#define GTP_FLAGS_SGSN		(1U << 0)
> +#define GTP_FLAGS_MASK		(GTP_FLAGS_SGSN)
> +
> enum {
> 	IFLA_GTP_UNSPEC,
> 	IFLA_GTP_FD0,
> 	IFLA_GTP_FD1,
> 	IFLA_GTP_PDP_HASHSIZE,
> +	IFLA_GTP_FLAGS,
> 	__IFLA_GTP_MAX,
> };
> #define IFLA_GTP_MAX (__IFLA_GTP_MAX - 1)
> --
> 2.9.3

Andreas

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Pablo Neira Ayuso]

On Mon, Feb 13, 2017 at 10:25:19AM +0100, Andreas Schultz wrote:
> Hi,
> 
> I'm a bit late to comment, but maybe you can consider an additional
> change for v2...
> 
> ----- On Feb 3, 2017, at 10:12 AM, Jonas Bonn jonas@southpole.se wrote:
> 
> > 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 write an SGSN, then we want to be idenityfing PDP contexts
> > based on _source_ address.
> > 
> > This patch adds a "flags" 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>
> > ---
> > 
> > drivers/net/gtp.c            | 43 ++++++++++++++++++++++++++++++++-----------
> > include/uapi/linux/gtp.h     |  2 +-
> > include/uapi/linux/if_link.h |  5 +++++
> > 3 files changed, 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
> > index 50349a9..1bbac69 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		flags;
> 
> This should IMHO not go into the gtp_dev, the right place
> is the PDP context.

So you want to allow mixed configurations where some PDP ctx may be in
SGSN mode while others in GGSN.

This doesn't make any sense to me. On top of this, don't forget this
is just for testing, so I don't see any valid usecase for such a fine
grain thing.

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Andreas Schultz]

Hi,

----- On Feb 13, 2017, at 12:16 PM, pablo pablo@netfilter.org wrote:

> On Mon, Feb 13, 2017 at 10:25:19AM +0100, Andreas Schultz wrote:
>> Hi,
>> 
>> I'm a bit late to comment, but maybe you can consider an additional
>> change for v2...
>> 
>> ----- On Feb 3, 2017, at 10:12 AM, Jonas Bonn jonas@southpole.se wrote:
>> 
>> > 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 write an SGSN, then we want to be idenityfing PDP contexts
>> > based on _source_ address.
>> > 
>> > This patch adds a "flags" 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>
>> > ---
>> > 
>> > drivers/net/gtp.c            | 43 ++++++++++++++++++++++++++++++++-----------
>> > include/uapi/linux/gtp.h     |  2 +-
>> > include/uapi/linux/if_link.h |  5 +++++
>> > 3 files changed, 38 insertions(+), 12 deletions(-)
>> > 
>> > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
>> > index 50349a9..1bbac69 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		flags;
>> 
>> This should IMHO not go into the gtp_dev, the right place
>> is the PDP context.
> 
> So you want to allow mixed configurations where some PDP ctx may be in
> SGSN mode while others in GGSN.
> 
> This doesn't make any sense to me. On top of this, don't forget this
> is just for testing, so I don't see any valid usecase for such a fine
> grain thing.

You are right, running such a configuration does not make sense.
However, when I wrote this the PDP context looked like the most
sensible palace to me.

Anyhow, thinking about this again, I think that integrating that flag
in a rewrite of the validation logic in the Rx path make more sense.
Currently we validate the MS as soon as we have found the PDP context.
This should be delayed a bit and the validation should happen after
pulling the GTP header and right before injecting the payload into
the net device. The flag would then indeed go into the gtp_dev.

Andreas

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Harald Welte]

Hi Andreas, Pablo, Jonas,

I think that the SGSN/GGSN role flag (or whatever it may end up being
called) logically belongs in the gtp-device at this point, and will in
the future belong to the UDP/GTP-socket (with Andreas' proposed
changes).   Having it per-pdp-context indeed seems odd and just provide
a way to create broken configurations (and increase the memory use per
pdp context, of which you have many more than netdevs or gtp-sockets).

-- 
- 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 1/1] gtp: support SGSN-side tunnels

[Harald Welte]

Hi Jonas,

are you working on the review feedback that was provided back in early
February?  I think there were some comments like
* remove unrelated cosmetic change in comment
* change from FLAGS to a dedicated MODE netlink attribute
* add libgtpnl code and some usage information or even sample scripts

I would definitely like to see this move forward, particularly in order
to test the GGSN-side code.

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 1/1] gtp: support SGSN-side tunnels

[Pablo Neira Ayuso]

On Wed, Mar 15, 2017 at 05:39:16PM +0100, Harald Welte wrote:
> Hi Jonas,
> 
> are you working on the review feedback that was provided back in early
> February?  I think there were some comments like
> * remove unrelated cosmetic change in comment
> * change from FLAGS to a dedicated MODE netlink attribute
> * add libgtpnl code and some usage information or even sample scripts
> 
> I would definitely like to see this move forward, particularly in order
> to test the GGSN-side code.

Agreed.

It would be good if we provide a way to configure GTP via iproute2 for
testing purposes. We would need to create some dummy socket from
kernel too though so we don't need any userspace daemon for this
testing mode.

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Harald Welte]

Hi Pablo,

On Wed, Mar 15, 2017 at 06:23:48PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Mar 15, 2017 at 05:39:16PM +0100, Harald Welte wrote:
> > 
> > I would definitely like to see this move forward, particularly in order
> > to test the GGSN-side code.
> 
> Agreed.

I've modified the patch slightly, see below (compile-tested, but not
otherwise tested yet).  Basically rename the flags attribute to 'role',
expand the commit log and removed unrelated cosmetic changes.

I've also prepared a corresponding change to libgtpnl into the
laforge/sgsn-rol branch, see
http://git.osmocom.org/libgtpnl/commit/?h=laforge/sgsn-role

This is not yet tested in any way, but I'm planning to add some
associated support to the command line tools and then give it some
testing (both against the kernel GTP in GGSN mode, as well as an
independent userspace GTP implementation).

> It would be good if we provide a way to configure GTP via iproute2 for
> testing purposes.

I don't really care about which tool is used, as long as it is easily
available [and FOSS, of course].

> We would need to create some dummy socket from
> kernel too though so we don't need any userspace daemon for this
> testing mode.

I don't really like that latter idea. It sounds too much like a hack to
me.  But then, I don't have enough phantasy right now ti imagine how an
actual implementation would look like.

To me, it is perfectly fine to run a simple, small utility in userspace
even for testing.

Regards,
	Harald

>From 63920950f9498069993def78e178bde85c174e0c Mon Sep 17 00:00:00 2001
From: Jonas Bonn <jonas@southpole.se>
Date: Wed, 15 Mar 2017 17:52:28 +0100
Subject: [PATCH] gtp: support SGSN-side tunnels

The GTP-tunnel driver is explicitly GGSN-side as it searches for PDP
contexts based on the incoming packets _destination_ address.  For
real-world use cases, this is sufficient, as the other side of a GTP
tunnel is not in fact implemented by GTP, but by the protocol stacking
of a mobile station / user equipment on the radio interface (like PDCP,
SNDCP).

However, if we want to simulate the mobile station, radio access network
and SGSN (for example to test the GGSN side implementation), then we
want to be identifying PDP contexts based on _source_ address.

This patch adds a "role" attribute at GTP-link creation time to specify
whether we behave like the GGSN or SGSN role of the tunnel; this
attribute 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>
Signed-off-by: Harald Welte <laforge@gnumonks.org>

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 99d3df788ce8..9aef4217f6e1 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -71,6 +71,7 @@ struct gtp_dev {
 
 	struct net_device	*dev;
 
+	enum ifla_gtp_role	role;
 	unsigned int		hash_size;
 	struct hlist_head	*tid_hash;
 	struct hlist_head	*addr_hash;
@@ -149,8 +150,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, enum ifla_gtp_role role)
 {
 	struct iphdr *iph;
 
@@ -159,18 +160,21 @@ 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
  * 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, enum ifla_gtp_role 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;
 }
@@ -204,7 +208,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;
@@ -261,7 +265,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;
@@ -490,7 +494,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);
@@ -665,12 +673,26 @@ 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;
+
+	/* The default role is GGSN, and for backwards compatibility
+	 * reasons, the lack of a IFLA_GTP_ROLE IE means that we're
+	 * operating in GGSN role. */
+	if (data[IFLA_GTP_ROLE]) {
+		role = nla_get_u32(data[IFLA_GTP_ROLE]);
+		if (role != GTP_ROLE_GGSN &&
+		    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]);
 
@@ -722,6 +744,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/gtp.h b/include/uapi/linux/gtp.h
index 72a04a0e8cce..79037cca6b51 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -19,7 +19,7 @@ enum gtp_attrs {
 	GTPA_LINK,
 	GTPA_VERSION,
 	GTPA_TID,	/* for GTPv0 only */
-	GTPA_SGSN_ADDRESS,
+	GTPA_SGSN_ADDRESS, /* Remote GSN, either SGSN or GGSN */
 	GTPA_MS_ADDRESS,
 	GTPA_FLOW,
 	GTPA_NET_NS_FD,
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 6b13e591abc9..de7bea223749 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -536,11 +536,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.11.0

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Harald Welte]

On Wed, Mar 15, 2017 at 08:10:38PM +0100, Harald Welte wrote:
> I've modified the patch slightly, see below (compile-tested, but not
> otherwise tested yet).  Basically rename the flags attribute to 'role',
> expand the commit log and removed unrelated cosmetic changes.

I also have a version against current net-next/master, in case anyone is interested..

>From 3274a3303d1ec997392a07a92666d57b13997658 Mon Sep 17 00:00:00 2001
From: Jonas Bonn <jonas@southpole.se>
Date: Wed, 15 Mar 2017 20:24:28 +0100
Subject: [PATCH] gtp: support SGSN-side tunnels

The GTP-tunnel driver is explicitly GGSN-side as it searches for PDP
contexts based on the incoming packets _destination_ address.  For
real-world use cases, this is sufficient, as the other side of a GTP
tunnel is not in fact implemented by GTP, but by the protocol stacking
of a mobile station / user equipment on the radio interface (like PDCP,
SNDCP).

However, if we want to simulate the mobile station, radio access network
and SGSN (for example to test the GGSN side implementation), then we
want to be identifying PDP contexts based on _source_ address.

This patch adds a "role" attribute at GTP-link creation time to specify
whether we behave like the GGSN or SGSN role of the tunnel; this
attribute 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>
Signed-off-by: Harald Welte <laforge@gnumonks.org>
---
 drivers/net/gtp.c            | 46 +++++++++++++++++++++++++++++++++-----------
 include/uapi/linux/gtp.h     |  2 +-
 include/uapi/linux/if_link.h |  7 +++++++
 3 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 3e1854f34420..3ab593b9be85 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -74,6 +74,7 @@ struct gtp_dev {
 
 	struct net_device	*dev;
 
+	enum ifla_gtp_role	role;
 	unsigned int		hash_size;
 	struct hlist_head	*tid_hash;
 	struct hlist_head	*addr_hash;
@@ -154,8 +155,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, enum ifla_gtp_role role)
 {
 	struct iphdr *iph;
 
@@ -164,27 +165,31 @@ 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
  * 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, enum ifla_gtp_role 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;
 }
 
-static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb, unsigned int hdrlen)
+static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb, unsigned int hdrlen,
+		  enum ifla_gtp_role role)
 {
 	struct pcpu_sw_netstats *stats;
 
-	if (!gtp_check_src_ms(skb, pctx, hdrlen)) {
+	if (!gtp_check_ms(skb, pctx, hdrlen, role)) {
 		netdev_dbg(pctx->dev, "No PDP ctx for this MS\n");
 		return 1;
 	}
@@ -239,7 +244,7 @@ static int gtp0_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
 		return 1;
 	}
 
-	return gtp_rx(pctx, skb, hdrlen);
+	return gtp_rx(pctx, skb, hdrlen, gtp->role);
 }
 
 static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
@@ -281,7 +286,7 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
 		return 1;
 	}
 
-	return gtp_rx(pctx, skb, hdrlen);
+	return gtp_rx(pctx, skb, hdrlen, gtp->role);
 }
 
 static void gtp_encap_destroy(struct sock *sk)
@@ -481,7 +486,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);
@@ -631,13 +640,27 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,
 {
 	struct gtp_dev *gtp;
 	struct gtp_net *gn;
+	unsigned int role;
 	int hashsize, err;
 
+	/* The default role is GGSN, and for backwards compatibility
+	 * reasons, the lack of a IFLA_GTP_ROLE IE means that we're
+	 * operating in GGSN role. */
+	if (data[IFLA_GTP_ROLE]) {
+		role = nla_get_u32(data[IFLA_GTP_ROLE]);
+		if (role != GTP_ROLE_GGSN &&
+		    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;
+
 	err = gtp_encap_enable(gtp, data);
 	if (err < 0)
 		return err;
@@ -685,6 +708,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/gtp.h b/include/uapi/linux/gtp.h
index 72a04a0e8cce..79037cca6b51 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -19,7 +19,7 @@ enum gtp_attrs {
 	GTPA_LINK,
 	GTPA_VERSION,
 	GTPA_TID,	/* for GTPv0 only */
-	GTPA_SGSN_ADDRESS,
+	GTPA_SGSN_ADDRESS, /* Remote GSN, either SGSN or GGSN */
 	GTPA_MS_ADDRESS,
 	GTPA_FLOW,
 	GTPA_NET_NS_FD,
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 320fc1e747ee..8b405afb2376 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -538,11 +538,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.11.0

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Pablo Neira Ayuso]

Hi Harald,

On Wed, Mar 15, 2017 at 08:10:38PM +0100, Harald Welte wrote:
> I've modified the patch slightly, see below (compile-tested, but not
> otherwise tested yet).  Basically rename the flags attribute to 'role',
> expand the commit log and removed unrelated cosmetic changes.
> 
> I've also prepared a corresponding change to libgtpnl into the
> laforge/sgsn-rol branch, see
> http://git.osmocom.org/libgtpnl/commit/?h=laforge/sgsn-role
> 
> This is not yet tested in any way, but I'm planning to add some
> associated support to the command line tools and then give it some
> testing (both against the kernel GTP in GGSN mode, as well as an
> independent userspace GTP implementation).

Thanks Harald.

> > It would be good if we provide a way to configure GTP via iproute2 for
> > testing purposes.
> 
> I don't really care about which tool is used, as long as it is easily
> available [and FOSS, of course].
>
> > We would need to create some dummy socket from
> > kernel too though so we don't need any userspace daemon for this
> > testing mode.
> 
> I don't really like that latter idea. It sounds too much like a hack to
> me.  But then, I don't have enough phantasy right now ti imagine how an
> actual implementation would look like.

It's not that far away, we can just create the udp socket from
kernelspace via udp_sock_create() in the test mode. So we don't need
to pass the file descriptor from userspace. But not asking you to work
on this, just an idea.

> To me, it is perfectly fine to run a simple, small utility in userspace
> even for testing.

No problem.

[PATCH v2 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>
---
 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..c51ebb0 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -19,7 +19,7 @@ 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 */
 	GTPA_MS_ADDRESS,
 	GTPA_FLOW,
 	GTPA_NET_NS_FD,
@@ -29,5 +29,6 @@ enum gtp_attrs {
 	__GTPA_MAX,
 };
 #define GTPA_MAX (__GTPA_MAX + 1)
+#define GTPA_SGSN_ADDRESS GTPA_PEER_ADDRESS /* maintain legacy attr name */
 
 #endif /* _UAPI_LINUX_GTP_H_ */
-- 
2.9.3

[PATCH v2 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 write an SGSN, then we want to be idenityfing PDP contexts
based on _source_ address.

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>
---
 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 v2 1/2] gtp: rename SGSN netlink attribute

[Pablo Neira Ayuso]

On Tue, Mar 21, 2017 at 04:04:29PM +0100, Jonas Bonn wrote:
> diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
> index 72a04a0..c51ebb0 100644
> --- a/include/uapi/linux/gtp.h
> +++ b/include/uapi/linux/gtp.h
> @@ -19,7 +19,7 @@ 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 */

We need here:

#define GTPA_SGSN_ADDRESS GTPA_PEER_ADDRESS

for backward compatibility. Anything that is exposed through uapi
cannot be changed.

>  	GTPA_MS_ADDRESS,
>  	GTPA_FLOW,
>  	GTPA_NET_NS_FD,
> @@ -29,5 +29,6 @@ enum gtp_attrs {
>  	__GTPA_MAX,
>  };
>  #define GTPA_MAX (__GTPA_MAX + 1)
> +#define GTPA_SGSN_ADDRESS GTPA_PEER_ADDRESS /* maintain legacy attr name */
>  
>  #endif /* _UAPI_LINUX_GTP_H_ */
> -- 
> 2.9.3
> 

Re: [PATCH v2 1/2] gtp: rename SGSN netlink attribute

[Jonas Bonn]

On 03/21/2017 04:07 PM, Pablo Neira Ayuso wrote:
> On Tue, Mar 21, 2017 at 04:04:29PM +0100, Jonas Bonn wrote:
>> diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
>> index 72a04a0..c51ebb0 100644
>> --- a/include/uapi/linux/gtp.h
>> +++ b/include/uapi/linux/gtp.h
>> @@ -19,7 +19,7 @@ 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 */
> We need here:
>
> #define GTPA_SGSN_ADDRESS GTPA_PEER_ADDRESS
>
> for backward compatibility. Anything that is exposed through uapi
> cannot be changed.

Yes... look a couple of lines further down...

>>   	GTPA_MS_ADDRESS,
>>   	GTPA_FLOW,
>>   	GTPA_NET_NS_FD,
>> @@ -29,5 +29,6 @@ enum gtp_attrs {
>>   	__GTPA_MAX,
>>   };
>>   #define GTPA_MAX (__GTPA_MAX + 1)
>> +#define GTPA_SGSN_ADDRESS GTPA_PEER_ADDRESS /* maintain legacy attr name */

^^^^... there it is! :)

/Jonas
>>   
>>   #endif /* _UAPI_LINUX_GTP_H_ */
>> -- 
>> 2.9.3
>>

Re: [PATCH 1/1] gtp: support SGSN-side tunnels

[Jonas Bonn]

Hi Harald,

On 03/15/2017 05:39 PM, Harald Welte wrote:
> Hi Jonas,
>
> are you working on the review feedback that was provided back in early
> February?  I think there were some comments like
> * remove unrelated cosmetic change in comment
> * change from FLAGS to a dedicated MODE netlink attribute
> * add libgtpnl code and some usage information or even sample scripts
>
> I would definitely like to see this move forward, particularly in order
> to test the GGSN-side code.

Sorry for the delay in this.

I've sent, just now, revised patches to the kernel module.

I was going to send some libgtpnl patches but I noticed, when gathering 
these up, that you have already made most of the necessary changes on a 
new branch.  What you've got there is almost identical to what I've got.

Since you used the terminology "role" instead of "mode" in your libgtpnl 
branch, I made the corresponding change in the kernel module, too, so it 
now calls this role instead of mode.

Regards,
Jonas




>
> Regards,
> 	Harald

Re: [PATCH v2 1/2] gtp: rename SGSN netlink attribute

[Pablo Neira Ayuso]

On Tue, Mar 21, 2017 at 04:10:26PM +0100, Jonas Bonn wrote:
> On 03/21/2017 04:07 PM, Pablo Neira Ayuso wrote:
> >On Tue, Mar 21, 2017 at 04:04:29PM +0100, Jonas Bonn wrote:
> >>diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
> >>index 72a04a0..c51ebb0 100644
> >>--- a/include/uapi/linux/gtp.h
> >>+++ b/include/uapi/linux/gtp.h
> >>@@ -19,7 +19,7 @@ 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 */
> >We need here:
> >
> >#define GTPA_SGSN_ADDRESS GTPA_PEER_ADDRESS
> >
> >for backward compatibility. Anything that is exposed through uapi
> >cannot be changed.
> 
> Yes... look a couple of lines further down...
> 
> >>  	GTPA_MS_ADDRESS,
> >>  	GTPA_FLOW,
> >>  	GTPA_NET_NS_FD,
> >>@@ -29,5 +29,6 @@ enum gtp_attrs {
> >>  	__GTPA_MAX,
> >>  };
> >>  #define GTPA_MAX (__GTPA_MAX + 1)
> >>+#define GTPA_SGSN_ADDRESS GTPA_PEER_ADDRESS /* maintain legacy attr name */
> 
> ^^^^... there it is! :)

Oh right!

Please, move it there to the enum definition, just after the new
GTPA_PEER_ADDRESS. We usually do this in other areas of the networking
code.

You also have to resubmit indicating net-next in your patch subject,
ie.  [PATCH net-next v3 1/2] gtp: rename SGSN netlink attribute

David usually requests that you explicitly indicate the target tree in
some way.

Thanks!

Re: [PATCH v2 1/2] gtp: rename SGSN netlink attribute

[David Miller]

Can I get a review of these two patches from some GTP experts?

Thanks.

Re: [PATCH v2 1/2] gtp: rename SGSN netlink attribute

[Pablo Neira Ayuso]

On Thu, Mar 23, 2017 at 02:16:22PM -0700, David Miller wrote:
> Can I get a review of these two patches from some GTP experts?

I asked Jonas to resend indicating [PATCH net-next] in the subject and
some minor comestic change.

Apart from patches look good so Jonas, you can add this in your follow
up version.

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>