On Thu, May 28, 2020 at 07:06:04PM +0200, Maxime Villard wrote:
> 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.
Very good. I see a function in subr_msan.c that looks like it does the
needful here:
void __msan_instrument_asm_store(void *addr, size_t size)
I don't see any uses in tree. Is there a reason for that?
Thanks,
Andrew
