linux-kernel - Re: [PATCH 05/17] Blackfin: convert to generic checksum code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200906191433.34608.arnd@arndb.de>
Date:	Fri, 19 Jun 2009 14:33:33 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Mike Frysinger <vapier.adi@...il.com>
Cc:	linux-kernel@...r.kernel.org,
	uclinux-dist-devel@...ckfin.uclinux.org,
	Michal Simek <monstr@...str.eu>,
	Paul Mundt <lethal@...ux-sh.org>
Subject: Re: [PATCH 05/17] Blackfin: convert to generic checksum code

On Friday 19 June 2009, Mike Frysinger wrote:
> On Fri, Jun 19, 2009 at 05:05, Arnd Bergmann wrote:
> > x86 and blackfin are both little-endian, so your variant is correct
> > there. They add the 0x01 to the low byte of the 16-bit word, while
> > on big-endian machines, you have to add it to the high byte.
> 
> can we think of enough simple examples to through together an optional
> boot-time self test ?

sounds like a good idea.

> > I've committed the patch below now.
> 
> closer, but not quite just yet ...
> do_csum: mismatch 0x1700 != 0xa0d len:2
> do_csum: {  0xd, 0xa }

I've compared the code again with Paul's sh version and found another
difference in the handling of unaligned data.

> when do_csum does return correct values, csum_partial sometimes does not:
> csum_partial: mismatch 0x1101eefe != 0xffff len:32
> csum_partial: {  0x92, 0x3b, 0x0, 0x17, 0xcf, 0xc1, 0x90, 0xec, 0x1c,
> 0x3f, 0xff, 0x99, 0x80, 0x10, 0x0, 0x5c, 0x22, 0xfa, 0x0, 0x0, 0x1,
> 0x1, 0x8, 0xa, 0x6, 0x83, 0xdd, 0x50, 0xff, 0xfe, 0xdf, 0x58 }

That looks like it's both valid output. csum_partial returns a 32 bit
value that always needs to get passed to csum_fold in order to get
checked. 0x1101eefe and 0xffff both evaluate to 0xffff in csum_fold,
so this would just be an implementation detail, not a bug.

> btw, would it make sense to change do_csum like so:
> -static unsigned int do_csum(const unsigned char *buff, int len)
> +__weak unsigned int do_csum(const unsigned char *buff, int len)
> 
> after all, it looks like that's really the function most arches would
> want to hand optimize.  all the rest are simple standard wrappers are
> do_csum().  forcing an arch to copy & paste all the other functions
> just so they can implement an optimized do_csum() seems pretty
> unfriendly.  plus, it isnt like being static here makes any optimized
> difference -- it isnt going to get inlined anywhere in this file.

Agreed, this should be overridable by the architecture, I'll submit a patch
once we found out what caused the bugs.

> or rather than weaks (in case some toolchains force indirect pointer
> loading all the time), use the same interface as the rest of the
> generic checksum code:
> #ifndef do_csum
> static inline unsigned short from32to16(unsigned long x)
> {
>     ...
> }
> static unsigned int do_csum(const unsigned char *buff, int len)
> {
>     ...
> }
> #endif

I'd prefer the second version, I've done it that way all over
include/asm-generic, and weak functions tend to cause more trouble.

---
lib/checksum.c: Fix another endianess bug

will fold this patch into the previous one.

Signed-off-by: Arnd Bergmann <arnd@...db.de>
--- a/lib/checksum.c
+++ b/lib/checksum.c
@@ -55,7 +55,11 @@ static unsigned int do_csum(const unsigned char *buff, int len)
 		goto out;
 	odd = 1 & (unsigned long) buff;
 	if (odd) {
+#ifdef __LITTLE_ENDIAN
 		result = *buff;
+#else
+		result += (*buff << 8);
+#endif
 		len--;
 		buff++;
 	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/