diff mbox series

[net-next,04/14] gtp: udp recv clean up

Message ID 20170919003904.5124-5-tom@quantonium.net
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series gtp: Additional feature support | expand

Commit Message

Tom Herbert Sept. 19, 2017, 12:38 a.m. UTC 
Create separate UDP receive functions for GTP version 0 and version 1.
Set encap_rcv appropriately when configuring a socket. Also, convert to
using gro_cells.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 drivers/net/gtp.c | 130 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 71 insertions(+), 59 deletions(-)

Comments

kernel test robot Sept. 19, 2017, 7:44 a.m. UTC | #1 
Hi Tom,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Tom-Herbert/gtp-Additional-feature-support/20170919-143920
config: i386-randconfig-x016-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: the linux-review/Tom-Herbert/gtp-Additional-feature-support/20170919-143920 HEAD 737a09b8f9cd56706d01703d17523b0fea907f41 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers//net/gtp.c: In function 'gtp_rx':
>> drivers//net/gtp.c:222:21: error: 'gtp' undeclared (first use in this function)
     gro_cells_receive(&gtp->gro_cells, skb);
                        ^~~
   drivers//net/gtp.c:222:21: note: each undeclared identifier is reported only once for each function it appears in
   drivers//net/gtp.c: In function 'gtp_link_setup':
   drivers//net/gtp.c:628:18: error: 'gtp' undeclared (first use in this function)
     gro_cells_init(&gtp->gro_cells, dev);
                     ^~~

vim +/gtp +222 drivers//net/gtp.c

   190	
   191	static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
   192				unsigned int hdrlen, unsigned int role)
   193	{
   194		struct pcpu_sw_netstats *stats;
   195	
   196		if (!gtp_check_ms(skb, pctx, hdrlen, role)) {
   197			netdev_dbg(pctx->dev, "No PDP ctx for this MS\n");
   198			return 1;
   199		}
   200	
   201		/* Get rid of the GTP + UDP headers. */
   202		if (iptunnel_pull_header(skb, hdrlen, skb->protocol,
   203					 !net_eq(sock_net(pctx->sk), dev_net(pctx->dev))))
   204			return -1;
   205	
   206		netdev_dbg(pctx->dev, "forwarding packet from GGSN to uplink\n");
   207	
   208		/* Now that the UDP and the GTP header have been removed, set up the
   209		 * new network header. This is required by the upper layer to
   210		 * calculate the transport header.
   211		 */
   212		skb_reset_network_header(skb);
   213	
   214		skb->dev = pctx->dev;
   215	
   216		stats = this_cpu_ptr(pctx->dev->tstats);
   217		u64_stats_update_begin(&stats->syncp);
   218		stats->rx_packets++;
   219		stats->rx_bytes += skb->len;
   220		u64_stats_update_end(&stats->syncp);
   221	
 > 222		gro_cells_receive(&gtp->gro_cells, skb);
   223	
   224		return 0;
   225	}
   226	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Harald Welte Sept. 19, 2017, 11:32 a.m. UTC | #2 
Hi Tom,

I think this patch does too many things at once:
* introduce separate rx functions
* convert from netif_rx to gro_cells_receive
* cosmetic changes like "return -1" to "goto drop"

In the context of reviewability and the "one patch per topic", I would
prefer to see those separated, thanks.
diff mbox series

Patch

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 95df3bcebbb2..1de2ea6217ea 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -80,6 +80,8 @@  struct gtp_dev {
 	unsigned int		hash_size;
 	struct hlist_head	*tid_hash;
 	struct hlist_head	*addr_hash;
+
+	struct gro_cells	gro_cells;
 };
 
 static unsigned int gtp_net_id __read_mostly;
@@ -217,55 +219,83 @@  static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
 	stats->rx_bytes += skb->len;
 	u64_stats_update_end(&stats->syncp);
 
-	netif_rx(skb);
+	gro_cells_receive(&gtp->gro_cells, skb);
+
 	return 0;
 }
 
-/* 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)
+/* UDP encapsulation receive handler for GTPv0-U . See net/ipv4/udp.c.
+ * Return codes: 0: success, <0: error, >0: pass up to userspace UDP socket.
+ */
+static int gtp0_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
+	struct gtp_dev *gtp = rcu_dereference_sk_user_data(sk);
 	unsigned int hdrlen = sizeof(struct udphdr) +
 			      sizeof(struct gtp0_header);
 	struct gtp0_header *gtp0;
 	struct pdp_ctx *pctx;
 
+	if (!gtp)
+		goto pass;
+
 	if (!pskb_may_pull(skb, hdrlen))
-		return -1;
+		goto drop;
 
 	gtp0 = (struct gtp0_header *)(skb->data + sizeof(struct udphdr));
 
 	if ((gtp0->flags >> 5) != GTP_V0)
-		return 1;
+		goto pass;
 
 	if (gtp0->type != GTP_TPDU)
-		return 1;
+		goto pass;
+
+	netdev_dbg(gtp->dev, "received GTP0 packet\n");
 
 	pctx = gtp0_pdp_find(gtp, be64_to_cpu(gtp0->tid));
 	if (!pctx) {
 		netdev_dbg(gtp->dev, "No PDP ctx to decap skb=%p\n", skb);
-		return 1;
+		goto pass;
+	}
+
+	if (!gtp_rx(pctx, skb, hdrlen, gtp->role)) {
+		/* Successfully received */
+		return 0;
 	}
 
