Dear Joakim Tjernlund,

In message OFDFF8A95F.374EB267-ONC1257681.0042613C-C1257681.004485DD@transmode.se you wrote:

...

...

•   if (len == 1) {
  

•           xsum += (*p & 0xff00);
  

I doubt that this code is endianess-clean.

Nope, I would think some thing like this would work better: count = len >> 1; /* div by 2 */ for(p--; count; --count) xsum += *++p;

if (len & 1) /* Odd */ xsum += *(u_char *)(++p); /* one byte only */

It probably depends on the definition of "better" ;-)

Running this test code:

----------------------------------------- #include <stdio.h> #include <string.h>

#define LENGTH 6

int main (void) { char string[LENGTH] = { 1, 2, 3, 4, 5, 0, }; unsigned short array[LENGTH/2]; unsigned short *p; unsigned short xsum, xsum1, xsum2; int i, count;

memcpy (array, string, LENGTH);

count = LENGTH / 2;

xsum = 0; p = array;

while (count > 1) { printf ("Adding 0x%04x\n", *p); xsum += *p++; --count; }

xsum1 = xsum + (*p & 0xff00);

xsum2 = xsum + *(unsigned char *)(p);

printf ("*p = 0x%04x\n", *p); printf ("xsum = %04x, xsum1 = %04x, xsum2 = %04x\n", xsum, xsum1, xsum2); return (0); } -----------------------------------------

on a little endian system gives:

-> ./f-le Adding 0x0201 Adding 0x0403 *p = 0x0005 xsum = 0604, xsum1 = 0604, xsum2 = 0609

while running it on a big endian system gives

-> ./f-be Adding 0x0102 Adding 0x0304 *p = 0x0500 xsum = 0406, xsum1 = 0906, xsum2 = 040b

I don't know what you consider to be the exact result, but fact is that even if we ignore byte swapping, none of the implementations is endianess clean.

Of course, there is a chance that I mis-implemented your suggestion.

Best regards,

Wolfgang Denk

Thanks.

It was not a clean solution as I only experimented on our processor which is big-endian.

The fact is that the original code is not endianess proof. It was coded for big-endian processors.

Greg Ren

-----Original Message----- From: Joakim Tjernlund [mailto:joakim.tjernlund@transmode.se] Sent: Thursday, December 03, 2009 6:41 AM To: Wolfgang Denk Cc: Greg Ren; u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH]Fix checksum to handle odd-length packet

Wolfgang Denk wd@denx.de wrote on 03/12/2009 15:08:24:

Dear Joakim Tjernlund,

In message <OFDFF8A95F.374EB267-ONC1257681.0042613C-C1257681. 004485DD@transmode.se> you wrote:

...

...

...

•   if (len == 1) {
  

•           xsum += (*p & 0xff00);
  

I doubt that this code is endianess-clean.

Nope, I would think some thing like this would work better: count = len >> 1; /* div by 2 */ for(p--; count; --count) xsum += *++p;

if (len & 1) /* Odd */ xsum += *(u_char *)(++p); /* one byte only */

It probably depends on the definition of "better" ;-)

Running this test code:

---

#include <stdio.h> #include <string.h>

#define LENGTH 6

int main (void) { char string[LENGTH] = { 1, 2, 3, 4, 5, 0, }; unsigned short array[LENGTH/2]; unsigned short *p; unsigned short xsum, xsum1, xsum2;

ulong, not short :)

int i, count;

memcpy (array, string, LENGTH);

count = LENGTH / 2;

xsum = 0; p = array;

while (count > 1) { printf ("Adding 0x%04x\n", *p); xsum += *p++; --count; }

for(p--; count; --count) xsum += *++p;

if (LENGTH & 1) /* Odd */ xsum += *(unsigned char *)(++p); /* one byte only */

xsum1 = xsum + (*p & 0xff00);

ehh, this has to go.

xsum2 = xsum + *(unsigned char *)(p);

You are not folding the sum, unsure if that has any significans

printf ("*p = 0x%04x\n", *p); printf ("xsum = %04x, xsum1 = %04x, xsum2 = %04x\n", xsum, xsum1, xsum2);

return (0); }

---

on a little endian system gives:

-> ./f-le Adding 0x0201 Adding 0x0403 *p = 0x0005 xsum = 0604, xsum1 = 0604, xsum2 = 0609

while running it on a big endian system gives

-> ./f-be Adding 0x0102 Adding 0x0304 *p = 0x0500 xsum = 0406, xsum1 = 0906, xsum2 = 040b

I don't know what you consider to be the exact result, but fact is that even if we ignore byte swapping, none of the implementations is endianess clean.

Of course, there is a chance that I mis-implemented your suggestion.

Best regards,

Wolfgang Denk

-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de There is enough for the need of everyone in this world, but not for the greed of everyone. - Mahatma Gandhi

Ah. I just realize that the endianess can be supported as simple as:

In stead of this: xsum += (*p & 0xff00); Use this: xsum += (*p & ntohs(0xff00));

Did you look at the suggestion I sent you? I know it works because I use in ospf.

Jocke

Hi Greg,

Greg Ren wrote:

I must have missed it. The only suggestion that I remember was referring to how to generate patch using git email. As for technical discussion, it was ended as "not endianess clean".

Please don't top post. While I agree that the network code should handle this properly, how do you end up with odd-length packets? U-boot's been around for years and nobody else has had this problem...

regards, Ben

Hi Greg,

Greg Ren wrote:

Hi Ben:

Ben Warren wrote:

...

Please don't top post. While I agree that the network code should handle this properly, how

do

...

you end up with odd-length packets? U-boot's been around for years

and

...

nobody else has had this problem...

I simply ping it with odd length. I also checked the latest u-boot online and the issue has not been fixed. It could be that most usage had involved only with even length packet, and no one had seen this issue. It is a boot loader and may not get used as often as some popular applications.

Regards

Greg

When you respin your patch, please put the "ping with an odd length causes an incorrect checksum bug" in the commit message for future reference.

Thanks, gvb

Greg Ren wrote:

Jerry Van Baren wrote:

...

When you respin your patch, please put the "ping with an odd length causes an incorrect checksum bug" in the commit message for future reference.

...

Thanks, gvb

The change was to the general checksum calculation. So there is enough reason to believe that any odd-length packet may suffer the same fate. Ping is just an easier way to test and verify the fix.

regards Greg Ren

Understood. My point was your original commit message did not identify *how* to exercise the bug (i.e. ping with an odd length packet). That is very valuable information because it helps us remember what was broken and how to test both the brokenness and the fix.

Thanks, gvb