Re: [stos] Re: CVS commit: src/sys/arch

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 27/05/2020 à 21:58, Maxime Villard a écrit :
> Le 27/05/2020 à 21:33, Andrew Doran a écrit :
> > Module Name:    src
> > Committed By:    ad
> > Date:        Wed May 27 19:33:40 UTC 2020
> >
> > Modified Files:
> >     src/sys/arch/amd64/amd64: cpufunc.S locore.S
> >     src/sys/arch/i386/i386: cpufunc.S locore.S
> >     src/sys/arch/x86/include: pmap.h
> >     src/sys/arch/x86/x86: pmap.c
> >
> > Log Message:
> > - Add a couple of wrapper functions around STOS and MOVS and use them to zero
> >    and copy PTEs in preference to memset()/memcpy().
> >
> > - Remove related SSE / pageidlezero stuff.
> >
> >
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.56 -r1.57 src/sys/arch/amd64/amd64/cpufunc.S
> > cvs rdiff -u -r1.208 -r1.209 src/sys/arch/amd64/amd64/locore.S
> > cvs rdiff -u -r1.42 -r1.43 src/sys/arch/i386/i386/cpufunc.S
> > cvs rdiff -u -r1.184 -r1.185 src/sys/arch/i386/i386/locore.S
> > cvs rdiff -u -r1.121 -r1.122 src/sys/arch/x86/include/pmap.h
> > cvs rdiff -u -r1.395 -r1.396 src/sys/arch/x86/x86/pmap.c
> >
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
>
>      -END(x86_stos)
>      +END(x86_movs)
>
> The naming convention should also be more explicit I think, because movs
> does not say if it is b/w/l/q. Don't have anything meaningful to suggest
> though

I see that syzbot-kMSan is failing because of this change. Looking at it more
closely:

 - Having MI ASM copy functions is unwanted, because the sanitizers won't be
   able to sanitize the accesses. kMSan misses the initialization and reports
   false positives. As well kASan will miss memory corruptions and kCSan will
   miss races.

 - It appears you replaced memcpy/memset by x86_movs/x86_stos in places where
   it is unnecessary and unwanted. Eg the majority of the changes in pmap.c
   are unwanted and should remain memcpy/memset.

Please revert this change promptly in order to unbreak kMSan. We really need
to have a clear policy on which function should be in ASM, and not introduce
wild MI things like that which not only are rarely justified but also are a
big PITA when sanitizers come into play.

Thanks,
Maxime