[PATCH] netpoll endian fixes

[PATCH] netpoll endian fixes

[Duncan Sands]

The big-endians took their revenge in netpoll.c: on i386,
the ip header length / version nibbles need to be the other
way round; and the htonl leaves only zeros in tot_len...

All the best, 

Duncan.


--- linux-2.5/net/core/netpoll.c.orig	2004-09-08 01:15:22.000000000 +0200
+++ linux-2.5/net/core/netpoll.c	2004-09-08 01:05:33.000000000 +0200
@@ -22,6 +22,7 @@
 #include <net/tcp.h>
 #include <net/udp.h>
 #include <asm/unaligned.h>
+#include <asm/byteorder.h>
 
 /*
  * We maintain a small pool of fully-sized skbs, to make sure the
@@ -242,9 +243,13 @@
 	iph = (struct iphdr *)skb_push(skb, sizeof(*iph));
 
 	/* iph->version = 4; iph->ihl = 5; */
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	put_unaligned(0x45, (unsigned char *)iph);
+#else
 	put_unaligned(0x54, (unsigned char *)iph);
+#endif
 	iph->tos      = 0;
-	put_unaligned(htonl(ip_len), &(iph->tot_len));
+	put_unaligned(htons(ip_len), &(iph->tot_len));
 	iph->id       = 0;
 	iph->frag_off = 0;
 	iph->ttl      = 64;

Re: [PATCH] netpoll endian fixes

[Matt Mackall]

On Wed, Sep 08, 2004 at 01:24:43AM +0200, Duncan Sands wrote:
> The big-endians took their revenge in netpoll.c: on i386,
> the ip header length / version nibbles need to be the other
> way round; and the htonl leaves only zeros in tot_len...

I'm completely baffled as to how length / version nibbles could be
swapped. Endianness here should be a matter of _bytes_.

The htons seems reasonable.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] netpoll endian fixes

[Duncan Sands]

On Wednesday 08 September 2004 01:29, Matt Mackall wrote:
> On Wed, Sep 08, 2004 at 01:24:43AM +0200, Duncan Sands wrote:
> > The big-endians took their revenge in netpoll.c: on i386,
> > the ip header length / version nibbles need to be the other
> > way round; and the htonl leaves only zeros in tot_len...
> 
> I'm completely baffled as to how length / version nibbles could be
> swapped. Endianness here should be a matter of _bytes_.

I also don't understand it.  The definition of struct iphdr contains:

#if defined(__LITTLE_ENDIAN_BITFIELD)
        __u8    ihl:4,
                version:4;
#elif defined (__BIG_ENDIAN_BITFIELD)
        __u8    version:4,
                ihl:4;

What does it mean?

All the best,

Duncan.

Re: [PATCH] netpoll endian fixes

[Matt Mackall]

On Wed, Sep 08, 2004 at 01:24:43AM +0200, Duncan Sands wrote:
> The big-endians took their revenge in netpoll.c: on i386,
> the ip header length / version nibbles need to be the other
> way round; and the htonl leaves only zeros in tot_len...
> 
> All the best, 
> 
> Duncan.
> 
> 
> --- linux-2.5/net/core/netpoll.c.orig	2004-09-08 01:15:22.000000000 +0200
> +++ linux-2.5/net/core/netpoll.c	2004-09-08 01:05:33.000000000 +0200
> @@ -22,6 +22,7 @@
>  #include <net/tcp.h>
>  #include <net/udp.h>
>  #include <asm/unaligned.h>
> +#include <asm/byteorder.h>
>  
>  /*
>   * We maintain a small pool of fully-sized skbs, to make sure the
> @@ -242,9 +243,13 @@
>  	iph = (struct iphdr *)skb_push(skb, sizeof(*iph));
>  
>  	/* iph->version = 4; iph->ihl = 5; */
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +	put_unaligned(0x45, (unsigned char *)iph);
> +#else
>  	put_unaligned(0x54, (unsigned char *)iph);
> +#endif

Probably what's happened is this:

The original person who sent me the alignment fixes sent a similar
bizarre endian #ifdef thing. He didn't send it as a proper patch
though, so I redid it and copied the not-so-obviously incorrect
clause.

It looks like it ought to be 0x45 everywhere, meaning it's currently
broken everywhere. But no one's complained. Unfortunately at the
present moment I've got one machine short of a test rig unpacked, so
I'm loathe to push another fix until I get some other test reports.
Anyone else run into trouble with netpoll/netconsole in recent -mm or
-bk?

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] netpoll endian fixes

[Duncan Sands]

> It looks like it ought to be 0x45 everywhere, meaning it's currently
> broken everywhere. But no one's complained. Unfortunately at the
> present moment I've got one machine short of a test rig unpacked, so
> I'm loathe to push another fix until I get some other test reports.