-	return gtp_rx(pctx, skb, hdrlen, gtp->role);
+drop:
+	kfree_skb(skb);
+	return 0;
+
+pass:
+	return 1;
 }
 
-static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
+/* UDP encapsulation receive handler for GTPv0-U . See net/ipv4/udp.c.
+ * Return codes: 0: success, <0: error, >0: pass up to userspace UDP socket.
+ */
+static int gtp1u_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
+	struct gtp_dev *gtp = rcu_dereference_sk_user_data(sk);
 	unsigned int hdrlen = sizeof(struct udphdr) +
 			      sizeof(struct gtp1_header);
 	struct gtp1_header *gtp1;
 	struct pdp_ctx *pctx;
 
+	if (!gtp)
+		goto pass;
+
 	if (!pskb_may_pull(skb, hdrlen))
-		return -1;
+		goto drop;
 
 	gtp1 = (struct gtp1_header *)(skb->data + sizeof(struct udphdr));
 
 	if ((gtp1->flags >> 5) != GTP_V1)
-		return 1;
+		goto pass;
 
 	if (gtp1->type != GTP_TPDU)
-		return 1;
+		goto pass;
+
+	netdev_dbg(gtp->dev, "received GTP1 packet\n");
 
 	/* From 29.060: "This field shall be present if and only if any one or
 	 * more of the S, PN and E flags are set.".
@@ -278,17 +308,27 @@  static int gtp1u_udp_encap_recv(struct gtp_dev *gtp, struct sk_buff *skb)
 
 	/* Make sure the header is larger enough, including extensions. */
 	if (!pskb_may_pull(skb, hdrlen))
-		return -1;
+		goto drop;
 
 	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;
+		goto pass;
+	}
+
+	if (!gtp_rx(pctx, skb, hdrlen, gtp->role)) {
+		/* Successfully received */
+		return 0;
 	}
 
-	return gtp_rx(pctx, skb, hdrlen, gtp->role);
+drop:
+	kfree_skb(skb);
+	return 0;
+
+pass:
+	return 1;
 }
 
 static void gtp_encap_destroy(struct sock *sk)
@@ -317,49 +357,6 @@  static void gtp_encap_disable(struct gtp_dev *gtp)
 	gtp_encap_disable_sock(gtp->sk1u);
 }
 
-/* UDP encapsulation receive handler. See net/ipv4/udp.c.
- * Return codes: 0: success, <0: error, >0: pass up to userspace UDP socket.
- */
-static int gtp_encap_recv(struct sock *sk, struct sk_buff *skb)
-{
-	struct gtp_dev *gtp;
-	int ret = 0;
-
-	gtp = rcu_dereference_sk_user_data(sk);
-	if (!gtp)
-		return 1;
-
-	netdev_dbg(gtp->dev, "encap_recv sk=%p\n", sk);
-
-	switch (udp_sk(sk)->encap_type) {
-	case UDP_ENCAP_GTP0:
-		netdev_dbg(gtp->dev, "received GTP0 packet\n");
-		ret = gtp0_udp_encap_recv(gtp, skb);
-		break;
-	case UDP_ENCAP_GTP1U:
-		netdev_dbg(gtp->dev, "received GTP1U packet\n");
-		ret = gtp1u_udp_encap_recv(gtp, skb);
-		break;
-	default:
-		ret = -1; /* Shouldn't happen. */
-	}
-
-	switch (ret) {
-	case 1:
-		netdev_dbg(gtp->dev, "pass up to the process\n");
-		break;
-	case 0:
-		break;
-	case -1:
-		netdev_dbg(gtp->dev, "GTP packet has been dropped\n");
-		kfree_skb(skb);
-		ret = 0;
-		break;
-	}
-
-	return ret;
-}
-
 static int gtp_dev_init(struct net_device *dev)
 {
 	struct gtp_dev *gtp = netdev_priv(dev);
@@ -627,6 +624,8 @@  static void gtp_link_setup(struct net_device *dev)
 				  sizeof(struct iphdr) +
 				  sizeof(struct udphdr) +
 				  sizeof(struct gtp0_header);
+
+	gro_cells_init(&gtp->gro_cells, dev);
 }
 
 static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize);
@@ -683,6 +682,7 @@  static void gtp_dellink(struct net_device *dev, struct list_head *head)
 {
 	struct gtp_dev *gtp = netdev_priv(dev);
 
+	gro_cells_destroy(&gtp->gro_cells);
 	gtp_encap_disable(gtp);
 	gtp_hashtable_free(gtp);
 	list_del_rcu(&gtp->list);
@@ -804,9 +804,21 @@  static struct sock *gtp_encap_enable_socket(int fd, int type,
 	sk = sock->sk;
 	sock_hold(sk);
 
+	switch (type) {
+	case UDP_ENCAP_GTP0:
+		tuncfg.encap_rcv = gtp0_udp_encap_recv;
+		break;
+	case UDP_ENCAP_GTP1U:
+		tuncfg.encap_rcv = gtp1u_udp_encap_recv;
+		break;
+	default:
+		pr_debug("Unknown encap type %u\n", type);
+		sk = ERR_PTR(-EINVAL);
+		goto out_sock;
+	}
+
 	tuncfg.sk_user_data = gtp;
 	tuncfg.encap_type = type;
-	tuncfg.encap_rcv = gtp_encap_recv;
 	tuncfg.encap_destroy = gtp_encap_destroy;
 
 	setup_udp_tunnel_sock(sock_net(sock->sk), sock, &tuncfg);