Re: [Bug 6421] kernel 2.6.10-2.6.16 on alpha: arch/alpha/kernel/io.c, iowrite16_rep() BUG_ON((unsigned long)src & 0x1) triggered

Re: [Bug 6421] kernel 2.6.10-2.6.16 on alpha: arch/alpha/kernel/io.c, iowrite16_rep() BUG_ON((unsigned long)src & 0x1) triggered

[Herbert Xu]

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

On Sat, May 27, 2006 at 10:31:19AM -0700, bugme-daemon@bugzilla.kernel.org wrote:
> 
> Ok, see the attachment (ALL.txt.gz) for the tcpdump output. If you would like
> the hexdump or more, please give the tcpdump filter rule to me.

Thanks a lot.  So it wasn't as uncommon as I thought.  In fact, partial
odd acks like yours happen all the time as part of FIN handling.  So this
is something that we want to address.

BTW, we should also fix arch/alpha/kernel/io.c to not crash on
unaligned data.  Most other common architectures will grin and
bear it.

[TCP]: Avoid skb_pull if possible when trimming head

Trimming the head of an skb by calling skb_pull can cause the packet
to become unaligned if the length pulled is odd.  Since the length is
entirely arbitrary for a FIN packet carrying data, this is actually
quite common.

Unaligned data is not the end of the world, but we should avoid it if
it's easily done.  In this case it is trivial.  Since we're discarding
all of the head data it doesn't matter whether we move skb->data forward
or back.

However, it is still possible to have unaligned skb->data in general.
So network drivers should be prepared to handle it instead of crashing.

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

