Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Harald Welte]

Hi Andreas,

On Wed, Sep 20, 2017 at 05:37:52PM +0200, Andreas Schultz wrote:
> I think we had this discussion before. The sending IP and port are not part
> of the identity of the PDP context. So IMHO the sender is permitted
> to change the source IP at random.

Thanks for the reminder: You are correct, at least in the uplink case
(MS->GGSN) where there is mobility of the MS. In the downlink case
(GGSN->MS), which is the "sending" part for the kernel GTP code used at
a GGSN, I'm not sure if that theory holds true in reality.

Do you agree that the current behavior of not using automatic source
address selection for encapsulated GTP packets but rather using the
source address of the socket is intended?

Do you further agree that the dst_cache support patch by Tom retains
that intended behavior and it should be merged?

-- 
- Harald Welte    http://laforge.gnumonks.org/

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

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Andreas Schultz]

On 19/09/17 14:09, Harald Welte wrote:

Hi Dave,

On Mon, Sep 18, 2017 at 09:17:51PM -0700, David Miller wrote:

This and the new dst caching code ignores any source address selection
done by ip_route_output_key() or the new tunnel route lookup helpers.

Either source address selection should be respected, or if saddr will
never be modified by a route lookup for some specific reason here,
that should be documented.


The IP source address is fixed by signaling on the GTP-C control plane
and nothing that the kernel can unilaterally decide to change.  Such a
change of address would have to be decided by and first be signaled on
GTP-C to the peer by the userspace daemon, which would then update the
PDP context in the kernel.


I think we had this discussion before. The sending IP and port are not 
part of the identity of the PDP context. So IMHO the sender is permitted

to change the source IP at random.

Regards
Andreas



So I guess you're asking us to document that rationale as form of a
source code comment ?

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[David Miller]

From: Harald Welte 
Date: Tue, 19 Sep 2017 20:09:42 +0800

> So I guess you're asking us to document that rationale as form of a
> source code comment ?

Yes that would make ignoring the potential changing of the non-const
'saddr' argument at least be documented.

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Tom Herbert]

On Mon, Sep 18, 2017 at 9:17 PM, David Miller  wrote:
> From: Tom Herbert 
> Date: Mon, 18 Sep 2017 17:38:53 -0700
>
>> Call ip_tunnel_get_route and dst_cache to pdp context which should
>> improve performance by obviating the need to perform a route lookup
>> on every packet.
>>
>> Signed-off-by: Tom Herbert 
>
> Not caused by your changes, but something to think about:
>
>> -static struct rtable *ip4_route_output_gtp(struct flowi4 *fl4,
>> -const struct sock *sk,
>> -__be32 daddr)
>> -{
>> - memset(fl4, 0, sizeof(*fl4));
>> - fl4->flowi4_oif = sk->sk_bound_dev_if;
>> - fl4->daddr  = daddr;
>> - fl4->saddr  = inet_sk(sk)->inet_saddr;
>> - fl4->flowi4_tos = RT_CONN_FLAGS(sk);
>> - fl4->flowi4_proto   = sk->sk_protocol;
>> -
>> - return ip_route_output_key(sock_net(sk), fl4);
>> -}
>
> This and the new dst caching code ignores any source address selection
> done by ip_route_output_key() or the new tunnel route lookup helpers.
>
> Either source address selection should be respected, or if saddr will
> never be modified by a route lookup for some specific reason here,
> that should be documented.

Yes, I noticed that. In this case the source address is intended to be
taken bound on the socket which would imply we aren't interested in
source address selection.

Tom

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Harald Welte]

Hi Dave,

On Mon, Sep 18, 2017 at 09:17:51PM -0700, David Miller wrote:
> This and the new dst caching code ignores any source address selection
> done by ip_route_output_key() or the new tunnel route lookup helpers.
> 
> Either source address selection should be respected, or if saddr will
> never be modified by a route lookup for some specific reason here,
> that should be documented.

The IP source address is fixed by signaling on the GTP-C control plane
and nothing that the kernel can unilaterally decide to change.  Such a
change of address would have to be decided by and first be signaled on
GTP-C to the peer by the userspace daemon, which would then update the
PDP context in the kernel.

So I guess you're asking us to document that rationale as form of a
source code comment ?

-- 
- Harald Welte    http://laforge.gnumonks.org/

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

