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

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

Le 28/05/2020 à 23:58, Andrew Doran a écrit :
> 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?

The functions that begin by __msan_* are called from the compiler-generated
instrumentation directly, not from the kernel (even though they could).

Adding calls to this function from asm would be a big hack that I don't want
in the kernel, and it wouldn't be addressing the real problem, which is, that
the introduction of x86_movs/x86_stos is unnecessary in the first place, and
the way they are used right now is just wrong -- memcpy/memset should have
stayed in place.

The whole change you made is for svs_pdir_switch() to use quads, but why not
use atomic_load_relaxed? With that it should fetch quads without taking the
bus lock, and we don't have to resort to ugly ASM functions.

Thanks,
Maxime