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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 9 Jan 2025 17:47:06 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

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

Ah, yes, sure. I'll just add that line.

-- 
Stefano