From f3d5e3a4155b6f42f6f6f0a2cc95ca0adbabe1af Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Wed, 19 Sep 2007 10:46:28 -0700 Subject: [PATCH] [PPP] L2TP: Fix skb handling in pppol2tp_xmit This patch makes pppol2tp_xmit call skb_cow_head so that we don't modify cloned skb data. It also gets rid of skb2 we only need to preserve the original skb for congestion notification, which is only applicable for ppp_async and ppp_sync. The other semantic change made here is the removal of socket accounting for data tranmitted out of pppol2tp_xmit. The original code leaked any existing socket skb accounting. We could fix this by dropping the original skb owner. However, this is undesirable as the packet has not physically left the host yet. In fact, all other tunnels in the kernel do not account skb's passing through to their own socket. In partciular, ESP over UDP does not do so and it is the closest tunnel type to PPPoL2TP. So this patch simply removes the socket accounting in pppol2tp_xmit. The accounting still applies to control packets of course. I've also added a reminder that the outgoing checksum here doesn't work. I suppose existing deployments don't actually enable checksums. Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- drivers/net/pppol2tp.c | 63 +++++++++++++++--------------------------- 1 file changed, 23 insertions(+), 40 deletions(-) diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c index 440e190778a1..abe91cb595f4 100644 --- a/drivers/net/pppol2tp.c +++ b/drivers/net/pppol2tp.c @@ -962,7 +962,6 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) int data_len = skb->len; struct inet_sock *inet; __wsum csum = 0; - struct sk_buff *skb2 = NULL; struct udphdr *uh; unsigned int len; @@ -993,41 +992,30 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) */ headroom = NET_SKB_PAD + sizeof(struct iphdr) + sizeof(struct udphdr) + hdr_len + sizeof(ppph); - if (skb_headroom(skb) < headroom) { - skb2 = skb_realloc_headroom(skb, headroom); - if (skb2 == NULL) - goto abort; - } else - skb2 = skb; - - /* Check that the socket has room */ - if (atomic_read(&sk_tun->sk_wmem_alloc) < sk_tun->sk_sndbuf) - skb_set_owner_w(skb2, sk_tun); - else - goto discard; + if (skb_cow_head(skb, headroom)) + goto abort; /* Setup PPP header */ - skb_push(skb2, sizeof(ppph)); - skb2->data[0] = ppph[0]; - skb2->data[1] = ppph[1]; + __skb_push(skb, sizeof(ppph)); + skb->data[0] = ppph[0]; + skb->data[1] = ppph[1]; /* Setup L2TP header */ - skb_push(skb2, hdr_len); - pppol2tp_build_l2tp_header(session, skb2->data); + pppol2tp_build_l2tp_header(session, __skb_push(skb, hdr_len)); /* Setup UDP header */ inet = inet_sk(sk_tun); - skb_push(skb2, sizeof(struct udphdr)); - skb_reset_transport_header(skb2); - uh = (struct udphdr *) skb2->data; + __skb_push(skb, sizeof(*uh)); + skb_reset_transport_header(skb); + uh = udp_hdr(skb); uh->source = inet->sport; uh->dest = inet->dport; uh->len = htons(sizeof(struct udphdr) + hdr_len + sizeof(ppph) + data_len); uh->check = 0; - /* Calculate UDP checksum if configured to do so */ + /* *BROKEN* Calculate UDP checksum if configured to do so */ if (sk_tun->sk_no_check != UDP_CSUM_NOXMIT) - csum = udp_csum_outgoing(sk_tun, skb2); + csum = udp_csum_outgoing(sk_tun, skb); /* Debug */ if (session->send_seq) @@ -1040,7 +1028,7 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) if (session->debug & PPPOL2TP_MSG_DATA) { int i; - unsigned char *datap = skb2->data; + unsigned char *datap = skb->data; printk(KERN_DEBUG "%s: xmit:", session->name); for (i = 0; i < data_len; i++) { @@ -1053,18 +1041,18 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) printk("\n"); } - memset(&(IPCB(skb2)->opt), 0, sizeof(IPCB(skb2)->opt)); - IPCB(skb2)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE | IPSKB_XFRM_TRANSFORMED | - IPSKB_REROUTED); - nf_reset(skb2); + memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt)); + IPCB(skb)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE | IPSKB_XFRM_TRANSFORMED | + IPSKB_REROUTED); + nf_reset(skb); /* Get routing info from the tunnel socket */ - dst_release(skb2->dst); - skb2->dst = sk_dst_get(sk_tun); + dst_release(skb->dst); + skb->dst = sk_dst_get(sk_tun); /* Queue the packet to IP for output */ - len = skb2->len; - rc = ip_queue_xmit(skb2, 1); + len = skb->len; + rc = ip_queue_xmit(skb, 1); /* Update stats */ if (rc >= 0) { @@ -1077,17 +1065,12 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb) session->stats.tx_errors++; } - /* Free the original skb */ - kfree_skb(skb); - return 1; -discard: - /* Free the new skb. Caller will free original skb. */ - if (skb2 != skb) - kfree_skb(skb2); abort: - return 0; + /* Free the original skb */ + kfree_skb(skb); + return 1; } /***************************************************************************** -- 2.30.2