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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 9 Jan 2025 18:17:42 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

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

Applied.

-- 
Stefano