From 31bac44468257986484703cc09da8a9dcae88a36 Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Sun, 16 Sep 2007 16:19:20 -0700 Subject: [PPP] pppoe: Fix skb_unshare_check call position The skb_unshare_check call needs to be made before pskb_may_pull, not after. Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- drivers/net/pppoe.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c index 68631a5721ac..5ac3eff6a2a6 100644 --- a/drivers/net/pppoe.c +++ b/drivers/net/pppoe.c @@ -385,12 +385,12 @@ static int pppoe_rcv(struct sk_buff *skb, struct pppoe_hdr *ph; struct pppox_sock *po; - if (!pskb_may_pull(skb, sizeof(struct pppoe_hdr))) - goto drop; - if (!(skb = skb_share_check(skb, GFP_ATOMIC))) goto out; + if (!pskb_may_pull(skb, sizeof(struct pppoe_hdr))) + goto drop; + ph = pppoe_hdr(skb); po = get_item((unsigned long) ph->sid, eth_hdr(skb)->h_source, dev->ifindex); -- cgit v1.2.1 From db7bf6d97c6956b7eb0f22131cb5c37bd41f33c0 Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Sun, 16 Sep 2007 16:19:50 -0700 Subject: [PPP] pppoe: Fix data clobbering in __pppoe_xmit and return value The function __pppoe_xmit modifies the skb data and therefore it needs to copy and skb data if it's cloned. In fact, it currently allocates a new skb so that it can return 0 in case of error without freeing the original skb. This is totally wrong because returning zero is meant to indicate congestion whereupon pppoe is supposed to wake up the upper layer once the congestion subsides. This makes sense for ppp_async and ppp_sync but is out-of-place for pppoe. This patch makes it always return 1 and free the skb. Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- drivers/net/pppoe.c | 50 +++++++++++++------------------------------------- 1 file changed, 13 insertions(+), 37 deletions(-) (limited to 'drivers') diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c index 5ac3eff6a2a6..8818253102f2 100644 --- a/drivers/net/pppoe.c +++ b/drivers/net/pppoe.c @@ -850,9 +850,7 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb) struct net_device *dev = po->pppoe_dev; struct pppoe_hdr hdr; struct pppoe_hdr *ph; - int headroom = skb_headroom(skb); int data_len = skb->len; - struct sk_buff *skb2; if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED)) goto abort; @@ -866,53 +864,31 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb) if (!dev) goto abort; - /* Copy the skb if there is no space for the header. */ - if (headroom < (sizeof(struct pppoe_hdr) + dev->hard_header_len)) { - skb2 = dev_alloc_skb(32+skb->len + - sizeof(struct pppoe_hdr) + - dev->hard_header_len); - - if (skb2 == NULL) - goto abort; - - skb_reserve(skb2, dev->hard_header_len + sizeof(struct pppoe_hdr)); - skb_copy_from_linear_data(skb, skb_put(skb2, skb->len), - skb->len); - } else { - /* Make a clone so as to not disturb the original skb, - * give dev_queue_xmit something it can free. - */ - skb2 = skb_clone(skb, GFP_ATOMIC); - - if (skb2 == NULL) - goto abort; - } + /* Copy the data if there is no space for the header or if it's + * read-only. + */ + if (skb_cow(skb, sizeof(*ph) + dev->hard_header_len)) + goto abort; - ph = (struct pppoe_hdr *) skb_push(skb2, sizeof(struct pppoe_hdr)); + ph = (struct pppoe_hdr *) skb_push(skb, sizeof(struct pppoe_hdr)); memcpy(ph, &hdr, sizeof(struct pppoe_hdr)); - skb2->protocol = __constant_htons(ETH_P_PPP_SES); + skb->protocol = __constant_htons(ETH_P_PPP_SES); - skb_reset_network_header(skb2); + skb_reset_network_header(skb); - skb2->dev = dev; + skb->dev = dev; - dev->hard_header(skb2, dev, ETH_P_PPP_SES, + dev->hard_header(skb, dev, ETH_P_PPP_SES, po->pppoe_pa.remote, NULL, data_len); - /* We're transmitting skb2, and assuming that dev_queue_xmit - * will free it. The generic ppp layer however, is expecting - * that we give back 'skb' (not 'skb2') in case of failure, - * but free it in case of success. - */ - - if (dev_queue_xmit(skb2) < 0) + if (dev_queue_xmit(skb) < 0) goto abort; - kfree_skb(skb); return 1; abort: - return 0; + kfree_skb(skb); + return 1; } -- cgit v1.2.1 From 9355ec23397af32799038d0e8edbfa5b6f425c27 Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Sun, 16 Sep 2007 16:20:21 -0700 Subject: [PPP] pppoe: Fill in header directly in __pppoe_xmit This patch removes the hdr variable (which is copied into the skb) and instead sets the header directly in the skb. It also uses __skb_push instead of skb_push since we've just checked using skb_cow for enough head room. Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- drivers/net/pppoe.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) (limited to 'drivers') diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c index 8818253102f2..bac36546e0bf 100644 --- a/drivers/net/pppoe.c +++ b/drivers/net/pppoe.c @@ -848,19 +848,12 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb) { struct pppox_sock *po = pppox_sk(sk); struct net_device *dev = po->pppoe_dev; - struct pppoe_hdr hdr; struct pppoe_hdr *ph; int data_len = skb->len; if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED)) goto abort; - hdr.ver = 1; - hdr.type = 1; - hdr.code = 0; - hdr.sid = po->num; - hdr.length = htons(skb->len); - if (!dev) goto abort; @@ -870,12 +863,17 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb) if (skb_cow(skb, sizeof(*ph) + dev->hard_header_len)) goto abort; - ph = (struct pppoe_hdr *) skb_push(skb, sizeof(struct pppoe_hdr)); - memcpy(ph, &hdr, sizeof(struct pppoe_hdr)); - skb->protocol = __constant_htons(ETH_P_PPP_SES); - + __skb_push(skb, sizeof(*ph)); skb_reset_network_header(skb); + ph = pppoe_hdr(skb); + ph->ver = 1; + ph->type = 1; + ph->code = 0; + ph->sid = po->num; + ph->length = htons(data_len); + + skb->protocol = __constant_htons(ETH_P_PPP_SES); skb->dev = dev; dev->hard_header(skb, dev, ETH_P_PPP_SES, -- cgit v1.2.1 From d9cc20484e5e48c6a5deb4387c20fd45bfbdde8c Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Sun, 16 Sep 2007 16:21:16 -0700 Subject: [NET] skbuff: Add skb_cow_head This patch adds an optimised version of skb_cow that avoids the copy if the header can be modified even if the rest of the payload is cloned. This can be used in encapsulating paths where we only need to modify the header. As it is, this can be used in PPPOE and bridging. Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- drivers/net/pppoe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c index bac36546e0bf..0d7f570b9a54 100644 --- a/drivers/net/pppoe.c +++ b/drivers/net/pppoe.c @@ -860,7 +860,7 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb) /* Copy the data if there is no space for the header or if it's * read-only. */ - if (skb_cow(skb, sizeof(*ph) + dev->hard_header_len)) + if (skb_cow_head(skb, sizeof(*ph) + dev->hard_header_len)) goto abort; __skb_push(skb, sizeof(*ph)); -- cgit v1.2.1 From 7b797d5b150775d717cb03b5ada28b8bad99afab Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Sun, 16 Sep 2007 16:21:42 -0700 Subject: [PPP] generic: Call skb_cow_head before scribbling over skb It's rude to write over data that other people are still using. So call skb_cow_head before PPP proceeds to modify the skb data. Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- drivers/net/ppp_generic.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) (limited to 'drivers') diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c index 9293c82ef2af..7e21342becb2 100644 --- a/drivers/net/ppp_generic.c +++ b/drivers/net/ppp_generic.c @@ -899,17 +899,9 @@ ppp_start_xmit(struct sk_buff *skb, struct net_device *dev) /* Put the 2-byte PPP protocol number on the front, making sure there is room for the address and control fields. */ - if (skb_headroom(skb) < PPP_HDRLEN) { - struct sk_buff *ns; - - ns = alloc_skb(skb->len + dev->hard_header_len, GFP_ATOMIC); - if (ns == 0) - goto outf; - skb_reserve(ns, dev->hard_header_len); - skb_copy_bits(skb, 0, skb_put(ns, skb->len), skb->len); - kfree_skb(skb); - skb = ns; - } + if (skb_cow_head(skb, PPP_HDRLEN)) + goto outf; + pp = skb_push(skb, 2); proto = npindex_to_proto[npi]; pp[0] = proto >> 8; -- cgit v1.2.1 From 2a38b775b77f99308a4e571c13d908df78ac5e57 Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Sun, 16 Sep 2007 16:22:13 -0700 Subject: [PPP] generic: Fix receive path data clobbering & non-linear handling This patch adds missing pskb_may_pull calls to deal with non-linear packets that may arrive from pppoe or pppol2tp. It also copies cloned packets before writing over them. Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- drivers/net/ppp_generic.c | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-) (limited to 'drivers') diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c index 7e21342becb2..4b49d0e8c7eb 100644 --- a/drivers/net/ppp_generic.c +++ b/drivers/net/ppp_generic.c @@ -1525,7 +1525,7 @@ ppp_input_error(struct ppp_channel *chan, int code) static void ppp_receive_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch) { - if (skb->len >= 2) { + if (pskb_may_pull(skb, 2)) { #ifdef CONFIG_PPP_MULTILINK /* XXX do channel-level decompression here */ if (PPP_PROTO(skb) == PPP_MP) @@ -1577,7 +1577,7 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb) if (ppp->vj == 0 || (ppp->flags & SC_REJ_COMP_TCP)) goto err; - if (skb_tailroom(skb) < 124) { + if (skb_tailroom(skb) < 124 || skb_cloned(skb)) { /* copy to a new sk_buff with more tailroom */ ns = dev_alloc_skb(skb->len + 128); if (ns == 0) { @@ -1648,23 +1648,29 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb) /* check if the packet passes the pass and active filters */ /* the filter instructions are constructed assuming a four-byte PPP header on each packet */ - *skb_push(skb, 2) = 0; - if (ppp->pass_filter - && sk_run_filter(skb, ppp->pass_filter, - ppp->pass_len) == 0) { - if (ppp->debug & 1) - printk(KERN_DEBUG "PPP: inbound frame not passed\n"); - kfree_skb(skb); - return; - } - if (!(ppp->active_filter - && sk_run_filter(skb, ppp->active_filter, - ppp->active_len) == 0)) - ppp->last_recv = jiffies; - skb_pull(skb, 2); -#else - ppp->last_recv = jiffies; + if (ppp->pass_filter || ppp->active_filter) { + if (skb_cloned(skb) && + pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) + goto err; + + *skb_push(skb, 2) = 0; + if (ppp->pass_filter + && sk_run_filter(skb, ppp->pass_filter, + ppp->pass_len) == 0) { + if (ppp->debug & 1) + printk(KERN_DEBUG "PPP: inbound frame " + "not passed\n"); + kfree_skb(skb); + return; + } + if (!(ppp->active_filter + && sk_run_filter(skb, ppp->active_filter, + ppp->active_len) == 0)) + ppp->last_recv = jiffies; + __skb_pull(skb, 2); + } else #endif /* CONFIG_PPP_FILTER */ + ppp->last_recv = jiffies; if ((ppp->dev->flags & IFF_UP) == 0 || ppp->npmode[npi] != NPMODE_PASS) { @@ -1762,7 +1768,7 @@ ppp_receive_mp_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch) struct channel *ch; int mphdrlen = (ppp->flags & SC_MP_SHORTSEQ)? MPHDRLEN_SSN: MPHDRLEN; - if (!pskb_may_pull(skb, mphdrlen) || ppp->mrru == 0) + if (!pskb_may_pull(skb, mphdrlen + 1) || ppp->mrru == 0) goto err; /* no good, throw it away */ /* Decode sequence number and begin/end bits */ -- cgit v1.2.1