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

[Laurent Vivier <lvivier@redhat.com>]

On 09/01/2025 16:36, Stefano Brivio wrote:
> [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)
> 

Could you update the patch on merge accordingly to your comments?

Thanks,
Laurent