2005-05-29 19:48:52

by Philippe De Muyter

[permalink] [raw]
Subject: PATCH : ppp + big-endian = kernel crash

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;


2005-05-29 20:53:45

by David Miller

[permalink] [raw]
Subject: Re: PATCH : ppp + big-endian = kernel crash

From: "Philippe De Muyter" <[email protected]>
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.

2005-05-29 21:39:32

by Philippe De Muyter

[permalink] [raw]
Subject: Re: PATCH : ppp + big-endian = kernel crash

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

David S. Miller wrote :
> From: "Philippe De Muyter" <[email protected]>
> 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

2005-05-29 21:55:58

by David Miller

[permalink] [raw]
Subject: Re: PATCH : ppp + big-endian = kernel crash

From: "Philippe De Muyter" <[email protected]>
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.

2005-05-30 02:53:48

by Andrew Morton

[permalink] [raw]
Subject: Re: PATCH : ppp + big-endian = kernel crash

"David S. Miller" <[email protected]> wrote:
>
> From: "Philippe De Muyter" <[email protected]>
> 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...

2005-05-30 03:12:05

by David Miller

[permalink] [raw]
Subject: Re: PATCH : ppp + big-endian = kernel crash

From: Andrew Morton <[email protected]>
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.

2005-05-30 07:16:16

by Giuliano Pochini

[permalink] [raw]
Subject: Re: PATCH : ppp + big-endian = kernel crash


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.

2005-05-30 10:22:45

by Andi Kleen

[permalink] [raw]
Subject: Re: PATCH : ppp + big-endian = kernel crash

Andrew Morton <[email protected]> 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

2005-05-30 12:09:09

by cutaway

[permalink] [raw]
Subject: Re: PATCH : ppp + big-endian = kernel crash

----- Original Message -----
From: "Andi Kleen" <[email protected]>
>
> 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.

2005-05-30 14:03:47

by Greg Ungerer

[permalink] [raw]
Subject: Re: PATCH : ppp + big-endian = kernel crash

Andi Kleen <ak () muc ! de> writes:
> Andrew Morton <[email protected]> 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