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

[Stefano Brivio <sbrivio@redhat.com>]

[Cc'ed Mike who reported this]

On Thu,  9 Jan 2025 14:06:48 +0100
Laurent Vivier <lvivier@redhat.com> 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)

Thanks, this description is really helpful.

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

Reported-by: Mike Jones <mike@mjones.io>

> Link: https://bugs.passt.top/show_bug.cgi?id=108
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  checksum.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/checksum.c b/checksum.c
> index 1c4354d35734..2fd6867cdf75 100644
> --- a/checksum.c
> +++ b/checksum.c
> @@ -452,7 +452,7 @@ uint32_t csum_unfolded(const void *buf, size_t len, uint32_t init)
>  	intptr_t align = ROUND_UP((intptr_t)buf, sizeof(__m256i));
>  	unsigned int pad = align - (intptr_t)buf;
>  
> -	if (len < pad)
> +	if (pad & 1 || len < pad)

I'm fine applying this as it is, because the issue is quite nasty and we
have this great commit message anyway, but for clarity, could we have a
comment mentioning why we're doing this? Something like:

  /* Don't mix sum_16b() and csum_avx2() with odd padding lengths */

(I'm not quite satisfied with it but I find it better than nothing).

>  		pad = len;
>  
>  	if (pad)

-- 
Stefano