Uh oh!
There was an error while loading. Please reload this page.
[RISC-V] Fix context restoration as #101709 describes - #101865
Merged
Conversation
t-mustafin
commented
May 3, 2024
ContributorAuthor
Link to original discussion #101709. |
janvorli
commented
May 6, 2024
Member
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-mustafinforce-pushed
the
riscv_context_restore_101709
branch
from
May 14, 2024 00:04
c4ec523 to
1e6b313Comparet-mustafin
marked this pull request as ready for review
May 14, 2024 00:04
t-mustafin
commented
May 14, 2024
ContributorAuthor
@janvorli Thanks for detailed explanation! In the recent commit Regarding red zone, I did not find reliable information for riscv64. |
ContributorAuthor
|
clamp03
commented
May 16, 2024
Member
Sorry, I don't know either. CC @dotnet/samsung Does anyone know about redzone in riscv? |
tomeksowi
commented
May 16, 2024
Member
@t-mustafin is right, there is no red zone on RISC-V. This wording was added to our ABI to clarify it:
|
risc-vv
commented
May 24, 2024
RISC-V test results for qemu-prio0-checked: 2717 / 2722 (99.82%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
risc-vv
commented
May 24, 2024
RISC-V test results for starfive-prio0-checked: 2720 / 2722 (99.93%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
risc-vv
commented
May 24, 2024
RISC-V test results for qemu-prio1-checked: 9396 / 9401 (99.95%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
risc-vv
commented
May 24, 2024
RISC-V test results for starfive-prio1-checked: 9399 / 9401 (99.98%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
risc-vv
commented
Jun 3, 2024
RISC-V test results for starfive-prio1-checked: 9399 / 9402 (99.97%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
risc-vv
commented
Jun 3, 2024
RISC-V test results for qemu-prio1-checked: 9397 / 9402 (99.95%)detailsGIT: 1e6b313 failed testskilled testsskipped tests |
risc-vv
commented
Jun 5, 2024
RISC-V test results for qemu-prio1-checked ( |
gbalykov
commented
Jun 5, 2024
Member
@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) |
Member
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
ContributorAuthor
There was a problem hiding this comment.
Thanks, seems your solution should work undependable to fields of context structure.
risc-vv
commented
Jun 5, 2024
RISC-V test results for starfive-prio1-checked ( |
t-mustafinforce-pushed
the
riscv_context_restore_101709
branch
from
June 24, 2024 16:07
1e6b313 to
3599429Comparetomeksowi
approved these changes
Jun 24, 2024
risc-vv
commented
Jun 24, 2024
RISC-V test results for qemu-prio1-checked ( |
This was referenced Jun 24, 2024
t-mustafin
commented
Jun 25, 2024
ContributorAuthor
This file contains hidden or 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
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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