| Message ID | 20161103051055.GA8368@fergus.ozlabs.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Paul Mackerras <paulus@ozlabs.org> writes: > These functions compute an IP checksum by computing a 64-bit sum and > folding it to 32 bits (the "nofold" in their names refers to folding > down to 16 bits). However, doing (u32) (s + (s >> 32)) is not > sufficient to fold a 64-bit sum to 32 bits correctly. The addition > can produce a carry out from bit 31, which needs to be added in to > the sum to produce the correct result. > > To fix this, we copy the from64to32() function from lib/checksum.c > and use that. This seems to have been broken since ~forever. Do we just not hit that case very often, or do we just incorrectly report checksum failures? Should it go to stable? cheers
On Tue, Nov 08, 2016 at 06:23:30PM +1100, Michael Ellerman wrote: > Paul Mackerras <paulus@ozlabs.org> writes: > > > These functions compute an IP checksum by computing a 64-bit sum and > > folding it to 32 bits (the "nofold" in their names refers to folding > > down to 16 bits). However, doing (u32) (s + (s >> 32)) is not > > sufficient to fold a 64-bit sum to 32 bits correctly. The addition > > can produce a carry out from bit 31, which needs to be added in to > > the sum to produce the correct result. > > > > To fix this, we copy the from64to32() function from lib/checksum.c > > and use that. > > This seems to have been broken since ~forever. Do we just not hit that > case very often, or do we just incorrectly report checksum failures? I think there would be about a 1 in a billion chance of hitting it by chance, though you could probably construct a test case that would hit it every time. If you did hit it in real life it would result in a packet being dropped and presumably retransmitted, and I expect that the IP header of the retransmitted packet would be sufficiently different (i.e. different id field or something) that it wouldn't hit the bug a second time. > Should it go to stable? Probably... though nobody has actually noticed a problem in real life and pinned it down to this. Paul.
Paul Mackerras <paulus@ozlabs.org> writes: > These functions compute an IP checksum by computing a 64-bit sum and > folding it to 32 bits (the "nofold" in their names refers to folding > down to 16 bits). However, doing (u32) (s + (s >> 32)) is not > sufficient to fold a 64-bit sum to 32 bits correctly. The addition > can produce a carry out from bit 31, which needs to be added in to > the sum to produce the correct result. > > To fix this, we copy the from64to32() function from lib/checksum.c > and use that. This collided with: f9d4286b9516 ("arch/powerpc: Update parameters for csum_tcpudp_magic & csum_tcpudp_nofold") Mind rebasing? cheers
On Thu, 2016-11-03 at 05:10:55 UTC, Paul Mackerras wrote: > These functions compute an IP checksum by computing a 64-bit sum and > folding it to 32 bits (the "nofold" in their names refers to folding > down to 16 bits). However, doing (u32) (s + (s >> 32)) is not > sufficient to fold a 64-bit sum to 32 bits correctly. The addition > can produce a carry out from bit 31, which needs to be added in to > the sum to produce the correct result. > > To fix this, we copy the from64to32() function from lib/checksum.c > and use that. > > Signed-off-by: Paul Mackerras <paulus@ozlabs.org> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/b492f7e4e07a28e706db26cf4943bb cheers
diff --git a/arch/powerpc/include/asm/checksum.h b/arch/powerpc/include/asm/checksum.h index ee655ed..c16c6f8 100644 --- a/arch/powerpc/include/asm/checksum.h +++ b/arch/powerpc/include/asm/checksum.h @@ -53,19 +53,27 @@ static inline __sum16 csum_fold(__wsum sum) return (__force __sum16)(~((__force u32)sum + tmp) >> 16); } +static inline u32 from64to32(u64 x) +{ + /* add up 32-bit and 32-bit for 32+c bit */ + x = (x & 0xffffffff) + (x >> 32); + /* add up carry.. */ + x = (x & 0xffffffff) + (x >> 32); + return (u32)x; +} + static inline __wsum csum_tcpudp_nofold(__be32 saddr, __be32 daddr, unsigned short len, unsigned short proto, __wsum sum) { #ifdef __powerpc64__ - unsigned long s = (__force u32)sum; + u64 s = (__force u32)sum; s += (__force u32)saddr; s += (__force u32)daddr; s += proto + len; - s += (s >> 32); - return (__force __wsum) s; + return (__force __wsum) from64to32(s); #else __asm__("\n\ addc %0,%0,%1 \n\ @@ -127,8 +135,7 @@ static inline __wsum ip_fast_csum_nofold(const void *iph, unsigned int ihl) for (i = 0; i < ihl - 1; i++, ptr++) s += *ptr; - s += (s >> 32); - return (__force __wsum)s; + return (__force __wsum)from64to32(s); #else __wsum sum, tmp;
These functions compute an IP checksum by computing a 64-bit sum and folding it to 32 bits (the "nofold" in their names refers to folding down to 16 bits). However, doing (u32) (s + (s >> 32)) is not sufficient to fold a 64-bit sum to 32 bits correctly. The addition can produce a carry out from bit 31, which needs to be added in to the sum to produce the correct result. To fix this, we copy the from64to32() function from lib/checksum.c and use that. Signed-off-by: Paul Mackerras <paulus@ozlabs.org> --- arch/powerpc/include/asm/checksum.h | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)