Re: malformed captured packets

Re: malformed captured packets

[James Chapman]

Toralf Förster wrote:
> I use at home an DSL connection (stable Gentoo system),
> current kernel is 2.6.22-gentoo-r5.
> 
> I attached 2 pcap files with the communication of the KDE program kscd with the
> CDDB server freedb.org, sniffed from interface ppp0 and eth0 respectively.
> 
> The sniffed network stream over the ppp0 interface looks _always_ fine whereas
> the sniffed packets from the eth0 always looks bad :-(
> 
> I don't undestand why the ppp0 stream is ok, whereas the eth0 stream has always
> a malformed package (eg #13 in kscd_eth0.pcap) which contains the content of the CD.
> And I'm really confused about the fact, that within a LAN I did not have a
> malformed packet.
> 
> I discussed it
> here http://www.wireshark.org/lists/wireshark-users/200707/msg00187.html
> and here http://bugzilla.kernel.org/show_bug.cgi?id=8793 and many different
> places in the past but w/o success.
> 
> Any explanation are appreciated.

Can you provide more information about the problem, please? Are you 
using a simple DSL modem with PPPoE, such that the ppp0 interface is 
that of the pppd started by a local PPPoE server? Is this a problem only 
with packet capture or are you seeing actual data corruption? Did this 
work with previous kernels? What is the network topology related to the 
DSL interface?

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

Re: malformed captured packets

[Toralf Förster]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

Am Mittwoch, 29. August 2007 schrieb James Chapman:

> Can you provide more information about the problem, please? Are you 
> using a simple DSL modem with PPPoE, such that the ppp0 interface is 
> that of the pppd started by a local PPPoE server? Is this a problem only 
> with packet capture or are you seeing actual data corruption? Did this 
> work with previous kernels? What is the network topology related to the 
> DSL interface?
> 

I use a ThinkPad T41 with this Ethernet controller:

n22 ~ # lspci | grep Eth
02:01.0 Ethernet controller: Intel Corporation 82540EP Gigabit Ethernet Controller (Mobile) (rev 03)
02:02.0 Ethernet controller: Atheros Communications, Inc. AR5212 802.11abg NIC (rev 01)

My DSL provider is Alice DSL (formerly Hansenet) in Hamburg. The T41 is connected
with an Ethernet cable to a Siemens DSL modem. The modem (just a modem, not a
router) itself is connected to the DSL splitter which itself is plugged into socket.

The current ppp version I'm using is net-dialup/ppp-2.4.4-r9

Here are my kernel config settings:

n22 ~ # zgrep PPP /proc/config.gz
CONFIG_PPP=m
# CONFIG_PPP_MULTILINK is not set
CONFIG_PPP_FILTER=y
# CONFIG_PPP_ASYNC is not set
# CONFIG_PPP_SYNC_TTY is not set
CONFIG_PPP_DEFLATE=m
# CONFIG_PPP_BSDCOMP is not set
# CONFIG_PPP_MPPE is not set
CONFIG_PPPOE=m

I observed this problem since a long time with different kernel versions (Gentoo,
plain vanilla kernel, git sources) while playing with ethereal - currently known
as wireshark.

I'm wondering b/c for kscd eg. it is always the IP packet containing the content
information of a CD (or even a <CD is unknown> message) with is struggled.
This packets prevents me from using the "Follow TCP Strem" feature of wireshark
for an easy look into the plain text of all TCP packets of this HTTP stream
(which was in fact the trigger for me to have a deeper look into the sniffed
stream from ppp0 and eth0).

For other apps I observed similar things which cannot fully be explained by
terms like "TCP checksum offloading". 

I didn't observed any malfunction at application level so it might be an issue
with the capturing itself.

Why is the ppp stream always ok in opposite to the eth0 stream ?

-- 
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 --]

Re: malformed captured packets

[James Chapman]

Toralf Förster wrote:
> Am Mittwoch, 29. August 2007 schrieb James Chapman:
> 
>> Can you provide more information about the problem, please? Are you 
>> using a simple DSL modem with PPPoE, such that the ppp0 interface is 
>> that of the pppd started by a local PPPoE server? Is this a problem only 
>> with packet capture or are you seeing actual data corruption? Did this 
>> work with previous kernels? What is the network topology related to the 
>> DSL interface?
>>
> 
> I use a ThinkPad T41 with this Ethernet controller:
> 
> n22 ~ # lspci | grep Eth
> 02:01.0 Ethernet controller: Intel Corporation 82540EP Gigabit Ethernet Controller (Mobile) (rev 03)
> 02:02.0 Ethernet controller: Atheros Communications, Inc. AR5212 802.11abg NIC (rev 01)
> 
> My DSL provider is Alice DSL (formerly Hansenet) in Hamburg. The T41 is connected
> with an Ethernet cable to a Siemens DSL modem. The modem (just a modem, not a
> router) itself is connected to the DSL splitter which itself is plugged into socket.
> 
> The current ppp version I'm using is net-dialup/ppp-2.4.4-r9
> 
> Here are my kernel config settings:
> 
> n22 ~ # zgrep PPP /proc/config.gz
> CONFIG_PPP=m
> # CONFIG_PPP_MULTILINK is not set
> CONFIG_PPP_FILTER=y
> # CONFIG_PPP_ASYNC is not set
> # CONFIG_PPP_SYNC_TTY is not set
> CONFIG_PPP_DEFLATE=m
> # CONFIG_PPP_BSDCOMP is not set
> # CONFIG_PPP_MPPE is not set
> CONFIG_PPPOE=m
> 
> I observed this problem since a long time with different kernel versions (Gentoo,
> plain vanilla kernel, git sources) while playing with ethereal - currently known
> as wireshark.
> 
> I'm wondering b/c for kscd eg. it is always the IP packet containing the content
> information of a CD (or even a <CD is unknown> message) with is struggled.
> This packets prevents me from using the "Follow TCP Strem" feature of wireshark
> for an easy look into the plain text of all TCP packets of this HTTP stream
> (which was in fact the trigger for me to have a deeper look into the sniffed
> stream from ppp0 and eth0).
> 
> For other apps I observed similar things which cannot fully be explained by
> terms like "TCP checksum offloading". 
> 
> I didn't observed any malfunction at application level so it might be an issue
> with the capturing itself.
> 
> Why is the ppp stream always ok in opposite to the eth0 stream ?

Toralf, thanks for providing more info about your setup.

Are you using kernel-mode PPPoE? I know some PPPoE servers do the PPPoE 
datapath in userspace...

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.

Is it possible to get captures from ppp0 and eth0 simultaneously such 
that they show the same ppp instance? This might give more clues.

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

Re: malformed captured packets

[Toralf Förster]

[-- Attachment #1.1: Type: text/plain, Size: 3945 bytes --]

@wireshark-devs:

The topic is related to
http://www.wireshark.org/lists/wireshark-users/200707/msg00187.html
and http://bugzilla.kernel.org/show_bug.cgi?id=8793

@all:
Hi,

Am Donnerstag, 30. August 2007 schrieb James Chapman:
> Toralf Förster wrote:
> > Am Mittwoch, 29. August 2007 schrieb James Chapman:
> > 
> >> Can you provide more information about the problem, please? Are you 
> >> using a simple DSL modem with PPPoE, such that the ppp0 interface is 
> >> that of the pppd started by a local PPPoE server? Is this a problem only 
> >> with packet capture or are you seeing actual data corruption? Did this 
> >> work with previous kernels? What is the network topology related to the 
> >> DSL interface?
> >>
> > 
> > I use a ThinkPad T41 with this Ethernet controller:
> > 
> > n22 ~ # lspci | grep Eth
> > 02:01.0 Ethernet controller: Intel Corporation 82540EP Gigabit Ethernet Controller (Mobile) (rev 03)
> > 02:02.0 Ethernet controller: Atheros Communications, Inc. AR5212 802.11abg NIC (rev 01)
> > 
> > My DSL provider is Alice DSL (formerly Hansenet) in Hamburg. The T41 is connected
> > with an Ethernet cable to a Siemens DSL modem. The modem (just a modem, not a
> > router) itself is connected to the DSL splitter which itself is plugged into socket.
> > 
> > The current ppp version I'm using is net-dialup/ppp-2.4.4-r9
> > 
> > Here are my kernel config settings:
> > 
> > n22 ~ # zgrep PPP /proc/config.gz
> > CONFIG_PPP=m
> > # CONFIG_PPP_MULTILINK is not set
> > CONFIG_PPP_FILTER=y
> > # CONFIG_PPP_ASYNC is not set
> > # CONFIG_PPP_SYNC_TTY is not set
> > CONFIG_PPP_DEFLATE=m
> > # CONFIG_PPP_BSDCOMP is not set
> > # CONFIG_PPP_MPPE is not set
> > CONFIG_PPPOE=m
> > 
> > I observed this problem since a long time with different kernel versions (Gentoo,
> > plain vanilla kernel, git sources) while playing with ethereal - currently known
> > as wireshark.
> > 
> > I'm wondering b/c for kscd eg. it is always the IP packet containing the content
> > information of a CD (or even a <CD is unknown> message) with is struggled.
> > This packets prevents me from using the "Follow TCP Strem" feature of wireshark
> > for an easy look into the plain text of all TCP packets of this HTTP stream
> > (which was in fact the trigger for me to have a deeper look into the sniffed
> > stream from ppp0 and eth0).
> > 
> > For other apps I observed similar things which cannot fully be explained by
> > terms like "TCP checksum offloading". 
> > 
> > I didn't observed any malfunction at application level so it might be an issue
> > with the capturing itself.
> > 
> > Why is the ppp stream always ok in opposite to the eth0 stream ?
> 
> Toralf, thanks for providing more info about your setup.
> 
> Are you using kernel-mode PPPoE? I know some PPPoE servers do the PPPoE 
> datapath in userspace...
> 
> 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.
> 
> Is it possible to get captures from ppp0 and eth0 simultaneously such 
> that they show the same ppp instance? This might give more clues.
> 

Hi,

yes, I'm using kernel-mode PPPoE. I sniffed at both interfaces the same network stream with the commands:

$>tcpdump -i eth0 -p -s 0 -w tcpdump_eth0.pcap
$>tcpdump -i eth0 -p -s 0 -w tcpdump_eth0.pcap

After that I made an
$>rm -rf .cddb/; kscd
at the 3rd konsole and attached the 2 pcap streams onto this mail.

Thanks for your help.


-- 
MfG/Sincerely

Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

[-- Attachment #1.2: tcpdump_ppp0.pcap --]
[-- Type: application/octet-stream, Size: 6294 bytes --]

[-- Attachment #1.3: tcpdump_eth0.pcap --]
[-- Type: application/octet-stream, Size: 6684 bytes --]

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets)

[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,
-- 
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

[PATCH 1/7] [PPP] pppoe: Fix skb_unshare_check call position

[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);

[PATCH 2/7] [PPP] pppoe: Fix data clobbering in __pppoe_xmit and return value

[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;
 }
 
 

[PATCH 3/7] [PPP] pppoe: Fill in header directly in __pppoe_xmit

[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,

[PATCH 4/7] [BRIDGE]: Kill clone argument to br_flood_*

[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);

[PATCH 5/7] [NET] skbuff: Add skb_cow_head

[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;
 

[PATCH 6/7] [PPP] generic: Call skb_cow_head before scribbling over skb

[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;

[PATCH 7/7] [PPP] generic: Fix receive path data clobbering & non-linear handling

[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 */

Re: [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets)

[Toralf Förster]

[-- 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 --]

Re: [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets)

[Toralf Förster]

[-- 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 --]

Re: [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets)

[Toralf Förster]

[-- 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 --]

Re: [PATCH 1/7] [PPP] pppoe: Fix skb_unshare_check call position

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 31 Aug 2007 17:08:49 +0800

> [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>

Patch applied, thanks Herbert.

Re: [PATCH 2/7] [PPP] pppoe: Fix data clobbering in __pppoe_xmit and return value

[David Miller]

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.

Re: [PATCH 3/7] [PPP] pppoe: Fill in header directly in __pppoe_xmit

[David Miller]

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.

Re: [PATCH 4/7] [BRIDGE]: Kill clone argument to br_flood_*

[David Miller]

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.

Re: [PATCH 5/7] [NET] skbuff: Add skb_cow_head

[David Miller]

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.

Re: [PATCH 6/7] [PPP] generic: Call skb_cow_head before scribbling over skb

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 31 Aug 2007 17:09:16 +0800

> [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>

Applied, thanks Herbert.

Re: [PATCH 7/7] [PPP] generic: Fix receive path data clobbering & non-linear handling

[David Miller]

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.

[0/3] [PPP]: Fix pppol2tp skb bugs

[Herbert Xu]

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

[PATCH 1/3] [PPP] L2TP: Disallow non-UDP datagram sockets

[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;

[PATCH 2/3] [PPP] L2TP: Fix skb handling in pppol2tp_recv_core

[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);
 

[PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[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;
 }
 
 /*****************************************************************************

Re: [PATCH 1/3] [PPP] L2TP: Disallow non-UDP datagram sockets

[James Chapman]

Herbert Xu wrote:
> [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>

Acked-by: James Chapman <jchapman@katalix.com>

Re: [PATCH 2/3] [PPP] L2TP: Fix skb handling in pppol2tp_recv_core

[James Chapman]

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

Re: [0/3] [PPP]: Fix pppol2tp skb bugs

[David Miller]

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.

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[James Chapman]

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

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[David Miller]

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.

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[Herbert Xu]

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;
 }
 
 /*****************************************************************************

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[Herbert Xu]

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;
 

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[James Chapman]

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

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[Herbert Xu]

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

Re: [0/7] [PPP]: Fix shared/cloned/non-linear skb bugs (was: malformed captured packets)

[Herbert Xu]

On Tue, Sep 11, 2007 at 08:12:50PM +0200, Toralf Förster wrote:
> 
> 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.

Maybe your wireshark is broken? I've tried wireshark and tcpdump
here and the sums look fine.

> and I'm wondering why it is still possible to capture such packets at eth0.

tcpdump happens before the packet goes into the IP stack which
is whare iptables lives.

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

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[David Miller]

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.

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[David Miller]

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.

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[Adrian Bunk]

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

Re: [PATCH 3/3] [PPP] L2TP: Fix skb handling in pppol2tp_xmit

[David Miller]

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.