Re: [PATCH] checksum: fix checksum with odd base address

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]

On Thu, Jan 09, 2025 at 02:06:48PM +0100, Laurent Vivier wrote:
> csum_unfolded() must call csum_avx2() with a 32byte aligned base address.
> 
> To be able to do that if the buffer is not correctly aligned,
> it splits the buffers in 2 parts, the second part is 32byte aligned and
> can be used with csum_avx2(), the first part is the remaining part, that
> is not 32byte aligned and we use sum_16b() to compute the checksum.
> 
> A problem appears if the length of the first part is odd because
> the checksum is using 16bit words to do the checksum.
> 
> If the length is odd, when the second part is computed, all words are
> shifted by 1 byte, meaning weight of upper and lower byte is swapped.
> 
> For instance a 13 bytes buffer:
> 
> bytes:
> 
> aa AA bb BB cc CC dd DD ee EE ff FF gg
> 
> 16bit words:
> 
> AAaa BBbb CCcc DDdd EEee FFff 00gg
> 
> If we don't split the sequence, the checksum is:
> 
> AAaa + BBbb + CCcc + DDdd + EEee + FFff + 00gg
> 
> If we split the sequence with an even length for the first part:
> 
> (AAaa + BBbb) + (CCcc + DDdd + EEee + FFff + 00gg)
> 
> But if the first part has an odd length:
> 
> (AAaa + BBbb + 00cc) + (ddCC + eeDD + ffEE + ggFF)
> 
> To avoid the problem, do not call csum_avx2() if the first part cannot
> have an even length, and compute the checksum of all the buffer using
> sum_16b().
> 
> This is slower but it can only happen if the buffer base address is odd,
> and this can only happen if the binary is built using '-Os', and that
> means we have chosen to prioritize size over speed.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=108
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

In that it's a real bug and we need to fix it quickly.

That said, I think we can do a bit better long term: I believe it
should be possible to correct the value from the of-by-one
csum_avx2(), I think with just an unconditional byteswap.  The TCP/UDP
checksum has the curious property that it doesn't matter if you
compute it big-endian or little-endian, as long as you're consistent.
We already rely on this.  Having one odd byte piece essentially means
we're using inconsistent endianness between the two pieces.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]