Skip to contentSkip to conversation

netpoll endian fixes

PATCH In review Duncan Sands 2 participants 7 messages Sep 7, 2004 – Sep 9, 2004 Networking

lore
Share & tools
lore

Duncan Sands

author
#0 lore raw

PATCH 1/1 netpoll endian fixes

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.

net/core/netpoll.c renamed from net/core/netpoll.c.orig +6 -1
@@ -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;
#1 lore raw
Quoted 3 lines · 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...

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.

Signature · 1 line
Mathematics is the supreme nostalgia of our time.
author
#2 lore raw
Quoted 7 lines · Matt Mackall
Quoted 3 lines · 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...

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.

#3 lore raw
In reply to Duncan Sands
Quoted 20 lines · 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.

net/core/netpoll.c renamed from net/core/netpoll.c.orig +1 -0
@@ -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

net/core/netpoll.c line 250

Quoted 8 lines
net/core/netpoll.c renamed from net/core/netpoll.c.orig +4 -0
@@ -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?

Signature · 1 line
Mathematics is the supreme nostalgia of our time.
author
#4 lore raw
Quoted 4 lines

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.

#5 lore raw
Quoted 24 lines · Duncan Sands
Quoted 4 lines

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

Quoted 3 lines

Ciao,

Duncan.

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

Signature · 1 line
Mathematics is the supreme nostalgia of our time.
author
#6 lore raw
Quoted 1 line

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 <[email protected]>
net/core/netpoll.c renamed from net/core/netpoll.c.orig +2 -2
@@ -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;