[RISC-V] Fix context restoration as #101709 describes #101865
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-policy-service
bot
added
the
community-contribution
t-mustafin
changed the title
|
Link to original discussion #101709. |
The t4 could get a random value in case of the race and in case the stack would overwrite the context. Would there be a way to do something along the lines I did for arm (32 bit) in my PR - storing the t4 value below the SP that we are restoring to, restoring the SP to the location of the stored t4 and popping it off the stack? That would keep it safe. I am not familiar with the RISC-V instruction set though, so maybe it cannot be done there. And in case the ABI supports red zone, it would need to be done slightly differently to not to corrupt it. |
t-mustafin
force-pushed
the
riscv_context_restore_101709
branch
from
c4ec523
to
1e6b313
Compare
|
@janvorli Thanks for detailed explanation! In the recent commit Regarding red zone, I did not find reliable information for riscv64. |
|
|
|
Sorry, I don't know either. CC @dotnet/samsung Does anyone know about redzone in riscv? |
@t-mustafin is right, there is no red zone on RISC-V. This wording was added to our ABI to clarify it:
|
RISC-V test results for qemu-prio0-checked: 2717 / 2722 (99.82%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
RISC-V test results for starfive-prio0-checked: 2720 / 2722 (99.93%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
RISC-V test results for qemu-prio1-checked: 9396 / 9401 (99.95%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
RISC-V test results for starfive-prio1-checked: 9399 / 9401 (99.98%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
RISC-V test results for starfive-prio1-checked: 9399 / 9402 (99.97%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
RISC-V test results for qemu-prio1-checked: 9397 / 9402 (99.95%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
RISC-V test results for qemu-prio1-checked (
|
|
@janvorli can you please take a look? |
janvorli
reviewed
Jun 5, 2024
| ld t1, (CONTEXT_Pc)(t4) // Since we cannot control $pc directly, we're going to corrupt t1 | ||
| ld t4, (CONTEXT_T4)(t4) | ||
| ld sp, (CONTEXT_Sp)(t4) | ||
| ld t4, -8(sp) |
There was a problem hiding this comment.
I am not sure if I read this correctly, but this seems to still have an issue. It seems that if an async signal occurs after we load the sp, but before we load t4 from (sp-8), the t4 value could be corrupted by the stack of the async signal handler.
We would need to load the sp with an address that is 8 bytes below the target SP, then load the t4 from there, then increment sp by 8 and finally jump to t1. I have attempted to write that in RISC-V code below, please take it with a grain of salt as this is my first RISC-V code ever.
ld t4, (CONTEXT_Sp)(t4)
addi sp, t4, -8
ld t4, (sp)
addi sp, sp, 8
jr t1
There was a problem hiding this comment.
Thanks, seems your solution should work undependable to fields of context structure.
RISC-V test results for starfive-prio1-checked (
|
t-mustafin
force-pushed
the
riscv_context_restore_101709
branch
from
1e6b313
to
3599429
Compare
tomeksowi
approved these changes
Jun 24, 2024
RISC-V test results for qemu-prio1-checked (
|
This was referenced Jun 24, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@janvorli Is it proper fix? Seems for race condition case it would trash 2 temporary registers - t1 and t4, so it may be better completely remove t4 restoration
ld t4, (CONTEXT_T4)(t4).cc @jakobbotsch cc @dotnet/samsung