* [PATCH net-next v1 0/3] Flow Based GTP Tunneling
@ 2017-07-13 0:44 Jiannan Ouyang
[not found] ` <20170713004455.3946570-1-ouyangj-b10kYP2dOMg@public.gmane.org>
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Jiannan Ouyang @ 2017-07-13 0:44 UTC (permalink / raw)
To: osmocom-net-gprs, netdev, dev
Cc: pablo, laforge, pshelar, wieger.ijntema.tno, yi.y.yang, joe,
amarpadmanabhan, Jiannan Ouyang
This patch series augmented the existing GTP module to support flow
based GTP tunneling and modified the openvswitch datapath to support the
GTP vport type.
A flow based GTP net device enables that,
1) on the RX path, the outer (IP/UDP/GTP) header information could to be
stored in the metadata_dst struct, and embedded into the skb.
2) on the TX path, packets are encapsulated following instructions in
the metadata_dst field of the skb.
A flow based GTP net device can be integrated with Open vSwitch, which
allows SDN controllers to program GTP tunnels via Open vSwitch.
Open vSwitch changes are based on patch set
[PATCH] Add GTP vport based on upstream datapath
Example usage with OVS:
ovs-vsctl add-port br0 gtp-vport -- set interface gtp-vport \
ofport_request=2 type=gtp option:remote_ip=flow options:key=flow
ovs-ofctl add-flow br0
"in_port=2,tun_src=192.168.60.141,tun_id=123, \
actions=set_field:02:00:00:00:00:00->eth_src, \
set_field:ff:ff:ff:ff:ff:ff->eth_dst,LOCAL"
ovs-ofctl add-flow br0 \
"in_port=LOCAL,actions=set_tunnel:888, \
set_field:192.168.60.141->tun_dst,2"
arp -s 10.1.1.122 02:00:00:00:00:00
Jiannan Ouyang (3):
gtp: refactor to support flow-based gtp encap and decap
gtp: Support creating flow-based gtp net_device
openvswitch: Add GPRS Tunnel Protocol (GTP) vport support
drivers/net/gtp.c | 375 ++++++++++++++++++++++++++++++++-------
include/net/gtp.h | 8 +
include/uapi/linux/openvswitch.h | 1 +
net/openvswitch/Kconfig | 10 ++
net/openvswitch/Makefile | 1 +
net/openvswitch/vport-gtp.c | 144 +++++++++++++++
6 files changed, 475 insertions(+), 64 deletions(-)
create mode 100644 net/openvswitch/vport-gtp.c
--
2.9.3
^ permalink raw reply [flat|nested] 17+ messages in thread[parent not found: <20170713004455.3946570-1-ouyangj-b10kYP2dOMg@public.gmane.org>]
* [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap [not found] ` <20170713004455.3946570-1-ouyangj-b10kYP2dOMg@public.gmane.org> @ 2017-07-13 0:44 ` Jiannan Ouyang 2017-07-13 7:26 ` Harald Welte 2017-07-31 7:21 ` Andreas Schultz 2017-07-13 0:44 ` [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device Jiannan Ouyang 2017-07-13 1:28 ` [PATCH net-next v1 0/3] Flow Based GTP Tunneling Joe Stringer 2 siblings, 2 replies; 17+ messages in thread From: Jiannan Ouyang @ 2017-07-13 0:44 UTC (permalink / raw) To: osmocom-net-gprs-qjLDD68F18N4m7O/Vxda39i2O/JbrIOy, netdev-u79uwXL29TY76Z2rM5mHXA, dev-yBygre7rU0TnMu66kgdUjQ Cc: laforge-TgoAw6mPHtdg9hUCZPvPmw, pablo-Cap9r6Oaw4JrovVCs/uTlw If flow-based encap/decap is enabled, a separate code path is created for both packet RX and TX. PDP contexts are not used in flow-based mode since all metadata is maintained in metadata_dst: - for RX, pdp lookup and ms check are bypassed, while metadata_dst is constructed and attached to the skb. - for TX, pdp lookup is bypassed. Packets are encapsulated following instructions specified in metadata_dst. Signed-off-by: Jiannan Ouyang <ouyangj-b10kYP2dOMg@public•gmane.org> --- drivers/net/gtp.c | 162 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 102 insertions(+), 60 deletions(-) diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c index 1542e83..5a7b504 100644 --- a/drivers/net/gtp.c +++ b/drivers/net/gtp.c @@ -25,6 +25,7 @@ #include <linux/file.h> #include <linux/gtp.h> +#include <net/dst_metadata.h> #include <net/net_namespace.h> #include <net/protocol.h> #include <net/ip.h> @@ -36,6 +37,8 @@ #include <net/netns/generic.h> #include <net/gtp.h> +#define GTP_PDP_HASHSIZE 1024 + /* An active session for the subscriber. */ struct pdp_ctx { struct hlist_node hlist_tid; @@ -59,7 +62,7 @@ struct pdp_ctx { struct in_addr peer_addr_ip4; struct sock *sk; - struct net_device *dev; + struct net_device *dev; atomic_t tx_seq; struct rcu_head rcu_head; @@ -73,11 +76,15 @@ struct gtp_dev { struct sock *sk1u; struct net_device *dev; + struct net *net; unsigned int role; unsigned int hash_size; struct hlist_head *tid_hash; struct hlist_head *addr_hash; + + unsigned int collect_md; + struct ip_tunnel_info info; }; static unsigned int gtp_net_id __read_mostly; @@ -184,22 +191,23 @@ static bool gtp_check_ms(struct sk_buff *skb, struct pdp_ctx *pctx, return false; } -static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb, - unsigned int hdrlen, unsigned int role) +static int gtp_rx(struct gtp_dev *gtp, struct sk_buff *skb, + unsigned int hdrlen, struct sock *sk, + struct metadata_dst *tun_dst) { struct pcpu_sw_netstats *stats; - if (!gtp_check_ms(skb, pctx, hdrlen, role)) { - netdev_dbg(pctx->dev, "No PDP ctx for this MS\n"); - 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)))) + !net_eq(sock_net(sk), dev_net(gtp->dev)))) return -1; - netdev_dbg(pctx->dev, "forwarding packet from GGSN to uplink\n"); + netdev_dbg(gtp->dev, "forwarding packet from GGSN to uplink\n"); + + if (tun_dst) { + skb_dst_set(skb, (struct dst_entry *)tun_dst); + netdev_dbg(gtp->dev, "attaching metadata_dst to skb\n"); + } /* Now that the UDP and the GTP header have been removed, set up the * new network header. This is required by the upper layer to @@ -207,15 +215,16 @@ static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb, */ skb_reset_network_header(skb); - skb->dev = pctx->dev; + skb->dev = gtp->dev; - stats = this_cpu_ptr(pctx->dev->tstats); + stats = this_cpu_ptr(gtp->dev->tstats); u64_stats_update_begin(&stats->syncp); stats->rx_packets++; stats->rx_bytes += skb->len; u64_stats_update_end(&stats->syncp); netif_rx(skb); + return 0; } @@ -244,7 +253,12 @@ static int gtp0_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb) return 1; } - return gtp_rx(pctx, skb, hdrlen, gtp->role); + if (!gtp_check_ms(skb, pctx, hdrlen, gtp->role)) { + netdev_dbg(gtp->dev, "No PDP ctx for this MS\n"); + return 1; + } + + return gtp_rx(gtp, skb, hdrlen, pctx->sk, NULL); } static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb) @@ -253,6 +267,7 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb) sizeof(struct gtp1_header); struct gtp1_header *gtp1; struct pdp_ctx *pctx; + struct metadata_dst *tun_dst = NULL; if (!pskb_may_pull(skb, hdrlen)) return -1; @@ -280,13 +295,24 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb) gtp1 = (struct gtp1_header *)(skb->data + sizeof(struct udphdr)); - pctx = gtp1_pdp_find(gtp, ntohl(gtp1->tid)); - if (!pctx) { - netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb); - return 1; + if (ip_tunnel_collect_metadata() || gtp->collect_md) { + tun_dst = udp_tun_rx_dst(skb, gtp->sk1u->sk_family, TUNNEL_KEY, + key32_to_tunnel_id(gtp1->tid), 0); + } else { + pctx = gtp1_pdp_find(gtp, ntohl(gtp1->tid)); + if (!pctx) { + netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", + skb); + return 1; + } + + if (!gtp_check_ms(skb, pctx, hdrlen, gtp->role)) { + netdev_dbg(gtp->dev, "No PDP ctx for this MS\n"); + return 1; + } } - return gtp_rx(pctx, skb, hdrlen, gtp->role); + return gtp_rx(gtp, skb, hdrlen, gtp->sk1u, tun_dst); } static void gtp_encap_destroy(struct sock *sk) @@ -410,7 +436,7 @@ static inline void gtp0_push_header(struct sk_buff *skb, struct pdp_ctx *pctx) gtp0->tid = cpu_to_be64(pctx->u.v0.tid); } -static inline void gtp1_push_header(struct sk_buff *skb, struct pdp_ctx *pctx) +static inline void gtp1_push_header(struct sk_buff *skb, __be32 tid) { int payload_len = skb->len; struct gtp1_header *gtp1; @@ -426,7 +452,7 @@ static inline void gtp1_push_header(struct sk_buff *skb, struct pdp_ctx *pctx) gtp1->flags = 0x30; /* v1, GTP-non-prime. */ gtp1->type = GTP_TPDU; gtp1->length = htons(payload_len); - gtp1->tid = htonl(pctx->u.v1.o_tei); + gtp1->tid = tid; /* TODO: Suppport for extension header, sequence number and N-PDU. * Update the length field if any of them is available. @@ -438,34 +464,17 @@ struct gtp_pktinfo { struct iphdr *iph; struct flowi4 fl4; struct rtable *rt; - struct pdp_ctx *pctx; struct net_device *dev; __be16 gtph_port; }; -static void gtp_push_header(struct sk_buff *skb, struct gtp_pktinfo *pktinfo) -{ - switch (pktinfo->pctx->gtp_version) { - case GTP_V0: - pktinfo->gtph_port = htons(GTP0_PORT); - gtp0_push_header(skb, pktinfo->pctx); - break; - case GTP_V1: - pktinfo->gtph_port = htons(GTP1U_PORT); - gtp1_push_header(skb, pktinfo->pctx); - break; - } -} - static inline void gtp_set_pktinfo_ipv4(struct gtp_pktinfo *pktinfo, struct sock *sk, struct iphdr *iph, - struct pdp_ctx *pctx, struct rtable *rt, - struct flowi4 *fl4, + struct rtable *rt, struct flowi4 *fl4, struct net_device *dev) { pktinfo->sk = sk; pktinfo->iph = iph; - pktinfo->pctx = pctx; pktinfo->rt = rt; pktinfo->fl4 = *fl4; pktinfo->dev = dev; @@ -479,36 +488,58 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, struct rtable *rt; struct flowi4 fl4; struct iphdr *iph; + struct ip_tunnel_info *info = NULL; + struct sock *sk = NULL; __be16 df; + __be32 tun_id; + __be32 daddr; + u8 gtp_version; int mtu; - /* Read the IP destination address and resolve the PDP context. - * Prepend PDP header with TEI/TID from PDP ctx. - */ iph = ip_hdr(skb); - if (gtp->role == GTP_ROLE_SGSN) - pctx = ipv4_pdp_find(gtp, iph->saddr); - else - pctx = ipv4_pdp_find(gtp, iph->daddr); - - if (!pctx) { - netdev_dbg(dev, "no PDP ctx found for %pI4, skip\n", - &iph->daddr); - return -ENOENT; + + // flow-based GTP1U encap + info = skb_tunnel_info(skb); + if (gtp->collect_md && info && ntohs(info->key.tp_dst) == GTP1U_PORT) { + pctx = NULL; + gtp_version = GTP_V1; + tun_id = tunnel_id_to_key32(info->key.tun_id); + daddr = info->key.u.ipv4.dst; + sk = gtp->sk1u; + + netdev_dbg(dev, "flow-based GTP1U encap: tunnel id %d\n", + be32_to_cpu(tun_id)); + } else { + /* Read the IP destination address and resolve the PDP context. + * Prepend PDP header with TEI/TID from PDP ctx. + */ + 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); + return -ENOENT; + } + netdev_dbg(dev, "found PDP context %p\n", pctx); + + gtp_version = pctx->gtp_version; + tun_id = htonl(pctx->u.v1.o_tei); + daddr = pctx->peer_addr_ip4.s_addr; + sk = pctx->sk; } - netdev_dbg(dev, "found PDP context %p\n", pctx); - rt = ip4_route_output_gtp(&fl4, pctx->sk, pctx->peer_addr_ip4.s_addr); + rt = ip4_route_output_gtp(&fl4, sk, daddr); if (IS_ERR(rt)) { - netdev_dbg(dev, "no route to SSGN %pI4\n", - &pctx->peer_addr_ip4.s_addr); + netdev_dbg(dev, "no route to SSGN %pI4\n", &daddr); dev->stats.tx_carrier_errors++; goto err; } if (rt->dst.dev == dev) { - netdev_dbg(dev, "circular route to SSGN %pI4\n", - &pctx->peer_addr_ip4.s_addr); + netdev_dbg(dev, "circular route to SSGN %pI4\n", &daddr); dev->stats.collisions++; goto err_rt; } @@ -520,7 +551,7 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, if (df) { mtu = dst_mtu(&rt->dst) - dev->hard_header_len - sizeof(struct iphdr) - sizeof(struct udphdr); - switch (pctx->gtp_version) { + switch (gtp_version) { case GTP_V0: mtu -= sizeof(struct gtp0_header); break; @@ -543,8 +574,18 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, goto err_rt; } - gtp_set_pktinfo_ipv4(pktinfo, pctx->sk, iph, pctx, rt, &fl4, dev); - gtp_push_header(skb, pktinfo); + gtp_set_pktinfo_ipv4(pktinfo, sk, iph, rt, &fl4, dev); + + switch (gtp_version) { + case GTP_V0: + pktinfo->gtph_port = htons(GTP0_PORT); + gtp0_push_header(skb, pctx); + break; + case GTP_V1: + pktinfo->gtph_port = htons(GTP1U_PORT); + gtp1_push_header(skb, tun_id); + break; + } return 0; err_rt: @@ -647,13 +688,14 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, return -EINVAL; gtp = netdev_priv(dev); + gtp->net = src_net; err = gtp_encap_enable(gtp, data); if (err < 0) return err; if (!data[IFLA_GTP_PDP_HASHSIZE]) - hashsize = 1024; + hashsize = GTP_PDP_HASHSIZE; else hashsize = nla_get_u32(data[IFLA_GTP_PDP_HASHSIZE]); -- 2.9.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap 2017-07-13 0:44 ` [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap Jiannan Ouyang @ 2017-07-13 7:26 ` Harald Welte 2017-07-14 0:55 ` Jiannan Ouyang 2017-07-31 7:21 ` Andreas Schultz 1 sibling, 1 reply; 17+ messages in thread From: Harald Welte @ 2017-07-13 7:26 UTC (permalink / raw) To: Jiannan Ouyang Cc: osmocom-net-gprs, netdev, dev, pablo, pshelar, wieger.ijntema.tno, yi.y.yang, joe, amarpadmanabhan Hi Jiannan, On Wed, Jul 12, 2017 at 05:44:53PM -0700, Jiannan Ouyang wrote: > +#define GTP_PDP_HASHSIZE 1024 please don't mix cosmetic cleanups with actual functional code changes. > - struct net_device *dev; > + struct net_device *dev; please don't mix cosmetic cleanups with actual functional code changes. > -static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb, > - unsigned int hdrlen, unsigned int role) > +static int gtp_rx(struct gtp_dev *gtp, struct sk_buff *skb, > + unsigned int hdrlen, struct sock *sk, > + struct metadata_dst *tun_dst) > { > struct pcpu_sw_netstats *stats; > > - if (!gtp_check_ms(skb, pctx, hdrlen, role)) { > - netdev_dbg(pctx->dev, "No PDP ctx for this MS\n"); > - 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)))) > + !net_eq(sock_net(sk), dev_net(gtp->dev)))) > return -1; > > - netdev_dbg(pctx->dev, "forwarding packet from GGSN to uplink\n"); > + netdev_dbg(gtp->dev, "forwarding packet from GGSN to uplink\n"); > + > + if (tun_dst) { > + skb_dst_set(skb, (struct dst_entry *)tun_dst); > + netdev_dbg(gtp->dev, "attaching metadata_dst to skb\n"); > + } > > /* Now that the UDP and the GTP header have been removed, set up the > * new network header. This is required by the upper layer to > @@ -207,15 +215,16 @@ static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb, > */ > skb_reset_network_header(skb); > > - skb->dev = pctx->dev; > + skb->dev = gtp->dev; > > - stats = this_cpu_ptr(pctx->dev->tstats); > + stats = this_cpu_ptr(gtp->dev->tstats); > u64_stats_update_begin(&stats->syncp); > stats->rx_packets++; > stats->rx_bytes += skb->len; > u64_stats_update_end(&stats->syncp); > > netif_rx(skb); > + > return 0; > } > > @@ -244,7 +253,12 @@ static int gtp0_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb) > return 1; > } > > - return gtp_rx(pctx, skb, hdrlen, gtp->role); > + if (!gtp_check_ms(skb, pctx, hdrlen, gtp->role)) { > + netdev_dbg(gtp->dev, "No PDP ctx for this MS\n"); > + return 1; > + } > + > + return gtp_rx(gtp, skb, hdrlen, pctx->sk, NULL); > } > > static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb) > @@ -253,6 +267,7 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb) > sizeof(struct gtp1_header); > struct gtp1_header *gtp1; > struct pdp_ctx *pctx; > + struct metadata_dst *tun_dst = NULL; > > if (!pskb_may_pull(skb, hdrlen)) > return -1; > @@ -280,13 +295,24 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb) > > gtp1 = (struct gtp1_header *)(skb->data + sizeof(struct udphdr)); > > - pctx = gtp1_pdp_find(gtp, ntohl(gtp1->tid)); > - if (!pctx) { > - netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb); > - return 1; > + if (ip_tunnel_collect_metadata() || gtp->collect_md) { > + tun_dst = udp_tun_rx_dst(skb, gtp->sk1u->sk_family, TUNNEL_KEY, > + key32_to_tunnel_id(gtp1->tid), 0); > + } else { > + pctx = gtp1_pdp_find(gtp, ntohl(gtp1->tid)); > + if (!pctx) { > + netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", > + skb); > + return 1; > + } > + > + if (!gtp_check_ms(skb, pctx, hdrlen, gtp->role)) { > + netdev_dbg(gtp->dev, "No PDP ctx for this MS\n"); > + return 1; > + } > } > > - return gtp_rx(pctx, skb, hdrlen, gtp->role); > + return gtp_rx(gtp, skb, hdrlen, gtp->sk1u, tun_dst); > } > > static void gtp_encap_destroy(struct sock *sk) > @@ -410,7 +436,7 @@ static inline void gtp0_push_header(struct sk_buff *skb, struct pdp_ctx *pctx) > gtp0->tid = cpu_to_be64(pctx->u.v0.tid); > } > > -static inline void gtp1_push_header(struct sk_buff *skb, struct pdp_ctx *pctx) > +static inline void gtp1_push_header(struct sk_buff *skb, __be32 tid) > { > int payload_len = skb->len; > struct gtp1_header *gtp1; > @@ -426,7 +452,7 @@ static inline void gtp1_push_header(struct sk_buff *skb, struct pdp_ctx *pctx) > gtp1->flags = 0x30; /* v1, GTP-non-prime. */ > gtp1->type = GTP_TPDU; > gtp1->length = htons(payload_len); > - gtp1->tid = htonl(pctx->u.v1.o_tei); > + gtp1->tid = tid; > > /* TODO: Suppport for extension header, sequence number and N-PDU. > * Update the length field if any of them is available. > @@ -438,34 +464,17 @@ struct gtp_pktinfo { > struct iphdr *iph; > struct flowi4 fl4; > struct rtable *rt; > - struct pdp_ctx *pctx; > struct net_device *dev; > __be16 gtph_port; > }; > > -static void gtp_push_header(struct sk_buff *skb, struct gtp_pktinfo *pktinfo) > -{ > - switch (pktinfo->pctx->gtp_version) { > - case GTP_V0: > - pktinfo->gtph_port = htons(GTP0_PORT); > - gtp0_push_header(skb, pktinfo->pctx); > - break; > - case GTP_V1: > - pktinfo->gtph_port = htons(GTP1U_PORT); > - gtp1_push_header(skb, pktinfo->pctx); > - break; > - } > -} > - > static inline void gtp_set_pktinfo_ipv4(struct gtp_pktinfo *pktinfo, > struct sock *sk, struct iphdr *iph, > - struct pdp_ctx *pctx, struct rtable *rt, > - struct flowi4 *fl4, > + struct rtable *rt, struct flowi4 *fl4, > struct net_device *dev) > { > [...] > + __be32 tun_id; you are breaking GTPv0 functionality here. GTPv0 has 64 bit tunnel identifiers, and this function is called both from GTPv1 and GTPv0 context. This makes me wonder how you did verify that your changes do not break the existing operation with both GTPv0 and GTPv1? > + // flow-based GTP1U encap > + info = skb_tunnel_info(skb); > + if (gtp->collect_md && info && ntohs(info->key.tp_dst) == GTP1U_PORT) { I think it's typically safe to assume that GTP is only operated on standard ports, but it is something you chould/should think about, i.e. whether you want that kind of restriction. In the existing use case, we have the v0/v1 information stored in the per-pdp context structure. > + tun_id = htonl(pctx->u.v1.o_tei); here is where you're assuming GTPv1 in two ways from code that is called from both v0 and v1. * you're dereferencing a v1 specific element in the pctx union * you're storing the result in a 32bit variable > gtp = netdev_priv(dev); > + gtp->net = src_net; Isn't this a generic change that's independent of your work on OVS GTP? Regards, Harald -- - Harald Welte <laforge@gnumonks•org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap 2017-07-13 7:26 ` Harald Welte @ 2017-07-14 0:55 ` Jiannan Ouyang [not found] ` <3729E0DA-08AB-4C5C-B9EC-C76DAAA60E10-b10kYP2dOMg@public.gmane.org> 0 siblings, 1 reply; 17+ messages in thread From: Jiannan Ouyang @ 2017-07-14 0:55 UTC (permalink / raw) To: Harald Welte Cc: dev-yBygre7rU0TnMu66kgdUjQ@public•gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public•gmane.org, osmocom-net-gprs-qjLDD68F18N4m7O/Vxda39i2O/JbrIOy@public•gmane.org, pablo-Cap9r6Oaw4JrovVCs/uTlw@public•gmane.org Hi Harald, > On 7/13/17, 12:26 AM, "Harald Welte" <laforge@gnumonks•org> wrote: >· > > static inline void gtp_set_pktinfo_ipv4(struct gtp_pktinfo *pktinfo, > > struct sock *sk, struct iphdr *iph, > > - struct pdp_ctx *pctx, struct rtable *rt, > > - struct flowi4 *fl4, > > + struct rtable *rt, struct flowi4 *fl4, > > struct net_device *dev) > > { > > [...] > > + __be32 tun_id; >· > you are breaking GTPv0 functionality here. GTPv0 has 64 bit tunnel > identifiers, and this function is called both from GTPv1 and GTPv0 > context. >· > This makes me wonder how you did verify that your changes do not break > the existing operation with both GTPv0 and GTPv1? >· Good catch. I only fully tested the GTPv1 path against oai-cn. Will fix this and test the GTPv0 path as well. I had doubts on how this flow-based GTPv1 code path should fit in, which is why the GTPv0 and the GTPv1 code pieces are mixed in my changes. Should I explicitly claim that the flow-based change is for GTPv1 only? > > + // flow-based GTP1U encap > > + info = skb_tunnel_info(skb); > > + if (gtp->collect_md && info && ntohs(info->key.tp_dst) == GTP1U_PORT) { >· > I think it's typically safe to assume that GTP is only operated on > standard ports, but it is something you chould/should think about, i.e. > whether you want that kind of restriction. In the existing use case, we > have the v0/v1 information stored in the per-pdp context structure. >· The reason I’m checking GTP1U_PORT here is to filter GTP1U traffic. It possible to pass a port number from ovs into the gtp module. I will investigate how to support programmable port. > > + tun_id = htonl(pctx->u.v1.o_tei); >· > here is where you're assuming GTPv1 in two ways from code that is called > from both v0 and v1. > * you're dereferencing a v1 specific element in the pctx union > * you're storing the result in a 32bit variable >· Right, will fix this for GTPv0. > > gtp = netdev_priv(dev); > > + gtp->net = src_net; >· > Isn't this a generic change that's independent of your work on OVS GTP? It is meant to be OVS independent. What makes it not? Should I leave this field un-initialized? Thanks -Jiannan _______________________________________________ dev mailing list dev@openvswitch•org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <3729E0DA-08AB-4C5C-B9EC-C76DAAA60E10-b10kYP2dOMg@public.gmane.org>]
* Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap [not found] ` <3729E0DA-08AB-4C5C-B9EC-C76DAAA60E10-b10kYP2dOMg@public.gmane.org> @ 2017-07-14 8:03 ` Harald Welte 0 siblings, 0 replies; 17+ messages in thread From: Harald Welte @ 2017-07-14 8:03 UTC (permalink / raw) To: Jiannan Ouyang Cc: dev-yBygre7rU0TnMu66kgdUjQ@public•gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public•gmane.org, osmocom-net-gprs-qjLDD68F18N4m7O/Vxda39i2O/JbrIOy@public•gmane.org, pablo-Cap9r6Oaw4JrovVCs/uTlw@public•gmane.org Hi Jiannan, > > > gtp = netdev_priv(dev); > > > + gtp->net = src_net; > >· > > Isn't this a generic change that's independent of your work on OVS GTP? > > It is meant to be OVS independent. What makes it not? Should I leave > this field un-initialized? In general, in all FOSS projects I have worked (and particularly the Linux kernel), it is a strict rule that any given patch adresses only one logical change. So if your change is for flow-based "OVS" support in the GTP code, and the "gtp->net = src_net" is a generic change (and not something specifically required by flow/OVS) then it should be a separate patch. Similarly to the cosmetic changes which should be a separate patch. -- - Harald Welte <laforge-TgoAw6mPHtdg9hUCZPvPmw@public•gmane.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap 2017-07-13 0:44 ` [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap Jiannan Ouyang 2017-07-13 7:26 ` Harald Welte @ 2017-07-31 7:21 ` Andreas Schultz 2017-08-02 12:52 ` Pablo Neira Ayuso 1 sibling, 1 reply; 17+ messages in thread From: Andreas Schultz @ 2017-07-31 7:21 UTC (permalink / raw) To: Jiannan Ouyang Cc: osmocom-net-gprs, netdev, dev, pablo, laforge, pshelar, wieger ijntema tno, yi y yang, joe, Amar Padmanabhan Hi Jiannan, ----- On Jul 13, 2017, at 2:44 AM, Jiannan Ouyang ouyangj@fb•com wrote: [...] > -static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb, > - unsigned int hdrlen, unsigned int role) > +static int gtp_rx(struct gtp_dev *gtp, struct sk_buff *skb, > + unsigned int hdrlen, struct sock *sk, > + struct metadata_dst *tun_dst) Some time ago, there was an extensive discussion about the relation of PDP context and network devices. You are basically reverting one of the changes that was made in that context. I think it is wrong to couple GTP devices and PDP context the way you do here (there are people that disagree, though). The GTP network device of one of two structures owning the PDP context, the other is the GTP socket. GTP network devices and GTP sockets should be strictly separated. The GTP network device owns the IP given to the MS, handles mapping IP's into GTP tunnels (peer GSN + TEIDs) and hands the resulting GTP packets of to the GTP socket for sending. The GTP socket decaps the GTP packet, find the right context and based on information therein passes it to the GTP network device. By separating is that way, you can have MS with overlapping or colliding IP's on the same GTP socket as long as they belong to different GTP network devices. We had a length discussion about whether the above scenario makes sense. I'm not sure if we reached a final verdict, but the 3GPP specifications clearly permit such a setup. Regards Andreas ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap 2017-07-31 7:21 ` Andreas Schultz @ 2017-08-02 12:52 ` Pablo Neira Ayuso 0 siblings, 0 replies; 17+ messages in thread From: Pablo Neira Ayuso @ 2017-08-02 12:52 UTC (permalink / raw) To: Andreas Schultz, g Cc: Jiannan Ouyang, osmocom-net-gprs, netdev, dev, laforge, pshelar, wieger ijntema tno, yi y yang, joe, Amar Padmanabhan On Mon, Jul 31, 2017 at 09:21:36AM +0200, Andreas Schultz wrote: > Hi Jiannan, > > ----- On Jul 13, 2017, at 2:44 AM, Jiannan Ouyang ouyangj@fb•com wrote: > > [...] > > > -static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb, > > - unsigned int hdrlen, unsigned int role) > > +static int gtp_rx(struct gtp_dev *gtp, struct sk_buff *skb, > > + unsigned int hdrlen, struct sock *sk, > > + struct metadata_dst *tun_dst) > > Some time ago, there was an extensive discussion about the relation > of PDP context and network devices. You are basically reverting one > of the changes that was made in that context. I think it is wrong to > couple GTP devices and PDP context the way you do here (there are > people that disagree, though). > > The GTP network device of one of two structures owning the PDP context, > the other is the GTP socket. GTP network devices and GTP sockets should > be strictly separated. > > The GTP network device owns the IP given to the MS, handles mapping > IP's into GTP tunnels (peer GSN + TEIDs) and hands the resulting GTP > packets of to the GTP socket for sending. The GTP socket decaps the GTP > packet, find the right context and based on information therein passes > it to the GTP network device. > > By separating is that way, you can have MS with overlapping or colliding > IP's on the same GTP socket as long as they belong to different GTP network > devices. > > We had a length discussion about whether the above scenario makes sense. > I'm not sure if we reached a final verdict, but the 3GPP specifications > clearly permit such a setup. We need a netlink interface to retrieve GTP information accordingly, this includes a top-level APN object to represent what you need. That would allow to accomodate all usecases. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device [not found] ` <20170713004455.3946570-1-ouyangj-b10kYP2dOMg@public.gmane.org> 2017-07-13 0:44 ` [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap Jiannan Ouyang @ 2017-07-13 0:44 ` Jiannan Ouyang [not found] ` <20170713004455.3946570-3-ouyangj-b10kYP2dOMg@public.gmane.org> 2017-07-13 18:01 ` Joe Stringer 2017-07-13 1:28 ` [PATCH net-next v1 0/3] Flow Based GTP Tunneling Joe Stringer 2 siblings, 2 replies; 17+ messages in thread From: Jiannan Ouyang @ 2017-07-13 0:44 UTC (permalink / raw) To: osmocom-net-gprs-qjLDD68F18N4m7O/Vxda39i2O/JbrIOy, netdev-u79uwXL29TY76Z2rM5mHXA, dev-yBygre7rU0TnMu66kgdUjQ Cc: laforge-TgoAw6mPHtdg9hUCZPvPmw, pablo-Cap9r6Oaw4JrovVCs/uTlw Add the gtp_create_flow_based_dev() interface to create flow-based gtp net_device, which sets gtp->collect_md. Under flow-based mode, UDP sockets are created and maintained in kernel. Signed-off-by: Jiannan Ouyang <ouyangj-b10kYP2dOMg@public•gmane.org> --- drivers/net/gtp.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++- include/net/gtp.h | 8 ++ 2 files changed, 217 insertions(+), 4 deletions(-) diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c index 5a7b504..09712c9 100644 --- a/drivers/net/gtp.c +++ b/drivers/net/gtp.c @@ -642,9 +642,94 @@ static netdev_tx_t gtp_dev_xmit(struct sk_buff *skb, struct net_device *dev) return NETDEV_TX_OK; } +static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize); +static void gtp_hashtable_free(struct gtp_dev *gtp); +static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[]); + +static int gtp_change_mtu(struct net_device *dev, int new_mtu, bool strict) +{ + int max_mtu = IP_MAX_MTU - dev->hard_header_len - sizeof(struct iphdr) + - sizeof(struct udphdr) - sizeof(struct gtp1_header); + + if (new_mtu < ETH_MIN_MTU) + return -EINVAL; + + if (new_mtu > max_mtu) { + if (strict) + return -EINVAL; + + new_mtu = max_mtu; + } + + dev->mtu = new_mtu; + return 0; +} + +static int gtp_dev_open(struct net_device *dev) +{ + struct gtp_dev *gtp = netdev_priv(dev); + struct net *net = gtp->net; + struct socket *sock1u; + struct socket *sock0; + struct udp_tunnel_sock_cfg tunnel_cfg; + struct udp_port_cfg udp_conf; + int err; + + memset(&udp_conf, 0, sizeof(udp_conf)); + + udp_conf.family = AF_INET; + udp_conf.local_ip.s_addr = htonl(INADDR_ANY); + udp_conf.local_udp_port = htons(GTP1U_PORT); + + err = udp_sock_create(gtp->net, &udp_conf, &sock1u); + if (err < 0) + return err; + + udp_conf.local_udp_port = htons(GTP0_PORT); + err = udp_sock_create(gtp->net, &udp_conf, &sock0); + if (err < 0) + return err; + + memset(&tunnel_cfg, 0, sizeof(tunnel_cfg)); + tunnel_cfg.sk_user_data = gtp; + tunnel_cfg.encap_rcv = gtp_encap_recv; + tunnel_cfg.encap_destroy = gtp_encap_destroy; + tunnel_cfg.encap_type = UDP_ENCAP_GTP0; + setup_udp_tunnel_sock(net, sock0, &tunnel_cfg); + + tunnel_cfg.encap_type = UDP_ENCAP_GTP1U; + + setup_udp_tunnel_sock(net, sock1u, &tunnel_cfg); + + sock_hold(sock0->sk); + sock_hold(sock1u->sk); + + gtp->sk0 = sock0->sk; + gtp->sk1u = sock1u->sk; + + return 0; +} + +static int gtp_dev_stop(struct net_device *dev) +{ + struct gtp_dev *gtp = netdev_priv(dev); + struct sock *sk = gtp->sk1u; + + udp_tunnel_sock_release(gtp->sk0->sk_socket); + udp_tunnel_sock_release(gtp->sk1u->sk_socket); + + udp_sk(sk)->encap_type = 0; + rcu_assign_sk_user_data(sk, NULL); + sock_put(sk); + + return 0; +} + static const struct net_device_ops gtp_netdev_ops = { .ndo_init = gtp_dev_init, .ndo_uninit = gtp_dev_uninit, + .ndo_open = gtp_dev_open, + .ndo_stop = gtp_dev_stop, .ndo_start_xmit = gtp_dev_xmit, .ndo_get_stats64 = ip_tunnel_get_stats64, }; @@ -672,10 +757,6 @@ static void gtp_link_setup(struct net_device *dev) sizeof(struct gtp0_header); } -static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize); -static void gtp_hashtable_free(struct gtp_dev *gtp); -static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[]); - static int gtp_newlink(struct net *src_net, struct net_device *dev, struct nlattr *tb[], struct nlattr *data[], struct netlink_ext_ack *extack) @@ -780,6 +861,130 @@ static struct rtnl_link_ops gtp_link_ops __read_mostly = { .fill_info = gtp_fill_info, }; +static void init_tnl_info(struct ip_tunnel_info *info, __u16 dst_port) +{ + memset(info, 0, sizeof(*info)); + info->key.tp_dst = htons(dst_port); +} + +static struct gtp_dev *gtp_find_flow_based_dev( + struct net *net) +{ + struct gtp_net *gn = net_generic(net, gtp_net_id); + struct gtp_dev *gtp, *t = NULL; + + list_for_each_entry(gtp, &gn->gtp_dev_list, list) { + if (gtp->collect_md) + t = gtp; + } + + return t; +} + +static int gtp_configure(struct net *net, struct net_device *dev, + const struct ip_tunnel_info *info) +{ + struct gtp_net *gn = net_generic(net, gtp_net_id); + struct gtp_dev *gtp = netdev_priv(dev); + int err; + + gtp->net = net; + gtp->dev = dev; + + if (gtp_find_flow_based_dev(net)) + return -EBUSY; + + dev->netdev_ops = >p_netdev_ops; + dev->priv_destructor = free_netdev; + + dev->hard_header_len = 0; + dev->addr_len = 0; + + /* Zero header length. */ + dev->type = ARPHRD_NONE; + dev->flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST; + + dev->priv_flags |= IFF_NO_QUEUE; + dev->features |= NETIF_F_LLTX; + netif_keep_dst(dev); + + /* Assume largest header, ie. GTPv0. */ + dev->needed_headroom = LL_MAX_HEADER + + sizeof(struct iphdr) + + sizeof(struct udphdr) + + sizeof(struct gtp0_header); + + dst_cache_reset(>p->info.dst_cache); + gtp->info = *info; + gtp->collect_md = true; + + err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free?? + if (err < 0) { + pr_err("Error gtp_hashtable_new"); + return err; + } + + err = register_netdevice(dev); + if (err) { + pr_err("Error when registering net device"); + return err; + } + + list_add_rcu(>p->list, &gn->gtp_dev_list); + return 0; +} + +struct net_device *gtp_create_flow_based_dev(struct net *net, + const char *name, + u8 name_assign_type, + u16 dst_port) +{ + struct nlattr *tb[IFLA_MAX + 1]; + struct net_device *dev; + struct ip_tunnel_info info; + LIST_HEAD(list_kill); + int err; + + memset(&tb, 0, sizeof(tb)); + dev = rtnl_create_link(net, name, name_assign_type, + >p_link_ops, tb); + if (IS_ERR(dev)) { + pr_err("error rtnl_create_link"); + return dev; + } + + init_tnl_info(&info, dst_port); + err = gtp_configure(net, dev, &info); + if (err < 0) { + pr_err("error gtp_configure"); + free_netdev(dev); + return ERR_PTR(err); + } + + /* openvswitch users expect packet sizes to be unrestricted, + * so set the largest MTU we can. + */ + err = gtp_change_mtu(dev, IP_MAX_MTU, false); + if (err) { + pr_err("error gtp_change_mtu"); + goto err; + } + + err = rtnl_configure_link(dev, NULL); + if (err < 0) { + pr_err("error rtnl_configure_link"); + goto err; + } + + return dev; + +err: + gtp_dellink(dev, &list_kill); + unregister_netdevice_many(&list_kill); + return ERR_PTR(err); +} +EXPORT_SYMBOL_GPL(gtp_create_flow_based_dev); + static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize) { int i; diff --git a/include/net/gtp.h b/include/net/gtp.h index 6398891..a76b26c 100644 --- a/include/net/gtp.h +++ b/include/net/gtp.h @@ -31,4 +31,12 @@ struct gtp1_header { /* According to 3GPP TS 29.060. */ #define GTP1_F_EXTHDR 0x04 #define GTP1_F_MASK 0x07 +#ifdef CONFIG_INET + +struct net_device *gtp_create_flow_based_dev( + struct net *net, const char *name, + u8 name_assign_type, u16 dst_port); + +#endif /*ifdef CONFIG_INET */ + #endif -- 2.9.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
[parent not found: <20170713004455.3946570-3-ouyangj-b10kYP2dOMg@public.gmane.org>]
* Re: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device [not found] ` <20170713004455.3946570-3-ouyangj-b10kYP2dOMg@public.gmane.org> @ 2017-07-13 7:35 ` Harald Welte 2017-07-14 1:01 ` Jiannan Ouyang 0 siblings, 1 reply; 17+ messages in thread From: Harald Welte @ 2017-07-13 7:35 UTC (permalink / raw) To: Jiannan Ouyang Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA, osmocom-net-gprs-qjLDD68F18N4m7O/Vxda39i2O/JbrIOy, pablo-Cap9r6Oaw4JrovVCs/uTlw Hi Jiannan, please keep in mind I have zero clue about OVS, so I cannot really comment on what is customary in that context and what not. However, some general review from the GTP point of view below: On Wed, Jul 12, 2017 at 05:44:54PM -0700, Jiannan Ouyang wrote: > Add the gtp_create_flow_based_dev() interface to create flow-based gtp > net_device, which sets gtp->collect_md. Under flow-based mode, UDP sockets are > created and maintained in kernel. > > Signed-off-by: Jiannan Ouyang <ouyangj-b10kYP2dOMg@public•gmane.org> > --- > drivers/net/gtp.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++- > include/net/gtp.h | 8 ++ > 2 files changed, 217 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c > index 5a7b504..09712c9 100644 > --- a/drivers/net/gtp.c > +++ b/drivers/net/gtp.c > @@ -642,9 +642,94 @@ static netdev_tx_t gtp_dev_xmit(struct sk_buff *skb, struct net_device *dev) > return NETDEV_TX_OK; > } > > +static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize); > +static void gtp_hashtable_free(struct gtp_dev *gtp); > +static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[]); > + > +static int gtp_change_mtu(struct net_device *dev, int new_mtu, bool strict) > +{ > + int max_mtu = IP_MAX_MTU - dev->hard_header_len - sizeof(struct iphdr) > + - sizeof(struct udphdr) - sizeof(struct gtp1_header); > + > + if (new_mtu < ETH_MIN_MTU) > + return -EINVAL; > + > + if (new_mtu > max_mtu) { > + if (strict) > + return -EINVAL; > + > + new_mtu = max_mtu; > + } > + > + dev->mtu = new_mtu; > + return 0; > +} > + > +static int gtp_dev_open(struct net_device *dev) > +{ > + struct gtp_dev *gtp = netdev_priv(dev); > + struct net *net = gtp->net; > + struct socket *sock1u; > + struct socket *sock0; > + struct udp_tunnel_sock_cfg tunnel_cfg; > + struct udp_port_cfg udp_conf; > + int err; > + > + memset(&udp_conf, 0, sizeof(udp_conf)); > + > + udp_conf.family = AF_INET; > + udp_conf.local_ip.s_addr = htonl(INADDR_ANY); > + udp_conf.local_udp_port = htons(GTP1U_PORT); > + > + err = udp_sock_create(gtp->net, &udp_conf, &sock1u); > + if (err < 0) > + return err; > + > + udp_conf.local_udp_port = htons(GTP0_PORT); > + err = udp_sock_create(gtp->net, &udp_conf, &sock0); > + if (err < 0) > + return err; you're unconditionally binding to both GTP0 and GTP1 UDP ports. This is done selectively based on netlink attributes in the existing "normal" non-OVS kernel code, i.e. the control is left to the user. Is this function is only called/used in the context of OVS? If so, since you explicitly implement only GTPv1, why bind to GTPv0 port? > + setup_udp_tunnel_sock(net, sock1u, &tunnel_cfg); even here, you're only setting up the v1 and not v0. > + /* Assume largest header, ie. GTPv0. */ > + dev->needed_headroom = LL_MAX_HEADER + > + sizeof(struct iphdr) + > + sizeof(struct udphdr) + > + sizeof(struct gtp0_header); ... and here you're using headroom for a GTPv0 header, despite (I think) only supporting GTPv1 from this configuration? > + err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free?? I think that question about when to free needs to be resolved before any merge. Did you check that it persists even after the device is closed/removed? -- - Harald Welte <laforge-TgoAw6mPHtdg9hUCZPvPmw@public•gmane.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device 2017-07-13 7:35 ` Harald Welte @ 2017-07-14 1:01 ` Jiannan Ouyang 2017-07-14 8:12 ` Harald Welte 0 siblings, 1 reply; 17+ messages in thread From: Jiannan Ouyang @ 2017-07-14 1:01 UTC (permalink / raw) To: Harald Welte Cc: dev-yBygre7rU0TnMu66kgdUjQ@public•gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public•gmane.org, osmocom-net-gprs-qjLDD68F18N4m7O/Vxda39i2O/JbrIOy@public•gmane.org, pablo-Cap9r6Oaw4JrovVCs/uTlw@public•gmane.org Hi Harald, > On 7/13/17, 12:35 AM, "Harald Welte" <laforge-TgoAw6mPHtdg9hUCZPvPmw@public•gmane.org> wrote: > > > +static int gtp_dev_open(struct net_device *dev) > > +{ > > + struct gtp_dev *gtp = netdev_priv(dev); > > + struct net *net = gtp->net; > > + struct socket *sock1u; > > + struct socket *sock0; > > + struct udp_tunnel_sock_cfg tunnel_cfg; > > + struct udp_port_cfg udp_conf; > > + int err; > > + > > + memset(&udp_conf, 0, sizeof(udp_conf)); > > + > > + udp_conf.family = AF_INET; > > + udp_conf.local_ip.s_addr = htonl(INADDR_ANY); > > + udp_conf.local_udp_port = htons(GTP1U_PORT); > > + > > + err = udp_sock_create(gtp->net, &udp_conf, &sock1u); > > + if (err < 0) > > + return err; > > + > > + udp_conf.local_udp_port = htons(GTP0_PORT); > > + err = udp_sock_create(gtp->net, &udp_conf, &sock0); > > + if (err < 0) > > + return err; > > you're unconditionally binding to both GTP0 and GTP1 UDP ports. This is > done selectively based on netlink attributes in the existing "normal" > non-OVS kernel code, i.e. the control is left to the user. > > Is this function is only called/used in the context of OVS? If so, > since you explicitly implement only GTPv1, why bind to GTPv0 port? > I had doubts on how this flow-based GTPv1 code path should fit in, which is why the GTPv0 and the GTPv1 code pieces are mixed in my changes. Should I explicitly claim that the flow-based change is for GTPv1 only? > > + setup_udp_tunnel_sock(net, sock1u, &tunnel_cfg); > > even here, you're only setting up the v1 and not v0. > same reason as above. > > + /* Assume largest header, ie. GTPv0. */ > > + dev->needed_headroom = LL_MAX_HEADER + > > + sizeof(struct iphdr) + > > + sizeof(struct udphdr) + > > + sizeof(struct gtp0_header); > > ... and here you're using headroom for a GTPv0 header, despite (I think) > only supporting GTPv1 from this configuration? Yes, only GTPv1 is supported. > > > + err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free?? > > I think that question about when to free needs to be resolved before any > merge. Did you check that it persists even after the device is > closed/removed? I didn't investigate it. What do you mean by persist? Thanks -Jiannan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device 2017-07-14 1:01 ` Jiannan Ouyang @ 2017-07-14 8:12 ` Harald Welte 0 siblings, 0 replies; 17+ messages in thread From: Harald Welte @ 2017-07-14 8:12 UTC (permalink / raw) To: Jiannan Ouyang Cc: osmocom-net-gprs@lists•osmocom.org, netdev@vger•kernel.org, dev@openvswitch•org, pablo@netfilter•org, pshelar@nicira•com, wieger.ijntema.tno@gmail•com, yi.y.yang@intel•com, joe@ovn•org, Amar Padmanabhan Hi Jiannan, On Fri, Jul 14, 2017 at 01:01:34AM +0000, Jiannan Ouyang wrote: > > you're unconditionally binding to both GTP0 and GTP1 UDP ports. This is > > done selectively based on netlink attributes in the existing "normal" > > non-OVS kernel code, i.e. the control is left to the user. > > > > Is this function is only called/used in the context of OVS? If so, > > since you explicitly implement only GTPv1, why bind to GTPv0 port? > > > > I had doubts on how this flow-based GTPv1 code path should fit in, which is why > the GTPv0 and the GTPv1 code pieces are mixed in my changes. Well, I know nothing about flow-based paths and OVS, so if you are the one proposing related changes to me as the maintainer, you need to make sure that your changes are consistend and useful within your use case/scenario while making sure that the existing features don't break. If you refactor generic code (used by "classic" GTP tunneling + your new flow based tunneling), and that old code worked with GTPv0 and GTPv1, then your modifications must make sure that they continue to support GTPv0 and v1 in the "classic tunnel" use case. If your new code for flow-based tunneling simply only implements v0, it is fine to me - but then those restrictions must be in the flow-based part only, and things must be consistent. I.e. in this case, for the flow-based tunnel approach you must not bind the v0 port, if you don't handle related packets. > Should I explicitly claim that the flow-based change is for GTPv1 > only? That's definitely important, too - but is not the point I raised (see above). > > > + setup_udp_tunnel_sock(net, sock1u, &tunnel_cfg); > > > > even here, you're only setting up the v1 and not v0. > > same reason as above. yes, but if I read your code correctly, this is generic/shared code that will break the existing GTPv0 support in the "classic tunnel" case! > > > + /* Assume largest header, ie. GTPv0. */ > > > + dev->needed_headroom = LL_MAX_HEADER + > > > + sizeof(struct iphdr) + > > > + sizeof(struct udphdr) + > > > + sizeof(struct gtp0_header); > > > > ... and here you're using headroom for a GTPv0 header, despite (I think) > > only supporting GTPv1 from this configuration? > > Yes, only GTPv1 is supported. well, then I suggest you don't generate headroom for a v0 header (which is larger) in a v1-only code path :) > > > + err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free?? > > > > I think that question about when to free needs to be resolved before any > > merge. Did you check that it persists even after the device is > > closed/removed? > > I didn't investigate it. What do you mean by persist? "persist" means "remains allocated after the release of the network device". Whatever you allocate during device creation you must de-allocate on device release. I cannot tell you when exactly (as I'm not familiar with OVS or flow-based tunneling, as indicateD). However, I know for sure we cannot introduce code that looks like it introduces memory leaks to the kernel :) Regards, Harald -- - Harald Welte <laforge@gnumonks•org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device 2017-07-13 0:44 ` [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device Jiannan Ouyang [not found] ` <20170713004455.3946570-3-ouyangj-b10kYP2dOMg@public.gmane.org> @ 2017-07-13 18:01 ` Joe Stringer 1 sibling, 0 replies; 17+ messages in thread From: Joe Stringer @ 2017-07-13 18:01 UTC (permalink / raw) To: Jiannan Ouyang Cc: osmocom-net-gprs, netdev, ovs dev, Pablo Neira Ayuso, Harald Welte, Pravin B Shelar, Wieger IJntema, Yang, Yi Y, Amar Padmanabhan On 12 July 2017 at 17:44, Jiannan Ouyang <ouyangj@fb•com> wrote: > Add the gtp_create_flow_based_dev() interface to create flow-based gtp > net_device, which sets gtp->collect_md. Under flow-based mode, UDP sockets are > created and maintained in kernel. > > Signed-off-by: Jiannan Ouyang <ouyangj@fb•com> > --- <snip> > +static int gtp_configure(struct net *net, struct net_device *dev, > + const struct ip_tunnel_info *info) > +{ > + struct gtp_net *gn = net_generic(net, gtp_net_id); > + struct gtp_dev *gtp = netdev_priv(dev); > + int err; > + > + gtp->net = net; > + gtp->dev = dev; > + > + if (gtp_find_flow_based_dev(net)) > + return -EBUSY; > + > + dev->netdev_ops = >p_netdev_ops; > + dev->priv_destructor = free_netdev; > + > + dev->hard_header_len = 0; > + dev->addr_len = 0; > + > + /* Zero header length. */ > + dev->type = ARPHRD_NONE; > + dev->flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST; > + > + dev->priv_flags |= IFF_NO_QUEUE; > + dev->features |= NETIF_F_LLTX; > + netif_keep_dst(dev); > + > + /* Assume largest header, ie. GTPv0. */ > + dev->needed_headroom = LL_MAX_HEADER + > + sizeof(struct iphdr) + > + sizeof(struct udphdr) + > + sizeof(struct gtp0_header); > + > + dst_cache_reset(>p->info.dst_cache); > + gtp->info = *info; > + gtp->collect_md = true; > + > + err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free?? > + if (err < 0) { > + pr_err("Error gtp_hashtable_new"); > + return err; > + } Firstly, if this succeeds then the subsequent register_netdevice() call below could still fail, in which case the error handling there free this hashtable. Don't forget that even if this whole function succeeds, the caller may still fail to set up the device and in those error cases everything needs to be freed/cleaned up as well. Then in terms of the overall lifecycle, I guess it's a matter of 1) If there is a hashtable allocated then it must be freed on device close; 2) If the device can be reconfigured, and there is already one of these allocated then it would need to be freed upon reconfiguration. Cheers, Joe ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 0/3] Flow Based GTP Tunneling [not found] ` <20170713004455.3946570-1-ouyangj-b10kYP2dOMg@public.gmane.org> 2017-07-13 0:44 ` [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap Jiannan Ouyang 2017-07-13 0:44 ` [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device Jiannan Ouyang @ 2017-07-13 1:28 ` Joe Stringer 2 siblings, 0 replies; 17+ messages in thread From: Joe Stringer @ 2017-07-13 1:28 UTC (permalink / raw) To: Jiannan Ouyang Cc: ovs dev, netdev, osmocom-net-gprs-qjLDD68F18N4m7O/Vxda39i2O/JbrIOy, laforge-TgoAw6mPHtdg9hUCZPvPmw, Pablo Neira Ayuso On 12 July 2017 at 17:44, Jiannan Ouyang <ouyangj-b10kYP2dOMg@public•gmane.org> wrote: > This patch series augmented the existing GTP module to support flow > based GTP tunneling and modified the openvswitch datapath to support the > GTP vport type. > > A flow based GTP net device enables that, > 1) on the RX path, the outer (IP/UDP/GTP) header information could to be > stored in the metadata_dst struct, and embedded into the skb. > 2) on the TX path, packets are encapsulated following instructions in > the metadata_dst field of the skb. > > A flow based GTP net device can be integrated with Open vSwitch, which > allows SDN controllers to program GTP tunnels via Open vSwitch. > > Open vSwitch changes are based on patch set > [PATCH] Add GTP vport based on upstream datapath > > Example usage with OVS: > > ovs-vsctl add-port br0 gtp-vport -- set interface gtp-vport \ > ofport_request=2 type=gtp option:remote_ip=flow options:key=flow > > ovs-ofctl add-flow br0 > "in_port=2,tun_src=192.168.60.141,tun_id=123, \ > actions=set_field:02:00:00:00:00:00->eth_src, \ > set_field:ff:ff:ff:ff:ff:ff->eth_dst,LOCAL" > > ovs-ofctl add-flow br0 \ > "in_port=LOCAL,actions=set_tunnel:888, \ > set_field:192.168.60.141->tun_dst,2" > > arp -s 10.1.1.122 02:00:00:00:00:00 > > Jiannan Ouyang (3): > gtp: refactor to support flow-based gtp encap and decap > gtp: Support creating flow-based gtp net_device > openvswitch: Add GPRS Tunnel Protocol (GTP) vport support Hi Jiannan, net-next is closed, Dave won't accept patches at this time. Some brief feedback in regards to patch #3, the preference these days is for OVS userspace to use rtnetlink to configure devices in COLLECT_METADATA mode, then attach those devices as regular vport-netdev device type to OVS kernel datapath. I think that should mean that no kernel changes to openvswitch are required for providing GTP vports. Instead of this patch it would require something similar to the IFLA_GRE_COLLECT_METADATA flag which GRE has, but for the GTP devices. The latest OVS master now supports configuring devices in this way, perhaps you could take a look at OVS tree's lib/dpif-netlink-rtnl.c to see how other tunnel devices are configured and see if that makes sense for GTP as well? Cheers, Joe ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v1 3/3] openvswitch: Add GPRS Tunnel Protocol (GTP) vport support 2017-07-13 0:44 [PATCH net-next v1 0/3] Flow Based GTP Tunneling Jiannan Ouyang [not found] ` <20170713004455.3946570-1-ouyangj-b10kYP2dOMg@public.gmane.org> @ 2017-07-13 0:44 ` Jiannan Ouyang 2017-07-13 7:12 ` [PATCH net-next v1 0/3] Flow Based GTP Tunneling Harald Welte 2 siblings, 0 replies; 17+ messages in thread From: Jiannan Ouyang @ 2017-07-13 0:44 UTC (permalink / raw) To: osmocom-net-gprs, netdev, dev Cc: pablo, laforge, pshelar, wieger.ijntema.tno, yi.y.yang, joe, amarpadmanabhan, Jiannan Ouyang Add OVS_VPORT_TYPE_GTP type and vport-gtp support. Signed-off-by: Jiannan Ouyang <ouyangj@fb•com> --- include/uapi/linux/openvswitch.h | 1 + net/openvswitch/Kconfig | 10 +++ net/openvswitch/Makefile | 1 + net/openvswitch/vport-gtp.c | 144 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 156 insertions(+) create mode 100644 net/openvswitch/vport-gtp.c diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h index 156ee4c..82b87b2 100644 --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h @@ -213,6 +213,7 @@ enum ovs_vport_type { OVS_VPORT_TYPE_GRE, /* GRE tunnel. */ OVS_VPORT_TYPE_VXLAN, /* VXLAN tunnel. */ OVS_VPORT_TYPE_GENEVE, /* Geneve tunnel. */ + OVS_VPORT_TYPE_GTP, /* GTP tunnel. */ __OVS_VPORT_TYPE_MAX }; diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig index ce94729..d30d0ff 100644 --- a/net/openvswitch/Kconfig +++ b/net/openvswitch/Kconfig @@ -71,3 +71,13 @@ config OPENVSWITCH_GENEVE If you say Y here, then the Open vSwitch will be able create geneve vport. Say N to exclude this support and reduce the binary size. + +config OPENVSWITCH_GTP + tristate "Open vSwitch GTP tunneling support" + depends on OPENVSWITCH + depends on GTP + default OPENVSWITCH + ---help--- + If you say Y here, then the Open vSwitch will be able create gtp vport. + + Say N to exclude this support and reduce the binary size. diff --git a/net/openvswitch/Makefile b/net/openvswitch/Makefile index 60f8090..d77fcc0 100644 --- a/net/openvswitch/Makefile +++ b/net/openvswitch/Makefile @@ -22,3 +22,4 @@ endif obj-$(CONFIG_OPENVSWITCH_VXLAN)+= vport-vxlan.o obj-$(CONFIG_OPENVSWITCH_GENEVE)+= vport-geneve.o obj-$(CONFIG_OPENVSWITCH_GRE) += vport-gre.o +obj-$(CONFIG_OPENVSWITCH_GTP) += vport-gtp.o diff --git a/net/openvswitch/vport-gtp.c b/net/openvswitch/vport-gtp.c new file mode 100644 index 0000000..ed736ef --- /dev/null +++ b/net/openvswitch/vport-gtp.c @@ -0,0 +1,144 @@ +/* + * Copyright (c) 2017 Facebook, Inc. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/in.h> +#include <linux/ip.h> +#include <linux/net.h> +#include <linux/rculist.h> +#include <linux/udp.h> +#include <linux/if_vlan.h> +#include <linux/module.h> + +#include <net/gtp.h> +#include <net/icmp.h> +#include <net/ip.h> +#include <net/route.h> +#include <net/udp.h> +#include <net/xfrm.h> + +#include "datapath.h" +#include "vport.h" +#include "vport-netdev.h" + +static struct vport_ops ovs_gtp_vport_ops; +/** + * struct gtp_port - Keeps track of open UDP ports + * @dst_port: destination port. + */ +struct gtp_port { + u16 port_no; +}; + +static inline struct gtp_port *gtp_vport(const struct vport *vport) +{ + return vport_priv(vport); +} + +static int gtp_get_options(const struct vport *vport, + struct sk_buff *skb) +{ + struct gtp_port *gtp_port = gtp_vport(vport); + + if (nla_put_u16(skb, OVS_TUNNEL_ATTR_DST_PORT, gtp_port->port_no)) + return -EMSGSIZE; + return 0; +} + +static struct vport *gtp_tnl_create(const struct vport_parms *parms) +{ + struct net *net = ovs_dp_get_net(parms->dp); + struct nlattr *options = parms->options; + struct gtp_port *gtp_port; + struct net_device *dev; + struct vport *vport; + struct nlattr *a; + u16 dst_port; + int err; + + if (!options) { + err = -EINVAL; + goto error; + } + + a = nla_find_nested(options, OVS_TUNNEL_ATTR_DST_PORT); + if (a && nla_len(a) == sizeof(u16)) { + dst_port = nla_get_u16(a); + } else { + /* Require destination port from userspace. */ + err = -EINVAL; + goto error; + } + + vport = ovs_vport_alloc(sizeof(struct gtp_port), + &ovs_gtp_vport_ops, parms); + if (IS_ERR(vport)) + return vport; + + gtp_port = gtp_vport(vport); + gtp_port->port_no = dst_port; + + rtnl_lock(); + dev = gtp_create_flow_based_dev(net, parms->name, + NET_NAME_USER, dst_port); + if (IS_ERR(dev)) { + rtnl_unlock(); + ovs_vport_free(vport); + return ERR_CAST(dev); + } + + err = dev_change_flags(dev, dev->flags | IFF_UP); + if (err < 0) { + rtnl_delete_link(dev); + rtnl_unlock(); + ovs_vport_free(vport); + goto error; + } + + rtnl_unlock(); + return vport; +error: + return ERR_PTR(err); +} + +static struct vport *gtp_create(const struct vport_parms *parms) +{ + struct vport *vport; + + vport = gtp_tnl_create(parms); + if (IS_ERR(vport)) + return vport; + + return ovs_netdev_link(vport, parms->name); +} + +static struct vport_ops ovs_gtp_vport_ops = { + .type = OVS_VPORT_TYPE_GTP, + .create = gtp_create, + .destroy = ovs_netdev_tunnel_destroy, + .get_options = gtp_get_options, + .send = dev_queue_xmit, +}; + +static int __init ovs_gtp_tnl_init(void) +{ + return ovs_vport_ops_register(&ovs_gtp_vport_ops); +} +late_initcall(ovs_gtp_tnl_init); + +static void __exit ovs_gtp_tnl_exit(void) +{ + ovs_vport_ops_unregister(&ovs_gtp_vport_ops); +} +module_exit(ovs_gtp_tnl_exit); + +MODULE_DESCRIPTION("OVS: GTP switching port"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("vport-type-6"); -- 2.9.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 0/3] Flow Based GTP Tunneling 2017-07-13 0:44 [PATCH net-next v1 0/3] Flow Based GTP Tunneling Jiannan Ouyang [not found] ` <20170713004455.3946570-1-ouyangj-b10kYP2dOMg@public.gmane.org> 2017-07-13 0:44 ` [PATCH net-next v1 3/3] openvswitch: Add GPRS Tunnel Protocol (GTP) vport support Jiannan Ouyang @ 2017-07-13 7:12 ` Harald Welte 2017-07-13 18:14 ` Joe Stringer 2 siblings, 1 reply; 17+ messages in thread From: Harald Welte @ 2017-07-13 7:12 UTC (permalink / raw) To: Jiannan Ouyang Cc: osmocom-net-gprs, netdev, dev, pablo, pshelar, wieger.ijntema.tno, yi.y.yang, joe, amarpadmanabhan hi Jiannan, net-next si closed, as it has been pointed out already by Joe. On Wed, Jul 12, 2017 at 05:44:52PM -0700, Jiannan Ouyang wrote: > ovs-ofctl add-flow br0 > "in_port=2,tun_src=192.168.60.141,tun_id=123, \ > actions=set_field:02:00:00:00:00:00->eth_src, \ > set_field:ff:ff:ff:ff:ff:ff->eth_dst,LOCAL" I'm not familiar with the details here, but does this imply that you are matching on the outer (transport layer) source IP address? If so, please note this is in violation of the 3GPP specifications for GTP, which require explicitly that the TEID is the *only* criteria for matching an encapsulated packet to the tunnel. Basically anyone can send you an encapsulated packet from any random source IP, just as long as the TEID matches a tunnel, it will be decapsulated. This is [presumably] in order to take care of mobility, as the subscribers' phone moves around different MME/S-GW/SGSN, each having different source IP addresses. -- - Harald Welte <laforge@gnumonks•org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 0/3] Flow Based GTP Tunneling 2017-07-13 7:12 ` [PATCH net-next v1 0/3] Flow Based GTP Tunneling Harald Welte @ 2017-07-13 18:14 ` Joe Stringer 2017-07-13 22:54 ` Jiannan Ouyang 0 siblings, 1 reply; 17+ messages in thread From: Joe Stringer @ 2017-07-13 18:14 UTC (permalink / raw) To: Harald Welte Cc: Jiannan Ouyang, osmocom-net-gprs, netdev, ovs dev, Pablo Neira Ayuso, Pravin B Shelar, Wieger IJntema, Yang, Yi Y, Amar Padmanabhan On 13 July 2017 at 00:12, Harald Welte <laforge@gnumonks•org> wrote: > hi Jiannan, > > net-next si closed, as it has been pointed out already by Joe. > > On Wed, Jul 12, 2017 at 05:44:52PM -0700, Jiannan Ouyang wrote: >> ovs-ofctl add-flow br0 >> "in_port=2,tun_src=192.168.60.141,tun_id=123, \ >> actions=set_field:02:00:00:00:00:00->eth_src, \ >> set_field:ff:ff:ff:ff:ff:ff->eth_dst,LOCAL" > > I'm not familiar with the details here, but does this imply that you > are matching on the outer (transport layer) source IP address? If so, > please note this is in violation of the 3GPP specifications for GTP, > which require explicitly that the TEID is the *only* criteria for > matching an encapsulated packet to the tunnel. Basically anyone can > send you an encapsulated packet from any random source IP, just as long > as the TEID matches a tunnel, it will be decapsulated. > > This is [presumably] in order to take care of mobility, as the > subscribers' phone moves around different MME/S-GW/SGSN, each having > different source IP addresses. I think that this will be hard to avoid; the several existing tunnel implementations that OVS plugs into all allow matching on the outer addresses/ports, I don't see a good way to restrict it without introducing completely new metadata_dst paths for GTP. I'd prefer not to introduce something like this if we can avoid it; several tunnels currently share all of the same metadata_dst code and that's proven sufficient for all cases so far. If someone wishes to implement the 3GPP standard correctly then they should not create matches like this. In quite a few cases, OVS tends to take the approach that we give the user the tools to do what they need to do, but if they wish to shoot themselves in the foot then that's up to them. We can of course work towards ensuring the OVS userspace guides users in the right direction though. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v1 0/3] Flow Based GTP Tunneling 2017-07-13 18:14 ` Joe Stringer @ 2017-07-13 22:54 ` Jiannan Ouyang 0 siblings, 0 replies; 17+ messages in thread From: Jiannan Ouyang @ 2017-07-13 22:54 UTC (permalink / raw) To: Joe Stringer, Harald Welte Cc: osmocom-net-gprs@lists•osmocom.org, netdev, ovs dev, Pablo Neira Ayuso, Pravin B Shelar, Wieger IJntema, Yang, Yi Y, Amar Padmanabhan Hi Harald and Jeo, Thank you for the code review. They are really helpful! > On 7/13/17, 11:14 AM, "Joe Stringer" <joe@ovn•org> wrote: > > On 13 July 2017 at 00:12, Harald Welte <laforge@gnumonks•org> wrote: >> I'm not familiar with the details here, but does this imply that you >> are matching on the outer (transport layer) source IP address? If so, >> please note this is in violation of the 3GPP specifications for GTP, >> which require explicitly that the TEID is the *only* criteria for >> matching an encapsulated packet to the tunnel. Basically anyone can >> send you an encapsulated packet from any random source IP, just as long >> as the TEID matches a tunnel, it will be decapsulated. >> >> This is [presumably] in order to take care of mobility, as the >> subscribers' phone moves around different MME/S-GW/SGSN, each having >> different source IP addresses. > > I think that this will be hard to avoid; the several existing tunnel > implementations that OVS plugs into all allow matching on the outer > addresses/ports, I don't see a good way to restrict it without > introducing completely new metadata_dst paths for GTP. I'd prefer not > to introduce something like this if we can avoid it; several tunnels > currently share all of the same metadata_dst code and that's proven > sufficient for all cases so far. If someone wishes to implement the > 3GPP standard correctly then they should not create matches like this. > In quite a few cases, OVS tends to take the approach that we give the > user the tools to do what they need to do, but if they wish to shoot > themselves in the foot then that's up to them. We can of course work > towards ensuring the OVS userspace guides users in the right direction > though. The flow listed out here is an example for the nature of the match that can be performed, but the actual match rule that is programmed by the control plane should wildcard the tun_src as noted by Harald. It is the control plane’s responsibility to enforce the 3GPP Specifications, e.g. creating flow rules to make TEID the only criteria for matching an encapsulated packet to the tunnel. I will provide a 3GPP compliant example in the next version, thank Harald for pointing it out. -Jiannan ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-08-02 12:53 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-13 0:44 [PATCH net-next v1 0/3] Flow Based GTP Tunneling Jiannan Ouyang
[not found] ` <20170713004455.3946570-1-ouyangj-b10kYP2dOMg@public.gmane.org>
2017-07-13 0:44 ` [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap Jiannan Ouyang
2017-07-13 7:26 ` Harald Welte
2017-07-14 0:55 ` Jiannan Ouyang
[not found] ` <3729E0DA-08AB-4C5C-B9EC-C76DAAA60E10-b10kYP2dOMg@public.gmane.org>
2017-07-14 8:03 ` Harald Welte
2017-07-31 7:21 ` Andreas Schultz
2017-08-02 12:52 ` Pablo Neira Ayuso
2017-07-13 0:44 ` [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device Jiannan Ouyang
[not found] ` <20170713004455.3946570-3-ouyangj-b10kYP2dOMg@public.gmane.org>
2017-07-13 7:35 ` Harald Welte
2017-07-14 1:01 ` Jiannan Ouyang
2017-07-14 8:12 ` Harald Welte
2017-07-13 18:01 ` Joe Stringer
2017-07-13 1:28 ` [PATCH net-next v1 0/3] Flow Based GTP Tunneling Joe Stringer
2017-07-13 0:44 ` [PATCH net-next v1 3/3] openvswitch: Add GPRS Tunnel Protocol (GTP) vport support Jiannan Ouyang
2017-07-13 7:12 ` [PATCH net-next v1 0/3] Flow Based GTP Tunneling Harald Welte
2017-07-13 18:14 ` Joe Stringer
2017-07-13 22:54 ` Jiannan Ouyang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox