* [PATCH 1/7] [PPP] pppoe: Fix skb_unshare_check call position
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
@ 2007-08-31 9:11 ` Herbert Xu
2007-08-31 9:11 ` [PATCH 2/7] [PPP] pppoe: Fix data clobbering in __pppoe_xmit and return value Herbert Xu
` (16 subsequent siblings)
17 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-08-31 9:11 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[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 <herbert@gondor.apana.org.au>
---
drivers/net/pppoe.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index 68631a5..5ac3eff 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);
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 2/7] [PPP] pppoe: Fix data clobbering in __pppoe_xmit and return value
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
2007-08-31 9:11 ` [PATCH 1/7] [PPP] pppoe: Fix skb_unshare_check call position Herbert Xu
@ 2007-08-31 9:11 ` Herbert Xu
2007-08-31 9:11 ` [PATCH 3/7] [PPP] pppoe: Fill in header directly in __pppoe_xmit Herbert Xu
` (15 subsequent siblings)
17 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-08-31 9:11 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[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 <herbert@gondor.apana.org.au>
---
drivers/net/pppoe.c | 50 +++++++++++++-------------------------------------
1 files changed, 13 insertions(+), 37 deletions(-)
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index 5ac3eff..8818253 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;
}
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 3/7] [PPP] pppoe: Fill in header directly in __pppoe_xmit
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
2007-08-31 9:11 ` [PATCH 1/7] [PPP] pppoe: Fix skb_unshare_check call position Herbert Xu
2007-08-31 9:11 ` [PATCH 2/7] [PPP] pppoe: Fix data clobbering in __pppoe_xmit and return value Herbert Xu
@ 2007-08-31 9:11 ` Herbert Xu
2007-08-31 9:11 ` [PATCH 4/7] [BRIDGE]: Kill clone argument to br_flood_* Herbert Xu
` (14 subsequent siblings)
17 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-08-31 9:11 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[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 <herbert@gondor.apana.org.au>
---
drivers/net/pppoe.c | 20 +++++++++-----------
1 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index 8818253..bac3654 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,
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 4/7] [BRIDGE]: Kill clone argument to br_flood_*
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
` (2 preceding siblings ...)
2007-08-31 9:11 ` [PATCH 3/7] [PPP] pppoe: Fill in header directly in __pppoe_xmit Herbert Xu
@ 2007-08-31 9:11 ` Herbert Xu
2007-08-31 9:11 ` [PATCH 5/7] [NET] skbuff: Add skb_cow_head Herbert Xu
` (13 subsequent siblings)
17 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-08-31 9:11 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[BRIDGE]: Kill clone argument to br_flood_*
The clone argument is only used by one caller and that caller can clone
the packet itself. This patch moves the clone call into the caller and
kills the clone argument.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
net/bridge/br_device.c | 4 ++--
net/bridge/br_forward.c | 21 +++++----------------
net/bridge/br_input.c | 48 ++++++++++++++++++++++--------------------------
net/bridge/br_private.h | 8 ++------
4 files changed, 31 insertions(+), 50 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 0eded17..99292e8 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -41,11 +41,11 @@ int br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
skb_pull(skb, ETH_HLEN);
if (dest[0] & 1)
- br_flood_deliver(br, skb, 0);
+ br_flood_deliver(br, skb);
else if ((dst = __br_fdb_get(br, dest)) != NULL)
br_deliver(dst->dst, skb);
else
- br_flood_deliver(br, skb, 0);
+ br_flood_deliver(br, skb);
return 0;
}
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index ada7f49..bdd7c35 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -100,24 +100,13 @@ void br_forward(const struct net_bridge_port *to, struct sk_buff *skb)
}
/* called under bridge lock */
-static void br_flood(struct net_bridge *br, struct sk_buff *skb, int clone,
+static void br_flood(struct net_bridge *br, struct sk_buff *skb,
void (*__packet_hook)(const struct net_bridge_port *p,
struct sk_buff *skb))
{
struct net_bridge_port *p;
struct net_bridge_port *prev;
- if (clone) {
- struct sk_buff *skb2;
-
- if ((skb2 = skb_clone(skb, GFP_ATOMIC)) == NULL) {
- br->statistics.tx_dropped++;
- return;
- }
-
- skb = skb2;
- }
-
prev = NULL;
list_for_each_entry_rcu(p, &br->port_list, list) {
@@ -148,13 +137,13 @@ static void br_flood(struct net_bridge *br, struct sk_buff *skb, int clone,
/* called with rcu_read_lock */
-void br_flood_deliver(struct net_bridge *br, struct sk_buff *skb, int clone)
+void br_flood_deliver(struct net_bridge *br, struct sk_buff *skb)
{
- br_flood(br, skb, clone, __br_deliver);
+ br_flood(br, skb, __br_deliver);
}
/* called under bridge lock */
-void br_flood_forward(struct net_bridge *br, struct sk_buff *skb, int clone)
+void br_flood_forward(struct net_bridge *br, struct sk_buff *skb)
{
- br_flood(br, skb, clone, __br_forward);
+ br_flood(br, skb, __br_forward);
}
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 5c18595..069a4e1 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -43,7 +43,7 @@ int br_handle_frame_finish(struct sk_buff *skb)
struct net_bridge_port *p = rcu_dereference(skb->dev->br_port);
struct net_bridge *br;
struct net_bridge_fdb_entry *dst;
- int passedup = 0;
+ struct sk_buff *skb2;
if (!p || p->state == BR_STATE_DISABLED)
goto drop;
@@ -55,39 +55,35 @@ int br_handle_frame_finish(struct sk_buff *skb)
if (p->state == BR_STATE_LEARNING)
goto drop;
- if (br->dev->flags & IFF_PROMISC) {
- struct sk_buff *skb2;
+ /* The packet skb2 goes to the local host (NULL to skip). */
+ skb2 = NULL;
- skb2 = skb_clone(skb, GFP_ATOMIC);
- if (skb2 != NULL) {
- passedup = 1;
- br_pass_frame_up(br, skb2);
- }
- }
+ if (br->dev->flags & IFF_PROMISC)
+ skb2 = skb;
+
+ dst = NULL;
if (is_multicast_ether_addr(dest)) {
br->statistics.multicast++;
- br_flood_forward(br, skb, !passedup);
- if (!passedup)
- br_pass_frame_up(br, skb);
- goto out;
+ skb2 = skb;
+ } else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
+ skb2 = skb;
+ /* Do not forward the packet since it's local. */
+ skb = NULL;
}
- dst = __br_fdb_get(br, dest);
- if (dst != NULL && dst->is_local) {
- if (!passedup)
- br_pass_frame_up(br, skb);
- else
- kfree_skb(skb);
- goto out;
- }
+ if (skb2 == skb)
+ skb2 = skb_clone(skb, GFP_ATOMIC);
- if (dst != NULL) {
- br_forward(dst->dst, skb);
- goto out;
- }
+ if (skb2)
+ br_pass_frame_up(br, skb2);
- br_flood_forward(br, skb, 0);
+ if (skb) {
+ if (dst)
+ br_forward(dst->dst, skb);
+ else
+ br_flood_forward(br, skb);
+ }
out:
return 0;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 21bf3a9..e6dc6f5 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -170,12 +170,8 @@ extern int br_dev_queue_push_xmit(struct sk_buff *skb);
extern void br_forward(const struct net_bridge_port *to,
struct sk_buff *skb);
extern int br_forward_finish(struct sk_buff *skb);
-extern void br_flood_deliver(struct net_bridge *br,
- struct sk_buff *skb,
- int clone);
-extern void br_flood_forward(struct net_bridge *br,
- struct sk_buff *skb,
- int clone);
+extern void br_flood_deliver(struct net_bridge *br, struct sk_buff *skb);
+extern void br_flood_forward(struct net_bridge *br, struct sk_buff *skb);
/* br_if.c */
extern void br_port_carrier_check(struct net_bridge_port *p);
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 5/7] [NET] skbuff: Add skb_cow_head
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
` (3 preceding siblings ...)
2007-08-31 9:11 ` [PATCH 4/7] [BRIDGE]: Kill clone argument to br_flood_* Herbert Xu
@ 2007-08-31 9:11 ` Herbert Xu
2007-08-31 9:11 ` [PATCH 6/7] [PPP] generic: Call skb_cow_head before scribbling over skb Herbert Xu
` (12 subsequent siblings)
17 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-08-31 9:11 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[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 <herbert@gondor.apana.org.au>
---
drivers/net/pppoe.c | 2 +-
include/linux/skbuff.h | 40 +++++++++++++++++++++++++++++++---------
net/bridge/br_netfilter.c | 2 +-
3 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index bac3654..0d7f570 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));
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 93c27f7..a656cec 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1352,6 +1352,22 @@ static inline int skb_clone_writable(struct sk_buff *skb, int len)
skb_headroom(skb) + len <= skb->hdr_len;
}
+static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom,
+ int cloned)
+{
+ int delta = 0;
+
+ if (headroom < NET_SKB_PAD)
+ headroom = NET_SKB_PAD;
+ if (headroom > skb_headroom(skb))
+ delta = headroom - skb_headroom(skb);
+
+ if (delta || cloned)
+ return pskb_expand_head(skb, ALIGN(delta, NET_SKB_PAD), 0,
+ GFP_ATOMIC);
+ return 0;
+}
+
/**
* skb_cow - copy header of skb when it is required
* @skb: buffer to cow
@@ -1366,16 +1382,22 @@ static inline int skb_clone_writable(struct sk_buff *skb, int len)
*/
static inline int skb_cow(struct sk_buff *skb, unsigned int headroom)
{
- int delta = (headroom > NET_SKB_PAD ? headroom : NET_SKB_PAD) -
- skb_headroom(skb);
-
- if (delta < 0)
- delta = 0;
+ return __skb_cow(skb, headroom, skb_cloned(skb));
+}
- if (delta || skb_cloned(skb))
- return pskb_expand_head(skb, (delta + (NET_SKB_PAD-1)) &
- ~(NET_SKB_PAD-1), 0, GFP_ATOMIC);
- return 0;
+/**
+ * skb_cow_head - skb_cow but only making the head writable
+ * @skb: buffer to cow
+ * @headroom: needed headroom
+ *
+ * This function is identical to skb_cow except that we replace the
+ * skb_cloned check by skb_header_cloned. It should be used when
+ * you only need to push on some header and do not need to modify
+ * the data.
+ */
+static inline int skb_cow_head(struct sk_buff *skb, unsigned int headroom)
+{
+ return __skb_cow(skb, headroom, skb_header_cloned(skb));
}
/**
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 3ee2022..fc13130 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -183,7 +183,7 @@ int nf_bridge_copy_header(struct sk_buff *skb)
int err;
int header_size = ETH_HLEN + nf_bridge_encap_header_len(skb);
- err = skb_cow(skb, header_size);
+ err = skb_cow_head(skb, header_size);
if (err)
return err;
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 6/7] [PPP] generic: Call skb_cow_head before scribbling over skb
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
` (4 preceding siblings ...)
2007-08-31 9:11 ` [PATCH 5/7] [NET] skbuff: Add skb_cow_head Herbert Xu
@ 2007-08-31 9:11 ` Herbert Xu
2007-08-31 9:11 ` [PATCH 7/7] [PPP] generic: Fix receive path data clobbering & non-linear handling Herbert Xu
` (11 subsequent siblings)
17 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-08-31 9:11 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[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 <herbert@gondor.apana.org.au>
---
drivers/net/ppp_generic.c | 14 +++-----------
1 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
index 9293c82..7e21342 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;
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 7/7] [PPP] generic: Fix receive path data clobbering & non-linear handling
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
` (5 preceding siblings ...)
2007-08-31 9:11 ` [PATCH 6/7] [PPP] generic: Call skb_cow_head before scribbling over skb Herbert Xu
@ 2007-08-31 9:11 ` Herbert Xu
2007-08-31 14:02 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Toralf Förster
` (10 subsequent siblings)
17 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-08-31 9:11 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[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 <herbert@gondor.apana.org.au>
---
drivers/net/ppp_generic.c | 44 +++++++++++++++++++++++++-------------------
1 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
index 7e21342..4b49d0e 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 */
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets)
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
` (6 preceding siblings ...)
2007-08-31 9:11 ` [PATCH 7/7] [PPP] generic: Fix receive path data clobbering & non-linear handling Herbert Xu
@ 2007-08-31 14:02 ` Toralf Förster
2007-09-03 9:32 ` Toralf Förster
` (9 subsequent siblings)
17 siblings, 0 replies; 40+ messages in thread
From: Toralf Förster @ 2007-08-31 14:02 UTC (permalink / raw)
To: Herbert Xu
Cc: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
netdev
[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]
Am Freitag, 31. August 2007 schrieb Herbert Xu:
> On Thu, Aug 30, 2007 at 09:51:31AM +0000, James Chapman wrote:
> >
> > The captured PPPoE stream seems to show incorrect data lengths in the
> > PPPoE header for some captured PPPoE packets. The kernel's PPPoE
> > datapath uses this length to extract the PPP frame and send it through
> > to the ppp interface. Since your ppp stream is fine, the actual PPPoE
> > header contents must be correct when it is parsed by the kernel PPPoE
> > code. It seems more likely that this is a wireshark bug to me.
>
> If he were using the kernel pppoe driver, then this is because
> PPP filtering is writing over a cloned skb without copying it.
>
> In fact, there seems to be quite a few bugs of this kind in
> the various ppp*.c files.
>
> Please try the following patches to see if they make a
> difference.
>
> I've audited ppp_generic.c and pppoe.c. I'll do pppol2tp
> tomorrow.
>
> Cheers,
Herbert,
your patches - applied against 2.6.23-rc4-g2d8348b4 - works like a charm :-)
Among many other places at least
http://bugzilla.kernel.org/show_bug.cgi?id=8409
but probably also
http://bugzilla.kernel.org/show_bug.cgi?id=7938 are solved by your 7 patches.
Many thanks
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets)
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
` (7 preceding siblings ...)
2007-08-31 14:02 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Toralf Förster
@ 2007-09-03 9:32 ` Toralf Förster
2007-09-11 18:12 ` Toralf Förster
` (8 subsequent siblings)
17 siblings, 0 replies; 40+ messages in thread
From: Toralf Förster @ 2007-09-03 9:32 UTC (permalink / raw)
To: Herbert Xu
Cc: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
netdev
[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]
Am Freitag, 31. August 2007 schrieb Herbert Xu:
> On Thu, Aug 30, 2007 at 09:51:31AM +0000, James Chapman wrote:
> >
> > The captured PPPoE stream seems to show incorrect data lengths in the
> > PPPoE header for some captured PPPoE packets. The kernel's PPPoE
> > datapath uses this length to extract the PPP frame and send it through
> > to the ppp interface. Since your ppp stream is fine, the actual PPPoE
> > header contents must be correct when it is parsed by the kernel PPPoE
> > code. It seems more likely that this is a wireshark bug to me.
>
> If he were using the kernel pppoe driver, then this is because
> PPP filtering is writing over a cloned skb without copying it.
>
> In fact, there seems to be quite a few bugs of this kind in
> the various ppp*.c files.
>
> Please try the following patches to see if they make a
> difference.
>
> I've audited ppp_generic.c and pppoe.c. I'll do pppol2tp
> tomorrow.
>
> Cheers,
I've applied the patch series onto a Gentoo-2.6.22-r5 kernel and use this kernel
now since some days w/o any problems both at work and at home.
Many thanks.
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets)
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
` (8 preceding siblings ...)
2007-09-03 9:32 ` Toralf Förster
@ 2007-09-11 18:12 ` Toralf Förster
2007-09-19 11:51 ` Herbert Xu
[not found] ` <E1IR2Uj-0007MU-00@gondolin.me.apana.org.au>
` (7 subsequent siblings)
17 siblings, 1 reply; 40+ messages in thread
From: Toralf Förster @ 2007-09-11 18:12 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, Paul Mackerras, James Chapman
[-- Attachment #1.1: Type: text/plain, Size: 2148 bytes --]
Am Freitag, 31. August 2007 schrieb Herbert Xu:
> On Thu, Aug 30, 2007 at 09:51:31AM +0000, James Chapman wrote:
> >
> > The captured PPPoE stream seems to show incorrect data lengths in the
> > PPPoE header for some captured PPPoE packets. The kernel's PPPoE
> > datapath uses this length to extract the PPP frame and send it through
> > to the ppp interface. Since your ppp stream is fine, the actual PPPoE
> > header contents must be correct when it is parsed by the kernel PPPoE
> > code. It seems more likely that this is a wireshark bug to me.
>
> If he were using the kernel pppoe driver, then this is because
> PPP filtering is writing over a cloned skb without copying it.
>
> In fact, there seems to be quite a few bugs of this kind in
> the various ppp*.c files.
>
> Please try the following patches to see if they make a
> difference.
>
> I've audited ppp_generic.c and pppoe.c. I'll do pppol2tp
> tomorrow.
>
> Cheers,
Running a stable Gentoo kernel 2.6.22-gentoo-r5 now for a while there's only
one thing left related to this topic.
I'm wondering why some UDP packets of the MS messenger protocol (with the usual
text like "please click at www.we-destroy-your-computer.com") always have wrong
check sums regardless whether sniffed at ppp0 or eth0 interface.
But from all UDP packets of this (today) useless protocol only those have wrong
check sums which are marked as "[Long frame (2 bytes)]" within wireshark.
And - last but now least - I have defined the following rule for this protocol :
Chain INPUT (policy DROP 0 packets, 0 bytes)
num pkts bytes target prot opt in out source destination
...
8 1 485 DROP udp -- any any anywhere anywhere multiport dports 1026,1027
and this kernel options :
n22 ~ # zgrep ^CONFIG_PPP /proc/config.gz
CONFIG_PPP=m
CONFIG_PPP_FILTER=y
CONFIG_PPPOE=m
and I'm wondering why it is still possible to capture such packets at eth0.
Thanks for an answer.
--
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3
[-- Attachment #1.2: messenger_ethereal_eth0.pcap --]
[-- Type: application/octet-stream, Size: 3934 bytes --]
[-- Attachment #1.3: messenger_tcpdump_ppp0.pcap --]
[-- Type: application/octet-stream, Size: 3886 bytes --]
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread[parent not found: <E1IR2Uj-0007MU-00@gondolin.me.apana.org.au>]
[parent not found: <E1IR2Uy-0007Ms-00@gondolin.me.apana.org.au>]
* Re: [PATCH 2/7] [PPP] pppoe: Fix data clobbering in __pppoe_xmit and return value
[not found] ` <E1IR2Uy-0007Ms-00@gondolin.me.apana.org.au>
@ 2007-09-16 23:20 ` David Miller
0 siblings, 0 replies; 40+ messages in thread
From: David Miller @ 2007-09-16 23:20 UTC (permalink / raw)
To: herbert; +Cc: jchapman, mostrows, paulus, toralf.foerster, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 31 Aug 2007 17:09:04 +0800
> [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 <herbert@gondor.apana.org.au>
Applied, thanks Herbert.
^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <E1IR2V6-0007N6-00@gondolin.me.apana.org.au>]
* Re: [PATCH 3/7] [PPP] pppoe: Fill in header directly in __pppoe_xmit
[not found] ` <E1IR2V6-0007N6-00@gondolin.me.apana.org.au>
@ 2007-09-16 23:20 ` David Miller
0 siblings, 0 replies; 40+ messages in thread
From: David Miller @ 2007-09-16 23:20 UTC (permalink / raw)
To: herbert; +Cc: jchapman, mostrows, paulus, toralf.foerster, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 31 Aug 2007 17:09:12 +0800
> [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 <herbert@gondor.apana.org.au>
Applied, thanks Herbert.
^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <E1IR2V8-0007NE-00@gondolin.me.apana.org.au>]
* Re: [PATCH 4/7] [BRIDGE]: Kill clone argument to br_flood_*
[not found] ` <E1IR2V8-0007NE-00@gondolin.me.apana.org.au>
@ 2007-09-16 23:21 ` David Miller
0 siblings, 0 replies; 40+ messages in thread
From: David Miller @ 2007-09-16 23:21 UTC (permalink / raw)
To: herbert; +Cc: jchapman, mostrows, paulus, toralf.foerster, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 31 Aug 2007 17:09:14 +0800
> [BRIDGE]: Kill clone argument to br_flood_*
>
> The clone argument is only used by one caller and that caller can clone
> the packet itself. This patch moves the clone call into the caller and
> kills the clone argument.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied, thanks Herbert.
^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <E1IR2V9-0007NM-00@gondolin.me.apana.org.au>]
* Re: [PATCH 5/7] [NET] skbuff: Add skb_cow_head
[not found] ` <E1IR2V9-0007NM-00@gondolin.me.apana.org.au>
@ 2007-09-16 23:21 ` David Miller
0 siblings, 0 replies; 40+ messages in thread
From: David Miller @ 2007-09-16 23:21 UTC (permalink / raw)
To: herbert; +Cc: jchapman, mostrows, paulus, toralf.foerster, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 31 Aug 2007 17:09:15 +0800
> [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 <herbert@gondor.apana.org.au>
Applied, thanks Herbert.
^ permalink raw reply [flat|nested] 40+ messages in thread
[parent not found: <E1IR2VA-0007NU-00@gondolin.me.apana.org.au>]
[parent not found: <E1IR2VB-0007Nc-00@gondolin.me.apana.org.au>]
* Re: [PATCH 7/7] [PPP] generic: Fix receive path data clobbering & non-linear handling
[not found] ` <E1IR2VB-0007Nc-00@gondolin.me.apana.org.au>
@ 2007-09-16 23:22 ` David Miller
0 siblings, 0 replies; 40+ messages in thread
From: David Miller @ 2007-09-16 23:22 UTC (permalink / raw)
To: herbert; +Cc: jchapman, mostrows, paulus, toralf.foerster, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 31 Aug 2007 17:09:17 +0800
> [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 <herbert@gondor.apana.org.au>
Applied, thanks Herbert.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [0/3] [PPP]: Fix pppol2tp skb bugs
2007-08-31 9:06 ` [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets) Herbert Xu
` (16 preceding siblings ...)
[not found] ` <E1IR2VB-0007Nc-00@gondolin.me.apana.org.au>
@ 2007-09-18 12:04 ` Herbert Xu
2007-09-18 12:08 ` [PATCH 1/3] [PPP] L2TP: Disallow non-UDP datagram sockets Herbert Xu
` (6 more replies)
17 siblings, 7 replies; 40+ messages in thread
From: Herbert Xu @ 2007-09-18 12:04 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras
Cc: Toralf Förster, netdev
On Fri, Aug 31, 2007 at 05:06:25PM +0800, Herbert Xu wrote:
>
> I've audited ppp_generic.c and pppoe.c. I'll do pppol2tp
> tomorrow.
Tomrrow took a while to come :)
Here are the fixes for pppol2tp.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 40+ messages in thread* [PATCH 1/3] [PPP] L2TP: Disallow non-UDP datagram sockets
2007-09-18 12:04 ` [0/3] [PPP]: Fix pppol2tp skb bugs Herbert Xu
@ 2007-09-18 12:08 ` Herbert Xu
2007-09-18 12:08 ` [PATCH 2/3] [PPP] L2TP: Fix skb handling in pppol2tp_recv_core Herbert Xu
` (5 subsequent siblings)
6 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-09-18 12:08 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[PPP] L2TP: Disallow non-UDP datagram sockets
With the addition of UDP-Lite we need to refine the socket check so that
only genuine UDP sockets are allowed through.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
drivers/net/pppol2tp.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c
index 266e8b3..ed8ead4 100644
--- a/drivers/net/pppol2tp.c
+++ b/drivers/net/pppol2tp.c
@@ -1326,12 +1326,14 @@ static struct sock *pppol2tp_prepare_tunnel_socket(int fd, u16 tunnel_id,
goto err;
}
+ sk = sock->sk;
+
/* Quick sanity checks */
- err = -ESOCKTNOSUPPORT;
- if (sock->type != SOCK_DGRAM) {
+ err = -EPROTONOSUPPORT;
+ if (sk->sk_protocol != IPPROTO_UDP) {
PRINTK(-1, PPPOL2TP_MSG_CONTROL, KERN_ERR,
- "tunl %hu: fd %d wrong type, got %d, expected %d\n",
- tunnel_id, fd, sock->type, SOCK_DGRAM);
+ "tunl %hu: fd %d wrong protocol, got %d, expected %d\n",
+ tunnel_id, fd, sk->sk_protocol, IPPROTO_UDP);
goto err;
}
err = -EAFNOSUPPORT;
@@ -1343,7 +1345,6 @@ static struct sock *pppol2tp_prepare_tunnel_socket(int fd, u16 tunnel_id,
}
err = -ENOTCONN;
- sk = sock->sk;
/* Check if this socket has already been prepped */
tunnel = (struct pppol2tp_tunnel *)sk->sk_user_data;
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 2/3] [PPP] L2TP: Fix skb handling in pppol2tp_recv_core
2007-09-18 12:04 ` [0/3] [PPP]: Fix pppol2tp skb bugs Herbert Xu
2007-09-18 12:08 ` [PATCH 1/3] [PPP] L2TP: Disallow non-UDP datagram sockets Herbert Xu
@ 2007-09-18 12:08 ` Herbert Xu
2007-09-18 12:08 ` [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit Herbert Xu
` (4 subsequent siblings)
6 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-09-18 12:08 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[PPP] L2TP: Fix skb handling in pppol2tp_recv_core
The function pppol2tp_recv_core doesn't handle non-linear packets properly.
It also fails to check the remote offset field.
This patch fixes these problems. It also removes an unnecessary check on
the UDP header which has already been performed by the UDP layer.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
drivers/net/pppol2tp.c | 44 ++++++++++++++++++++++++--------------------
1 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c
index ed8ead4..440e190 100644
--- a/drivers/net/pppol2tp.c
+++ b/drivers/net/pppol2tp.c
@@ -491,44 +491,46 @@ static int pppol2tp_recv_core(struct sock *sock, struct sk_buff *skb)
u16 hdrflags;
u16 tunnel_id, session_id;
int length;
- struct udphdr *uh;
+ int offset;
tunnel = pppol2tp_sock_to_tunnel(sock);
if (tunnel == NULL)
goto error;
+ /* UDP always verifies the packet length. */
+ __skb_pull(skb, sizeof(struct udphdr));
+
/* Short packet? */
- if (skb->len < sizeof(struct udphdr)) {
+ if (!pskb_may_pull(skb, 12)) {
PRINTK(tunnel->debug, PPPOL2TP_MSG_DATA, KERN_INFO,
"%s: recv short packet (len=%d)\n", tunnel->name, skb->len);
goto error;
}
/* Point to L2TP header */
- ptr = skb->data + sizeof(struct udphdr);
+ ptr = skb->data;
/* Get L2TP header flags */
hdrflags = ntohs(*(__be16*)ptr);
/* Trace packet contents, if enabled */
if (tunnel->debug & PPPOL2TP_MSG_DATA) {
+ length = min(16u, skb->len);
+ if (!pskb_may_pull(skb, length))
+ goto error;
+
printk(KERN_DEBUG "%s: recv: ", tunnel->name);
- for (length = 0; length < 16; length++)
- printk(" %02X", ptr[length]);
+ offset = 0;
+ do {
+ printk(" %02X", ptr[offset]);
+ } while (++offset < length);
+
printk("\n");
}
/* Get length of L2TP packet */
- uh = (struct udphdr *) skb_transport_header(skb);
- length = ntohs(uh->len) - sizeof(struct udphdr);
-
- /* Too short? */
- if (length < 12) {
- PRINTK(tunnel->debug, PPPOL2TP_MSG_DATA, KERN_INFO,
- "%s: recv short L2TP packet (len=%d)\n", tunnel->name, length);
- goto error;
- }
+ length = skb->len;
/* If type is control packet, it is handled by userspace. */
if (hdrflags & L2TP_HDRFLAG_T) {
@@ -606,7 +608,6 @@ static int pppol2tp_recv_core(struct sock *sock, struct sk_buff *skb)
"%s: recv data has no seq numbers when required. "
"Discarding\n", session->name);
session->stats.rx_seq_discards++;
- session->stats.rx_errors++;
goto discard;
}
@@ -625,7 +626,6 @@ static int pppol2tp_recv_core(struct sock *sock, struct sk_buff *skb)
"%s: recv data has no seq numbers when required. "
"Discarding\n", session->name);
session->stats.rx_seq_discards++;
- session->stats.rx_errors++;
goto discard;
}
@@ -634,10 +634,14 @@ static int pppol2tp_recv_core(struct sock *sock, struct sk_buff *skb)
}
/* If offset bit set, skip it. */
- if (hdrflags & L2TP_HDRFLAG_O)
- ptr += 2 + ntohs(*(__be16 *) ptr);
+ if (hdrflags & L2TP_HDRFLAG_O) {
+ offset = ntohs(*(__be16 *)ptr);
+ skb->transport_header += 2 + offset;
+ if (!pskb_may_pull(skb, skb_transport_offset(skb) + 2))
+ goto discard;
+ }
- skb_pull(skb, ptr - skb->data);
+ __skb_pull(skb, skb_transport_offset(skb));
/* Skip PPP header, if present. In testing, Microsoft L2TP clients
* don't send the PPP header (PPP header compression enabled), but
@@ -673,7 +677,6 @@ static int pppol2tp_recv_core(struct sock *sock, struct sk_buff *skb)
*/
if (PPPOL2TP_SKB_CB(skb)->ns != session->nr) {
session->stats.rx_seq_discards++;
- session->stats.rx_errors++;
PRINTK(session->debug, PPPOL2TP_MSG_SEQ, KERN_DEBUG,
"%s: oos pkt %hu len %d discarded, "
"waiting for %hu, reorder_q_len=%d\n",
@@ -698,6 +701,7 @@ static int pppol2tp_recv_core(struct sock *sock, struct sk_buff *skb)
return 0;
discard:
+ session->stats.rx_errors++;
kfree_skb(skb);
sock_put(session->sock);
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-18 12:04 ` [0/3] [PPP]: Fix pppol2tp skb bugs Herbert Xu
2007-09-18 12:08 ` [PATCH 1/3] [PPP] L2TP: Disallow non-UDP datagram sockets Herbert Xu
2007-09-18 12:08 ` [PATCH 2/3] [PPP] L2TP: Fix skb handling in pppol2tp_recv_core Herbert Xu
@ 2007-09-18 12:08 ` Herbert Xu
[not found] ` <E1IXbqW-0002OB-00@gondolin.me.apana.org.au>
` (3 subsequent siblings)
6 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-09-18 12:08 UTC (permalink / raw)
To: James Chapman, David S. Miller, Michal Ostrowski, Paul Mackerras,
Herbert Xu
[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 <herbert@gondor.apana.org.au>
---
drivers/net/pppol2tp.c | 60 +++++++++++++++++--------------------------------
1 files changed, 21 insertions(+), 39 deletions(-)
diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c
index 440e190..e259f45 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) {
@@ -1078,16 +1066,10 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
}
/* Free the original skb */
+abort:
kfree_skb(skb);
return 1;
-
-discard:
- /* Free the new skb. Caller will free original skb. */
- if (skb2 != skb)
- kfree_skb(skb2);
-abort:
- return 0;
}
/*****************************************************************************
^ permalink raw reply related [flat|nested] 40+ messages in thread[parent not found: <E1IXbqW-0002OB-00@gondolin.me.apana.org.au>]
[parent not found: <E1IXbqo-0002OW-00@gondolin.me.apana.org.au>]
* Re: [PATCH 2/3] [PPP] L2TP: Fix skb handling in pppol2tp_recv_core
[not found] ` <E1IXbqo-0002OW-00@gondolin.me.apana.org.au>
@ 2007-09-18 20:17 ` James Chapman
0 siblings, 0 replies; 40+ messages in thread
From: James Chapman @ 2007-09-18 20:17 UTC (permalink / raw)
To: Herbert Xu
Cc: David S. Miller, Michal Ostrowski, Paul Mackerras,
Toralf Förster, netdev
Herbert Xu wrote:
> [PPP] L2TP: Fix skb handling in pppol2tp_recv_core
>
> The function pppol2tp_recv_core doesn't handle non-linear packets properly.
> It also fails to check the remote offset field.
>
> This patch fixes these problems. It also removes an unnecessary check on
> the UDP header which has already been performed by the UDP layer.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Acked-by: James Chapman <jchapman@katalix.com>
--
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [0/3] [PPP]: Fix pppol2tp skb bugs
2007-09-18 12:04 ` [0/3] [PPP]: Fix pppol2tp skb bugs Herbert Xu
` (4 preceding siblings ...)
[not found] ` <E1IXbqo-0002OW-00@gondolin.me.apana.org.au>
@ 2007-09-18 20:17 ` David Miller
[not found] ` <E1IXbqs-0002Od-00@gondolin.me.apana.org.au>
6 siblings, 0 replies; 40+ messages in thread
From: David Miller @ 2007-09-18 20:17 UTC (permalink / raw)
To: herbert; +Cc: jchapman, mostrows, paulus, toralf.foerster, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 18 Sep 2007 20:04:38 +0800
> On Fri, Aug 31, 2007 at 05:06:25PM +0800, Herbert Xu wrote:
> >
> > I've audited ppp_generic.c and pppoe.c. I'll do pppol2tp
> > tomorrow.
>
> Tomrrow took a while to come :)
It took me two weeks to apply the original patch set so you
get a reprieve too :-)
> Here are the fixes for pppol2tp.
I'll apply these, thanks Herbert.
^ permalink raw reply [flat|nested] 40+ messages in thread[parent not found: <E1IXbqs-0002Od-00@gondolin.me.apana.org.au>]
* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
[not found] ` <E1IXbqs-0002Od-00@gondolin.me.apana.org.au>
@ 2007-09-18 20:19 ` James Chapman
2007-09-18 20:32 ` David Miller
2007-09-19 1:25 ` Herbert Xu
0 siblings, 2 replies; 40+ messages in thread
From: James Chapman @ 2007-09-18 20:19 UTC (permalink / raw)
To: Herbert Xu
Cc: David S. Miller, Michal Ostrowski, Paul Mackerras,
Toralf Förster, netdev
Herbert Xu wrote:
> [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 <herbert@gondor.apana.org.au>
This one causes my test system to lock up. I'll investigate. Please
don't apply this patch for now.
--
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-18 20:19 ` [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit James Chapman
@ 2007-09-18 20:32 ` David Miller
2007-09-19 1:30 ` Herbert Xu
2007-09-19 1:25 ` Herbert Xu
1 sibling, 1 reply; 40+ messages in thread
From: David Miller @ 2007-09-18 20:32 UTC (permalink / raw)
To: jchapman; +Cc: herbert, mostrows, paulus, toralf.foerster, netdev
From: James Chapman <jchapman@katalix.com>
Date: Tue, 18 Sep 2007 21:19:33 +0100
> This one causes my test system to lock up. I'll investigate. Please
> don't apply this patch for now.
I'll make sure not to push this until we figure out what's
wrong, thanks for checking James.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-18 20:32 ` David Miller
@ 2007-09-19 1:30 ` Herbert Xu
2007-09-19 17:45 ` David Miller
0 siblings, 1 reply; 40+ messages in thread
From: Herbert Xu @ 2007-09-19 1:30 UTC (permalink / raw)
To: David Miller; +Cc: jchapman, mostrows, paulus, toralf.foerster, netdev
On Tue, Sep 18, 2007 at 01:32:40PM -0700, David Miller wrote:
>
> I'll make sure not to push this until we figure out what's
> wrong, thanks for checking James.
I think it was because of a double-free that I created after
transmitting the packet. In fact I had the same bug in PPPOE
too.
[PPP] pppoe: Fix double-free on skb after transmit failure
When I got rid of the second packet in __pppoe_xmit I created
a double-free on the skb because of the goto abort on failure.
This patch removes that.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index 0d7f570..9b30cd6 100644
--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -879,8 +879,7 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb)
dev->hard_header(skb, dev, ETH_P_PPP_SES,
po->pppoe_pa.remote, NULL, data_len);
- if (dev_queue_xmit(skb) < 0)
- goto abort;
+ dev_queue_xmit(skb);
return 1;
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-19 1:30 ` Herbert Xu
@ 2007-09-19 17:45 ` David Miller
2007-09-19 23:11 ` Adrian Bunk
0 siblings, 1 reply; 40+ messages in thread
From: David Miller @ 2007-09-19 17:45 UTC (permalink / raw)
To: herbert; +Cc: jchapman, mostrows, paulus, toralf.foerster, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 19 Sep 2007 09:30:18 +0800
> [PPP] pppoe: Fix double-free on skb after transmit failure
>
> When I got rid of the second packet in __pppoe_xmit I created
> a double-free on the skb because of the goto abort on failure.
> This patch removes that.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-19 17:45 ` David Miller
@ 2007-09-19 23:11 ` Adrian Bunk
2007-09-19 23:12 ` David Miller
0 siblings, 1 reply; 40+ messages in thread
From: Adrian Bunk @ 2007-09-19 23:11 UTC (permalink / raw)
To: David Miller; +Cc: herbert, jchapman, mostrows, paulus, toralf.foerster, netdev
On Wed, Sep 19, 2007 at 10:45:13AM -0700, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 19 Sep 2007 09:30:18 +0800
>
> > [PPP] pppoe: Fix double-free on skb after transmit failure
> >
> > When I got rid of the second packet in __pppoe_xmit I created
> > a double-free on the skb because of the goto abort on failure.
> > This patch removes that.
> >
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Applied.
Please excuse in case this was already clear, but since this regression
made it into Linus' tree it should be fixed there before 2.6.23.
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-19 23:11 ` Adrian Bunk
@ 2007-09-19 23:12 ` David Miller
0 siblings, 0 replies; 40+ messages in thread
From: David Miller @ 2007-09-19 23:12 UTC (permalink / raw)
To: bunk; +Cc: herbert, jchapman, mostrows, paulus, toralf.foerster, netdev
From: Adrian Bunk <bunk@kernel.org>
Date: Thu, 20 Sep 2007 01:11:14 +0200
> On Wed, Sep 19, 2007 at 10:45:13AM -0700, David Miller wrote:
> > From: Herbert Xu <herbert@gondor.apana.org.au>
> > Date: Wed, 19 Sep 2007 09:30:18 +0800
> >
> > > [PPP] pppoe: Fix double-free on skb after transmit failure
> > >
> > > When I got rid of the second packet in __pppoe_xmit I created
> > > a double-free on the skb because of the goto abort on failure.
> > > This patch removes that.
> > >
> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> >
> > Applied.
>
> Please excuse in case this was already clear, but since this regression
> made it into Linus' tree it should be fixed there before 2.6.23.
I will make sure this happens.
I'm just waiting for Alexey to test the SFQ crash fix and
then I'll send everything queued.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-18 20:19 ` [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit James Chapman
2007-09-18 20:32 ` David Miller
@ 2007-09-19 1:25 ` Herbert Xu
2007-09-19 8:43 ` James Chapman
2007-09-19 17:47 ` David Miller
1 sibling, 2 replies; 40+ messages in thread
From: Herbert Xu @ 2007-09-19 1:25 UTC (permalink / raw)
To: James Chapman
Cc: David S. Miller, Michal Ostrowski, Paul Mackerras,
Toralf Förster, netdev
On Tue, Sep 18, 2007 at 09:19:33PM +0100, James Chapman wrote:
>
> This one causes my test system to lock up. I'll investigate. Please
> don't apply this patch for now.
Sorry, I added a double-free on the skb after ip_queue_xmit.
Please try this one instead.
[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 <herbert@gondor.apana.org.au>
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/drivers/net/pppol2tp.c b/drivers/net/pppol2tp.c
index 266e8b3..6e4c8a7 100644
--- a/drivers/net/pppol2tp.c
+++ b/drivers/net/pppol2tp.c
@@ -958,7 +958,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;
@@ -989,41 +988,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)
@@ -1036,7 +1024,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++) {
@@ -1049,18 +1037,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) {
@@ -1073,17 +1061,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;
}
/*****************************************************************************
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-19 1:25 ` Herbert Xu
@ 2007-09-19 8:43 ` James Chapman
2007-09-19 8:51 ` Herbert Xu
2007-09-19 17:47 ` David Miller
1 sibling, 1 reply; 40+ messages in thread
From: James Chapman @ 2007-09-19 8:43 UTC (permalink / raw)
To: Herbert Xu
Cc: David S. Miller, Michal Ostrowski, Paul Mackerras,
Toralf Förster, netdev
Herbert Xu wrote:
> On Tue, Sep 18, 2007 at 09:19:33PM +0100, James Chapman wrote:
>> This one causes my test system to lock up. I'll investigate. Please
>> don't apply this patch for now.
>
> Sorry, I added a double-free on the skb after ip_queue_xmit.
> Please try this one instead.
>
> - /* 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;
> }
Shouldn't this return 0 in the error case and without the kfree_skb()?
This lets ppp requeue the skb.
--
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-19 8:43 ` James Chapman
@ 2007-09-19 8:51 ` Herbert Xu
0 siblings, 0 replies; 40+ messages in thread
From: Herbert Xu @ 2007-09-19 8:51 UTC (permalink / raw)
To: James Chapman
Cc: David S. Miller, Michal Ostrowski, Paul Mackerras,
Toralf Förster, netdev
On Wed, Sep 19, 2007 at 09:43:49AM +0100, James Chapman wrote:
>
> >-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;
> > }
>
> Shouldn't this return 0 in the error case and without the kfree_skb()?
> This lets ppp requeue the skb.
No. As I described in the changelog, the return value of 0
is only meaningful for ppp_async and ppp_sync. Returning 0
means that you're congested, not that there has been a
temporary error and the packet should be retried.
Retransmission should be left to the higher protocols.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit
2007-09-19 1:25 ` Herbert Xu
2007-09-19 8:43 ` James Chapman
@ 2007-09-19 17:47 ` David Miller
1 sibling, 0 replies; 40+ messages in thread
From: David Miller @ 2007-09-19 17:47 UTC (permalink / raw)
To: herbert; +Cc: jchapman, mostrows, paulus, toralf.foerster, netdev
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 19 Sep 2007 09:25:29 +0800
> [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 <herbert@gondor.apana.org.au>
I've replaced the older patch with the leak with this one,
thanks Herbert.
^ permalink raw reply [flat|nested] 40+ messages in thread