Re: [stos, again] Re: CVS commit: src/sys/arch/amd64
On 04.06.2020 00:42, Andrew Doran wrote: > On Wed, Jun 03, 2020 at 02:03:22AM +0200, Kamil Rytarowski wrote: > >> On 03.06.2020 01:49, Andrew Doran wrote: >>> On the assembly thing recall that recently you expressed a desire to remove >>> all of the amd64 assembly string functions from libc because of sanitizers - >>> I invested my time to do up a little demo to try and show you why that's not >>> a good idea: >>> >>> http://mail-index.netbsd.org/port-amd64/2020/04/19/msg003226.html >> >> Please note that interceptors for string functions are not just some >> extra burden, but also very useful approach to feedback a fuzzer through >> additional coverage. >> >> At least libFuzzer and honggfuzz wrap many kinds of string functions and >> use it for fuzzing. We should add a special mode in KCOV to feedback >> userland (syzkaller) with traces from string functions. >> >> https://github.com/google/honggfuzz/blob/bbb476eec95ad927d6d7d3d367d2b3e38eed3569/libhfuzz/memorycmp.c#L24 > > No argument from me there at all. I think that's a great idea and was > looking at the interceptors in TSAN recently to see how they work. > > Andrew > My note was not about switching away from ASM functions for certain functions, but rather giving an option to disable them under a sanitizer and adding an interceptor that can be useful for feedbacking a fuzzer. It's still not clear whether we will create such interface in KCOV as it has to be coordinated with Google+Linux as we would like to have a compatible interface for syzkaller. TSAN - do you mean the userland ones? BTW. There is a work-in-progress MKSANITIZER support for TSan, but it used to create unkillable processes (kernel bug). Basically when using a TSanitized userland applications, you will quickly end up with such processes (from AFAIR calling ls(1) and other simple applications are enough). If you are interested, I can share a reproducer.
Re: [stos, again] Re: CVS commit: src/sys/arch/amd64
Maxime, I read your e-mail carefully and conclude that the best way forward here is put this one to core@ for a technical decision. Cheers, Andrew On Wed, Jun 03, 2020 at 08:25:32AM +0200, Maxime Villard wrote: > Le 03/06/2020 ? 01:49, Andrew Doran a ?crit?: > > On Tue, Jun 02, 2020 at 08:41:53AM +0200, Maxime Villard wrote: > > > > > Le 02/06/2020 ? 00:58, Andrew Doran a ?crit?: > > > > Module Name:src > > > > Committed By: ad > > > > Date: Mon Jun 1 22:58:06 UTC 2020 > > > > > > > > Modified Files: > > > > src/sys/arch/amd64/amd64: cpufunc.S > > > > src/sys/arch/amd64/include: frameasm.h > > > > > > > > Log Message: > > > > Reported-by: syzbot+6dd5a230d19f0cbc7...@syzkaller.appspotmail.com > > > > > > > > Instrument STOS/MOVS for KMSAN to unbreak it. > > > > > > > > > > > > To generate a diff of this commit: > > > > cvs rdiff -u -r1.58 -r1.59 src/sys/arch/amd64/amd64/cpufunc.S > > > > cvs rdiff -u -r1.49 -r1.50 src/sys/arch/amd64/include/frameasm.h > > > > > > > > Please note that diffs are not public domain; they are subject to the > > > > copyright notices on the relevant files. > > > > > > Can you just stop ignoring the remarks that are made? > > > > That's up to you Maxime. If you habitually make it difficult for people to > > come to a reasonable compromise with you, then you're asking to not be taken > > seriously and will find yourself being ignored. > > You are confused. I asked for KMSAN to be unbroken, and proposed an > alternative, > which is atomic_load_relaxed. You were free to explain why my point was a bad > idea or why it didn't matter, but you refused, and just added a hack on top of > another. I'm afraid that's up to you. > > But whatever, it doesn't matter. Thanks for reverting the pmap.c changes, it > is more correct now than before. > > > > I said explicitly > > > that adding manual instrumentation here is _wrong_. > > > > I don't share your assessment in the general sense. It should be possible > > to instrument assembly code because KMSAN is useful AND we can't get by > > without assembly - there are some things that C just can't do (or do well > > enough). > > > > On the assembly thing recall that recently you expressed a desire to remove > > all of the amd64 assembly string functions from libc because of sanitizers - > > I invested my time to do up a little demo to try and show you why that's not > > a good idea: > > > > http://mail-index.netbsd.org/port-amd64/2020/04/19/msg003226.html > > I saw, yes. I answered explaining that a conversation with Ryo Shimizu had > changed my mind a bit, and seeing your results (which as far as I can tell > do not indicate a performance improvement significant enough to not be > considered as noise), I asked you whether it was that relevant. You didn't > follow up though. > > > The system is a balancing act. There are lots of factors to be taken into > > account: maintainability, tooling like KMSAN, user's varying needs, the > > capabilites of different machines, performance, feature set and so on, and > > dare I say it even the enjoyment of the people working on the project is > > important too. Myopic focus on one factor only to the detriment of others > > is no good. > > I am well aware of that. > > > > The kMSan ASM fixups > > > are limited to args/returns, and that is part of a sensical policy that > > > _should not_ be changed without a good reason. > > > > > > x86_movs/x86_stos have strictly _no reason_ to exist. Of the 18 > > > conversions > > > you made to them in pmap.c, not one is justified. memcpy/memset were all > > > correct. > > > > I introduced these functions as a compromise because you were unhappy with > > use of memcpy() to copy PDEs. See: > > > > http://mail-index.netbsd.org/port-amd64/2020/05/23/msg003280.html > > > > You wrote: > > > > In the [XXX] window, the PTEs could be used by userland. If you > > copied them using memcpy(), some parts of the bytes could contain > > stale values. > > Sure, I was explicitly referring to SVS, which has an unusual way of > accessing PTEs (contrary to pmap), which is why it needs special atomic > care that other places do not. > > > Live PDEs are also copied in pmap.c so I made a change there too. After > > that I decided "why not" and used the new functions everywhere PDEs/PTEs or > > pages are block zeroed / copied. But I'm also happy with memcpy()/memset(). > > Either way will work. In fairness I do work too fast sometimes. > > > > > The only reason you made these big unneeded changes is for SVS not to take > > > the bus lock, > > > > There is no bus lock on x86 (not since the 1990s anyway). > > > > > but as was said already, atomic_load_relaxed will do what > > > you want without the need for these functions. > > > > > > Please revert _both changes now_, this one and the previous one which > > > introduced both functions, and let's use atomic_load_relaxed. > > > > You're focusing on only
Re: [stos, again] Re: CVS commit: src/sys/arch/amd64
On Wed, Jun 03, 2020 at 02:03:22AM +0200, Kamil Rytarowski wrote: > On 03.06.2020 01:49, Andrew Doran wrote: > > On the assembly thing recall that recently you expressed a desire to remove > > all of the amd64 assembly string functions from libc because of sanitizers - > > I invested my time to do up a little demo to try and show you why that's not > > a good idea: > > > > http://mail-index.netbsd.org/port-amd64/2020/04/19/msg003226.html > > Please note that interceptors for string functions are not just some > extra burden, but also very useful approach to feedback a fuzzer through > additional coverage. > > At least libFuzzer and honggfuzz wrap many kinds of string functions and > use it for fuzzing. We should add a special mode in KCOV to feedback > userland (syzkaller) with traces from string functions. > > https://github.com/google/honggfuzz/blob/bbb476eec95ad927d6d7d3d367d2b3e38eed3569/libhfuzz/memorycmp.c#L24 No argument from me there at all. I think that's a great idea and was looking at the interceptors in TSAN recently to see how they work. Andrew
Re: [stos, again] Re: CVS commit: src/sys/arch/amd64
Le 03/06/2020 à 02:03, Kamil Rytarowski a écrit : On 03.06.2020 01:49, Andrew Doran wrote: On the assembly thing recall that recently you expressed a desire to remove all of the amd64 assembly string functions from libc because of sanitizers - I invested my time to do up a little demo to try and show you why that's not a good idea: http://mail-index.netbsd.org/port-amd64/2020/04/19/msg003226.html Please note that interceptors for string functions are not just some extra burden, but also very useful approach to feedback a fuzzer through additional coverage. At least libFuzzer and honggfuzz wrap many kinds of string functions and use it for fuzzing. We should add a special mode in KCOV to feedback userland (syzkaller) with traces from string functions. https://github.com/google/honggfuzz/blob/bbb476eec95ad927d6d7d3d367d2b3e38eed3569/libhfuzz/memorycmp.c#L24 Yes, and not just that either. When you use ASM instead of C, you basically prevent _any kind_ of useful transformation the compiler could make. It includes sanitizers, but also coverage as you said; and also retpoline, PAC, BTI, CET, SafeStack, and in short, a very big bunch of modern features. Favoring C rather than ASM in the general sense offers much bigger benefits than just "it accomodates kMSan". Maxime
Re: [stos, again] Re: CVS commit: src/sys/arch/amd64
Le 03/06/2020 à 01:49, Andrew Doran a écrit : On Tue, Jun 02, 2020 at 08:41:53AM +0200, Maxime Villard wrote: Le 02/06/2020 ? 00:58, Andrew Doran a ?crit?: Module Name:src Committed By: ad Date: Mon Jun 1 22:58:06 UTC 2020 Modified Files: src/sys/arch/amd64/amd64: cpufunc.S src/sys/arch/amd64/include: frameasm.h Log Message: Reported-by: syzbot+6dd5a230d19f0cbc7...@syzkaller.appspotmail.com Instrument STOS/MOVS for KMSAN to unbreak it. To generate a diff of this commit: cvs rdiff -u -r1.58 -r1.59 src/sys/arch/amd64/amd64/cpufunc.S cvs rdiff -u -r1.49 -r1.50 src/sys/arch/amd64/include/frameasm.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Can you just stop ignoring the remarks that are made? That's up to you Maxime. If you habitually make it difficult for people to come to a reasonable compromise with you, then you're asking to not be taken seriously and will find yourself being ignored. You are confused. I asked for KMSAN to be unbroken, and proposed an alternative, which is atomic_load_relaxed. You were free to explain why my point was a bad idea or why it didn't matter, but you refused, and just added a hack on top of another. I'm afraid that's up to you. But whatever, it doesn't matter. Thanks for reverting the pmap.c changes, it is more correct now than before. I said explicitly that adding manual instrumentation here is _wrong_. I don't share your assessment in the general sense. It should be possible to instrument assembly code because KMSAN is useful AND we can't get by without assembly - there are some things that C just can't do (or do well enough). On the assembly thing recall that recently you expressed a desire to remove all of the amd64 assembly string functions from libc because of sanitizers - I invested my time to do up a little demo to try and show you why that's not a good idea: http://mail-index.netbsd.org/port-amd64/2020/04/19/msg003226.html I saw, yes. I answered explaining that a conversation with Ryo Shimizu had changed my mind a bit, and seeing your results (which as far as I can tell do not indicate a performance improvement significant enough to not be considered as noise), I asked you whether it was that relevant. You didn't follow up though. The system is a balancing act. There are lots of factors to be taken into account: maintainability, tooling like KMSAN, user's varying needs, the capabilites of different machines, performance, feature set and so on, and dare I say it even the enjoyment of the people working on the project is important too. Myopic focus on one factor only to the detriment of others is no good. I am well aware of that. The kMSan ASM fixups are limited to args/returns, and that is part of a sensical policy that _should not_ be changed without a good reason. x86_movs/x86_stos have strictly _no reason_ to exist. Of the 18 conversions you made to them in pmap.c, not one is justified. memcpy/memset were all correct. I introduced these functions as a compromise because you were unhappy with use of memcpy() to copy PDEs. See: http://mail-index.netbsd.org/port-amd64/2020/05/23/msg003280.html You wrote: In the [XXX] window, the PTEs could be used by userland. If you copied them using memcpy(), some parts of the bytes could contain stale values. Sure, I was explicitly referring to SVS, which has an unusual way of accessing PTEs (contrary to pmap), which is why it needs special atomic care that other places do not. Live PDEs are also copied in pmap.c so I made a change there too. After that I decided "why not" and used the new functions everywhere PDEs/PTEs or pages are block zeroed / copied. But I'm also happy with memcpy()/memset(). Either way will work. In fairness I do work too fast sometimes. The only reason you made these big unneeded changes is for SVS not to take the bus lock, There is no bus lock on x86 (not since the 1990s anyway). but as was said already, atomic_load_relaxed will do what you want without the need for these functions. Please revert _both changes now_, this one and the previous one which introduced both functions, and let's use atomic_load_relaxed. You're focusing on only one factor. I'll explain in detail. Here is the original code I replaced: 685 static inline pt_entry_t 686 svs_pte_atomic_read(struct pmap *pmap, size_t idx) 687 { 688/* 689 * XXX: We don't have a basic atomic_fetch_64 function? 690 */ 691return atomic_cas_64(>pm_pdir[idx], 666, 666); 692 } ... 717/* User slots. */ 718for (i = 0; i < PDIR_SLOT_USERLIM; i++) { 719pte = svs_pte_atomic_read(pmap, i); 720ci->ci_svs_updir[i] = pte; 721} There's no need for an atomic op there because fetches on x86 are by definition atomic, and it