Hi Matt, I agree that it should be 0x45 everywhere.  After thinking a bit
I concluded that the

#if defined(__LITTLE_ENDIAN_BITFIELD)
        __u8    ihl:4,
                version:4;
#elif defined (__BIG_ENDIAN_BITFIELD)
        __u8    version:4,
                ihl:4;

in the definition of struct iphdr is to make sure that compiler uses the
first four bits of the byte to refer to version, rather than the last four;
and this only matters when you are accessing the nibbles via the ihl
or version structure fields.  Thus it makes sure that if you write 0x45
to the byte, then version will return 4 and ihl will return 5.  Presumably
the C standard specifies how bitfield expressions should be laid out
in the byte, and ihl:4, version:4; gives opposite results on little-endian
and big-endian machines...

Ciao,

Duncan.

Re: [PATCH] netpoll endian fixes

[Matt Mackall]

On Wed, Sep 08, 2004 at 12:01:28PM +0200, Duncan Sands wrote:
> > It looks like it ought to be 0x45 everywhere, meaning it's currently
> > broken everywhere. But no one's complained. Unfortunately at the
> > present moment I've got one machine short of a test rig unpacked, so
> > I'm loathe to push another fix until I get some other test reports.
> 
> Hi Matt, I agree that it should be 0x45 everywhere.  After thinking a bit
> I concluded that the
> 
> #if defined(__LITTLE_ENDIAN_BITFIELD)
>         __u8    ihl:4,
>                 version:4;
> #elif defined (__BIG_ENDIAN_BITFIELD)
>         __u8    version:4,
>                 ihl:4;
> 
> in the definition of struct iphdr is to make sure that compiler uses the
> first four bits of the byte to refer to version, rather than the last four;
> and this only matters when you are accessing the nibbles via the ihl
> or version structure fields.  Thus it makes sure that if you write 0x45
> to the byte, then version will return 4 and ihl will return 5.  Presumably
> the C standard specifies how bitfield expressions should be laid out
> in the byte, and ihl:4, version:4; gives opposite results on little-endian
> and big-endian machines...
> 
> Ciao,
> 
> Duncan.

Ok, could you send an updated patch with a Signed-off-by, please?

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] netpoll endian fixes

[Duncan Sands]

> Ok, could you send an updated patch with a Signed-off-by, please?

Here you are:

Correct wrong ip header in netpoll_send_udp.

Signed-off-by: Duncan Sands <baldrick@free.fr>

--- linux-2.5/net/core/netpoll.c.orig	2004-09-09 11:20:43.000000000 +0200
+++ linux-2.5/net/core/netpoll.c	2004-09-09 11:20:58.000000000 +0200
@@ -242,9 +242,9 @@
 	iph = (struct iphdr *)skb_push(skb, sizeof(*iph));
 
 	/* iph->version = 4; iph->ihl = 5; */
-	put_unaligned(0x54, (unsigned char *)iph);
+	put_unaligned(0x45, (unsigned char *)iph);
 	iph->tos      = 0;
-	put_unaligned(htonl(ip_len), &(iph->tot_len));
+	put_unaligned(htons(ip_len), &(iph->tot_len));
 	iph->id       = 0;
 	iph->frag_off = 0;
 	iph->ttl      = 64;

Re: [PATCH] netpoll endian fixes

[Peter Chubb]

>>>>> "Duncan" == Duncan Sands <baldrick@free.fr> writes:

Duncan> Hi Matt, I agree that it should be 0x45 everywhere.  After
Duncan> thinking a bit I concluded that the

Duncan> #if defined(__LITTLE_ENDIAN_BITFIELD) 
Duncan>        __u8 ihl:4, version:4;
Duncan> #elif defined (__BIG_ENDIAN_BITFIELD) 
Duncan>        __u8 version:4, ihl:4;

Duncan> in the definition of struct iphdr is to make sure that
Duncan> compiler uses the first four bits of the byte to refer to
Duncan> version, rather than the last four; and this only matters when
Duncan> you are accessing the nibbles via the ihl or version structure
Duncan> fields.  Thus it makes sure that if you write 0x45 to the
Duncan> byte, then version will return 4 and ihl will return 5.
Duncan> Presumably the C standard specifies how bitfield expressions
Duncan> should be laid out in the byte, and ihl:4, version:4; gives
Duncan> opposite results on little-endian and big-endian machines...

Actually, the C standard

	  -- leaves unspecified whether bitfields start at the low or
	  high address of the word they're in, and

	  -- states that the type of a bitfield may be ignored (except
	  for signed/unsigned), and that the size of the object
	  containing a bitfield is the same as the size of an int.

It's a GCC extension that allows 
     __u8 ihl:4;
to allocate only 8 bits and use four of them; GCC still assumes
integer alignment for the whole bitfield block.  Other compilers could
allocate 32 bits.

-- 
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*