Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap

[Pablo Neira Ayuso]

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 ouya...@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.

Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap

[Andreas Schultz]

Hi Jiannan,

- On Jul 13, 2017, at 2:44 AM, Jiannan Ouyang ouya...@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

Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap

[Harald Welte]

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    http://laforge.gnumonks.org/

"Privacy in residential applications is a desirable marketing option."
  (ETSI EN 300 175-7 Ch. A6)

Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap

[Jiannan Ouyang]

Hi Harald,

> On 7/13/17, 12:26 AM, "Harald Welte"  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

Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap

[Harald Welte]

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(>syncp);
>   stats->rx_packets++;
>   stats->rx_bytes += skb->len;
>   u64_stats_update_end(>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.

[PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap

[Jiannan Ouyang]

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 
---
 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 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -36,6 +37,8 @@
 #include 
 #include 
 
+#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_ttx_seq;
struct rcu_head rcu_head;
@@ -73,11 +76,15 @@ struct gtp_dev {
struct sock *sk1u;
 
struct net_device   *dev;
+   struct net  *net;
 
unsigned introle;
unsigned inthash_size;
struct hlist_head   *tid_hash;
struct hlist_head   *addr_hash;
+
+   unsigned intcollect_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(>syncp);
stats->rx_packets++;
stats->rx_bytes += skb->len;
u64_stats_update_end(>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);
+