Re: [PATCH net-next 1/4] gtp: move TEID hash to per socket structure

[Andreas Schultz]

- On Mar 14, 2017, at 12:33 PM, pablo pa...@netfilter.org wrote:

> On Tue, Mar 14, 2017 at 12:25:45PM +0100, Andreas Schultz wrote:
>> @@ -275,9 +280,9 @@ 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));
>> +pctx = gtp1_pdp_find(gsk, ntohl(gtp1->tid));
>>  if (!pctx) {
>> -netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb);
>> +pr_debug("No PDP ctx to decap skb=%p\n", skb);
>>  return 1;
> 
> Again the pr_debug() change has resurrected.

Yes, at that point in the code, there is now ways to resolve the network device.
Therefore the netdev_dbg has to go.

> I already told you: If we are going to have more than one gtp device,
> then this doesn't make sense. I have to repeat things over and over
> again, just because you don't want to rebase your patchset for some
> reason. I don't find any other explaination for this.

Without a PDP context, there is no network device, so netdev_dbg.
 
> So please remove this debugging rather than rendering this completely
> useful.

ACK
 
> Moreover this change has nothing to this patch, so this doesn't break
> the one logical change per patch.

This patch moves the incoming teid has from the network device to the
socket. This means that gtp1_pdp_find needs to change. So this related.
For the debug change, see above why it's related.

Andreas

Re: [PATCH net-next 1/4] gtp: move TEID hash to per socket structure

[Pablo Neira Ayuso]

On Tue, Mar 14, 2017 at 12:25:45PM +0100, Andreas Schultz wrote:
> @@ -275,9 +280,9 @@ 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));
> + pctx = gtp1_pdp_find(gsk, ntohl(gtp1->tid));
>   if (!pctx) {
> - netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb);
> + pr_debug("No PDP ctx to decap skb=%p\n", skb);
>   return 1;

Again the pr_debug() change has resurrected.

I already told you: If we are going to have more than one gtp device,
then this doesn't make sense. I have to repeat things over and over
again, just because you don't want to rebase your patchset for some
reason. I don't find any other explaination for this.

So please remove this debugging rather than rendering this completely
useful.

Moreover this change has nothing to this patch, so this doesn't break
the one logical change per patch.

[PATCH net-next 1/4] gtp: move TEID hash to per socket structure

[Andreas Schultz]

Untangele the TEID information from the network device and move
it into a per socket structure.

The removeal of the TEID form the netdevice also means that the
gtp genl API for retriving tunnel information and removing tunnels
needs to be adjusted.
Before this change it was possible to change a GTP tunnel using
the gtp netdevice id and the teid. The teid is no longer unique
per gtp netdevice. So after this change it has to be either the
netdevice and MS IP or the GTP socket and teid.

Fortunatly, libgtpnl has always populated the Link Id, TEID,
GSN Peer IP and MS IP. So, the library interface has ensured that
all information that is mandatory after this change is guranteed
to be present. The only project that doesn't use libgtpnl (OpenAir-CN)
is also populating all of those values.

Signed-off-by: Andreas Schultz 
---
 drivers/net/gtp.c | 145 +-
 1 file changed, 78 insertions(+), 67 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 3e1854f..66616f7 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -75,10 +75,15 @@ struct gtp_dev {
struct net_device   *dev;
 
unsigned inthash_size;
-   struct hlist_head   *tid_hash;
struct hlist_head   *addr_hash;
 };
 
+/* One instance of the GTP socket. */
+struct gtp_sock {
+   unsigned inthash_size;
+   struct hlist_head   tid_hash[];
+};
+
 static unsigned int gtp_net_id __read_mostly;
 
 struct gtp_net {
@@ -106,12 +111,12 @@ static inline u32 ipv4_hashfn(__be32 ip)
 }
 
 /* Resolve a PDP context structure based on the 64bit TID. */
-static struct pdp_ctx *gtp0_pdp_find(struct gtp_dev *gtp, u64 tid)
+static struct pdp_ctx *gtp0_pdp_find(struct gtp_sock *gsk, u64 tid)
 {
struct hlist_head *head;
struct pdp_ctx *pdp;
 
-   head = >tid_hash[gtp0_hashfn(tid) % gtp->hash_size];
+   head = >tid_hash[gtp0_hashfn(tid) % gsk->hash_size];
 
hlist_for_each_entry_rcu(pdp, head, hlist_tid) {
if (pdp->gtp_version == GTP_V0 &&
@@ -122,12 +127,12 @@ static struct pdp_ctx *gtp0_pdp_find(struct gtp_dev *gtp, 
u64 tid)
 }
 
 /* Resolve a PDP context structure based on the 32bit TEI. */
-static struct pdp_ctx *gtp1_pdp_find(struct gtp_dev *gtp, u32 tid)
+static struct pdp_ctx *gtp1_pdp_find(struct gtp_sock *gsk, u32 tid)
 {
struct hlist_head *head;
struct pdp_ctx *pdp;
 
-   head = >tid_hash[gtp1u_hashfn(tid) % gtp->hash_size];
+   head = >tid_hash[gtp1u_hashfn(tid) % gsk->hash_size];
 
hlist_for_each_entry_rcu(pdp, head, hlist_tid) {
if (pdp->gtp_version == GTP_V1 &&
@@ -215,7 +220,7 @@ static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff 
*skb, unsigned int hdrlen
 }
 
 /* 1 means pass up to the stack, -1 means drop and 0 means decapsulated. */
-static int gtp0_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
+static int gtp0_udp_encap_recv(struct gtp_sock *gsk, struct sk_buff *skb)
 {
unsigned int hdrlen = sizeof(struct udphdr) +
  sizeof(struct gtp0_header);
@@ -233,16 +238,16 @@ static int gtp0_udp_encap_recv(struct gtp_dev *gtp, 
struct sk_buff *skb)
if (gtp0->type != GTP_TPDU)
return 1;
 
-   pctx = gtp0_pdp_find(gtp, be64_to_cpu(gtp0->tid));
+   pctx = gtp0_pdp_find(gsk, be64_to_cpu(gtp0->tid));
if (!pctx) {
-   netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb);
+   pr_debug("No PDP ctx to decap skb=%p\n", skb);
return 1;
}
 
return gtp_rx(pctx, skb, hdrlen);
 }
 
-static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
+static int gtp1u_udp_encap_recv(struct gtp_sock *gsk, struct sk_buff *skb)
 {
unsigned int hdrlen = sizeof(struct udphdr) +
  sizeof(struct gtp1_header);
@@ -275,9 +280,9 @@ 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));
+   pctx = gtp1_pdp_find(gsk, ntohl(gtp1->tid));
if (!pctx) {
-   netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb);
+   pr_debug("No PDP ctx to decap skb=%p\n", skb);
return 1;
}
 
@@ -286,13 +291,21 @@ static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, 
struct sk_buff *skb)
 
 static void gtp_encap_destroy(struct sock *sk)
 {
-   struct gtp_dev *gtp;
+   struct gtp_sock *gsk;
+   struct pdp_ctx *pctx;
+   int i;
 
-   gtp = rcu_dereference_sk_user_data(sk);
-   if (gtp) {
+   gsk = rcu_dereference_sk_user_data(sk);
+   if (gsk) {
udp_sk(sk)->encap_type = 0;
rcu_assign_sk_user_data(sk, NULL);
-   sock_put(sk);
+
+   for (i = 0; i < gsk->hash_size; i++)
+