13 Jul
2017
13 Jul
'17
2:44 a.m.
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
13 Jul
13 Jul
2:44 a.m.
New subject: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap
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(a)fb.com>
---
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
9:26 a.m.
New subject: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap
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(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
14 Jul
14 Jul
2:55 a.m.
New subject: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap
Hi Harald,
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.
Right, will fix this for GTPv0.
It is meant to be OVS independent. What makes it not? Should I leave
this field un-initialized?
Thanks
-Jiannan
On 7/13/17, 12:26 AM, "Harald Welte"
<laforge(a)gnumonks.org> wrote:
·
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?
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?
10:15 a.m.
New subject: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap
Hi Jiannan,
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(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
gtp = netdev_priv(dev);
+ gtp->net = src_net;
·
Isn't this a generic change that's independent of your work on OVS GTP? 
31 Jul
31 Jul
9:21 a.m.
New subject: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap
Hi Jiannan,
----- On Jul 13, 2017, at 2:44 AM, Jiannan Ouyang ouyangj(a)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
2 Aug
2 Aug
2:52 p.m.
New subject: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap
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(a)fb.com wrote:
[...]
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.
-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.
13 Jul
13 Jul
2:44 a.m.
New subject: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device
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(a)fb.com>
---
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
9:35 a.m.
New subject: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device
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(a)fb.com>
---
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(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
14 Jul
14 Jul
3:01 a.m.
New subject: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device
Hi Harald,
same reason as above.
Yes, only GTPv1 is supported.
I didn't investigate it. What do you mean by persist?
Thanks
-Jiannan
On 7/13/17, 12:35 AM, "Harald Welte"
<laforge(a)gnumonks.org> wrote:
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?
+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?
10:15 a.m.
New subject: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device
Hi Jiannan,
On Fri, Jul 14, 2017 at 01:01:34AM +0000, Jiannan Ouyang wrote:
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.
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!
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 :)
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(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
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?
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. + /* 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? 
13 Jul
13 Jul
8:01 p.m.
New subject: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device
On 12 July 2017 at 17:44, Jiannan Ouyang <ouyangj(a)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(a)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
2:44 a.m.
New subject: [PATCH net-next v1 3/3] openvswitch: Add GPRS Tunnel Protocol (GTP) vport support
Add OVS_VPORT_TYPE_GTP type and vport-gtp support.
Signed-off-by: Jiannan Ouyang <ouyangj(a)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
3:28 a.m.
On 12 July 2017 at 17:44, Jiannan Ouyang <ouyangj(a)fb.com> 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
9:12 a.m.
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(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
8:14 p.m.
On 13 July 2017 at 00:12, Harald Welte <laforge(a)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:
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.
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.
14 Jul
14 Jul
12:54 a.m.
Hi Harald and Jeo,
Thank you for the code review. They are really helpful!
On 7/13/17, 11:14 AM, "Joe Stringer"
<joe(a)ovn.org> wrote:
On 13 July 2017 at 00:12, Harald Welte <laforge(a)gnumonks.org> wrote:
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
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.
2467
days inactive
2487
days old
osmocom-net-gprs@lists.osmocom.org
16 comments
6 participants
participants (6)
-
Andreas Schultz -
Harald Welte -
Harald Welte -
Jiannan Ouyang -
Joe Stringer -
Pablo Neira Ayuso