[-- Attachment #2: tcp-trim-head.patch --]
[-- Type: text/plain, Size: 435 bytes --]

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 743016b..bd7c89b 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -676,7 +676,7 @@ int tcp_trim_head(struct sock *sk, struc
 	    pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
 		return -ENOMEM;
 
-	if (len <= skb_headlen(skb)) {
+	if (len < skb_headlen(skb)) {
 		__skb_pull(skb, len);
 	} else {
 		if (__pskb_trim_head(skb, len-skb_headlen(skb)) == NULL)

Re: [Bug 6421] kernel 2.6.10-2.6.16 on alpha: arch/alpha/kernel/io.c, iowrite16_rep() BUG_ON((unsigned long)src & 0x1) triggered

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20060603095442.GA373@gondor.apana.org.au> (at Sat, 3 Jun 2006 19:54:42 +1000), Herbert Xu <herbert@gondor.apana.org.au> says:

> [TCP]: Avoid skb_pull if possible when trimming head
> 
> Trimming the head of an skb by calling skb_pull can cause the packet
> to become unaligned if the length pulled is odd.  Since the length is
> entirely arbitrary for a FIN packet carrying data, this is actually
> quite common.

I think that people will start thinking why we cannot
skb_pull(skb, len) if skb_headlen(skb) == len; some comment needed...

--yoshfuji

Re: [Bug 6421] kernel 2.6.10-2.6.16 on alpha: arch/alpha/kernel/io.c, iowrite16_rep() BUG_ON((unsigned long)src & 0x1) triggered

[Herbert Xu]

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

On Sat, Jun 03, 2006 at 07:51:23PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> I think that people will start thinking why we cannot
> skb_pull(skb, len) if skb_headlen(skb) == len; some comment needed...

Good idea.  Here is a better one.

[TCP]: Avoid skb_pull if possible when trimming head

Trimming the head of an skb by calling skb_pull can cause the packet
to become unaligned if the length pulled is odd.  Since the length is
entirely arbitrary for a FIN packet carrying data, this is actually
quite common.

Unaligned data is not the end of the world, but we should avoid it if
it's easily done.  In this case it is trivial.  Since we're discarding
all of the head data it doesn't matter whether we move skb->data forward
or back.

However, it is still possible to have unaligned skb->data in general.
So network drivers should be prepared to handle it instead of crashing.

This patch also adds an unlikely marking on len < headlen since partial
ACKs on head data are extremely rare in the wild.  As the return value
of __pskb_trim_head is no longer ever NULL that has been removed.

Signed-off-by: Herbert Xu ~{PmV>HI~} <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

[-- Attachment #2: tcp-trim-head-2.patch --]
[-- Type: text/plain, Size: 1203 bytes --]

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 743016b..f33c9dd 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -642,7 +642,7 @@ int tcp_fragment(struct sock *sk, struct
  * eventually). The difference is that pulled data not copied, but
  * immediately discarded.
  */
-static unsigned char *__pskb_trim_head(struct sk_buff *skb, int len)
+static void __pskb_trim_head(struct sk_buff *skb, int len)
 {
 	int i, k, eat;
 
@@ -667,7 +667,6 @@ static unsigned char *__pskb_trim_head(s
 	skb->tail = skb->data;
 	skb->data_len -= len;
 	skb->len = skb->data_len;
-	return skb->tail;
 }
 
 int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
@@ -676,12 +675,11 @@ int tcp_trim_head(struct sock *sk, struc
 	    pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
 		return -ENOMEM;
 
-	if (len <= skb_headlen(skb)) {
+	/* If len == headlen, we avoid __skb_pull to preserve alignment. */
+	if (unlikely(len < skb_headlen(skb)))
 		__skb_pull(skb, len);
-	} else {
-		if (__pskb_trim_head(skb, len-skb_headlen(skb)) == NULL)
-			return -ENOMEM;
-	}
+	else
+		__pskb_trim_head(skb, len - skb_headlen(skb));
 
 	TCP_SKB_CB(skb)->seq += len;
 	skb->ip_summed = CHECKSUM_HW;

Re: [Bug 6421] kernel 2.6.10-2.6.16 on alpha: arch/alpha/kernel/io.c, iowrite16_rep() BUG_ON((unsigned long)src & 0x1) triggered

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 3 Jun 2006 21:07:11 +1000

> On Sat, Jun 03, 2006 at 07:51:23PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> > 
> > I think that people will start thinking why we cannot
> > skb_pull(skb, len) if skb_headlen(skb) == len; some comment needed...
> 
> Good idea.  Here is a better one.
> 
> [TCP]: Avoid skb_pull if possible when trimming head

Applied, thanks a lot Herbert.

Re: [Bug 6421] kernel 2.6.10-2.6.16 on alpha: arch/alpha/kernel/io.c, iowrite16_rep() BUG_ON((unsigned long)src & 0x1) triggered

[Herbert Xu]

On Tue, May 16, 2006 at 11:44:17PM -0700, bugme-daemon@bugzilla.kernel.org wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=6421
>
> ------- Additional Comments From tomri@gmx.net  2006-05-16 23:43 -------
> Another stack trace. skb_padto() is not called.
> 
> May 16 23:50:34 merlin kernel: ei_start_xmit(): entry: skb->data unaligned
> fffffc0009b5098b skb fffffc001e763a08 length 66
> May 16 23:50:34 merlin kernel: Badness in ei_start_xmit at drivers/net/8390.c:283

...

> May 16 23:50:34 merlin kernel: Trace:
> May 16 23:50:34 merlin kernel: [<fffffc00005670f8>] qdisc_restart+0x88/0x2b0
> May 16 23:50:34 merlin kernel: [<fffffc00005d64b4>] packet_rcv_spkt+0x1a4/0x3b0
> May 16 23:50:34 merlin kernel: [<fffffc00005566c0>] dev_queue_xmit_nit+0x1a0/0x1f0
> May 16 23:50:34 merlin kernel: [<fffffc0000567250>] qdisc_restart+0x1e0/0x2b0
> May 16 23:50:34 merlin kernel: [<fffffc0000558f48>] dev_queue_xmit+0xb8/0x3b0
> May 16 23:50:34 merlin kernel: [<fffffc0000560508>] neigh_resolve_output+0x128/0x370
> May 16 23:50:34 merlin kernel: [<fffffc00005815b4>] ip_output+0x224/0x430
> May 16 23:50:34 merlin kernel: [<fffffc00005800bc>] ip_queue_xmit+0x2dc/0x630
> May 16 23:50:34 merlin kernel: [<fffffc0000596068>] tcp_transmit_skb+0x588/0xa30
> May 16 23:50:34 merlin kernel: [<fffffc000059bb6c>] tcp_v4_send_check+0x11c/0x150
> May 16 23:50:34 merlin kernel: [<fffffc0000595f68>] tcp_transmit_skb+0x488/0xa30
> May 16 23:50:34 merlin kernel: [<fffffc0000597ab0>] tcp_retransmit_skb+0x730/0x7b0

OK, I think I have a suspect in sight.  The unaligned packet is
probably caused by tcp_trim_head.  To fix it we could just copy
the data instead of trimming if it is not paged.

However, I'd like you to confirm that this is indeed the cause
with a tcpdump.

So please take a tcpdump and show us the relevant packets that
show up about the time when you get the unaligned warnings.
What I'm expecting is a partial odd ACK just before the error.

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