| Message ID | 20170203091231.10142-1-jonas@southpole.se |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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!
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
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
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
----- 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!
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
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!
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!
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
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.
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
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).
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
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.
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 >
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 >>
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
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!
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)
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(-)