PATCH : ppp + big-endian = kernel crash

PATCH : ppp + big-endian = kernel crash

[Philippe De Muyter]

Hello all,

My m68k linux crashed often in ksoftirqd/0 with an `address error' (word
access to an even address) in function ip_rcv, at line 405 (line numbers
valid in current sources as of 2005-05-29) of net/ipv4/ip_input.c,
failing to execute :

	    405                 __u32 len = ntohs(iph->tot_len);

because iph was odd, tot_len is a 16-bit field and ntohs is a nop on
big-endian machines.

I searched the origin of that odd pointer, and found it in
process_input_packet at line 819 of drivers/net/ppp_async.c :

	    819                 skb_push(skb, 1)[0] = 0; 

This subtracts 1 from the packet address, yielding an odd pointer if
we had an even one and conversely.  My proposed fix is below.

Feel free to apply that to the main source trees.

Philippe

Philippe De Muyter  phdm at macqel dot be  Tel +32 27029044
Macq Electronique SA  rue de l'Aeronef 2  B-1140 Bruxelles  Fax +32 27029077

--- ppp_async.c.orig	2005-05-29 20:28:44.000000000 +0200
+++ ppp_async.c	2005-05-29 21:16:16.000000000 +0200
@@ -817,6 +817,16 @@
 	if (proto & 1) {
 		/* protocol is compressed */
 		skb_push(skb, 1)[0] = 0;
+		/* If the address of the packet is odd now, fix it. */
+		if ((unsigned long)skb->data & 1) {
+			unsigned char *p;
+			int n;
+
+			p = skb_put(skb, 1);
+			for (n = skb->len; --n >= 0; p -= 1)
+				p[0] = p[-1];
+			skb_pull(skb, 1);
+		}
 	} else {
 		if (skb->len < 2)
 			goto err;
@@ -890,6 +900,12 @@
 				if (skb == 0)
 					goto nomem;
 				/* Try to get the payload 4-byte aligned */
+				/* This should match the
+				** PPP_ALLSTATIONS/PPP_UI/compressed tests
+				** in process_input_packet,
+				** but we do not have enough chars here and
+				** now to test buf[1] and buf[2].
+				*/
 				if (buf[0] != PPP_ALLSTATIONS)
 					skb_reserve(skb, 2 + (buf[0] & 1));
 				ap->rpkt = skb;

Re: PATCH : ppp + big-endian = kernel crash

[David S. Miller]

From: "Philippe De Muyter" <phdm@macqel.be>
Date: Sun, 29 May 2005 21:48:42 +0200 (CEST)

> +		/* If the address of the packet is odd now, fix it. */
> +		if ((unsigned long)skb->data & 1) {
> +			unsigned char *p;

And now it will crash when a packet is only 2-byte aligned
when the input packet processing does the first access
to the IP address in the packet header.

Please make your m68k port handle unaligned memory accesses
in kernel mode properly instead.

Re: PATCH : ppp + big-endian = kernel crash

[Philippe De Muyter]

I have also sent this message to uclinux-dev, where it can be of interest.

David S. Miller wrote :
> From: "Philippe De Muyter" <phdm@macqel.be>
> Date: Sun, 29 May 2005 21:48:42 +0200 (CEST)
> 
> > +		/* If the address of the packet is odd now, fix it. */
> > +		if ((unsigned long)skb->data & 1) {
> > +			unsigned char *p;
> 
> And now it will crash when a packet is only 2-byte aligned
> when the input packet processing does the first access
> to the IP address in the packet header.

Actually, my fix has been tested extensively, and m68k's won't crash when
accessing 4-bytes words on only 2-byte aligned addresses.  If you mean
that I moved the packet in the wrong direction to get the best alignment,
this can easily be fixed.

> 
> Please make your m68k port handle unaligned memory accesses
> in kernel mode properly instead.
> 

Do you mean that ip_rcv may not assume that packets are properly aligned ?

And some non-mmu m68k (Coldfires) do not provide enough information in
exception frames to restart instructions on an address error in the general
case.

Best regards

Philippe

Re: PATCH : ppp + big-endian = kernel crash

[David S. Miller]

From: "Philippe De Muyter" <phdm@macqel.be>
Date: Sun, 29 May 2005 23:38:53 +0200 (CEST)

> Do you mean that ip_rcv may not assume that packets are properly aligned ?
> 
> And some non-mmu m68k (Coldfires) do not provide enough information in
> exception frames to restart instructions on an address error in the general
> case.

So many variants of tunneling and protocol encapsulation can result in
unaligned packet headers, and as a result platforms really must
provide proper unaligned memory access handling in kernel mode in
order to use the networking fully.

All these patches to PPP and friends are merely papering over the
larger problem.

Re: PATCH : ppp + big-endian = kernel crash

[Andrew Morton]

"David S. Miller" <davem@davemloft.net> wrote:
>
> From: "Philippe De Muyter" <phdm@macqel.be>
> Date: Sun, 29 May 2005 23:38:53 +0200 (CEST)
> 
> > Do you mean that ip_rcv may not assume that packets are properly aligned ?
> > 
> > And some non-mmu m68k (Coldfires) do not provide enough information in
> > exception frames to restart instructions on an address error in the general
> > case.
> 
> So many variants of tunneling and protocol encapsulation can result in
> unaligned packet headers, and as a result platforms really must
> provide proper unaligned memory access handling in kernel mode in
> order to use the networking fully.

As Philippe mentioned, old 68k's simply cannot do this.

> All these patches to PPP and friends are merely papering over the
> larger problem.

It's not a thing we want to do in the general case, sure.  But it's
reasonable to identify those bits of net code which the nommu people care
about and look to see if there's some sane workaround to get them going.

Otherwise, things like PPP will simply unavailable to some architectures...

Re: PATCH : ppp + big-endian = kernel crash

[David S. Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Sun, 29 May 2005 19:52:45 -0700

> > All these patches to PPP and friends are merely papering over the
> > larger problem.
> 
> It's not a thing we want to do in the general case, sure.  But it's
> reasonable to identify those bits of net code which the nommu people care
> about and look to see if there's some sane workaround to get them going.
> 
> Otherwise, things like PPP will simply unavailable to some architectures...

Some time ago there was a proposal that would allow appropriate
handling of these sorts of things.

Accessors to packet headers would go through a macro, and this
along with some other defines would allow an architecture to
decide between two schemes:

1) Use normal loads and stores, let trap handler take care of
   unaligned cases.
2) Use something akin to get_unaligned(), no trap handler stuff.

Sure, to make things faster we can do something like this PPP
patch, but it needs lots of work, first of all you need to
replace this:

	for ( ... )
		p[i-1] = p[i];

stuff with a proper memmove() call.

Re: PATCH : ppp + big-endian = kernel crash

[Giuliano Pochini]

On 29-May-2005 David S. Miller wrote:
>
> And now it will crash when a packet is only 2-byte aligned
> when the input packet processing does the first access
> to the IP address in the packet header.

Accessing 4-byte integers in 2-byte aligned addresses is fine
on all "desktop" CISC m68k IIRC (the first m68k was a 16-bit
processor so it didn't require 32-bit alignment). I don't
know about embedded chips, coldfire and others.


--
Giuliano.

Re: PATCH : ppp + big-endian = kernel crash

[Andi Kleen]

Andrew Morton <akpm@osdl.org> writes:
>> 
>> So many variants of tunneling and protocol encapsulation can result in
>> unaligned packet headers, and as a result platforms really must
>> provide proper unaligned memory access handling in kernel mode in
>> order to use the networking fully.
>
> As Philippe mentioned, old 68k's simply cannot do this.

An 68000 cannot, but 68010+ can. Are there really that many 68000 users
left? 

-Andi

Re: PATCH : ppp + big-endian = kernel crash

[cutaway]

----- Original Message ----- 
From: "Andi Kleen" <ak@muc.de>
>
> An 68000 cannot, but 68010+ can. Are there really that many 68000 users
> left?

Desktop? Probably very few.  Probably a lot who don't even know they are
though - using things like the MC68SEC000.

The 16 bit external bus 68K variants are popular embedded processors.

Re: PATCH : ppp + big-endian = kernel crash

[Greg Ungerer]

Andi Kleen <ak () muc ! de> writes:
> Andrew Morton <akpm@osdl.org> writes:
>>> 
>>> So many variants of tunneling and protocol encapsulation can result in
>>> unaligned packet headers, and as a result platforms really must
>>> provide proper unaligned memory access handling in kernel mode in
>>> order to use the networking fully.
>>
>> As Philippe mentioned, old 68k's simply cannot do this.
> 
> An 68000 cannot, but 68010+ can. Are there really that many 68000 users
> left? 

Probably not of the 68000 as such, but the "new" generation of
68000 parts, Motorola/Freescales ColdFire family. There is quite
a few of them, used in all sorts of embedded applications.
And they are still churning out new varients of it. The majority
of them are MMUless - but not all.

Regards
Greg

Re: PATCH : ppp + big-endian = kernel crash

[Philippe De Muyter]

David S. Miller wrote :
> From: Andrew Morton <akpm@osdl.org>
> Date: Sun, 29 May 2005 19:52:45 -0700
> 
> > > All these patches to PPP and friends are merely papering over the
> > > larger problem.
> > 
> > It's not a thing we want to do in the general case, sure.  But it's
> > reasonable to identify those bits of net code which the nommu people care
> > about and look to see if there's some sane workaround to get them going.
> > 
> > Otherwise, things like PPP will simply unavailable to some architectures...
> 
> Some time ago there was a proposal that would allow appropriate
> handling of these sorts of things.
> 
> Accessors to packet headers would go through a macro, and this
> along with some other defines would allow an architecture to
> decide between two schemes:
> 
> 1) Use normal loads and stores, let trap handler take care of
>    unaligned cases.
> 2) Use something akin to get_unaligned(), no trap handler stuff.
> 
> Sure, to make things faster we can do something like this PPP
> patch, but it needs lots of work, first of all you need to
> replace this:
> 
> 	for ( ... )
> 		p[i-1] = p[i];
> 
> stuff with a proper memmove() call.
> 

Ok, here is a revised patch :

This patch avoids ppp-generated kernel crashes on machines where
unaligned accesses are forbidden.

Signed-off-by: Philippe De Muyter <phdm@macqel.be>

--- linux/drivers/net/ppp_async.c.orig	2005-06-01 10:46:58.000000000 +0200
+++ linux/drivers/net/ppp_async.c	2005-06-01 17:04:13.000000000 +0200
@@ -31,6 +31,7 @@
 #include <linux/spinlock.h>
 #include <linux/init.h>
 #include <asm/uaccess.h>
+#include <asm/string.h>
 
 #define PPP_VERSION	"2.4.2"
 
@@ -816,7 +817,15 @@ process_input_packet(struct asyncppp *ap
 	proto = p[0];
 	if (proto & 1) {
 		/* protocol is compressed */
-		skb_push(skb, 1)[0] = 0;
+		if ((unsigned long)skb->data & 1)
+			skb_push(skb, 1)[0] = 0;
+		else { /* Ditto, but realign the payload to 4-byte boundary */
+			short len = skb->len;
+
+			skb_put(skb, 3);
+			memmove(skb->data + 3, skb->data, len);
+			skb_pull(skb, 2)[0] = 0;
+		}
 	} else {
 		if (skb->len < 2)
 			goto err;
@@ -890,6 +899,12 @@ ppp_async_input(struct asyncppp *ap, con
 				if (skb == 0)
 					goto nomem;
 				/* Try to get the payload 4-byte aligned */
+				/* This should match the
+				** PPP_ALLSTATIONS/PPP_UI/compressed tests
+				** in process_input_packet,
+				** but we do not have enough chars here and
+				** now to test buf[1] and buf[2].
+				*/
 				if (buf[0] != PPP_ALLSTATIONS)
 					skb_reserve(skb, 2 + (buf[0] & 1));
 				ap->rpkt = skb;