From: Tom Herbert <hidden> Date: 2017-09-19 00:39:23
This patch set builds upon the initial GTP implementation to make
support closer to that enjoyed by other encapsulation protocols.
The major items are:
- IPv6 support
- Configurable networking interfaces so that GTP kernel can be
used and tested without needing GSN network emulation (i.e. no user
space daemon needed).
- GSO,GRO
- Control of zero UDP checksums
- Port numbers are configurable
- Addition of a dst_cache in the GTP structure and other cleanup
Additionally, this patch set also includes a couple of general support
capabilities:
- A facility that allows application specific GSO callbacks
- Common functions to get a route fo for an IP tunnel
For IPv6 support, the mobile subscriber needs to allow IPv6 addresses,
and the remote enpoint can be IPv6.
For configurable interfaces, configuration is added to allow an
alterate means to configure a GTP and device. This follows the
typical UDP encapsulation model of specifying a listener port for
receive, and a remote address and port for transmit.
GRO was straightfoward to implement following the model of other
UDP encapsulations.
Providing GSO support had one wrinkle-- the GTP header includes a
payload length field that needs to be set per GSO segment. In order
to address that in a general way, I create the concept of
application specific GSO.
To implement application layer GSO I reserved the top four bits of
shinfo(skb)->gso_type. The idea is that an application or encapsulation
protocol (like GTP in this case) can register a GSO segment callback.
The facility returns a gso_type with upper four bits set to a value
(index into a table). When the application sets up a packet it includes
the code in the gso_type for the skb. At some point (e.g. from UDP
segment) the gso_type is checked in the skb and if the application
specific GSO is indicated then the callback is called. The
registered callbacks include a set of other gso_types so that
an application callback can be matched to an appropriate instance.
FOr instance, the GTP callback checks for the UDP GSO flags.
Zero UDP checksum, port number configuration, and dst_cache are
straightforwad.
Configuration is performed by iproute2/ip. I will post that
in a subsequent patch set.
Tested:
Configured the matrix of IPv4/IPv6 mobile subscriber, IPv4/IPv6 remote
peer, and GTP version 0 and 1 (eight combinations). Observed
connectivity and proper GSO/GRO. Also, tested VXLAN for
regression.
Tom Herbert (14):
iptunnel: Add common functions to get a tunnel route
vxlan: Call common functions to get tunnel routes
gtp: Call common functions to get tunnel routes and add dst_cache
gtp: udp recv clean up
gtp: Remove special mtu handling
gtp: Eliminate pktinfo and add port configuration
gtp: Support encapsulation of IPv6 packets
gtp: Support encpasulating over IPv6
gtp: Allow configuring GTP interface as standalone
gtp: Add support for devnet
net: Add a facility to support application defined GSO
gtp: Configuration for zero UDP checksum
gtp: Support for GRO
gtp: GSO support
drivers/net/gtp.c | 1300 ++++++++++++++++++++++++++++++++----------
drivers/net/vxlan.c | 84 +--
include/linux/netdevice.h | 31 +
include/linux/skbuff.h | 25 +
include/net/ip6_tunnel.h | 33 ++
include/net/ip_tunnels.h | 33 ++
include/uapi/linux/gtp.h | 8 +
include/uapi/linux/if_link.h | 6 +
net/core/dev.c | 47 ++
net/ipv4/ip_tunnel.c | 41 ++
net/ipv4/ip_tunnel_core.c | 6 +
net/ipv4/udp_offload.c | 20 +-
net/ipv6/ip6_tunnel.c | 43 ++
13 files changed, 1306 insertions(+), 371 deletions(-)
--
2.11.0
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:32
ip_tunnel_get_route and ip6_tnl_get_route are create to return
routes for a tunnel. These functions are derived from the VXLAN
functions.
Signed-off-by: Tom Herbert <redacted>
---
include/net/ip6_tunnel.h | 33 +++++++++++++++++++++++++++++++++
include/net/ip_tunnels.h | 33 +++++++++++++++++++++++++++++++++
net/ipv4/ip_tunnel.c | 41 +++++++++++++++++++++++++++++++++++++++++
net/ipv6/ip6_tunnel.c | 43 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 150 insertions(+)
@@ -935,6 +935,47 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)}EXPORT_SYMBOL_GPL(ip_tunnel_ioctl);+structrtable*__ip_tunnel_get_route(structnet_device*dev,+structsk_buff*skb,u8proto,+intoif,u8tos,+__be32daddr,__be32*saddr,+__be16dport,__be16sport,+structdst_cache*dst_cache,+conststructip_tunnel_info*info,+booluse_cache)+{+structrtable*rt=NULL;+structflowi4fl4;++memset(&fl4,0,sizeof(fl4));+fl4.flowi4_oif=oif;+fl4.flowi4_tos=RT_TOS(tos);+fl4.flowi4_mark=skb->mark;+fl4.flowi4_proto=proto;+fl4.daddr=daddr;+fl4.saddr=*saddr;+fl4.fl4_dport=dport;+fl4.fl4_sport=sport;++rt=ip_route_output_key(dev_net(dev),&fl4);+if(likely(!IS_ERR(rt))){+if(rt->dst.dev==dev){+netdev_dbg(dev,"circular route to %pI4\n",&daddr);+ip_rt_put(rt);+returnERR_PTR(-ELOOP);+}++*saddr=fl4.saddr;+if(use_cache)+dst_cache_set_ip4(dst_cache,&rt->dst,fl4.saddr);+}else{+netdev_dbg(dev,"no route to %pI4\n",&daddr);+returnERR_PTR(-ENETUNREACH);+}+returnrt;+}+EXPORT_SYMBOL_GPL(__ip_tunnel_get_route);+int__ip_tunnel_change_mtu(structnet_device*dev,intnew_mtu,boolstrict){structip_tunnel*tunnel=netdev_priv(dev);
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:36
Call ip_tunnel_get_route and ip6_tnl_get_route to handle getting a route
and dealing with the dst_cache.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/vxlan.c | 84 ++++-------------------------------------------------
1 file changed, 5 insertions(+), 79 deletions(-)
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:39
Call ip_tunnel_get_route and dst_cache to pdp context which should
improve performance by obviating the need to perform a route lookup
on every packet.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 59 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 32 insertions(+), 27 deletions(-)
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:42
Create separate UDP receive functions for GTP version 0 and version 1.
Set encap_rcv appropriately when configuring a socket. Also, convert to
using gro_cells.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 130 +++++++++++++++++++++++++++++-------------------------
1 file changed, 71 insertions(+), 59 deletions(-)
@@ -217,55 +219,83 @@ static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,stats->rx_bytes+=skb->len;u64_stats_update_end(&stats->syncp);-netif_rx(skb);+gro_cells_receive(>p->gro_cells,skb);+return0;}-/* 1 means pass up to the stack, -1 means drop and 0 means decapsulated. */-staticintgtp0_udp_encap_recv(structgtp_dev*gtp,structsk_buff*skb)+/* UDP encapsulation receive handler for GTPv0-U . See net/ipv4/udp.c.+*Returncodes:0:success,<0:error,>0:passuptouserspaceUDPsocket.+*/+staticintgtp0_udp_encap_recv(structsock*sk,structsk_buff*skb){+structgtp_dev*gtp=rcu_dereference_sk_user_data(sk);unsignedinthdrlen=sizeof(structudphdr)+sizeof(structgtp0_header);structgtp0_header*gtp0;structpdp_ctx*pctx;+if(!gtp)+gotopass;+if(!pskb_may_pull(skb,hdrlen))-return-1;+gotodrop;gtp0=(structgtp0_header*)(skb->data+sizeof(structudphdr));if((gtp0->flags>>5)!=GTP_V0)-return1;+gotopass;if(gtp0->type!=GTP_TPDU)-return1;+gotopass;++netdev_dbg(gtp->dev,"received GTP0 packet\n");pctx=gtp0_pdp_find(gtp,be64_to_cpu(gtp0->tid));if(!pctx){netdev_dbg(gtp->dev,"No PDP ctx to decap skb=%p\n",skb);-return1;+gotopass;+}++if(!gtp_rx(pctx,skb,hdrlen,gtp->role)){+/* Successfully received */+return0;}-returngtp_rx(pctx,skb,hdrlen,gtp->role);+drop:+kfree_skb(skb);+return0;++pass:+return1;}-staticintgtp1u_udp_encap_recv(structgtp_dev*gtp,structsk_buff*skb)+/* UDP encapsulation receive handler for GTPv0-U . See net/ipv4/udp.c.+*Returncodes:0:success,<0:error,>0:passuptouserspaceUDPsocket.+*/+staticintgtp1u_udp_encap_recv(structsock*sk,structsk_buff*skb){+structgtp_dev*gtp=rcu_dereference_sk_user_data(sk);unsignedinthdrlen=sizeof(structudphdr)+sizeof(structgtp1_header);structgtp1_header*gtp1;structpdp_ctx*pctx;+if(!gtp)+gotopass;+if(!pskb_may_pull(skb,hdrlen))-return-1;+gotodrop;gtp1=(structgtp1_header*)(skb->data+sizeof(structudphdr));if((gtp1->flags>>5)!=GTP_V1)-return1;+gotopass;if(gtp1->type!=GTP_TPDU)-return1;+gotopass;++netdev_dbg(gtp->dev,"received GTP1 packet\n");/* From 29.060: "This field shall be present if and only if any one or*moreoftheS,PNandEflagsareset.".
@@ -278,17 +308,27 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)/* Make sure the header is larger enough, including extensions. */if(!pskb_may_pull(skb,hdrlen))-return-1;+gotodrop;gtp1=(structgtp1_header*)(skb->data+sizeof(structudphdr));pctx=gtp1_pdp_find(gtp,ntohl(gtp1->tid));if(!pctx){netdev_dbg(gtp->dev,"No PDP ctx to decap skb=%p\n",skb);-return1;+gotopass;+}++if(!gtp_rx(pctx,skb,hdrlen,gtp->role)){+/* Successfully received */+return0;}-returngtp_rx(pctx,skb,hdrlen,gtp->role);+drop:+kfree_skb(skb);+return0;++pass:+return1;}staticvoidgtp_encap_destroy(structsock*sk)
@@ -317,49 +357,6 @@ static void gtp_encap_disable(struct gtp_dev *gtp)gtp_encap_disable_sock(gtp->sk1u);}-/* UDP encapsulation receive handler. See net/ipv4/udp.c.-*Returncodes:0:success,<0:error,>0:passuptouserspaceUDPsocket.-*/-staticintgtp_encap_recv(structsock*sk,structsk_buff*skb)-{-structgtp_dev*gtp;-intret=0;--gtp=rcu_dereference_sk_user_data(sk);-if(!gtp)-return1;--netdev_dbg(gtp->dev,"encap_recv sk=%p\n",sk);--switch(udp_sk(sk)->encap_type){-caseUDP_ENCAP_GTP0:-netdev_dbg(gtp->dev,"received GTP0 packet\n");-ret=gtp0_udp_encap_recv(gtp,skb);-break;-caseUDP_ENCAP_GTP1U:-netdev_dbg(gtp->dev,"received GTP1U packet\n");-ret=gtp1u_udp_encap_recv(gtp,skb);-break;-default:-ret=-1;/* Shouldn't happen. */-}--switch(ret){-case1:-netdev_dbg(gtp->dev,"pass up to the process\n");-break;-case0:-break;-case-1:-netdev_dbg(gtp->dev,"GTP packet has been dropped\n");-kfree_skb(skb);-ret=0;-break;-}--returnret;-}-staticintgtp_dev_init(structnet_device*dev){structgtp_dev*gtp=netdev_priv(dev);
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:44
Removes MTU handling in gtp_build_skb_ip4. This is non standard relative
to how other tunneling protocols handle MTU. The model espoused is that
the inner interface should set it's MTU to be less than the expected
path MTU on the overlay network. Path MTU discovery is not typically
used for modifying tunnel MTUs.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 30 ------------------------------
1 file changed, 30 deletions(-)
@@ -466,8 +466,6 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,structiphdr*iph;structsock*sk;__be32saddr;-__be16df;-intmtu;/* Read the IP destination address and resolve the PDP context.*PrependPDPheaderwithTEI/TIDfromPDPctx.
@@ -510,34 +508,6 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev,skb_dst_drop(skb);-/* This is similar to tnl_update_pmtu(). */-df=iph->frag_off;-if(df){-mtu=dst_mtu(&rt->dst)-dev->hard_header_len--sizeof(structiphdr)-sizeof(structudphdr);-switch(pctx->gtp_version){-caseGTP_V0:-mtu-=sizeof(structgtp0_header);-break;-caseGTP_V1:-mtu-=sizeof(structgtp1_header);-break;-}-}else{-mtu=dst_mtu(&rt->dst);-}--rt->dst.ops->update_pmtu(&rt->dst,NULL,skb,mtu);--if(!skb_is_gso(skb)&&(iph->frag_off&htons(IP_DF))&&-mtu<ntohs(iph->tot_len)){-netdev_dbg(dev,"packet too big, fragmentation needed\n");-memset(IPCB(skb),0,sizeof(*IPCB(skb)));-icmp_send(skb,ICMP_DEST_UNREACH,ICMP_FRAG_NEEDED,-htonl(mtu));-gotoerr_rt;-}-gtp_set_pktinfo_ipv4(pktinfo,sk,iph,pctx,rt,&fl4,dev);gtp_push_header(skb,pktinfo);
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:47
The gtp pktinfo structure is unnecessary and needs a lot of code to
manage it. Remove it. Also, add per pdp port configuration for transmit.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 167 ++++++++++++++++++++---------------------------
include/uapi/linux/gtp.h | 1 +
2 files changed, 71 insertions(+), 97 deletions(-)
@@ -418,149 +419,112 @@ static inline void gtp1_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)*/}-structgtp_pktinfo{-structsock*sk;-structiphdr*iph;-structflowi4fl4;-structrtable*rt;-structpdp_ctx*pctx;-structnet_device*dev;-__be16gtph_port;-};--staticvoidgtp_push_header(structsk_buff*skb,structgtp_pktinfo*pktinfo)+staticvoidgtp_push_header(structsk_buff*skb,structpdp_ctx*pctx){-switch(pktinfo->pctx->gtp_version){+switch(pctx->gtp_version){caseGTP_V0:-pktinfo->gtph_port=htons(GTP0_PORT);-gtp0_push_header(skb,pktinfo->pctx);+gtp0_push_header(skb,pctx);break;caseGTP_V1:-pktinfo->gtph_port=htons(GTP1U_PORT);-gtp1_push_header(skb,pktinfo->pctx);+gtp1_push_header(skb,pctx);break;}}-staticinlinevoidgtp_set_pktinfo_ipv4(structgtp_pktinfo*pktinfo,-structsock*sk,structiphdr*iph,-structpdp_ctx*pctx,structrtable*rt,-structflowi4*fl4,-structnet_device*dev)-{-pktinfo->sk=sk;-pktinfo->iph=iph;-pktinfo->pctx=pctx;-pktinfo->rt=rt;-pktinfo->fl4=*fl4;-pktinfo->dev=dev;-}--staticintgtp_build_skb_ip4(structsk_buff*skb,structnet_device*dev,-structgtp_pktinfo*pktinfo)+staticintgtp_xmit(structsk_buff*skb,structnet_device*dev,+structpdp_ctx*pctx){-structgtp_dev*gtp=netdev_priv(dev);-structpdp_ctx*pctx;+structsock*sk=pctx->sk;+__be32saddr=inet_sk(sk)->inet_saddr;structrtable*rt;-structflowi4fl4;-structiphdr*iph;-structsock*sk;-__be32saddr;--/* Read the IP destination address and resolve the PDP context.-*PrependPDPheaderwithTEI/TIDfromPDPctx.-*/-iph=ip_hdr(skb);-if(gtp->role==GTP_ROLE_SGSN)-pctx=ipv4_pdp_find(gtp,iph->saddr);-else-pctx=ipv4_pdp_find(gtp,iph->daddr);+interr=0;-if(!pctx){-netdev_dbg(dev,"no PDP ctx found for %pI4, skip\n",-&iph->daddr);-return-ENOENT;-}-netdev_dbg(dev,"found PDP context %p\n",pctx);+/* Ensure there is sufficient headroom. */+err=skb_cow_head(skb,dev->needed_headroom);+if(unlikely(err))+gotoout_err;-sk=pctx->sk;-saddr=inet_sk(sk)->inet_saddr;+skb_reset_inner_headers(skb);rt=ip_tunnel_get_route(dev,skb,sk->sk_protocol,sk->sk_bound_dev_if,RT_CONN_FLAGS(sk),pctx->peer_addr_ip4.s_addr,&saddr,-pktinfo->gtph_port,pktinfo->gtph_port,+pctx->gtp_port,pctx->gtp_port,&pctx->dst_cache,NULL);if(IS_ERR(rt)){-if(rt==ERR_PTR(-ELOOP)){-netdev_dbg(dev,"circular route to SSGN %pI4\n",-&pctx->peer_addr_ip4.s_addr);-dev->stats.collisions++;-gotoerr_rt;-}else{-netdev_dbg(dev,"no route to SSGN %pI4\n",-&pctx->peer_addr_ip4.s_addr);-dev->stats.tx_carrier_errors++;-gotoerr;-}+err=PTR_ERR(rt);+gotoout_err;}skb_dst_drop(skb);-gtp_set_pktinfo_ipv4(pktinfo,sk,iph,pctx,rt,&fl4,dev);-gtp_push_header(skb,pktinfo);+gtp_push_header(skb,pctx);+udp_tunnel_xmit_skb(rt,sk,skb,saddr,+pctx->peer_addr_ip4.s_addr,+0,ip4_dst_hoplimit(&rt->dst),0,+pctx->gtp_port,pctx->gtp_port,+false,false);++netdev_dbg(dev,"gtp -> IP src: %pI4 dst: %pI4\n",+&saddr,&pctx->peer_addr_ip4.s_addr);return0;-err_rt:-ip_rt_put(rt);-err:-return-EBADMSG;++out_err:+if(err==-ELOOP)+dev->stats.collisions++;+else+dev->stats.tx_carrier_errors++;++returnerr;}staticnetdev_tx_tgtp_dev_xmit(structsk_buff*skb,structnet_device*dev){unsignedintproto=ntohs(skb->protocol);-structgtp_pktinfopktinfo;+structgtp_dev*gtp=netdev_priv(dev);+structpdp_ctx*pctx;interr;-/* Ensure there is sufficient headroom. */-if(skb_cow_head(skb,dev->needed_headroom))-gototx_err;--skb_reset_inner_headers(skb);-/* PDP context lookups in gtp_build_skb_*() need rcu read-side lock. */rcu_read_lock();switch(proto){-caseETH_P_IP:-err=gtp_build_skb_ip4(skb,dev,&pktinfo);+caseETH_P_IP:{+structiphdr*iph=ip_hdr(skb);++if(gtp->role==GTP_ROLE_SGSN)+pctx=ipv4_pdp_find(gtp,iph->saddr);+else+pctx=ipv4_pdp_find(gtp,iph->daddr);++if(!pctx){+netdev_dbg(dev,"no PDP ctx found for %pI4, skip\n",+&iph->daddr);+err=-ENOENT;+gototx_err;+}+break;+}default:err=-EOPNOTSUPP;-break;+gototx_err;}-rcu_read_unlock();++netdev_dbg(dev,"found PDP context %p\n",pctx);++err=gtp_xmit(skb,dev,pctx);if(err<0)gototx_err;-switch(proto){-caseETH_P_IP:-netdev_dbg(pktinfo.dev,"gtp -> IP src: %pI4 dst: %pI4\n",-&pktinfo.iph->saddr,&pktinfo.iph->daddr);-udp_tunnel_xmit_skb(pktinfo.rt,pktinfo.sk,skb,-pktinfo.fl4.saddr,pktinfo.fl4.daddr,-pktinfo.iph->tos,-ip4_dst_hoplimit(&pktinfo.rt->dst),-0,-pktinfo.gtph_port,pktinfo.gtph_port,-true,false);-break;-}+rcu_read_unlock();returnNETDEV_TX_OK;+tx_err:+rcu_read_unlock();dev->stats.tx_errors++;dev_kfree_skb(skb);returnNETDEV_TX_OK;
@@ -27,6 +27,7 @@ enum gtp_attrs {GTPA_I_TEI,/* for GTPv1 only */GTPA_O_TEI,/* for GTPv1 only */GTPA_PAD,+GTPA_PORT,__GTPA_MAX,};#define GTPA_MAX (__GTPA_MAX + 1)
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:49
Allow IPv6 mobile subscriber packets. This entails adding an IPv6 mobile
subscriber address to pdp context and IPv6 specific variants to find pdp
contexts by address.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 259 +++++++++++++++++++++++++++++++++++++----------
include/uapi/linux/gtp.h | 1 +
2 files changed, 209 insertions(+), 51 deletions(-)
@@ -111,6 +118,11 @@ static inline u32 ipv4_hashfn(__be32 ip)returnjhash_1word((__forceu32)ip,gtp_h_initval);}+staticinlineu32ipv6_hashfn(conststructin6_addr*a)+{+return__ipv6_addr_jhash(a,gtp_h_initval);+}+/* Resolve a PDP context structure based on the 64bit TID. */staticstructpdp_ctx*gtp0_pdp_find(structgtp_dev*gtp,u64tid){
@@ -176,32 +188,95 @@ static bool gtp_check_ms_ipv4(struct sk_buff *skb, struct pdp_ctx *pctx,returniph->saddr==pctx->ms_addr_ip4.s_addr;}+/* Resolve a PDP context based on IPv6 address of MS. */+staticstructpdp_ctx*ipv6_pdp_find(structgtp_dev*gtp,+conststructin6_addr*ms_addr)+{+structhlist_head*head;+structpdp_ctx*pdp;++head=>p->addr6_hash[ipv6_hashfn(ms_addr)%gtp->hash_size];++hlist_for_each_entry_rcu(pdp,head,hlist_addr){+if(pdp->ms_af==AF_INET6&&+ipv6_addr_equal(&pdp->ms_addr_ip6,ms_addr))+returnpdp;+}++returnNULL;+}++staticboolgtp_check_ms_ipv6(structsk_buff*skb,structpdp_ctx*pctx,+unsignedinthdrlen,unsignedintrole)+{+structipv6hdr*ipv6h;++if(!pskb_may_pull(skb,hdrlen+sizeof(structipv6hdr)))+returnfalse;++ipv6h=(structipv6hdr*)(skb->data+hdrlen);++if(role==GTP_ROLE_SGSN)+returnipv6_addr_equal(&ipv6h->daddr,&pctx->ms_addr_ip6);+else+returnipv6_addr_equal(&ipv6h->saddr,&pctx->ms_addr_ip6);+}+/* Check if the inner IP address in this packet is assigned to any*existingmobilesubscriber.*/staticboolgtp_check_ms(structsk_buff*skb,structpdp_ctx*pctx,unsignedinthdrlen,unsignedintrole){-switch(ntohs(skb->protocol)){-caseETH_P_IP:+structiphdr*iph;++/* Minimally there needs to be an IPv4 header */+if(!pskb_may_pull(skb,hdrlen+sizeof(structiphdr)))+returnfalse;++iph=(structiphdr*)(skb->data+hdrlen);++switch(iph->version){+case4:returngtp_check_ms_ipv4(skb,pctx,hdrlen,role);+case6:+returngtp_check_ms_ipv6(skb,pctx,hdrlen,role);}+returnfalse;}+staticu16ipver_to_eth(structiphdr*iph)+{+switch(iph->version){+case4:+returnhtons(ETH_P_IP);+case6:+returnhtons(ETH_P_IPV6);+default:+return0;+}+}+staticintgtp_rx(structpdp_ctx*pctx,structsk_buff*skb,-unsignedinthdrlen,unsignedintrole)+unsignedinthdrlen,unsignedintrole){structpcpu_sw_netstats*stats;+u16inner_protocol;if(!gtp_check_ms(skb,pctx,hdrlen,role)){netdev_dbg(pctx->dev,"No PDP ctx for this MS\n");return1;}+inner_protocol=ipver_to_eth((structiphdr*)(skb->data+hdrlen));+if(!inner_protocol)+return-1;+/* Get rid of the GTP + UDP headers. */-if(iptunnel_pull_header(skb,hdrlen,skb->protocol,-!net_eq(sock_net(pctx->sk),dev_net(pctx->dev))))+if(iptunnel_pull_header(skb,hdrlen,inner_protocol,+!net_eq(sock_net(pctx->sk),+dev_net(pctx->dev))))return-1;netdev_dbg(pctx->dev,"forwarding packet from GGSN to uplink\n");
@@ -239,7 +314,8 @@ static int gtp0_udp_encap_recv(struct sock *sk, struct sk_buff *skb)if(!gtp)gotopass;-if(!pskb_may_pull(skb,hdrlen))+/* Pull through IP header since gtp_rx looks at IP version */+if(!pskb_may_pull(skb,hdrlen+sizeof(structiphdr)))gotodrop;gtp0=(structgtp0_header*)(skb->data+sizeof(structudphdr));
@@ -285,7 +361,8 @@ static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)if(!gtp)gotopass;-if(!pskb_may_pull(skb,hdrlen))+/* Pull through IP header since gtp_rx looks at IP version */+if(!pskb_may_pull(skb,hdrlen+sizeof(structiphdr)))gotodrop;gtp1=(structgtp1_header*)(skb->data+sizeof(structudphdr));
@@ -307,8 +384,10 @@ static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)if(gtp1->flags>P1_F_MASK)hdrlen+=4;-/* Make sure the header is larger enough, including extensions. */-if(!pskb_may_pull(skb,hdrlen))+/* Make sure the header is larger enough, including extensions and+*alsoanIPheadersincegtp_rxlooksatIPversion+*/+if(!pskb_may_pull(skb,hdrlen+sizeof(structiphdr)))gotodrop;gtp1=(structgtp1_header*)(skb->data+sizeof(structudphdr));
@@ -862,33 +966,46 @@ static void ipv4_pdp_fill(struct pdp_ctx *pctx, struct genl_info *info)pctx->gtp_port=default_port;}-staticintipv4_pdp_add(structgtp_dev*gtp,structsock*sk,-structgenl_info*info)+staticintgtp_pdp_add(structgtp_dev*gtp,structsock*sk,+structgenl_info*info){structnet_device*dev=gtp->dev;+structhlist_head*addr_list;+structpdp_ctx*pctx=NULL;u32hash_ms,hash_tid=0;-structpdp_ctx*pctx;-boolfound=false;-__be32ms_addr;+structin6_addrms6_addr;+__be32ms_addr=0;+intms_af;interr;-ms_addr=nla_get_be32(info->attrs[GTPA_MS_ADDRESS]);-hash_ms=ipv4_hashfn(ms_addr)%gtp->hash_size;+/* Caller ensures we have either v4 or v6 mobile subscriber address */+if(info->attrs[GTPA_MS_ADDRESS]){+/* IPv4 mobile subscriber */-hlist_for_each_entry_rcu(pctx,>p->addr_hash[hash_ms],hlist_addr){-if(pctx->ms_addr_ip4.s_addr==ms_addr){-found=true;-break;-}+ms_addr=nla_get_in_addr(info->attrs[GTPA_MS_ADDRESS]);+hash_ms=ipv4_hashfn(ms_addr)%gtp->hash_size;+addr_list=>p->addr4_hash[hash_ms];+ms_af=AF_INET;++pctx=ipv4_pdp_find(gtp,ms_addr);+}else{+/* IPv6 mobile subscriber */++ms6_addr=nla_get_in6_addr(info->attrs[GTPA_MS6_ADDRESS]);+hash_ms=ipv6_hashfn(&ms6_addr)%gtp->hash_size;+addr_list=>p->addr6_hash[hash_ms];+ms_af=AF_INET6;++pctx=ipv6_pdp_find(gtp,&ms6_addr);}-if(found){+if(pctx){if(info->nlhdr->nlmsg_flags&NLM_F_EXCL)return-EEXIST;if(info->nlhdr->nlmsg_flags&NLM_F_REPLACE)return-EOPNOTSUPP;-ipv4_pdp_fill(pctx,info);+pdp_fill(pctx,info);if(pctx->gtp_version==GTP_V0)netdev_dbg(dev,"GTPv0-U: update tunnel id = %llx (pdp %p)\n",
@@ -973,11 +1101,17 @@ static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info)interr;if(!info->attrs[GTPA_VERSION]||-!info->attrs[GTPA_LINK]||-!info->attrs[GTPA_PEER_ADDRESS]||-!info->attrs[GTPA_MS_ADDRESS])+!info->attrs[GTPA_LINK]||+!info->attrs[GTPA_PEER_ADDRESS])return-EINVAL;+if(!(!!info->attrs[GTPA_MS_ADDRESS]^+!!info->attrs[GTPA_MS6_ADDRESS])){+/* Either v4 or v6 mobile subscriber address must be set */++return-EINVAL;+}+version=nla_get_u32(info->attrs[GTPA_VERSION]);switch(version){
@@ -837,6 +906,12 @@ static struct sock *gtp_encap_enable_socket(int fd, int type,gotoout_sock;}+if(sock->sk->sk_family!=(is_ipv6?AF_INET6:AF_INET)){+pr_debug("socket fd=%d not right family\n",fd);+sk=ERR_PTR(-EINVAL);+gotoout_sock;+}+if(rcu_dereference_sk_user_data(sock->sk)){sk=ERR_PTR(-EBUSY);gotoout_sock;
@@ -1101,9 +1178,15 @@ static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info)interr;if(!info->attrs[GTPA_VERSION]||-!info->attrs[GTPA_LINK]||-!info->attrs[GTPA_PEER_ADDRESS])+!info->attrs[GTPA_LINK])+return-EINVAL;++if(!(!!info->attrs[GTPA_PEER_ADDRESS]^+!!info->attrs[GTPA_PEER6_ADDRESS])){+/* Either v4 or v6 peer address must be set */+return-EINVAL;+}if(!(!!info->attrs[GTPA_MS_ADDRESS]^!!info->attrs[GTPA_MS6_ADDRESS])){
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:54
Add new configuration of GTP interfaces that allow specifying a port to
listen on (as opposed to having to get sockets from a userspace control
plane). This allows GTP interfaces to be configured and the data path
tested without requiring a GTP-C daemon.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 212 +++++++++++++++++++++++++++++++++++------------
include/uapi/linux/gtp.h | 5 ++
2 files changed, 166 insertions(+), 51 deletions(-)
@@ -710,13 +724,19 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,structnetlink_ext_ack*extack){unsignedintrole=GTP_ROLE_GGSN;+boolhave_fd,have_ports;boolis_ipv6=false;structgtp_dev*gtp;structgtp_net*gn;inthashsize,err;-if(!data[IFLA_GTP_FD0]&&!data[IFLA_GTP_FD1])+have_fd=!!data[IFLA_GTP_FD0]||!!data[IFLA_GTP_FD1];+have_ports=!!data[IFLA_GTP_PORT0]||!!data[IFLA_GTP_PORT1];++if(!(have_fd^have_ports)){+/* Either got fd(s) or port(s) */return-EINVAL;+}if(data[IFLA_GTP_ROLE]){role=nla_get_u32(data[IFLA_GTP_ROLE]);
@@ -773,7 +793,7 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev,out_hashtable:gtp_hashtable_free(gtp);out_encap:-gtp_encap_disable(gtp);+gtp_encap_release(gtp);returnerr;}
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:56
Add a net field to gtp that is derived from src_net. Use net_eq to make
cross net argument for transmit functions.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
@@ -285,8 +287,7 @@ static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,/* Get rid of the GTP + UDP headers. */if(iptunnel_pull_header(skb,hdrlen,inner_protocol,-!net_eq(sock_net(pctx->sk),-dev_net(pctx->dev))))+!net_eq(gtp->net,dev_net(pctx->dev))))return-1;netdev_dbg(pctx->dev,"forwarding packet from GGSN to uplink\n");
From: Tom Herbert <hidden> Date: 2017-09-19 00:39:58
Allow applications or encapsulation protocols to register a GSO segment
function to their specific protocol. To faciliate this I reserved the
upper four bits in the gso_type to indicate the application specific GSO
type. Zero in these bits indicates no application GSO, so there are
fifteen instance that can be defined.
An application registers a a gso_segment using the skb_gso_app_register
this takes a struct skb_gso_app that indicates a callback function as
well as a set of GSO types for which at least one must be matched before
calling he segment function. GSO returns one of the application GSO
types described above (not a fixed value for the applications).
Subsequently, when the application sends a GSO packet the application
gso_type is set in the skb gso_type along with any other types.
skb_gso_app_segment is the function called from another GSO segment
function to handle segmentation of the application or encapsulation
protocol. This function includes check flags that provides context for
the appropriate GSO instance to match. For instance, in order to handle
a protocol encapsulated in UDP (GTP for instance) skb_gso_app_segment is
call from udp_tunnel_segment and check flags would be
SKB_GSO_UDP_TUNNEL_CSUM | SKB_GSO_UDP_TUNNEL.
Signed-off-by: Tom Herbert <redacted>
---
include/linux/netdevice.h | 31 +++++++++++++++++++++++++++++++
include/linux/skbuff.h | 25 +++++++++++++++++++++++++
net/core/dev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
net/ipv4/ip_tunnel_core.c | 6 ++++++
net/ipv4/udp_offload.c | 20 +++++++++++++++-----
5 files changed, 124 insertions(+), 5 deletions(-)
@@ -3932,6 +3932,37 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,structsk_buff*skb_mac_gso_segment(structsk_buff*skb,netdev_features_tfeatures);+structskb_gso_app{+unsignedintcheck_flags;+structsk_buff*(*gso_segment)(structsk_buff*skb,+netdev_features_tfeatures);+};++externstructskb_gso_app*skb_gso_apps[];+intskb_gso_app_register(conststructskb_gso_app*app);+voidskb_gso_app_unregister(intnum,conststructskb_gso_app*app);++/* rcu_read_lock() must be held */+staticinlinestructskb_gso_app*skb_gso_app_lookup(structsk_buff*skb,+netdev_features_tfeatures,+unsignedintcheck_flags)+{+structskb_gso_app*app;+inttype;++if(!(skb_shinfo(skb)->gso_type&SKB_GSO_APP_MASK))+returnfalse;++type=skb_gso_app_to_index(skb_shinfo(skb)->gso_type);++app=rcu_dereference(skb_gso_apps[type]);+if(app&&app->gso_segment&&+(check_flags&app->check_flags))+returnapp;++returnNULL;+}+structnetdev_bonding_info{ifslaveslave;ifbondmaster;
@@ -2725,6 +2726,52 @@ struct sk_buff *skb_mac_gso_segment(struct sk_buff *skb,}EXPORT_SYMBOL(skb_mac_gso_segment);+structskb_gso_app*skb_gso_apps[SKB_GSO_APP_NUM];+EXPORT_SYMBOL(skb_gso_apps);++intskb_gso_app_register(conststructskb_gso_app*app)+{+inti,ret=0;++spin_lock(&skb_gso_app_lock);++for(i=0;i<SKB_GSO_APP_NUM;i++){+if(!rcu_dereference_protected(skb_gso_apps[i],+lockdep_is_held(&skb_gso_app_lock))){+/* Found an empty slot */+rcu_assign_pointer(skb_gso_apps[i],app);++ret=skb_gso_app_to_gso_type(i);++break;+}+}++spin_unlock(&skb_gso_app_lock);++returnret;+return0;+}+EXPORT_SYMBOL(skb_gso_app_register);++voidskb_gso_app_unregister(intnum,conststructskb_gso_app*app)+{+if(!num)+return;++num=skb_gso_app_to_index(num);++spin_lock(&skb_gso_app_lock);++if(app==rcu_dereference_protected(skb_gso_apps[num],+lockdep_is_held(&skb_gso_app_lock))){+/* Matched entry */+rcu_assign_pointer(skb_gso_apps[num],NULL);+}++spin_unlock(&skb_gso_app_lock);+}+EXPORT_SYMBOL(skb_gso_app_unregister);/* openvswitch calls this on rx path, so we need a different check.*/
@@ -171,6 +171,12 @@ int iptunnel_handle_offloads(struct sk_buff *skb,err=skb_header_unclone(skb,GFP_ATOMIC);if(unlikely(err))returnerr;+if(!!(gso_type_mask&SKB_GSO_APP_MASK)&&+!!(skb_shinfo(skb)->gso_type&SKB_GSO_APP_MASK)){+/* Only allow one GSO app per packet */+return-EALREADY;+}+skb_shinfo(skb)->gso_type|=gso_type_mask;return0;}
From: Tom Herbert <hidden> Date: 2017-09-19 00:40:01
Add configuration to control use of zero checksums on transmit for both
IPv4 and IPv6, and control over accepting zero IPv6 checksums on
receive.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 35 +++++++++++++++++++++++++++++++++--
include/uapi/linux/if_link.h | 4 ++++
2 files changed, 37 insertions(+), 2 deletions(-)
From: Tom Herbert <hidden> Date: 2017-09-19 00:40:03
Populate GRO receive and GRO complete functions for GTP-Uv0 and v1.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 204 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 204 insertions(+)
@@ -429,6 +430,205 @@ static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)return1;}+staticstructsk_buff**gtp_gro_receive_finish(structsock*sk,+structsk_buff**head,+structsk_buff*skb,+void*hdr,size_thdrlen)+{+conststructpacket_offload*ptype;+structsk_buff**pp;+__be16type;++type=ipver_to_eth((structiphdr*)((void*)hdr+hdrlen));+if(!type)+gotoout_err;++rcu_read_lock();++ptype=gro_find_receive_by_type(type);+if(!ptype)+gotoout_unlock_err;++skb_gro_pull(skb,hdrlen);+skb_gro_postpull_rcsum(skb,hdr,hdrlen);+pp=call_gro_receive(ptype->callbacks.gro_receive,head,skb);++rcu_read_unlock();++returnpp;++out_unlock_err:+rcu_read_unlock();+out_err:+NAPI_GRO_CB(skb)->flush|=1;+returnNULL;+}++staticstructsk_buff**gtp0_gro_receive(structsock*sk,+structsk_buff**head,+structsk_buff*skb)+{+structgtp0_header*gtp0;+size_tlen,hdrlen,off;+structsk_buff*p;++off=skb_gro_offset(skb);+len=off+sizeof(*gtp0);+hdrlen=sizeof(*gtp0);++gtp0=skb_gro_header_fast(skb,off);+if(skb_gro_header_hard(skb,len)){+gtp0=skb_gro_header_slow(skb,len,off);+if(unlikely(!gtp0))+gotoout;+}++if((gtp0->flags>>5)!=GTP_V0||gtp0->type!=GTP_TPDU)+gotoout;++hdrlen+=sizeof(*gtp0);++/* To get IP version */+len+=sizeof(structiphdr);++/* Now get header with GTP header an IPv4 header (for version) */+if(skb_gro_header_hard(skb,len)){+gtp0=skb_gro_header_slow(skb,len,off);+if(unlikely(!gtp0))+gotoout;+}++for(p=*head;p;p=p->next){+conststructgtp0_header*gtp0_t;++if(!NAPI_GRO_CB(p)->same_flow)+continue;++gtp0_t=(structgtp0_header*)(p->data+off);++if(gtp0->flags!=gtp0_t->flags||+gtp0->type!=gtp0_t->type||+gtp0->flow!=gtp0_t->flow||+gtp0->tid!=gtp0_t->tid){+NAPI_GRO_CB(p)->same_flow=0;+continue;+}+}++returngtp_gro_receive_finish(sk,head,skb,gtp0,hdrlen);++out:+NAPI_GRO_CB(skb)->flush|=1;++returnNULL;+}++staticstructsk_buff**gtp1u_gro_receive(structsock*sk,+structsk_buff**head,+structsk_buff*skb)+{+structgtp1_header*gtp1;+size_tlen,hdrlen,off;+structsk_buff*p;++off=skb_gro_offset(skb);+len=off+sizeof(*gtp1);+hdrlen=sizeof(*gtp1);++gtp1=skb_gro_header_fast(skb,off);+if(skb_gro_header_hard(skb,len)){+gtp1=skb_gro_header_slow(skb,len,off);+if(unlikely(!gtp1))+gotoout;+}++if((gtp1->flags>>5)!=GTP_V1||gtp1->type!=GTP_TPDU)+gotoout;++if(gtp1->flags>P1_F_MASK){+hdrlen+=4;+len+=4;+}++len+=sizeof(structiphdr);++/* Now get header with GTP header an IPv4 header (for version) */+if(skb_gro_header_hard(skb,len)){+gtp1=skb_gro_header_slow(skb,len,off);+if(unlikely(!gtp1))+gotoout;+}++for(p=*head;p;p=p->next){+conststructgtp1_header*gtp1_t;++if(!NAPI_GRO_CB(p)->same_flow)+continue;++gtp1_t=(structgtp1_header*)(p->data+off);++if(gtp1->flags!=gtp1_t->flags||+gtp1->type!=gtp1_t->type||+gtp1->tid!=gtp1_t->tid){+NAPI_GRO_CB(p)->same_flow=0;+continue;+}+}++returngtp_gro_receive_finish(sk,head,skb,gtp1,hdrlen);++out:+NAPI_GRO_CB(skb)->flush=1;++returnNULL;+}++staticintgtp_gro_complete_finish(structsock*sk,structsk_buff*skb,+intnhoff,size_thdrlen)+{+structpacket_offload*ptype;+interr=-EINVAL;+__be16type;++type=ipver_to_eth((structiphdr*)(skb->data+nhoff+hdrlen));+if(!type)+returnerr;++rcu_read_lock();+ptype=gro_find_complete_by_type(type);+if(ptype)+err=ptype->callbacks.gro_complete(skb,nhoff+hdrlen);++rcu_read_unlock();++skb_set_inner_mac_header(skb,nhoff+hdrlen);++returnerr;+}++staticintgtp0_gro_complete(structsock*sk,structsk_buff*skb,intnhoff)+{+structgtp0_header*gtp0=(structgtp0_header*)(skb->data+nhoff);+size_thdrlen=sizeof(structgtp0_header);++gtp0->length=htons(skb->len-nhoff-hdrlen);++returngtp_gro_complete_finish(sk,skb,nhoff,hdrlen);+}++staticintgtp1u_gro_complete(structsock*sk,structsk_buff*skb,intnhoff)+{+structgtp1_header*gtp1=(structgtp1_header*)(skb->data+nhoff);+size_thdrlen=sizeof(structgtp1_header);++if(gtp1->flags>P1_F_MASK)+hdrlen+=4;++gtp1->length=htons(skb->len-nhoff-hdrlen);++returngtp_gro_complete_finish(sk,skb,nhoff,hdrlen);+}+staticvoidgtp_encap_destroy(structsock*sk){structgtp_dev*gtp;
@@ -946,9 +1146,13 @@ static int gtp_encap_enable_sock(struct socket *sock, int type,switch(type){caseUDP_ENCAP_GTP0:tuncfg.encap_rcv=gtp0_udp_encap_recv;+tuncfg.gro_receive=gtp0_gro_receive;+tuncfg.gro_complete=gtp0_gro_complete;break;caseUDP_ENCAP_GTP1U:tuncfg.encap_rcv=gtp1u_udp_encap_recv;+tuncfg.gro_receive=gtp1u_gro_receive;+tuncfg.gro_complete=gtp1u_gro_complete;break;default:pr_debug("Unknown encap type %u\n",type);
From: Tom Herbert <hidden> Date: 2017-09-19 00:40:05
Need to define a gtp_gso_segment since the GTP header includes a length
field that must be set per packet. Also, GPv0 header includes a sequence
number that is incremented per packet.
Signed-off-by: Tom Herbert <redacted>
---
drivers/net/gtp.c | 176 +++++++++++++++++++++++++++++++++++++++----
include/uapi/linux/if_link.h | 1 -
2 files changed, 163 insertions(+), 14 deletions(-)
@@ -430,6 +432,69 @@ static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)return1;}+staticstructsk_buff*gtp_gso_segment(structsk_buff*skb,+netdev_features_tfeatures)+{+structsk_buff*segs=ERR_PTR(-EINVAL);+inttnl_hlen=skb->mac_len;+structgtp0_header*gtp0;++if(unlikely(!pskb_may_pull(skb,tnl_hlen)))+returnERR_PTR(-EINVAL);++/* Make sure we have a mininal GTP header */+if(unlikely(tnl_hlen<min_t(size_t,sizeof(structgtp0_header),+sizeof(structgtp1_header))))+returnERR_PTR(-EINVAL);++/* Determine version */+gtp0=(structgtp0_header*)skb->data;+switch(gtp0->flags>>5){+caseGTP_V0:{+u16tx_seq;++if(unlikely(tnl_hlen!=sizeof(structgtp0_header)))+returnERR_PTR(-EINVAL);++tx_seq=ntohs(gtp0->seq);++/* segment inner packet. */+segs=skb_mac_gso_segment(skb,features);+if(!IS_ERR_OR_NULL(segs)){+skb=segs;+do{+gtp0=(structgtp0_header*)+skb_mac_header(skb);+gtp0->length=ntohs(skb->len-tnl_hlen);+gtp0->seq=htons(tx_seq);+tx_seq++;+}while((skb=skb->next));+}+break;+}+caseGTP_V1:{+structgtp1_header*gtp1;++if(unlikely(tnl_hlen!=sizeof(structgtp1_header)))+returnERR_PTR(-EINVAL);++/* segment inner packet. */+segs=skb_mac_gso_segment(skb,features);+if(!IS_ERR_OR_NULL(segs)){+skb=segs;+do{+gtp1=(structgtp1_header*)+skb_mac_header(skb);+gtp1->length=ntohs(skb->len-tnl_hlen);+}while((skb=skb->next));+}+break;+}+}++returnsegs;+}+staticstructsk_buff**gtp_gro_receive_finish(structsock*sk,structsk_buff**head,structsk_buff*skb,
@@ -688,18 +753,25 @@ static inline void gtp0_push_header(struct sk_buff *skb, struct pdp_ctx *pctx){intpayload_len=skb->len;structgtp0_header*gtp0;+u32tx_seq;gtp0=skb_push(skb,sizeof(*gtp0));gtp0->flags=0x1e;/* v0, GTP-non-prime. */gtp0->type=GTP_TPDU;gtp0->length=htons(payload_len);-gtp0->seq=htons((atomic_inc_return(&pctx->tx_seq)-1)%-0xffff);gtp0->flow=htons(pctx->u.v0.flow);gtp0->number=0xff;gtp0->spare[0]=gtp0->spare[1]=gtp0->spare[2]=0xff;gtp0->tid=cpu_to_be64(pctx->u.v0.tid);++/* If skb is GSO allocate sequence numbers for all the segments */+tx_seq=skb_shinfo(skb)->gso_segs?+atomic_add_return(skb_shinfo(skb)->gso_segs,+&pctx->tx_seq):+atomic_inc_return(&pctx->tx_seq);++gtp0->seq=(htons((u16)tx_seq)-1)&0xffff;}staticinlinevoidgtp1_push_header(structsk_buff*skb,structpdp_ctx*pctx)
@@ -737,6 +809,59 @@ static void gtp_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)}}+staticsize_tgtp_max_header_len(intversion)++{+switch(version){+caseGTP_V0:+returnsizeof(structgtp0_header);+caseGTP_V1:+returnsizeof(structgtp1_header)+4;+}++/* Should not happen */+return0;+}++staticintgtp_build_skb(structsk_buff*skb,structdst_entry*dst,+structpdp_ctx*pctx,boolxnet,intip_hdr_len,+booludp_sum)+{+inttype=(udp_sum?SKB_GSO_UDP_TUNNEL_CSUM:SKB_GSO_UDP_TUNNEL)|+gtp_gso_type;+intmin_headroom;+u16protocol;+interr;++skb_scrub_packet(skb,xnet);++min_headroom=LL_RESERVED_SPACE(dst->dev)+dst->header_len++gtp_max_header_len(pctx->gtp_version)+ip_hdr_len;++err=skb_cow_head(skb,min_headroom);+if(unlikely(err))+gotofree_dst;++err=iptunnel_handle_offloads(skb,type);+if(err)+gotofree_dst;++protocol=ipver_to_eth(ip_hdr(skb));++gtp_push_header(skb,pctx);++/* GTP header is treated as inner MAC header */+skb_reset_inner_mac_header(skb);++skb_set_inner_protocol(skb,protocol);++return0;++free_dst:+dst_release(dst);+returnerr;+}+staticintgtp_xmit(structsk_buff*skb,structnet_device*dev,structpdp_ctx*pctx){
@@ -746,13 +871,6 @@ static int gtp_xmit(struct sk_buff *skb, struct net_device *dev,booludp_csum;interr=0;-/* Ensure there is sufficient headroom. */-err=skb_cow_head(skb,dev->needed_headroom);-if(unlikely(err))-gotoout_err;--skb_reset_inner_headers(skb);-if(pctx->peer_af==AF_INET){__be32saddr=inet_sk(sk)->inet_saddr;structrtable*rt;
From: David Miller <davem@davemloft.net> Date: 2017-09-19 04:17:53
From: Tom Herbert <redacted>
Date: Mon, 18 Sep 2017 17:38:53 -0700
Call ip_tunnel_get_route and dst_cache to pdp context which should
improve performance by obviating the need to perform a route lookup
on every packet.
Signed-off-by: Tom Herbert <redacted>
Not caused by your changes, but something to think about:
This and the new dst caching code ignores any source address selection
done by ip_route_output_key() or the new tunnel route lookup helpers.
Either source address selection should be respected, or if saddr will
never be modified by a route lookup for some specific reason here,
that should be documented.
I know you are just following the pattern of the existing "ipv4_hashfn()" here
but this kind of stuff is not very global namespace friendly. Even simply
adding a "gtp_" prefix to these hash functions would be a lot better.
From: David Miller <davem@davemloft.net> Date: 2017-09-19 04:21:37
From: Tom Herbert <redacted>
Date: Mon, 18 Sep 2017 17:39:01 -0700
Allow applications or encapsulation protocols to register a GSO segment
function to their specific protocol. To faciliate this I reserved the
upper four bits in the gso_type to indicate the application specific GSO
type. Zero in these bits indicates no application GSO, so there are
fifteen instance that can be defined.
An application registers a a gso_segment using the skb_gso_app_register
this takes a struct skb_gso_app that indicates a callback function as
well as a set of GSO types for which at least one must be matched before
calling he segment function. GSO returns one of the application GSO
types described above (not a fixed value for the applications).
Subsequently, when the application sends a GSO packet the application
gso_type is set in the skb gso_type along with any other types.
skb_gso_app_segment is the function called from another GSO segment
function to handle segmentation of the application or encapsulation
protocol. This function includes check flags that provides context for
the appropriate GSO instance to match. For instance, in order to handle
a protocol encapsulated in UDP (GTP for instance) skb_gso_app_segment is
call from udp_tunnel_segment and check flags would be
SKB_GSO_UDP_TUNNEL_CSUM | SKB_GSO_UDP_TUNNEL.
Signed-off-by: Tom Herbert <redacted>
What happens on cards that can offload existing arbitrary UDP tunnel
encapsulations?
Will something about the state of the GSO type bits you are adding
prevent that? Or do we need to add some new checks somewhere?
From: David Miller <davem@davemloft.net> Date: 2017-09-19 04:24:47
From: Tom Herbert <redacted>
Date: Mon, 18 Sep 2017 17:39:02 -0700
Add configuration to control use of zero checksums on transmit for both
IPv4 and IPv6, and control over accepting zero IPv6 checksums on
receive.
Signed-off-by: Tom Herbert <redacted>
I thought we were trying to move away from this special case of allowing
zero UDP checksums with tunnels, especially for ipv6.
From: kbuild test robot <hidden> Date: 2017-09-19 07:31:46
Hi Tom,
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Tom-Herbert/gtp-Additional-feature-support/20170919-143920
config: i386-randconfig-x074-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
In file included from net/ipv4/ip_tunnel_core.c:40:0:
include/net/ip6_tunnel.h: In function 'ip6_tnl_get_route':
quoted
include/net/ip6_tunnel.h:168:28: error: implicit declaration of function 'dst_cache_get_ip6' [-Werror=implicit-function-declaration]
From: kbuild test robot <hidden> Date: 2017-09-19 07:34:14
Hi Tom,
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Tom-Herbert/gtp-Additional-feature-support/20170919-143920
config: i386-randconfig-x016-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
Note: the linux-review/Tom-Herbert/gtp-Additional-feature-support/20170919-143920 HEAD 737a09b8f9cd56706d01703d17523b0fea907f41 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers//net/gtp.c: In function 'gtp_rx':
quoted
drivers//net/gtp.c:222:21: error: 'gtp' undeclared (first use in this function)
gro_cells_receive(>p->gro_cells, skb);
^~~
drivers//net/gtp.c:222:21: note: each undeclared identifier is reported only once for each function it appears in
drivers//net/gtp.c: In function 'gtp_link_setup':
drivers//net/gtp.c:628:18: error: 'gtp' undeclared (first use in this function)
gro_cells_init(>p->gro_cells, dev);
^~~
vim +/gtp +222 drivers//net/gtp.c
190
191 static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
192 unsigned int hdrlen, unsigned int role)
193 {
194 struct pcpu_sw_netstats *stats;
195
196 if (!gtp_check_ms(skb, pctx, hdrlen, role)) {
197 netdev_dbg(pctx->dev, "No PDP ctx for this MS\n");
198 return 1;
199 }
200
201 /* Get rid of the GTP + UDP headers. */
202 if (iptunnel_pull_header(skb, hdrlen, skb->protocol,
203 !net_eq(sock_net(pctx->sk), dev_net(pctx->dev))))
204 return -1;
205
206 netdev_dbg(pctx->dev, "forwarding packet from GGSN to uplink\n");
207
208 /* Now that the UDP and the GTP header have been removed, set up the
209 * new network header. This is required by the upper layer to
210 * calculate the transport header.
211 */
212 skb_reset_network_header(skb);
213
214 skb->dev = pctx->dev;
215
216 stats = this_cpu_ptr(pctx->dev->tstats);
217 u64_stats_update_begin(&stats->syncp);
218 stats->rx_packets++;
219 stats->rx_bytes += skb->len;
220 u64_stats_update_end(&stats->syncp);
221
> 222 gro_cells_receive(>p->gro_cells, skb);
223
224 return 0;
225 }
226
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: kbuild test robot <hidden> Date: 2017-09-19 11:59:09
Hi Tom,
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Tom-Herbert/gtp-Additional-feature-support/20170919-143920
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
vim +3958 include/linux/netdevice.h
5b33bc6e Tom Herbert 2017-09-18 3944
5b33bc6e Tom Herbert 2017-09-18 3945 /* rcu_read_lock() must be held */
5b33bc6e Tom Herbert 2017-09-18 3946 static inline struct skb_gso_app *skb_gso_app_lookup(struct sk_buff *skb,
5b33bc6e Tom Herbert 2017-09-18 3947 netdev_features_t features,
5b33bc6e Tom Herbert 2017-09-18 3948 unsigned int check_flags)
5b33bc6e Tom Herbert 2017-09-18 3949 {
5b33bc6e Tom Herbert 2017-09-18 3950 struct skb_gso_app *app;
5b33bc6e Tom Herbert 2017-09-18 3951 int type;
5b33bc6e Tom Herbert 2017-09-18 3952
5b33bc6e Tom Herbert 2017-09-18 3953 if (!(skb_shinfo(skb)->gso_type & SKB_GSO_APP_MASK))
5b33bc6e Tom Herbert 2017-09-18 3954 return false;
5b33bc6e Tom Herbert 2017-09-18 3955
5b33bc6e Tom Herbert 2017-09-18 3956 type = skb_gso_app_to_index(skb_shinfo(skb)->gso_type);
5b33bc6e Tom Herbert 2017-09-18 3957
5b33bc6e Tom Herbert 2017-09-18 @3958 app = rcu_dereference(skb_gso_apps[type]);
5b33bc6e Tom Herbert 2017-09-18 3959 if (app && app->gso_segment &&
5b33bc6e Tom Herbert 2017-09-18 3960 (check_flags & app->check_flags))
5b33bc6e Tom Herbert 2017-09-18 3961 return app;
5b33bc6e Tom Herbert 2017-09-18 3962
5b33bc6e Tom Herbert 2017-09-18 3963 return NULL;
5b33bc6e Tom Herbert 2017-09-18 3964 }
5b33bc6e Tom Herbert 2017-09-18 3965
:::::: The code at line 3958 was first introduced by commit
:::::: 5b33bc6e4fcae1113167c651a3d3a218c7e277c6 net: Add a facility to support application defined GSO
:::::: TO: Tom Herbert [off-list ref]
:::::: CC: 0day robot [off-list ref]
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-19 12:13:13
Hi Dave,
On Mon, Sep 18, 2017 at 09:17:51PM -0700, David Miller wrote:
This and the new dst caching code ignores any source address selection
done by ip_route_output_key() or the new tunnel route lookup helpers.
Either source address selection should be respected, or if saddr will
never be modified by a route lookup for some specific reason here,
that should be documented.
The IP source address is fixed by signaling on the GTP-C control plane
and nothing that the kernel can unilaterally decide to change. Such a
change of address would have to be decided by and first be signaled on
GTP-C to the peer by the userspace daemon, which would then update the
PDP context in the kernel.
So I guess you're asking us to document that rationale as form of a
source code comment ?
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-19 12:13:13
Hi Tom,
On Mon, Sep 18, 2017 at 05:38:55PM -0700, Tom Herbert wrote:
Removes MTU handling in gtp_build_skb_ip4. This is non standard relative
to how other tunneling protocols handle MTU. The model espoused is that
the inner interface should set it's MTU to be less than the expected
path MTU on the overlay network. Path MTU discovery is not typically
used for modifying tunnel MTUs.
The point of the kernel GTP module is to interoperate with existing
other GTP implementations and the practises established by cellular
operators when operating GTP in their networks.
While what you describe (chose interface MTU to be less than the
expected path MTU) is generally best practise in the Linux IP/networking
world, this is not generally reflected in the cellular
universe. You see quite a bit of GTP fragmentation due to the fact
that the transport network simply has to deal with the MTU that has
been established via the control plane between SGSN and MS/UE, without
the GGSN even being part of that negotiation.
Also, you may very well have one "gtp0" tunnel device at the GGSN,
but you are establishing individual GTP tunnels to dozesn to hundreds of
different SGSNs at operators all over the world. You cannot reliably
set the "gtp0" interface MTU to "the path MTU of the overlay network",
as the overlay network is in fact different for each of the SGSNs you're
talking to - and each may have a different path MTU.
So unless I'm missing something, I would currently vote for staying with
the current code, which uses the path MTU to the specific destination IP
address (the SGSN).
Regards,
Harald
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-19 12:13:13
On Mon, Sep 18, 2017 at 05:38:57PM -0700, Tom Herbert wrote:
Allow IPv6 mobile subscriber packets. This entails adding an IPv6 mobile
subscriber address to pdp context and IPv6 specific variants to find pdp
contexts by address.
Please note that there are three different PDP contexts for IP:
* IPv4 only (what gtp.c implements so far)
* IPv6 only
* dual IPv4+IPv6 (called IPv46)
This information will have to be provisioned by the control plane
via netlink for each PDP context. The kernel module then needs to
make sure that on a v4-only context no IPv6 packets are accepted
and vice-versa.
Your proposed patch is missing this kind of screening function and
I would imagine it could introduce all kinds of security problems :/
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-19 12:13:13
Hi Tom,
I think this patch does too many things at once:
* introduce separate rx functions
* convert from netif_rx to gro_cells_receive
* cosmetic changes like "return -1" to "goto drop"
In the context of reviewability and the "one patch per topic", I would
prefer to see those separated, thanks.
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-19 12:13:13
On Mon, Sep 18, 2017 at 05:39:03PM -0700, Tom Herbert wrote:
Populate GRO receive and GRO complete functions for GTP-Uv0 and v1.
looks fine to me, though I'm not the GRO expert here. Let's say what
the netdev gurus have to say in their review.
If you say it is tested with GRO-capable and non-GRO capable device
drivers, I'm fine with the patch.
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
this will not really work, as an union means that a PDP context
will be either IPv4-only or IPV6-only, while in reality there
are three types, see my other mail. So you have to deal
with v4-only, v6-only or v4v6.
The v6-only is legacy by now, and all modern phones I've tested in
recent years can do v4v6 rather than having a v4-only and a v6-only
PDP context in parallel.
From the operator point of view, v4v6 is very desirable, as it basically
halves the amount of PDP contexts compared to the old approach, which
significantly reduces signalling load across your network, as well as
the amount of memory (and thus capacity) in your core network elements.
I've recently implemented v6 + v4v6 support in osmo-ggsn (see
http://git.osmocom.org/osmo-ggsn/) in case you would like to see another
FOSS implementation for v6 + v4v6 - though in userspace, of course.
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
I know you are just following the pattern of the existing "ipv4_hashfn()" here
but this kind of stuff is not very global namespace friendly. Even simply
adding a "gtp_" prefix to these hash functions would be a lot better.
I would agree if this was an inline function defined in a header file or
a non-static function. But where is the global namespace concern in
case of static inline functions defined and used in the same .c file?
If it makes you happy, I'm all for adding the prefix - I just would like
to understand the rationale better, thanks :)
Regards,
Harald
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-19 12:44:12
Hi Tom,
first of all, thanks a lot for your patch series. It makes me happy to
see contributions on the GTP code :)
On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
- IPv6 support
see my detailed comments in other mails. It's unfortunately only
support for the already "deprecated" IPv6-only PDP contexts, not the
more modern v4v6 type. In order to interoperate with old and new
approach, all three cases (v4, v6 and v4v6) should be supported from one
code base.
- Configurable networking interfaces so that GTP kernel can be used
and tested without needing GSN network emulation (i.e. no user space
daemon needed).
We have some pretty decent userspace utilities for configuring the GTP
interfaces and tunnels in the libgtpnl repository, but if it helps
people to have another way of configuration, I won't be against it.
What we have to keep in mind is that the current model of 1:1 mapping of
a "UDP socket' to a GTP netdevice is conceptually broken and needs to be
refactored soon (without breaking backwards compatibility). See related
earlier discussions with patches submitted by Andreas Schultz.
Summary:
In real-world GGSNs you often want to host multiple virtual GGSNs on a
single GGSN (= UDP socket). Each virtual GGSN terminates into one
external PDN (packet data network), which can be a private corporate vpn
or any other IP network, with no routing between those networks.
Naively one would assume you "simply" run another virtual GGSN
instance on another IP address, and then differentiate like that.
However, the problem is that adding a new GGSN IP address will require
manual configuration changes at each of your roaming partners (easily
hundreds of operators!) and hence it is avoided at all cost due to the
related long schedule, requirement for interop testing with each of them,
etc.
So what you do in reality at operators is that you operate many of those
virtual GGSNs on the same IP:Port combination (and hence UDP socket),
which means you have PDP contexts for vGGSN A which terminate on e.g.
gtp0 and PDP contexts for vGGSN B on gtp1, and so on. The decision
which gtp-device a given PDP context is a member is made by the GTP-C
instance. In the kenel we'll have to decouple net-devices from sockets.
So whatever new configuration mechanism or architectural changes we
introduce, we need to make sure that those will accomodate the "new
model" rather than introducing further dependencies for which we will
have to maintain backwards compatibility workaronds later on.
- Port numbers are configurable
I'm not sure if this is a useful feature. GTP is used only in
operator-controlled networks and only on standard ports. It's not
possible to negotiate any non-standard ports on the signaling plane
either.
- Addition of a dst_cache in the GTP structure and other cleanup
looks fine to me.
- GSO,GRO
- Control of zero UDP checksums
[...]
Additionally, this patch set also includes a couple of general support
capabilities:
- A facility that allows application specific GSO callbacks
- Common functions to get a route fo for an IP tunnel
This is where the "core netdev" folks will have to comment. I'm too
remote from mainline kernel development these days and will focus on
reviewing the GTP specific bits of your patch series.
For IPv6 support, the mobile subscriber needs to allow IPv6 addresses,
and the remote enpoint can be IPv6.
Minor correction: The mobile subscriber specifically requests a PDP Type
when establishing the PDP context via Session Management related
signaling from MS/UE to SGSN. The SGSN simply translates this to GTP
and then forwards it to the GGSN. So it's acutally not "allow" but
"specifically request".
Configured the matrix of IPv4/IPv6 mobile subscriber, IPv4/IPv6 remote
peer, and GTP version 0 and 1 (eight combinations). Observed
connectivity and proper GSO/GRO. Also, tested VXLAN for
regression.
I presume those tests were done with manually configured GTP-devices and
PDP contexts to the (patched) kernel GTP module? If so, I would like to
strongly suggest interop testing with a different implementation, such
as real phones on the MS/UE side and e.g. OsmoSGSN. That would,
however, of course mean that the netlink related bits would have to be
added to libgtpnl and OsmoGGSN (or ergw) so that you have a daemon for
the control plane.
For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
related to the way how router solicitation/advertisements are modified
in the 3GPP world.
The address allocation in v4 is simple:
* MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
activation
* GGSN responds with IPv4 address in EUA of Activate PDP context
response (and then uses netlink to tell the kernel about that
IPv4 address)
In v6 or the v6 portion of v4v6 it works differently:
* MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
activation
* GGSN responds with an IPv6 address, but that address is *not* used
for communication, but simply used as an "interface identifier" to
build a link-local address.
* MS then uses router solicitation using that link-local address
* GGSN responds with router advertisement, allocating a single /64
prefix, from which the MS then generates a fully-qualified IPv6
source address for communication.
How did you envision this to be done with the v6 support you just added?
At the very least, the /64 prefix matching would have to be implemented
so that in fact all addresses within that /64 prefix are matched +
encapsulated for a given PDP context in the downlink (to phone)
direction.
Also, I think the responsibility for the router advertisements would be
in the kernel, too. Otherwise, a GTP-C userspace implementation would
have to inject packets into the user plane (which is otherwise handled
completely inside the kernel). Injecting packets would mean that in caes
GTP sequence numbers are used, that userspace implementation would have
to alter the sequence numbers of the kernel gtp.ko code using netlink,
but therre would be race conditions, ...
The router advertisements and neighbor advertisements basically have the
semantics of one link per PDP context. Each of them is a point-to-point
link, and it's not one router advertisement that's sent to all of the
PDP contexts on that gtp-device.
I know it all sucks. I'm still happy to see somebody tackling v6
support in gtp.c :)
Regards,
Harald
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Tom Herbert <hidden> Date: 2017-09-19 15:59:29
On Tue, Sep 19, 2017 at 5:43 AM, Harald Welte [off-list ref] wrote:
Hi Tom,
first of all, thanks a lot for your patch series. It makes me happy to
see contributions on the GTP code :)
On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
quoted
- IPv6 support
see my detailed comments in other mails. It's unfortunately only
support for the already "deprecated" IPv6-only PDP contexts, not the
more modern v4v6 type. In order to interoperate with old and new
approach, all three cases (v4, v6 and v4v6) should be supported from one
code base.
It sounds like something that can be subsequently added. Do you have a
reference to the spec?
quoted
- Configurable networking interfaces so that GTP kernel can be used
and tested without needing GSN network emulation (i.e. no user space
daemon needed).
We have some pretty decent userspace utilities for configuring the GTP
interfaces and tunnels in the libgtpnl repository, but if it helps
people to have another way of configuration, I won't be against it.
AFAIK those userspace utilities don't support IPv6. Being able to
configure GTP like any other encapsulation will facilitate development
of IPv6 and other features.
What we have to keep in mind is that the current model of 1:1 mapping of
a "UDP socket' to a GTP netdevice is conceptually broken and needs to be
refactored soon (without breaking backwards compatibility). See related
earlier discussions with patches submitted by Andreas Schultz.
I don't think I changed the model, so this can evolve.
Summary:
In real-world GGSNs you often want to host multiple virtual GGSNs on a
single GGSN (= UDP socket). Each virtual GGSN terminates into one
external PDN (packet data network), which can be a private corporate vpn
or any other IP network, with no routing between those networks.
Sounds like network virtualization and VNIs.
Naively one would assume you "simply" run another virtual GGSN
instance on another IP address, and then differentiate like that.
However, the problem is that adding a new GGSN IP address will require
manual configuration changes at each of your roaming partners (easily
hundreds of operators!) and hence it is avoided at all cost due to the
related long schedule, requirement for interop testing with each of them,
etc.
So what you do in reality at operators is that you operate many of those
virtual GGSNs on the same IP:Port combination (and hence UDP socket),
which means you have PDP contexts for vGGSN A which terminate on e.g.
gtp0 and PDP contexts for vGGSN B on gtp1, and so on. The decision
which gtp-device a given PDP context is a member is made by the GTP-C
instance. In the kenel we'll have to decouple net-devices from sockets.
So whatever new configuration mechanism or architectural changes we
introduce, we need to make sure that those will accomodate the "new
model" rather than introducing further dependencies for which we will
have to maintain backwards compatibility workaronds later on.
quoted
- Port numbers are configurable
I'm not sure if this is a useful feature. GTP is used only in
operator-controlled networks and only on standard ports. It's not
possible to negotiate any non-standard ports on the signaling plane
either.
Bear in mind that we're not required to do everything the GTP spec
says. Adding port configuration is another one of those things that
gives us flexibility and and better capability to test without needing
a full blown GSN network. One feature I didn't implement was UDP
source for flow entropy-- as we've seen with other encapsulation
protocols this helps significantly to get good ECMP in the network. My
impression is GTP designers probably didn't think in terms of getting
best performance. But we can ;-)
quoted
- Addition of a dst_cache in the GTP structure and other cleanup
looks fine to me.
quoted
- GSO,GRO
- Control of zero UDP checksums
[...]
quoted
Additionally, this patch set also includes a couple of general support
capabilities:
- A facility that allows application specific GSO callbacks
- Common functions to get a route fo for an IP tunnel
This is where the "core netdev" folks will have to comment. I'm too
remote from mainline kernel development these days and will focus on
reviewing the GTP specific bits of your patch series.
Thanks. Obviously, I and many on this list have more expertise on the
core networking side than GTP, so your review is quite welcome.
quoted
For IPv6 support, the mobile subscriber needs to allow IPv6 addresses,
and the remote enpoint can be IPv6.
Minor correction: The mobile subscriber specifically requests a PDP Type
when establishing the PDP context via Session Management related
signaling from MS/UE to SGSN. The SGSN simply translates this to GTP
and then forwards it to the GGSN. So it's acutally not "allow" but
"specifically request".
Okay.
quoted
Configured the matrix of IPv4/IPv6 mobile subscriber, IPv4/IPv6 remote
peer, and GTP version 0 and 1 (eight combinations). Observed
connectivity and proper GSO/GRO. Also, tested VXLAN for
regression.
I presume those tests were done with manually configured GTP-devices and
PDP contexts to the (patched) kernel GTP module? If so, I would like to
strongly suggest interop testing with a different implementation, such
as real phones on the MS/UE side and e.g. OsmoSGSN. That would,
however, of course mean that the netlink related bits would have to be
added to libgtpnl and OsmoGGSN (or ergw) so that you have a daemon for
the control plane.
I also brought up open_ggsn. ggsn to sgsn.
For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
related to the way how router solicitation/advertisements are modified
in the 3GPP world.
The address allocation in v4 is simple:
* MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
activation
* GGSN responds with IPv4 address in EUA of Activate PDP context
response (and then uses netlink to tell the kernel about that
IPv4 address)
In v6 or the v6 portion of v4v6 it works differently:
* MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
activation
* GGSN responds with an IPv6 address, but that address is *not* used
for communication, but simply used as an "interface identifier" to
build a link-local address.
* MS then uses router solicitation using that link-local address
* GGSN responds with router advertisement, allocating a single /64
prefix, from which the MS then generates a fully-qualified IPv6
source address for communication.
How did you envision this to be done with the v6 support you just added?
At the very least, the /64 prefix matching would have to be implemented
so that in fact all addresses within that /64 prefix are matched +
encapsulated for a given PDP context in the downlink (to phone)
direction.
Also, I think the responsibility for the router advertisements would be
in the kernel, too. Otherwise, a GTP-C userspace implementation would
have to inject packets into the user plane (which is otherwise handled
completely inside the kernel). Injecting packets would mean that in caes
GTP sequence numbers are used, that userspace implementation would have
to alter the sequence numbers of the kernel gtp.ko code using netlink,
but therre would be race conditions, ...
The router advertisements and neighbor advertisements basically have the
semantics of one link per PDP context. Each of them is a point-to-point
link, and it's not one router advertisement that's sent to all of the
PDP contexts on that gtp-device.
I know it all sucks. I'm still happy to see somebody tackling v6
support in gtp.c :)
I would hope all the above you're describing is mostly control plane
matters. At least a good design decouples data palne and control
plane. I know that GTP is a bit convoluted in this regard.
Tom
From: Tom Herbert <hidden> Date: 2017-09-19 16:05:19
On Mon, Sep 18, 2017 at 9:17 PM, David Miller [off-list ref] wrote:
From: Tom Herbert <redacted>
Date: Mon, 18 Sep 2017 17:38:53 -0700
quoted
Call ip_tunnel_get_route and dst_cache to pdp context which should
improve performance by obviating the need to perform a route lookup
on every packet.
Signed-off-by: Tom Herbert <redacted>
Not caused by your changes, but something to think about:
This and the new dst caching code ignores any source address selection
done by ip_route_output_key() or the new tunnel route lookup helpers.
Either source address selection should be respected, or if saddr will
never be modified by a route lookup for some specific reason here,
that should be documented.
Yes, I noticed that. In this case the source address is intended to be
taken bound on the socket which would imply we aren't interested in
source address selection.
Tom
I know you are just following the pattern of the existing "ipv4_hashfn()" here
but this kind of stuff is not very global namespace friendly. Even simply
adding a "gtp_" prefix to these hash functions would be a lot better.
I would agree if this was an inline function defined in a header file or
a non-static function. But where is the global namespace concern in
case of static inline functions defined and used in the same .c file?
The problem is if we create a generic ipv6_hashfn() in linux/ipv6.h or
something like that, then this driver stops building.
From: Tom Herbert <hidden> Date: 2017-09-19 18:12:46
On Tue, Sep 19, 2017 at 4:42 AM, Harald Welte [off-list ref] wrote:
Hi Tom,
On Mon, Sep 18, 2017 at 05:38:55PM -0700, Tom Herbert wrote:
quoted
Removes MTU handling in gtp_build_skb_ip4. This is non standard relative
to how other tunneling protocols handle MTU. The model espoused is that
the inner interface should set it's MTU to be less than the expected
path MTU on the overlay network. Path MTU discovery is not typically
used for modifying tunnel MTUs.
The point of the kernel GTP module is to interoperate with existing
other GTP implementations and the practises established by cellular
operators when operating GTP in their networks.
While what you describe (chose interface MTU to be less than the
expected path MTU) is generally best practise in the Linux IP/networking
world, this is not generally reflected in the cellular
universe. You see quite a bit of GTP fragmentation due to the fact
that the transport network simply has to deal with the MTU that has
been established via the control plane between SGSN and MS/UE, without
the GGSN even being part of that negotiation.
Also, you may very well have one "gtp0" tunnel device at the GGSN,
but you are establishing individual GTP tunnels to dozesn to hundreds of
different SGSNs at operators all over the world. You cannot reliably
set the "gtp0" interface MTU to "the path MTU of the overlay network",
as the overlay network is in fact different for each of the SGSNs you're
talking to - and each may have a different path MTU.
So unless I'm missing something, I would currently vote for staying with
the current code, which uses the path MTU to the specific destination IP
address (the SGSN).
Okay, I'll modify tnl_update_pmtu so we can call it from GTP and not
have to replicate that function. I suspect VXLAN might also what this
at some point.
Tom
Regards,
Harald
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-19 23:24:49
Hi Tom,
On Tue, Sep 19, 2017 at 08:59:28AM -0700, Tom Herbert wrote:
On Tue, Sep 19, 2017 at 5:43 AM, Harald Welte [off-list ref]
wrote:
quoted
On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
quoted
- IPv6 support
see my detailed comments in other mails. It's unfortunately only
support for the already "deprecated" IPv6-only PDP contexts, not the
more modern v4v6 type. In order to interoperate with old and new
approach, all three cases (v4, v6 and v4v6) should be supported from
one code base.
It sounds like something that can be subsequently added.
Not entirely, at least on the netlink (and any other configuration
interface) you will have to reflect this from the very beginning. You
have to have an explicit PDP type and cannot rely on the address type to
specify the type of PDP context. Whatever interfaces are introduced
now will have to remain compatible to any future change.
My strategy to avoid any such possible 'road blocks' from being
introduced would be to simply add v4v6 and v6 support in one go. The
differences are marginal (having both an IPv6 prefix and a v4 address in
parallel, rather than mutually exclusive only).
Do you have a reference to the spec?
See http://osmocom.org/issues/2418#note-7 which lists Section 11.2.1.3.2
of 3GPP TS 29.061 in combination with RFC3314, RFC7066, RFC6459 and
3GPP TS 23.060 9.2.1 as well as a summary of my understanding of it some
months ago.
quoted
quoted
- Configurable networking interfaces so that GTP kernel can be
used and tested without needing GSN network emulation (i.e. no
user space daemon needed).
We have some pretty decent userspace utilities for configuring the
GTP interfaces and tunnels in the libgtpnl repository, but if it
helps people to have another way of configuration, I won't be
against it.
AFAIK those userspace utilities don't support IPv6.
Of course not [yet]. libgtpnl and the command line tools have been
implemented specifically for the in-kernel GTP driver, and you have to
make sure to add related support on both the kernel and the userspace
side (libgtpnl). So there's little point in adding features on either
side before the other side. There would be no way to test...
Being able to configure GTP like any other encapsulation will
facilitate development of IPv6 and other features.
That may very well be the case, but adding "IPv6 support" to kernel GTP
in a way that is not in line with the existing userspace libraries and
control-plane implementations means that you're developing those
features in an artificial environment that doesn't resemble real 3GPP
interoperable networks out there.
As indicated, I'm not against adding additional interfaces, but we have
to make sure that we add IPv6 support (or any new feature support) to at
least libgtpnl, and to make sure we test interoperability with existing
3GPP network equipment such as real IPv6 capable phones and SGSNs.
quoted
I'm not sure if this is a useful feature. GTP is used only in
operator-controlled networks and only on standard ports. It's not
possible to negotiate any non-standard ports on the signaling plane
either.
Bear in mind that we're not required to do everything the GTP spec
says.
Yes, we are, at least as long as it affects interoperability with other
implemetations out there.
GTP uses well-known port numbers on *both* sides of the tunnel, and you
cannot deviate from that.
There's no point in having all kinds of feetures in the GTP user plane
which are not interoperable with other implementations, and which are
completely outside of the information model / architecture of GTP.
In the real world, GTP-U is only used in combination with GTP-C. And in
GTP-C you can only negotiate the IP address of both sides of GTP-U, and
not the port number information. As a result, the port numbers are
static on both sides.
My impression is GTP designers probably didn't think in terms of
getting best performance. But we can ;-)
I think it's wasted efforts if it's about "random udp ports" as no
standards-compliant implementation out there with which you will have to
interoperate will be able to support it.
GTP is used between home and roaming operator. If you want to introduce
changes to how it works, you will have to have control over both sides
of the implementation of both the GTP-C and the GTP-u plane, which is
very unlikely and rather the exception in the hundreds of operators you
interoperate with. Also keep in mind that there often are various
"middleboxes" that will suddenly have to reflect your changes. That
starts from packet filters at various locations in the operator networks
and/or roaming hubs, down to GTP hubs and others.
My opinion is: Non-standard GTP ports are not going to happen.
I also brought up open_ggsn. ggsn to sgsn.
That's good to hear. For both v4 and v6 PDP contexts? Whcih phones
did you use for testing? Particularly given how convolved the address
allocation is (see below), I'm surprised it would work.
quoted
For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
related to the way how router solicitation/advertisements are modified
in the 3GPP world.
The address allocation in v4 is simple:
* MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
activation
* GGSN responds with IPv4 address in EUA of Activate PDP context
response (and then uses netlink to tell the kernel about that
IPv4 address)
In v6 or the v6 portion of v4v6 it works differently:
* MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
activation
* GGSN responds with an IPv6 address, but that address is *not* used
for communication, but simply used as an "interface identifier" to
build a link-local address.
* MS then uses router solicitation using that link-local address
* GGSN responds with router advertisement, allocating a single /64
prefix, from which the MS then generates a fully-qualified IPv6
source address for communication.
How did you envision this to be done with the v6 support you just added?
At the very least, the /64 prefix matching would have to be implemented
so that in fact all addresses within that /64 prefix are matched +
encapsulated for a given PDP context in the downlink (to phone)
direction.
[...]
I would hope all the above you're describing is mostly control plane
matters.
It is not. The control plane is GTP-C and runs on different UDP ports
(at least for GTPv1/v2). The user plane is GTP-U and is what's done in
the kernel. And by its very nature, IPv6 router
solicitations/advertisements (as well as neighbor
solicitations/advertisements) are part of the user plane and thus
handled in GTP-U.
At least a good design decouples data palne and control
plane. I know that GTP is a bit convoluted in this regard.
The problem is that IPv6 has never been specified properly for
point-to-point links. There's no decent PPP specs for IPv6. So the
3GPP folks had to try to be as close as possible to the existing
(broadcast) link layer model to facilitate existing IPv6 implemetations
to work over 3GPP bearers. That's why they kept whatever possible to
re-use in terms of neighbor/router discovery.
So the problem is now: Unless you handle GTP-U *entirely* in the kernel
(including router + neighbor advertisement/solicitation), you will have
a "split GTP-U" plane between kernel and userspace. And in that context
the question is who owns the sequence numbers, how will you avoid race
conditions, ... - my simple suggestion is thus to keep with the current
split and do everything GTP-U related inside the kernel and everything
GTP-C related in userspace.
I think there has to be a clear plan/architecture on how to implement
those bits in terms of the kernel/userspace split, and at least a proof
of concept implementation that we can show works with some real phones
out there - otherwise there's no point in having IPv6 support that works
well with some custom tools.
Regards,
Harald
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Tom Herbert <hidden> Date: 2017-09-19 23:47:13
On Tue, Sep 19, 2017 at 4:19 PM, Harald Welte [off-list ref] wrote:
Hi Tom,
On Tue, Sep 19, 2017 at 08:59:28AM -0700, Tom Herbert wrote:
quoted
On Tue, Sep 19, 2017 at 5:43 AM, Harald Welte [off-list ref]
wrote:
quoted
On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
quoted
- IPv6 support
see my detailed comments in other mails. It's unfortunately only
support for the already "deprecated" IPv6-only PDP contexts, not the
more modern v4v6 type. In order to interoperate with old and new
approach, all three cases (v4, v6 and v4v6) should be supported from
one code base.
It sounds like something that can be subsequently added.
Not entirely, at least on the netlink (and any other configuration
interface) you will have to reflect this from the very beginning. You
have to have an explicit PDP type and cannot rely on the address type to
specify the type of PDP context. Whatever interfaces are introduced
now will have to remain compatible to any future change.
My strategy to avoid any such possible 'road blocks' from being
introduced would be to simply add v4v6 and v6 support in one go. The
differences are marginal (having both an IPv6 prefix and a v4 address in
parallel, rather than mutually exclusive only).
quoted
Do you have a reference to the spec?
See http://osmocom.org/issues/2418#note-7 which lists Section 11.2.1.3.2
of 3GPP TS 29.061 in combination with RFC3314, RFC7066, RFC6459 and
3GPP TS 23.060 9.2.1 as well as a summary of my understanding of it some
months ago.
quoted
quoted
quoted
- Configurable networking interfaces so that GTP kernel can be
used and tested without needing GSN network emulation (i.e. no
user space daemon needed).
We have some pretty decent userspace utilities for configuring the
GTP interfaces and tunnels in the libgtpnl repository, but if it
helps people to have another way of configuration, I won't be
against it.
AFAIK those userspace utilities don't support IPv6.
Of course not [yet]. libgtpnl and the command line tools have been
implemented specifically for the in-kernel GTP driver, and you have to
make sure to add related support on both the kernel and the userspace
side (libgtpnl). So there's little point in adding features on either
side before the other side. There would be no way to test...
quoted
Being able to configure GTP like any other encapsulation will
facilitate development of IPv6 and other features.
That may very well be the case, but adding "IPv6 support" to kernel GTP
in a way that is not in line with the existing userspace libraries and
control-plane implementations means that you're developing those
features in an artificial environment that doesn't resemble real 3GPP
interoperable networks out there.
As indicated, I'm not against adding additional interfaces, but we have
to make sure that we add IPv6 support (or any new feature support) to at
least libgtpnl, and to make sure we test interoperability with existing
3GPP network equipment such as real IPv6 capable phones and SGSNs.
quoted
quoted
I'm not sure if this is a useful feature. GTP is used only in
operator-controlled networks and only on standard ports. It's not
possible to negotiate any non-standard ports on the signaling plane
either.
Bear in mind that we're not required to do everything the GTP spec
says.
Yes, we are, at least as long as it affects interoperability with other
implemetations out there.
GTP uses well-known port numbers on *both* sides of the tunnel, and you
cannot deviate from that.
There's no point in having all kinds of feetures in the GTP user plane
which are not interoperable with other implementations, and which are
completely outside of the information model / architecture of GTP.
In the real world, GTP-U is only used in combination with GTP-C. And in
GTP-C you can only negotiate the IP address of both sides of GTP-U, and
not the port number information. As a result, the port numbers are
static on both sides.
quoted
My impression is GTP designers probably didn't think in terms of
getting best performance. But we can ;-)
I think it's wasted efforts if it's about "random udp ports" as no
standards-compliant implementation out there with which you will have to
interoperate will be able to support it.
GTP is used between home and roaming operator. If you want to introduce
changes to how it works, you will have to have control over both sides
of the implementation of both the GTP-C and the GTP-u plane, which is
very unlikely and rather the exception in the hundreds of operators you
interoperate with. Also keep in mind that there often are various
"middleboxes" that will suddenly have to reflect your changes. That
starts from packet filters at various locations in the operator networks
and/or roaming hubs, down to GTP hubs and others.
My opinion is: Non-standard GTP ports are not going to happen.
quoted
I also brought up open_ggsn. ggsn to sgsn.
That's good to hear. For both v4 and v6 PDP contexts? Whcih phones
did you use for testing? Particularly given how convolved the address
allocation is (see below), I'm surprised it would work.
quoted
quoted
For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
related to the way how router solicitation/advertisements are modified
in the 3GPP world.
The address allocation in v4 is simple:
* MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
activation
* GGSN responds with IPv4 address in EUA of Activate PDP context
response (and then uses netlink to tell the kernel about that
IPv4 address)
In v6 or the v6 portion of v4v6 it works differently:
* MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
activation
* GGSN responds with an IPv6 address, but that address is *not* used
for communication, but simply used as an "interface identifier" to
build a link-local address.
* MS then uses router solicitation using that link-local address
* GGSN responds with router advertisement, allocating a single /64
prefix, from which the MS then generates a fully-qualified IPv6
source address for communication.
How did you envision this to be done with the v6 support you just added?
At the very least, the /64 prefix matching would have to be implemented
so that in fact all addresses within that /64 prefix are matched +
encapsulated for a given PDP context in the downlink (to phone)
direction.
[...]
I would hope all the above you're describing is mostly control plane
matters.
It is not. The control plane is GTP-C and runs on different UDP ports
(at least for GTPv1/v2). The user plane is GTP-U and is what's done in
the kernel. And by its very nature, IPv6 router
solicitations/advertisements (as well as neighbor
solicitations/advertisements) are part of the user plane and thus
handled in GTP-U.
quoted
At least a good design decouples data palne and control
plane. I know that GTP is a bit convoluted in this regard.
The problem is that IPv6 has never been specified properly for
point-to-point links. There's no decent PPP specs for IPv6. So the
3GPP folks had to try to be as close as possible to the existing
(broadcast) link layer model to facilitate existing IPv6 implemetations
to work over 3GPP bearers. That's why they kept whatever possible to
re-use in terms of neighbor/router discovery.
So the problem is now: Unless you handle GTP-U *entirely* in the kernel
(including router + neighbor advertisement/solicitation), you will have
a "split GTP-U" plane between kernel and userspace. And in that context
the question is who owns the sequence numbers, how will you avoid race
conditions, ... - my simple suggestion is thus to keep with the current
split and do everything GTP-U related inside the kernel and everything
GTP-C related in userspace.
I think there has to be a clear plan/architecture on how to implement
those bits in terms of the kernel/userspace split, and at least a proof
of concept implementation that we can show works with some real phones
out there - otherwise there's no point in having IPv6 support that works
well with some custom tools.
OTOH, I will argue that the GTP patches should never have been allowed
in the kernel in the first place without IPv6 support! ;-) I think the
best plan forward is to get the IPv6 data path running that so can
demonstrate a functional GTP/IPv6 datapath (my primary purpose here to
have something to compare against with ILA). Since "real"
configuration path doesn't use the path to set up a standalone
interface, I would presume that that will be fleshed when someone has
cycles and expertise to work on both sides of the problem. Even if
this requires structural changes to how IPv6 is managed in GTP, I
doubt that the fundamental TX/RX, GRO/GSO data paths will change much.
In other words, please consider this to be a step on an evolutionary
path. More work is required to reach the ultimate deployable solution.
As for testing on real phones, that is cannot be a requirement for a
kernel feature. If you expect Linux community to support this, then we
need a way to be develop and test on commodity PC hardware. That is
one of the major values of creating a standalone interface
configuration-- we can test the datapath just like any other
encapsulation supported by the kernel.
Tom
I know you are just following the pattern of the existing "ipv4_hashfn()" here
but this kind of stuff is not very global namespace friendly. Even simply
adding a "gtp_" prefix to these hash functions would be a lot better.
I would agree if this was an inline function defined in a header file or
a non-static function. But where is the global namespace concern in
case of static inline functions defined and used in the same .c file?
The problem is if we create a generic ipv6_hashfn() in linux/ipv6.h or
something like that, then this driver stops building.
It was a carry over since ipv4_hashfn was already defined in the file.
I will prefix both functions.
From: Andreas Schultz <hidden> Date: 2017-09-20 15:34:27
Hi Harald,
On 20/09/17 01:19, Harald Welte wrote:
Hi Tom,
On Tue, Sep 19, 2017 at 08:59:28AM -0700, Tom Herbert wrote:
quoted
On Tue, Sep 19, 2017 at 5:43 AM, Harald Welte [off-list ref]
wrote:
quoted
On Mon, Sep 18, 2017 at 05:38:50PM -0700, Tom Herbert wrote:
quoted
- IPv6 support
see my detailed comments in other mails. It's unfortunately only
support for the already "deprecated" IPv6-only PDP contexts, not the
more modern v4v6 type. In order to interoperate with old and new
approach, all three cases (v4, v6 and v4v6) should be supported from
one code base.
It sounds like something that can be subsequently added.
Not entirely, at least on the netlink (and any other configuration
interface) you will have to reflect this from the very beginning. You
have to have an explicit PDP type and cannot rely on the address type to
specify the type of PDP context. Whatever interfaces are introduced
now will have to remain compatible to any future change.
My strategy to avoid any such possible 'road blocks' from being
introduced would be to simply add v4v6 and v6 support in one go. The
differences are marginal (having both an IPv6 prefix and a v4 address in
parallel, rather than mutually exclusive only).
quoted
Do you have a reference to the spec?
See http://osmocom.org/issues/2418#note-7 which lists Section 11.2.1.3.2
of 3GPP TS 29.061 in combination with RFC3314, RFC7066, RFC6459 and
3GPP TS 23.060 9.2.1 as well as a summary of my understanding of it some
months ago.
quoted
quoted
quoted
- Configurable networking interfaces so that GTP kernel can be
used and tested without needing GSN network emulation (i.e. no
user space daemon needed).
We have some pretty decent userspace utilities for configuring the
GTP interfaces and tunnels in the libgtpnl repository, but if it
helps people to have another way of configuration, I won't be
against it.
AFAIK those userspace utilities don't support IPv6.
Of course not [yet]. libgtpnl and the command line tools have been
implemented specifically for the in-kernel GTP driver, and you have to
make sure to add related support on both the kernel and the userspace
side (libgtpnl). So there's little point in adding features on either
side before the other side. There would be no way to test...
quoted
Being able to configure GTP like any other encapsulation will
facilitate development of IPv6 and other features.
That may very well be the case, but adding "IPv6 support" to kernel GTP
in a way that is not in line with the existing userspace libraries and
control-plane implementations means that you're developing those
features in an artificial environment that doesn't resemble real 3GPP
interoperable networks out there.
As indicated, I'm not against adding additional interfaces, but we have
to make sure that we add IPv6 support (or any new feature support) to at
least libgtpnl, and to make sure we test interoperability with existing
3GPP network equipment such as real IPv6 capable phones and SGSNs.
quoted
quoted
I'm not sure if this is a useful feature. GTP is used only in
operator-controlled networks and only on standard ports. It's not
possible to negotiate any non-standard ports on the signaling plane
either.
Bear in mind that we're not required to do everything the GTP spec
says.
Yes, we are, at least as long as it affects interoperability with other
implemetations out there.
GTP uses well-known port numbers on *both* sides of the tunnel, and you
cannot deviate from that.
Actually, the well-known port is only mandatory for the receiving side.
The sending side can choose any port it wishes as long as it is prepared
to receive possible error indication on the well-known port.
Of course, it makes the implementation simple to use only one port, but
for scalability it might be a good idea to support per PDP context
sending ports.
Regards
Andreas
There's no point in having all kinds of feetures in the GTP user plane
which are not interoperable with other implementations, and which are
completely outside of the information model / architecture of GTP.
In the real world, GTP-U is only used in combination with GTP-C. And in
GTP-C you can only negotiate the IP address of both sides of GTP-U, and
not the port number information. As a result, the port numbers are
static on both sides.
quoted
My impression is GTP designers probably didn't think in terms of
getting best performance. But we can ;-)
I think it's wasted efforts if it's about "random udp ports" as no
standards-compliant implementation out there with which you will have to
interoperate will be able to support it.
GTP is used between home and roaming operator. If you want to introduce
changes to how it works, you will have to have control over both sides
of the implementation of both the GTP-C and the GTP-u plane, which is
very unlikely and rather the exception in the hundreds of operators you
interoperate with. Also keep in mind that there often are various
"middleboxes" that will suddenly have to reflect your changes. That
starts from packet filters at various locations in the operator networks
and/or roaming hubs, down to GTP hubs and others.
My opinion is: Non-standard GTP ports are not going to happen.
quoted
I also brought up open_ggsn. ggsn to sgsn.
That's good to hear. For both v4 and v6 PDP contexts? Whcih phones
did you use for testing? Particularly given how convolved the address
allocation is (see below), I'm surprised it would work.
quoted
quoted
For IPv6 (and v4v6) PDP contexts there is quite a bit of extra headache
related to the way how router solicitation/advertisements are modified
in the 3GPP world.
The address allocation in v4 is simple:
* MS/UE requests dynamic or fixed IPv4 address via EUA IE of PDP context
activation
* GGSN responds with IPv4 address in EUA of Activate PDP context
response (and then uses netlink to tell the kernel about that
IPv4 address)
In v6 or the v6 portion of v4v6 it works differently:
* MS/UE requests dynamic or fixed IPv4 address in EUA IE of PDP context
activation
* GGSN responds with an IPv6 address, but that address is *not* used
for communication, but simply used as an "interface identifier" to
build a link-local address.
* MS then uses router solicitation using that link-local address
* GGSN responds with router advertisement, allocating a single /64
prefix, from which the MS then generates a fully-qualified IPv6
source address for communication.
How did you envision this to be done with the v6 support you just added?
At the very least, the /64 prefix matching would have to be implemented
so that in fact all addresses within that /64 prefix are matched +
encapsulated for a given PDP context in the downlink (to phone)
direction.
[...]
I would hope all the above you're describing is mostly control plane
matters.
It is not. The control plane is GTP-C and runs on different UDP ports
(at least for GTPv1/v2). The user plane is GTP-U and is what's done in
the kernel. And by its very nature, IPv6 router
solicitations/advertisements (as well as neighbor
solicitations/advertisements) are part of the user plane and thus
handled in GTP-U.
quoted
At least a good design decouples data palne and control
plane. I know that GTP is a bit convoluted in this regard.
The problem is that IPv6 has never been specified properly for
point-to-point links. There's no decent PPP specs for IPv6. So the
3GPP folks had to try to be as close as possible to the existing
(broadcast) link layer model to facilitate existing IPv6 implemetations
to work over 3GPP bearers. That's why they kept whatever possible to
re-use in terms of neighbor/router discovery.
So the problem is now: Unless you handle GTP-U *entirely* in the kernel
(including router + neighbor advertisement/solicitation), you will have
a "split GTP-U" plane between kernel and userspace. And in that context
the question is who owns the sequence numbers, how will you avoid race
conditions, ... - my simple suggestion is thus to keep with the current
split and do everything GTP-U related inside the kernel and everything
GTP-C related in userspace.
I think there has to be a clear plan/architecture on how to implement
those bits in terms of the kernel/userspace split, and at least a proof
of concept implementation that we can show works with some real phones
out there - otherwise there's no point in having IPv6 support that works
well with some custom tools.
Regards,
Harald
From: Andreas Schultz <hidden> Date: 2017-09-20 15:37:10
On 19/09/17 02:38, Tom Herbert wrote:
Add new configuration of GTP interfaces that allow specifying a port to
listen on (as opposed to having to get sockets from a userspace control
plane). This allows GTP interfaces to be configured and the data path
tested without requiring a GTP-C daemon.
This would imply that you can have multiple independent GTP sockets on
the same IP address.That is not permitted by the GTP specifications.
3GPP TS 29.281, section 4.3 states clearly that there is "only" one GTP
entity per IP address.A PDP context is defined by the destination IP and
the TEID. The destination port is not part of the identity of a PDP context.
Even the source IP and source port are not part of the tunnel identity.
This makes is possible to send traffic from a new SGSN/SGW during
handover before the control protocol has announced the handover.
At this point the usual response is: THAT IS NOT SAFE. Yes, GTP has been
designed for cooperative networks only and should not be used on
hostile/unsecured networks.
On the sending side, using multiple ports is permitted as long as the
default GTP port is always able to receive incoming messages.
Andreas
[...]
From: Andreas Schultz <hidden> Date: 2017-09-20 15:37:55
On 19/09/17 14:09, Harald Welte wrote:
Hi Dave,
On Mon, Sep 18, 2017 at 09:17:51PM -0700, David Miller wrote:
quoted
This and the new dst caching code ignores any source address selection
done by ip_route_output_key() or the new tunnel route lookup helpers.
Either source address selection should be respected, or if saddr will
never be modified by a route lookup for some specific reason here,
that should be documented.
The IP source address is fixed by signaling on the GTP-C control plane
and nothing that the kernel can unilaterally decide to change. Such a
change of address would have to be decided by and first be signaled on
GTP-C to the peer by the userspace daemon, which would then update the
PDP context in the kernel.
I think we had this discussion before. The sending IP and port are not
part of the identity of the PDP context. So IMHO the sender is permitted
to change the source IP at random.
Regards
Andreas
So I guess you're asking us to document that rationale as form of a
source code comment ?
From: Tom Herbert <hidden> Date: 2017-09-20 15:57:27
On Wed, Sep 20, 2017 at 8:27 AM, Andreas Schultz [off-list ref] wrote:
On 19/09/17 02:38, Tom Herbert wrote:
quoted
Add new configuration of GTP interfaces that allow specifying a port to
listen on (as opposed to having to get sockets from a userspace control
plane). This allows GTP interfaces to be configured and the data path
tested without requiring a GTP-C daemon.
This would imply that you can have multiple independent GTP sockets on the
same IP address.That is not permitted by the GTP specifications. 3GPP TS
29.281, section 4.3 states clearly that there is "only" one GTP entity per
IP address.A PDP context is defined by the destination IP and the TEID. The
destination port is not part of the identity of a PDP context.
We are in no way trying change GTP, if someone runs this in a real GTP
network then they need to abide by the specification. However, there
is nothing inconsistent and it breaks nothing if someone wishes to use
different port numbers in their own private network for testing or
development purposes. Every other UDP application that has assigned
port number allows configurable ports, I don't see that GTP is so
special that it should be an exception.
Tom
From: Andreas Schultz <hidden> Date: 2017-09-20 16:07:51
On 20/09/17 17:57, Tom Herbert wrote:
On Wed, Sep 20, 2017 at 8:27 AM, Andreas Schultz [off-list ref] wrote:
quoted
On 19/09/17 02:38, Tom Herbert wrote:
quoted
Add new configuration of GTP interfaces that allow specifying a port to
listen on (as opposed to having to get sockets from a userspace control
plane). This allows GTP interfaces to be configured and the data path
tested without requiring a GTP-C daemon.
This would imply that you can have multiple independent GTP sockets on the
same IP address.That is not permitted by the GTP specifications. 3GPP TS
29.281, section 4.3 states clearly that there is "only" one GTP entity per
IP address.A PDP context is defined by the destination IP and the TEID. The
destination port is not part of the identity of a PDP context.
We are in no way trying change GTP, if someone runs this in a real GTP
network then they need to abide by the specification. However, there
is nothing inconsistent and it breaks nothing if someone wishes to use
different port numbers in their own private network for testing or
development purposes. Every other UDP application that has assigned
port number allows configurable ports, I don't see that GTP is so
special that it should be an exception.
GTP isn't special, I just don't like to have testing only features in
there when the same goal can be reached without having to add extra
stuff. Adding code that is not going to be useful in real production
setups (or in this case would even break production setups when enabled
accidentally) makes the implementation more complex than it needs to be.
You can always add multiple IP's to your test system and have the same
effect without having to change the ports.
Regards
Andreas
From: Tom Herbert <hidden> Date: 2017-09-20 16:24:09
On Wed, Sep 20, 2017 at 9:07 AM, Andreas Schultz [off-list ref] wrote:
On 20/09/17 17:57, Tom Herbert wrote:
quoted
On Wed, Sep 20, 2017 at 8:27 AM, Andreas Schultz [off-list ref]
wrote:
quoted
On 19/09/17 02:38, Tom Herbert wrote:
quoted
Add new configuration of GTP interfaces that allow specifying a port to
listen on (as opposed to having to get sockets from a userspace control
plane). This allows GTP interfaces to be configured and the data path
tested without requiring a GTP-C daemon.
This would imply that you can have multiple independent GTP sockets on
the
same IP address.That is not permitted by the GTP specifications. 3GPP TS
29.281, section 4.3 states clearly that there is "only" one GTP entity
per
IP address.A PDP context is defined by the destination IP and the TEID.
The
destination port is not part of the identity of a PDP context.
We are in no way trying change GTP, if someone runs this in a real GTP
network then they need to abide by the specification. However, there
is nothing inconsistent and it breaks nothing if someone wishes to use
different port numbers in their own private network for testing or
development purposes. Every other UDP application that has assigned
port number allows configurable ports, I don't see that GTP is so
special that it should be an exception.
GTP isn't special, I just don't like to have testing only features in there
when the same goal can be reached without having to add extra stuff. Adding
code that is not going to be useful in real production setups (or in this
case would even break production setups when enabled accidentally) makes the
implementation more complex than it needs to be.
Well, you could make the same argument that allowing GTP to configured
as standalone interface is a problem since GTP is only allowed to be
with used with GTP-C. But, then we have something in the kernel that
the community is expected to support, but requires jumping through a
whole bunch of hoops just to run a simple netperf. The more that
patches and features look like other things in the kernel that are
already well established, the better the chances we can accept them
and support them. It's probably a natural consequence of any large
open source project, so sometimes it's worth the effort to add a few
lines of complexity to get the benefits of community contribution and
support.
Tom
From: Tom Herbert <hidden> Date: 2017-09-20 18:09:30
On Mon, Sep 18, 2017 at 9:24 PM, David Miller [off-list ref] wrote:
From: Tom Herbert <redacted>
Date: Mon, 18 Sep 2017 17:39:02 -0700
quoted
Add configuration to control use of zero checksums on transmit for both
IPv4 and IPv6, and control over accepting zero IPv6 checksums on
receive.
Signed-off-by: Tom Herbert <redacted>
I thought we were trying to move away from this special case of allowing
zero UDP checksums with tunnels, especially for ipv6.
I don't have a strong preference either way. I like consistency with
VXLAN and foo/UDP, but I guess it's not required. Interestingly, since
GTP only carries IP, IPv6 zero checksums are actually safer here than
VXLAN or GRE/UDP.
Tom
From: Tom Herbert <hidden> Date: 2017-09-20 20:40:56
On Wed, Sep 20, 2017 at 12:45 PM, David Miller [off-list ref] wrote:
From: Tom Herbert <redacted>
Date: Wed, 20 Sep 2017 11:03:52 -0700
quoted
On Mon, Sep 18, 2017 at 9:19 PM, David Miller [off-list ref] wrote:
quoted
From: Tom Herbert <redacted>
Date: Mon, 18 Sep 2017 17:38:58 -0700
quoted
Allow peers to be specified by IPv6 addresses.
Signed-off-by: Tom Herbert <redacted>
Hmmm, can you just check the socket family or something like that?
I'm not sure what code you're referring to.
There is a socket associated with the tunnel to do the encapsulation
and it has an address family, right?
If fd's are set from userspace for the sockets then we could derive
the address family from them. I'll change that. Although, looking at
now I am wondering why were passing fds into GTP instead of just
having the kernel create the UDP port like is done for other encaps.
Tom
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-21 00:13:30
Hi Tom,
On Wed, Sep 20, 2017 at 09:24:07AM -0700, Tom Herbert wrote:
On Wed, Sep 20, 2017 at 9:07 AM, Andreas Schultz [off-list ref] wrote:
quoted
GTP isn't special, I just don't like to have testing only features in there
when the same goal can be reached without having to add extra stuff. Adding
code that is not going to be useful in real production setups (or in this
case would even break production setups when enabled accidentally) makes the
implementation more complex than it needs to be.
Well, you could make the same argument that allowing GTP to configured
as standalone interface is a problem since GTP is only allowed to be
with used with GTP-C. But, then we have something in the kernel that
the community is expected to support, but requires jumping through a
whole bunch of hoops just to run a simple netperf.
"A whole bunch of hoops" without your new interface would consist of
running a single command-line program that is supplied with libgtpnl.
This is not a complete 3GPP network, but a simple libmnl-based helper
library with no other depenencies.
I'm not neccessarily against introducing features like the 'standalone
interface configuration'. However, we must make sure that any
significant new feature contributions like IPv6 are tested in a
"realistic setup" and not just using those 'interfaces added for easy
development'. Also, I would argue those 'interfaces added for easy
deveopment/benchmarking' should probably be clearly marked as such to
avoid raising the impression that this is what leads to a
standard-conforming / production-type setup.
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-21 00:13:31
Hi Tom,
On Wed, Sep 20, 2017 at 01:40:54PM -0700, Tom Herbert wrote:
On Wed, Sep 20, 2017 at 12:45 PM, David Miller [off-list ref] wrote:
quoted
There is a socket associated with the tunnel to do the encapsulation
and it has an address family, right?
If fd's are set from userspace for the sockets then we could derive
the address family from them. I'll change that. Although, looking at
now I am wondering why were passing fds into GTP instead of just
having the kernel create the UDP port like is done for other encaps.
because the userspace process has to take care of those bits of GTP-U
that the kernel doesn't, such as responding to GTP ECHO requests with
GTP echo responses. Only the "GTP Message type G-PDU" is handled in the
kernel, as only those frames contain user plane. See table 1 of Section
7.1 of 3GPP TS 29.060.
If you create the socket in the kernel, how would you hand the socket to
the userspace process later on?
IMHO, it feels more natural to simply create it in userspace (like you
would do in the non-kernel-accelerated case) and then simply handle the
G-PDU messages in the kernel while doing the rest in userspace.
But if there's another method that feels more usual to the kernel
community, I'm not against any changes - but given kernel policies, we'd
have to keep userspace compatbility, right?
Regards,
Harald
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Tom Herbert <hidden> Date: 2017-09-21 00:16:54
On Wed, Sep 20, 2017 at 5:04 PM, Harald Welte [off-list ref] wrote:
Hi Tom,
On Wed, Sep 20, 2017 at 01:40:54PM -0700, Tom Herbert wrote:
quoted
On Wed, Sep 20, 2017 at 12:45 PM, David Miller [off-list ref] wrote:
quoted
There is a socket associated with the tunnel to do the encapsulation
and it has an address family, right?
If fd's are set from userspace for the sockets then we could derive
the address family from them. I'll change that. Although, looking at
now I am wondering why were passing fds into GTP instead of just
having the kernel create the UDP port like is done for other encaps.
because the userspace process has to take care of those bits of GTP-U
that the kernel doesn't, such as responding to GTP ECHO requests with
GTP echo responses. Only the "GTP Message type G-PDU" is handled in the
kernel, as only those frames contain user plane. See table 1 of Section
7.1 of 3GPP TS 29.060.
From: Tom Herbert <hidden> Date: 2017-09-21 00:55:03
On Wed, Sep 20, 2017 at 5:13 PM, Harald Welte [off-list ref] wrote:
Hi Tom,
On Wed, Sep 20, 2017 at 09:24:07AM -0700, Tom Herbert wrote:
quoted
On Wed, Sep 20, 2017 at 9:07 AM, Andreas Schultz [off-list ref] wrote:
quoted
GTP isn't special, I just don't like to have testing only features in there
when the same goal can be reached without having to add extra stuff. Adding
code that is not going to be useful in real production setups (or in this
case would even break production setups when enabled accidentally) makes the
implementation more complex than it needs to be.
Well, you could make the same argument that allowing GTP to configured
as standalone interface is a problem since GTP is only allowed to be
with used with GTP-C. But, then we have something in the kernel that
the community is expected to support, but requires jumping through a
whole bunch of hoops just to run a simple netperf.
"A whole bunch of hoops" without your new interface would consist of
running a single command-line program that is supplied with libgtpnl.
This is not a complete 3GPP network, but a simple libmnl-based helper
library with no other depenencies.
You have the point of view of someone who has a lot of experience
dealing with this protocol. Try to imagine if you were some random
kernel network programmer with no experience in the area. If they
happen to find a one-off bug and want to do the right thing by running
a test, you want to make that as easy as possible. From that
perspective, building protocol specific libraries and finding the
right cmd line to run is significant hoops (I can attest to this).
There are other examples in the kernel of systems bigger than GTP that
require a whole lot of effort just to run a simple test; you'll notice
for those it's rare that best developers ever bother to look at them
unless they're making a global change that affects the code. We don't
want GTP to take be like that!
I'm not neccessarily against introducing features like the 'standalone
interface configuration'. However, we must make sure that any
significant new feature contributions like IPv6 are tested in a
"realistic setup" and not just using those 'interfaces added for easy
development'. Also, I would argue those 'interfaces added for easy
deveopment/benchmarking' should probably be clearly marked as such to
avoid raising the impression that this is what leads to a
standard-conforming / production-type setup.
Given the obvious complexity of running a real GTP stack, I don't
think we have to worry about this. In order to test a "realistic
setup" a whole bunch of other support is needed. So the forward
looking question now is how to get to be able to run a "realistic
setup"?
Tom
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-21 04:49:32
Hi Tom,
On Wed, Sep 20, 2017 at 11:09:29AM -0700, Tom Herbert wrote:
On Mon, Sep 18, 2017 at 9:24 PM, David Miller [off-list ref] wrote:
quoted
From: Tom Herbert <redacted>
quoted
Add configuration to control use of zero checksums on transmit for both
IPv4 and IPv6, and control over accepting zero IPv6 checksums on
receive.
I thought we were trying to move away from this special case of allowing
zero UDP checksums with tunnels, especially for ipv6.
I don't have a strong preference either way. I like consistency with
VXLAN and foo/UDP, but I guess it's not required. Interestingly, since
GTP only carries IP, IPv6 zero checksums are actually safer here than
VXLAN or GRE/UDP.
Just for the record: I don't care either way and I defer to the kernel
networking developers to decide if they want to have zero UDP checksum
in GTP or not.
The 3GPP specs don't say anything about UDP checksums. So there's no
requirement to use them, and hence operation without UDP checksums
should be compliant. Cisco GTP implementation has udp checksumming
configurable, so other implementations also seem to provide both ways.
In general, I would argue one wants UDP checksumming of GTP in all
setups, as while the inner IP packet might be protected, the GTP header
itself is not, and that's what contains important data suhc as the TEID
(Tunnel Endpoint ID). But that's of course just my personal opinion,
and I'm not saying we should prevent people from using lower protection
if that's what they want.
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-21 15:39:01
Hi Tom,
On Tue, Sep 19, 2017 at 04:47:11PM -0700, Tom Herbert wrote:
On Tue, Sep 19, 2017 at 4:19 PM, Harald Welte [off-list ref] wrote:
quoted
I think there has to be a clear plan/architecture on how to implement
those bits in terms of the kernel/userspace split, and at least a proof
of concept implementation that we can show works with some real phones
out there - otherwise there's no point in having IPv6 support that works
well with some custom tools.
OTOH, I will argue that the GTP patches should never have been allowed
in the kernel in the first place without IPv6 support! ;-)
Well, it could be shown that the code works with integration to OpenGGSN
(and later ergw, and now OsmoGGSN) and works within a complete 3GPP
network. So we were not merging something that we hypothesized it would
work once the rest would be implemented, but we could actually show it
was working before it got merged.
I think the best plan forward is to get the IPv6 data path running
that so can demonstrate a functional GTP/IPv6 datapath
Yes, but a functional "datapath" (= GTP-U) unfortunately includes the
way how address allocation/assignment is done. Putting something into
the mainline kernel that we know will for sure not work/interop in a
real scenario is not a good idea. I think what's worse than not
supporting a feature is to implying support for it while actually
doing it in an incomplete/incompliant way.
So from my point of view, what's needed is
* making sure router advertisement/solicitation are covered in some
way, either by doing it in the kernel or having a clear strategy how
it could be done from userspace while not a) introducing races regarding
who owns the sequence numbers
* making sure the implementation covers entire /64 prefixes for each
PDP context and not single addresses. That is non-negotiable and
mandatory by 3GPP specs.
Since "real" configuration path doesn't use the path to set up a
standalone interface, I would presume that that will be fleshed when
someone has cycles and expertise to work on both sides of the problem.
The configuration will be different, yes. but we need to ensure that the
actual *implementation* of the data path does what it is expected to do,
no matter who configures it via which interface.
Even if this requires structural changes to how IPv6 is managed in
GTP, I doubt that the fundamental TX/RX, GRO/GSO data paths will
change much. In other words, please consider this to be a step on an
evolutionary path. More work is required to reach the ultimate
deployable solution.
Agreed. But then I'm still against merging something that we know for
sure will not be compatible with real-world use case. It should be kept
out of mainline until we are sure of that, at the very least
theoretically, but even better which we can prove in practise will do
what it claims to do.
As for testing on real phones, that is cannot be a requirement for a
kernel feature.
GTP is not implemented on phones. GTP is implemented only inside the
fixed (land-side) of the cellular network. However, the inner IP data
originates from phones, and large parts of what you see on GTP
originates from phones in a different format. The inner IP data inside
GTP-U originates from phones. And that's where address configuration
for IPv6 works.
If you expect Linux community to support this, then we
need a way to be develop and test on commodity PC hardware.
I'm not arguing you need to run any of the code on a different
architecture such as a phone. I'm arguing for you to run a cellular
network including the kernel GTP code, and then use that cellular
network from a real phone. This is the only way to know for sure you
interoperate. See my other mail related to the Open Soruce based
configurations for both 2G and 3G that can be used for this.
As a replacement, one can e.g. look at protocol traces of real phones
and then simulate the behavior of one or several different phones and
implement that as test caess. This is what I did in the GGSN_Test
pointed out in my other mail in this thread. And btw, all I've asked is
for showing it works with *one* phone model at all. I'm not talking
about the various different implementation specifics, such as whether or
not the phone will insist on using neighbor solicitation and mandate
neighbor advertisement (on a point-to-point link, how absurd!) after
the (mandatory) router discovery.
That is one of the major values of creating a standalone interface
configuration-- we can test the datapath just like any other
encapsulation supported by the kernel.
Well, you cannot. You might be able to do some benchmarking to compare
if an old version of the kernel gtp driver will perform better or worse
than some optimizations introduced. But you can *not* have a realistic
functional test that will tell you if your implementation is 'valid'.
I'm not even talking about being 'complete' here, but simply about being
broken or not. Or test whether it will interoperate. Particularly for
IPv6 this is impossible, due to the conflated way of involving both
GTP-C and GTP-U with router advertisement+solicitation for PDP context
activation, as outlined several times in this thread.
I wish it was simpler, but I haven't created GTP, sorry :)
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <hidden> Date: 2017-09-21 15:39:01
Hi Tom,
On Wed, Sep 20, 2017 at 05:55:01PM -0700, Tom Herbert wrote:
You have the point of view of someone who has a lot of experience
dealing with this protocol. Try to imagine if you were some random
kernel network programmer with no experience in the area. If they
happen to find a one-off bug and want to do the right thing by running
a test, you want to make that as easy as possible.
Agreed. But we're not talking abut fixing a random bug in your patch
series, but we're talking about adding significant new features - and
those features need to be tested in real use caes, not just in an
artificial test setup that holds assumptions that are not true.
To improve performance, or to fix simple bugs that only affect the
processing of the GTP-U G-PDU, a much more limited and hence
"unrealistic" test scenario is probably sufficient/acceptable.
From that perspective, building protocol specific libraries and
finding the right cmd line to run is significant hoops (I can attest
to this).
I understand your argument. But then, there is actually quite some
tools to help you (see further below), as well as the wiki page at
http://osmocom.org/projects/linux-kernel-gtp-u/wiki/Basic_Testing
Of course, existing tools and existing wiki pages also only document
existing features of the kernel code :)
Yes, the documentation could be better. But then, how much more can you
expect from somebody who's doing this mostly for fun and who - despite
working in his dayjob on FOSS cellular projects - has no single
commercial project/context that uses the kernel GTP code.
In any case, working on a specific protocol or technology will require
that you understand that technology to some extent, including the
available tools. There's always a learning curve involved.
There are other examples in the kernel of systems bigger than GTP that
require a whole lot of effort just to run a simple test; you'll notice
for those it's rare that best developers ever bother to look at them
unless they're making a global change that affects the code. We don't
want GTP to take be like that!
I'm all for following your argument. My point is simply: You cannot
develop code solely based on mock-ups without any 'realistic' test
scenarios. Otherwise you will end up with something that works only in
your artificial lab setup, and follows all the best practises of the way
how the Linux kernel traditionally approaches tunneling implementations,
but it will never work/interop in the real world.
And I'm very strongly opposed to merging code where we have not been
able to show that it will inter-operate in at least one realistic
scenario. This would raise wrong expectations with users and all sorts
of downstream problems.
So let's say we merge your IPv6 support as-is, and kernels get released
+ shipped with it. Later on, we find that in order to turn it into a
standards-compliant implementation together with all the required bits
in userspace and on the control plane, we need to change some parts of
it, particularly those parts that affect the netlink or any other
exposed userspace interface. At that point, we cannot change the
interface as the kernel has a strict rule of never breaking userspace
ABI. But we must change it in order to make it work in the real world.
So what do we do? Add lots of cruft in order to emulate backwards
compatibility?
So the forward looking question now is how to get to be able to run a
"realistic setup"?
You can run this realistic setup entirely using Osmocom components.
For running a 2G network: OsmoBTS+OsmoNITB+OsmoSGSN
For running a 3G network: OsmoHNBGW+OsmoMSC+OsmoHLR+OsmoSGSN
Both above stacks/combinations will provide you with GTP-C and GTP-U
against a GGSN. As GGSN, you can then use either OpenGGSN, or OsmoGGSN,
or ergw. For OpenGGSN and ergw, this will work with kernel GTP today,
for those features present in kernel GTP (i.e. IPv4-only).
In both cases you need some RF hardware. I'm happy to contribute
related hardware (and support getting it set up) free of charge from my
company sysmocom to anyone who has a realistic prospect of either
* integrating your IPv6 support or other significant feature patches with
libgtpnl + OsmoGGSN (at which point you can run a complete setup with
real phones to verify it works end-to-end)
* building and documenting or operating a continuous integration setup
that would run tests on each new kernel version (or net-next, or
whatever tree makes sense) to help us catch any regressions as the
code proceeds
In order to have a smaller, but still realistic test scenario, I
implemented a series of GTP tests in
http://git.osmocom.org/osmo-ttcn3-hacks/tree/ggsn_tests/GGSN_Tests.ttcn
This code basically emulates the combination of everything from Phone to
SGSN, so that you have only two entities:
* the implementation under test = IUT (a GGSN implementing GTP-C + GTP-U)
* the test itself (GGSN_Test) executing against the IUT
The code so far implements PDP context activation + address allocation
for IPv4-only and IPv6-only cases and can be run against a GGSN
implementing those. The IPv6-only PDP context unit tests include the
convoluted two-phase address assignment including sending the router
solicitation from the simulated phone as well as verifying the router
advertisement sent in response from the GGSN.
Yes, I know they're written in an unknown niche programming language
called TTCN-3, but this was the best tool at hand for the job I could
find, and the tests are open source as is the Eclipse Titan toolchain
for compiling it.
We even have a Dockerfile that will build you a docker container
containing the compiled GGSN_test at
http://git.osmocom.org/docker-playground/tree/ggsn-test
That docker container is used by a jenkins build test job to test
current OsmoGGSN master every night against the test suite.
I was stupid enough to break the testsuite with an accidential commit,
so tests between August 27th and today failed. I've just reverted that
accident - don't let that mistake of mine mislead you.
I'm happy to contribute further to this by adding actual user-IP GTP-U
functional testing beyond the router soliciatation/advertisement to it.
So my suggestion in terms of a "realistic testbed without having to
configure + run dozens of programs and using real RF + phones" is to use
that testsuite.
What one cannot get around is having to implement support for new
features added on the kernel side such as IPv6 in libgtpnl and at least
one of the GGSN's using it, sorry. Without that, there is no way to
know if that code would do anything useful. You simply cannot
realistically test GTP-U alone without GTP-C.
I'd love to offer help on this, but it's really impossible right now.
I'm on holidays on a motorbike tour through rural Taiwan's mountains,
had to deal with a flat tire today, have limited connectivity and am
already cutting down hard on sleep every night to be able to respond to
the absolute minimally required work e-mails. And review/follow-up to
your much appreciated patch series the last couple of days has also used
a lot of (unexpected not scheduled for) time. I'm not complaining, I'm
just saying I am really not able to contribute more to this effort right
now beyond my review, the offer of free hardware for a real cellular
network, and the extension of the test cases for GTP-U beyond the
already implemented very important IPv6 address allocation/assignment
which I believe your current code would not pass.
Regards,
Harald
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Tom Herbert <hidden> Date: 2017-09-21 16:43:05
On Thu, Sep 21, 2017 at 8:12 AM, Harald Welte [off-list ref] wrote:
Hi Tom,
On Wed, Sep 20, 2017 at 05:55:01PM -0700, Tom Herbert wrote:
quoted
You have the point of view of someone who has a lot of experience
dealing with this protocol. Try to imagine if you were some random
kernel network programmer with no experience in the area. If they
happen to find a one-off bug and want to do the right thing by running
a test, you want to make that as easy as possible.
Agreed. But we're not talking abut fixing a random bug in your patch
series, but we're talking about adding significant new features - and
those features need to be tested in real use caes, not just in an
artificial test setup that holds assumptions that are not true.
To improve performance, or to fix simple bugs that only affect the
processing of the GTP-U G-PDU, a much more limited and hence
"unrealistic" test scenario is probably sufficient/acceptable.
quoted
From that perspective, building protocol specific libraries and
finding the right cmd line to run is significant hoops (I can attest
to this).
I understand your argument. But then, there is actually quite some
tools to help you (see further below), as well as the wiki page at
http://osmocom.org/projects/linux-kernel-gtp-u/wiki/Basic_Testing
Of course, existing tools and existing wiki pages also only document
existing features of the kernel code :)
Yes, the documentation could be better. But then, how much more can you
expect from somebody who's doing this mostly for fun and who - despite
working in his dayjob on FOSS cellular projects - has no single
commercial project/context that uses the kernel GTP code.
In any case, working on a specific protocol or technology will require
that you understand that technology to some extent, including the
available tools. There's always a learning curve involved.
quoted
There are other examples in the kernel of systems bigger than GTP that
require a whole lot of effort just to run a simple test; you'll notice
for those it's rare that best developers ever bother to look at them
unless they're making a global change that affects the code. We don't
want GTP to take be like that!
I'm all for following your argument. My point is simply: You cannot
develop code solely based on mock-ups without any 'realistic' test
scenarios. Otherwise you will end up with something that works only in
your artificial lab setup, and follows all the best practises of the way
how the Linux kernel traditionally approaches tunneling implementations,
but it will never work/interop in the real world.
And I'm very strongly opposed to merging code where we have not been
able to show that it will inter-operate in at least one realistic
scenario. This would raise wrong expectations with users and all sorts
of downstream problems.
Harald,
Please see the cover letter for the original GTP kernel patches dated
May 10, 2016. My first question on those was "Is there a timeline for
adding IPv6 support?". To which Pablo replied that there was a
preliminary patch for it that has not been released. That was almost a
year and half ago and we have not heard anything since. If you don't
like my patches or don't think that can be adapted to fully support
the GTP specification, that's fine. But then you need to provide a
viable alternative. We are at the point where a kernel networking
feature that only supports IPv4 when it could support IPv6 must be
considered incomplete.
Thanks,
Tom
From: Tom Herbert <hidden> Date: 2017-09-21 22:41:04
On Wed, Sep 20, 2017 at 6:55 PM, Harald Welte [off-list ref] wrote:
Hi Tom,
On Wed, Sep 20, 2017 at 11:09:29AM -0700, Tom Herbert wrote:
quoted
On Mon, Sep 18, 2017 at 9:24 PM, David Miller [off-list ref] wrote:
quoted
From: Tom Herbert <redacted>
quoted
Add configuration to control use of zero checksums on transmit for both
IPv4 and IPv6, and control over accepting zero IPv6 checksums on
receive.
I thought we were trying to move away from this special case of allowing
zero UDP checksums with tunnels, especially for ipv6.
I don't have a strong preference either way. I like consistency with
VXLAN and foo/UDP, but I guess it's not required. Interestingly, since
GTP only carries IP, IPv6 zero checksums are actually safer here than
VXLAN or GRE/UDP.
Just for the record: I don't care either way and I defer to the kernel
networking developers to decide if they want to have zero UDP checksum
in GTP or not.
The 3GPP specs don't say anything about UDP checksums. So there's no
requirement to use them, and hence operation without UDP checksums
should be compliant. Cisco GTP implementation has udp checksumming
configurable, so other implementations also seem to provide both ways.
In general, I would argue one wants UDP checksumming of GTP in all
setups, as while the inner IP packet might be protected, the GTP header
itself is not, and that's what contains important data suhc as the TEID
(Tunnel Endpoint ID). But that's of course just my personal opinion,
and I'm not saying we should prevent people from using lower protection
if that's what they want.
The tradeoffs and requirements of zero UDP6 checksums are discussed at
length in RFC6935 and RFC6936. Given other implementations make it
configurable it should also be here.
Tom
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-24 01:40:14
Hi Andreas,
On Wed, Sep 20, 2017 at 05:37:52PM +0200, Andreas Schultz wrote:
I think we had this discussion before. The sending IP and port are not part
of the identity of the PDP context. So IMHO the sender is permitted
to change the source IP at random.
Thanks for the reminder: You are correct, at least in the uplink case
(MS->GGSN) where there is mobility of the MS. In the downlink case
(GGSN->MS), which is the "sending" part for the kernel GTP code used at
a GGSN, I'm not sure if that theory holds true in reality.
Do you agree that the current behavior of not using automatic source
address selection for encapsulated GTP packets but rather using the
source address of the socket is intended?
Do you further agree that the dst_cache support patch by Tom retains
that intended behavior and it should be merged?
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Harald Welte <laforge@gnumonks.org> Date: 2017-09-24 02:20:14
Hi Tom,
On Thu, Sep 21, 2017 at 09:43:02AM -0700, Tom Herbert wrote:
Please see the cover letter for the original GTP kernel patches dated
May 10, 2016. My first question on those was "Is there a timeline for
adding IPv6 support?". To which Pablo replied that there was a
preliminary patch for it that has not been released.
I'll suggest Pablo to comment on that. I don't recall the details at
that time, I was only involved in the earliest development of the module
and then handed over.
If you don't like my patches or don't think that can be adapted to
fully support the GTP specification, that's fine.
It's not about "not liking". I'm very happy about contributions,
including (of course) yours. It's about making sure that code we merge
into the kernel GTP driver will actually be usable to create a
standards-compliant GTP application or not.
There's no use in merging an IPv6 support patch if already by code
review it can be shown that it's impossible to create a spec-compliant
implementation using that patch. To me, that would be "merging IPv6
support so we can check off a box on a management form or marketing
sheet", but not for any practical value.
But then you need to provide a viable alternative.
Why do *I* have to provide a viable alternative? Who says that *I* have
an obligation to do so? A (co-)maintainer of a given driver doesn't
have the obligation of implementing any feature as requested.
Community based collaborative development only gets those things done
that people contribute. I have already contributed almost a decade of
my life to creating Free Software implementations of cellular protocol
stacks, and it continues to be the center of my work and spare time.
GTP is only one protocol layer on one of those stacks.
Pablo, Andreas and I have contributed a Linux kernel implementation that
currently only implements IPv4. This implementation can by anyone
extended to support IPv6, and as you see from this e-mail thread, there
is interest in helping this along by
* providing code review (even at times when it's personally difficult
for me)
* providing free hardware for setting up a "private cellular network"
to test interoperability
* providing testing tools for validation in absence of such a cellular
network
We are at the point where a kernel networking feature that only
supports IPv4 when it could support IPv6 must be considered
incomplete.
I agree it is incomplete. There's no doubt about that. But then,
even the current "incomplete" implementation is working and can be used
to operate an interoperable, spec-compatible IPv4 GGSN. So it serves a
practical purpose. All I'm asking is that any IPv6 support patches are
developed with that same practical purpose in mind.
Going through the cover letter of your series again:
- IPv6 support
Cannot be merged as-is, see lengthy review discussion
- Configurable networking interfaces so that GTP kernel can be
used and tested without needing GSN network emulation (i.e. no user
space daemon needed).
- Port numbers are configurable
As I indicated, I'm not fundamentally opposed to it, but I'm wondering
how much value they bring in reality. Andreas has raised the valid
concern that we're adding code that is not used in production setups or
by any of the userspace implementations using this tunneling module.
The code gets more complex and gets code paths that will not be
exercised/tested.
Nevertheless, if it helps you to work on GTP, we can merge them from my
point of view - unless Pablo and/or Andreas object more strongly.
- GSO,GRO
- A facility that allows application specific GSO callbacks
Fine with me, but I think you need to convince other folks about the
"application specific GSO" and the usage of the upper bits of
shinfo(skb)->gso_type.
- Control of zero UDP checksums
Same as above, Dave was raising some question about it, not sure if
his concern remains.
- Addition of a dst_cache in the GTP structure
Fine with me.
As for the patches touching gtp.c:
* 04/14 udp recv clean up:
fine with me, but kbuild robot complaint?
On a minor note, I think you're mixing two unrelated topics:
Separating the UDP receive functions and conversion to gro_cells,
which violates the "one patch per feature" rule. I'd still
merge it, but would prefer two separate patches
* 05/14 Remove special mtu handling
Pending your rework
* 06/14 Eliminate pktinfo and add port configuration
I don't like the combination of a non-functional "cosmetic"
refactoring of removing a data structure with the introduction
of a new feature. Makes it harder to review, impossible to
merge only one of the two. For the rationale of introducing the
gtp_pktinfo struct, see
http://git.osmocom.org/osmo-gtp-kernel/commit/?id=3bc7019c7afd06b5c7d94e5621728d092b82bb85
it was actually intended to make IPv6 support easier, but the
partial IPv6 support was removed before mainline submission.
* 07/14 Support encapsulation of IPv6 packets
Not acceptable in its current form, see extensive review
* 08/14 Support encpasulating over IPv6
No concerns in principle. Pending you making it dependent on AF
of socket
* 09/14 Allow configuring GTP interface as standalone
Can be merged unless strong objection from Pablo/Andreas (see above)
* 10/14 Add support for devnet
No concerns from my side
* 12/14 Configuration for zero UDP checksum
Up to Dave, he raised a question on it
* 13/14 Support for GRO
No concerns from my side
* 14/14 GSO support
No concerns from my side
BTW: Where have the iproute2/ip patches been posted, which you mention
in your cover page of the patch series?
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Tom Herbert <hidden> Date: 2017-09-24 15:55:52
It's not about "not liking". I'm very happy about contributions,
including (of course) yours. It's about making sure that code we merge
into the kernel GTP driver will actually be usable to create a
standards-compliant GTP application or not.
Harald,
Do you believe that these patches are not at all on the right track,
that they can't be built upon to get to a standards-compliant
implementation, and that we are going to have to throw all of this and
start from scratch to provide IPv6 support?
There's no use in merging an IPv6 support patch if already by code
review it can be shown that it's impossible to create a spec-compliant
implementation using that patch. To me, that would be "merging IPv6
support so we can check off a box on a management form or marketing
sheet", but not for any practical value.
To be clear, these patches are not done because to be a bullet point
on a marketing sheet. IPv6 is becoming _the_ Internet protocol. It
continues to exhibit exponential growth (~20% of Internet, per Google
stats), I believe least two of the largest datacenter operators are
running everything over IPv6, and there are already proposals to start
official deprecation of IPv4. In the mobile space IPv6 is going to be
a critical enabler of IoT and security in technologies like 5G. If we
want Linux to be at the forefront of the next technology wave then we
need to focus on IPv6 now! We should be far past the days of vendors
only providing IPv4 in the kernel support because "that's what our
customers use" and they'll get to IPv6 support at their leisure. IMO,
davem has every right to unilaterally NAK patches that only support
IPv4 or only test IPv4 with not even a path or timeline for IPv6
support.
Thanks,
Tom
From: Harald Welte <hidden> Date: 2017-09-24 16:26:10
Hi Tom,
On Sun, Sep 24, 2017 at 08:55:49AM -0700, Tom Herbert wrote:
Do you believe that these patches are not at all on the right track,
that they can't be built upon to get to a standards-compliant
implementation, and that we are going to have to throw all of this and
start from scratch to provide IPv6 support?
I believe I have pointed out where the problem areas are, several times
by now. I see no reason why things would have to be started from
scratch. However, the issues pointed out in the IPv6 support patch[es]
have to be resolved *before* any merge to mainline.
I don't mind merging "incomplete" code that doesn't cover all parts of a
spec but provides basic interoperability. I also am not arguing that
code must be bug-free at the time it is merged (which is impossible
anyway). But I am arguing that we cannot merge something that is
a wrong implementation as per the spec, and hence it must be brought
in-line with the spec before it can be merged.
quoted
There's no use in merging an IPv6 support patch if already by code
review it can be shown that it's impossible to create a spec-compliant
implementation using that patch. To me, that would be "merging IPv6
support so we can check off a box on a management form or marketing
sheet", but not for any practical value.
To be clear, these patches are not done because to be a bullet point
on a marketing sheet.
Great.
IPv6 is becoming _the_ Internet protocol.
I'm all aware of that, and I've been a very early adopter, since the
1990ies with 6bone.
My argument is not against IPv6 support. My argument is against merging
something that introdues IPv6 in a way that's not in-line with the GTP
protocol specifications, as such a way is of no use to anyone (except
marketing sheets).
We should be far past the days of vendors only providing IPv4 in the
kernel support because "that's what our customers use" and they'll get
to IPv6 support at their leisure.
I'm not sure where a "vendor" is involved with the GTP patches so far. I
think we have to draw a distinction between what you expect from
professional, corporate "vendors" with a commercial interest in mind
(such as supporting their hardware) and what you can expect from people
doing things in their spare time, out of enthusiasm to finally bring
some Free Software into the closed world of telecommunications.
The Telecom world should have implemented something like a GTP kernel
module a decade to 15 years ago. They could have saved significant
investments in proprietary hardware by running open source GGSNs with an
accelerated user plane in the kernel. Nobody seemed to have an interest
in that, until today - as you can see from Pablo and me working on this
in our spare time, whenever we have a couple of spare cycles next to
many other projects. You can see from the osmo-gtp-kernel commit log it
took years of being a ultra-low-priority on-and-off project to ever get
to a point where we thought it was worth submitting it mainline.
Andreas deserves the praise for finally pushing it ahead.
I'm looking forward to reviewing the next version of the patch series.
--
- Harald Welte [off-list ref] http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
From: Tom Herbert <hidden> Date: 2017-09-24 17:18:55
I'm not sure where a "vendor" is involved with the GTP patches so far. I
think we have to draw a distinction between what you expect from
professional, corporate "vendors" with a commercial interest in mind
(such as supporting their hardware) and what you can expect from people
doing things in their spare time, out of enthusiasm to finally bring
some Free Software into the closed world of telecommunications.
If it makes you feel any better I am not getting paid for this work either :-)
The Telecom world should have implemented something like a GTP kernel
module a decade to 15 years ago. They could have saved significant
investments in proprietary hardware by running open source GGSNs with an
accelerated user plane in the kernel. Nobody seemed to have an interest
in that, until today - as you can see from Pablo and me working on this
in our spare time, whenever we have a couple of spare cycles next to
many other projects. You can see from the osmo-gtp-kernel commit log it
took years of being a ultra-low-priority on-and-off project to ever get
to a point where we thought it was worth submitting it mainline.
Andreas deserves the praise for finally pushing it ahead.
I completely agree, and your work is well appreciated! But I don't
believe it is to late to steer the ship away from proprietary
solutions. In fact, given the direction of the rest of the industry
direction, now is our best opportunity to try. That is a major reason
for these patches. We need to bring GTP into the limelight and get a
lot more people thinking about. This might even be the world's most
important tunneling protocol. If nothing else, a discussion like this
is good if it inspires others in the community to start to look at it.
Tom