Use QueueUserAPC2 for EE suspension on CoreCLR win-arm64. #101891
Conversation
|
Tagging subscribers to this area: @mangod9 |
|
Do we also need to enable the code around It sounds like we missed porting the return address hijacking part of the CET work to native AOT. Do you recall whether we have tested native AOT with CET enabled? |
|
Supporting CET in NativeAOT was always something that we would want to do eventually, but we never got to actually doing it. Using APC2 for suspension was mostly because in NativeAOT architecture it was easy to do and there are performance advantages. It was not a part of CET package. |
…mentation for now.
|
Do you plan to enable |
Implementing that would mean also implementing and testing the STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT support for arm64. From looking at Windows side of things, it appears that nothing of that is implemented and I think I will "implement" GetReturnAddressHijackTarget for arm64 just enough to have fewer |
| @@ -4747,7 +4747,7 @@ void Thread::HijackThread(ReturnKind returnKind, ExecutionState *esb) | |||
|
|
|||
| VOID *pvHijackAddr = reinterpret_cast<VOID *>(OnHijackTripThread); | |||
|
|
|||
| #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) | |||
| #if defined(TARGET_WINDOWS) | |||
| #elif defined(TARGET_WINDOWS) && defined(TARGET_AMD64) | ||
| // Only versions of Windows that have the special user mode APC have a correct implementation of the return address hijack handling | ||
| if (Thread::UseSpecialUserModeApc()) | ||
| #elif defined(TARGET_WINDOWS) |
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
src/coreclr/vm/threadsuspend.cpp
Outdated
| // Only versions of Windows that have the special user mode APC have a correct implementation of the return address hijack handling | ||
| if (Thread::UseSpecialUserModeApc()) | ||
| #elif defined(TARGET_WINDOWS) | ||
| if (Thread::AreCetShadowStacksEnabled()) |
There was a problem hiding this comment.
If you change this condition like this, the conditions in STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT handling can be simplified to if (Thread::AreCetShadowStacksEnabled() && (pExceptionInfo->ExceptionRecord->ExceptionCode == STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT))
There was a problem hiding this comment.
I thought about this and decided not to tie one condition to another.
STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT path can work if CET is not enabled. We had it that way, although it was a bit wasteful - lots of exceptions thrown/caught.
I can see such scenario enabled though, at least for test purposes. I did that a few times by turning the condition here into if(true), so I was hesitant to condition the ability to handle STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT on enablement of shadow stacks.
Perf savings from not looking at ExceptionCode are probably very minor.
There was a problem hiding this comment.
if we condition ability to handle STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT on enablement of CET, then some code inside support for STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT can also be simplified. - the places under
if (areCetShadowStacksEnabled)There was a problem hiding this comment.
Ok, the handling of STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT can stay as is then.
I have noticed that there is a bug in STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT handling that will show up when multiple runtimes in the process use it. We should bail if the thread is null or if we are not in cooperative mode. We know that it is some other code triggering STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT in that case.
There was a problem hiding this comment.
We should bail if the thread is null or if we are not in cooperative mode.
I think that would still allow for scenario where we try handling somebody's else hijacked method.
We may need to check for the code ownership - like have IsManaged() somewhere.
There was a problem hiding this comment.
Although we would not be cooperative if running somebody else's code.
There was a problem hiding this comment.
Although we would not be cooperative if running somebody else's code.
Right
|
Thanks!! |
) * use APC2 on arm64 * Made AreCetShadowStacksEnabled available on arm64, with trivial implementation for now. * Enabled querying for GetReturnAddressHijackTarget on arm64 * Apply suggestions from code review Co-authored-by: Jan Kotas <jkotas@microsoft.com> * updated #endif * not handling STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT if not in coop mode. * Assert that a hijack target is provided when CET is enabled * Nit * AreCetShadowStacksEnabled -> AreShadowStacksEnabled * Update src/coreclr/vm/threadsuspend.cpp * Update src/coreclr/vm/threads.cpp --------- Co-authored-by: Jan Kotas <jkotas@microsoft.com>
) * use APC2 on arm64 * Made AreCetShadowStacksEnabled available on arm64, with trivial implementation for now. * Enabled querying for GetReturnAddressHijackTarget on arm64 * Apply suggestions from code review Co-authored-by: Jan Kotas <jkotas@microsoft.com> * updated #endif * not handling STATUS_RETURN_ADDRESS_HIJACK_ATTEMPT if not in coop mode. * Assert that a hijack target is provided when CET is enabled * Nit * AreCetShadowStacksEnabled -> AreShadowStacksEnabled * Update src/coreclr/vm/threadsuspend.cpp * Update src/coreclr/vm/threads.cpp --------- Co-authored-by: Jan Kotas <jkotas@microsoft.com>
EE Suspension scheme that uses signal-like APIs scales better on manycore machines and should be used when available.
NativeAOT uses
QueueUserAPC2on win-arm64 since 8.0Re: #80087