Re: [PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[David Miller]

From: Tom Herbert 
Date: Mon, 18 Sep 2017 17:38:53 -0700

> Call ip_tunnel_get_route and dst_cache to pdp context which should
> improve performance by obviating the need to perform a route lookup
> on every packet.
> 
> Signed-off-by: Tom Herbert 

Not caused by your changes, but something to think about:

> -static struct rtable *ip4_route_output_gtp(struct flowi4 *fl4,
> -const struct sock *sk,
> -__be32 daddr)
> -{
> - memset(fl4, 0, sizeof(*fl4));
> - fl4->flowi4_oif = sk->sk_bound_dev_if;
> - fl4->daddr  = daddr;
> - fl4->saddr  = inet_sk(sk)->inet_saddr;
> - fl4->flowi4_tos = RT_CONN_FLAGS(sk);
> - fl4->flowi4_proto   = sk->sk_protocol;
> -
> - return ip_route_output_key(sock_net(sk), fl4);
> -}

This and the new dst caching code ignores any source address selection
done by ip_route_output_key() or the new tunnel route lookup helpers.

Either source address selection should be respected, or if saddr will
never be modified by a route lookup for some specific reason here,
that should be documented.

[PATCH net-next 03/14] gtp: Call common functions to get tunnel routes and add dst_cache

[Tom Herbert]

Call ip_tunnel_get_route and dst_cache to pdp context which should
improve performance by obviating the need to perform a route lookup
on every packet.

Signed-off-by: Tom Herbert 
---
 drivers/net/gtp.c | 59 ++-
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index f38e32a7ec9c..95df3bcebbb2 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -63,6 +63,8 @@ struct pdp_ctx {
 
atomic_ttx_seq;
struct rcu_head rcu_head;
+
+   struct dst_cachedst_cache;
 };
 
 /* One instance of the GTP device. */
@@ -379,20 +381,6 @@ static void gtp_dev_uninit(struct net_device *dev)
free_percpu(dev->tstats);
 }
 
-static struct rtable *ip4_route_output_gtp(struct flowi4 *fl4,
-  const struct sock *sk,
-  __be32 daddr)
-{
-   memset(fl4, 0, sizeof(*fl4));
-   fl4->flowi4_oif = sk->sk_bound_dev_if;
-   fl4->daddr  = daddr;
-   fl4->saddr  = inet_sk(sk)->inet_saddr;
-   fl4->flowi4_tos = RT_CONN_FLAGS(sk);
-   fl4->flowi4_proto   = sk->sk_protocol;
-
-   return ip_route_output_key(sock_net(sk), fl4);
-}
-
 static inline void gtp0_push_header(struct sk_buff *skb, struct pdp_ctx *pctx)
 {
int payload_len = skb->len;
@@ -479,6 +467,8 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct 
net_device *dev,
struct rtable *rt;
struct flowi4 fl4;
struct iphdr *iph;
+   struct sock *sk;
+   __be32 saddr;
__be16 df;
int mtu;
 
@@ -498,19 +488,27 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct 
net_device *dev,
}
netdev_dbg(dev, "found PDP context %p\n", pctx);
 
-   rt = ip4_route_output_gtp(, pctx->sk, pctx->peer_addr_ip4.s_addr);
-   if (IS_ERR(rt)) {
-   netdev_dbg(dev, "no route to SSGN %pI4\n",
-  >peer_addr_ip4.s_addr);
-   dev->stats.tx_carrier_errors++;
-   goto err;
-   }
+   sk = pctx->sk;
+   saddr = inet_sk(sk)->inet_saddr;
 
-   if (rt->dst.dev == dev) {
-   netdev_dbg(dev, "circular route to SSGN %pI4\n",
-  >peer_addr_ip4.s_addr);
-   dev->stats.collisions++;
-   goto err_rt;
+   rt = ip_tunnel_get_route(dev, skb, sk->sk_protocol,
+sk->sk_bound_dev_if, RT_CONN_FLAGS(sk),
+pctx->peer_addr_ip4.s_addr, ,
+pktinfo->gtph_port, pktinfo->gtph_port,
+>dst_cache, NULL);
+
+   if (IS_ERR(rt)) {
+   if (rt == ERR_PTR(-ELOOP)) {
+   netdev_dbg(dev, "circular route to SSGN %pI4\n",
+  >peer_addr_ip4.s_addr);
+   dev->stats.collisions++;
+   goto err_rt;
+   } else {
+   netdev_dbg(dev, "no route to SSGN %pI4\n",
+  >peer_addr_ip4.s_addr);
+   dev->stats.tx_carrier_errors++;
+   goto err;
+   }
}
 
skb_dst_drop(skb);
@@ -543,7 +541,7 @@ 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, , dev);
+   gtp_set_pktinfo_ipv4(pktinfo, sk, iph, pctx, rt, , dev);
gtp_push_header(skb, pktinfo);
 
return 0;
@@ -917,6 +915,7 @@ static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock 
*sk,
struct pdp_ctx *pctx;
bool found = false;
__be32 ms_addr;
+   int err;
 
ms_addr = nla_get_be32(info->attrs[GTPA_MS_ADDRESS]);
hash_ms = ipv4_hashfn(ms_addr) % gtp->hash_size;
@@ -951,6 +950,12 @@ static int ipv4_pdp_add(struct gtp_dev *gtp, struct sock 
*sk,
if (pctx == NULL)
return -ENOMEM;
 
+   err = dst_cache_init(>dst_cache, GFP_KERNEL);
+   if (err) {
+   kfree(pctx);
+   return err;
+   }
+
sock_hold(sk);
pctx->sk = sk;
pctx->dev = gtp->dev;
-- 
2.11.0