-
Notifications
You must be signed in to change notification settings - Fork 17.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
runtime: fix crash during VDSO calls on arm #34030
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
This PR (HEAD: 6ccc156) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/192937 to see it. Tip: You can toggle comments from me using the |
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Ian Lance Taylor: Patch Set 1: Do you have a test case that you can add? A change like this should really have a test case. I think that a better fix, that should work for all targets, would be to have Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Yuichi Nishiwaki: Patch Set 1:
I only have the test case you made. I will add it later.
Can we really enter go functions without setting up the G register? Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Ian Lance Taylor: Patch Set 1:
We can if the functions are marked //go:nosplit and if the function does not produce any write barriers. The latter will be enforced because sigtrampgo is marked //go:nowritebarrierrec. It might be necessary to change inVDSOPage to use Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Yuichi Nishiwaki: Patch Set 1:
Fair enough. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Ian Lance Taylor: Patch Set 1:
Why do we have to? Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Yuichi Nishiwaki: Patch Set 1:
I thought you meant that we would insert a branch in front of the body of sigtrampgo that conditionally restores the value of G. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Ian Lance Taylor: Patch Set 1:
If we could store the G value before entering a VDSO function in such a way that the signal handler can retrieve it, that would be nice. But I don't see how to do that in general. So what I'm suggesting is that if we are in the VDSO page on a GOARCH that stores the g value in a register, that we adjust the code to not require the g value. Any code that looks at will need a g == nil variant. I think this should be doable. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
This PR (HEAD: 3a273a9) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/192937 to see it. Tip: You can toggle comments from me using the |
Message from Yuichi Nishiwaki: Patch Set 2:
I roughly reimplemented the patch following your suggestion, and added a test case.
Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Ian Lance Taylor: Patch Set 2: (4 comments)
You need to add a "//go:nosplit" comment before inVDSOPage. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Yuichi Nishiwaki: Patch Set 3: Aw, I was misunderstanding the document. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
This PR (HEAD: 808b03f) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/192937 to see it. Tip: You can toggle comments from me using the |
Message from Ian Lance Taylor: Patch Set 4:
You can get the PC at which the signal occurred via Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
This PR (HEAD: 4b53547) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/192937 to see it. Tip: You can toggle comments from me using the |
Message from Yuichi Nishiwaki: Patch Set 4:
It works perfectly in my environment! Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Ian Lance Taylor: Patch Set 5: (2 comments) Thanks. Besides small change below, please update the description to describe the current code. Also we usually put the Fixes line at the end of the description. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Yuichi Nishiwaki: Patch Set 6: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
This PR (HEAD: dac3021) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/192937 to see it. Tip: You can toggle comments from me using the |
Message from Yuichi Nishiwaki: Patch Set 7:
Updated the patch and the description. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Ian Lance Taylor: Patch Set 8:
Thanks. The description doesn't seem to be updated. You need to update the initial comment in GitHub to get that reflected in Gerrit. See https://golang.org/wiki/CommitMessage. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Yuichi Nishiwaki: Patch Set 8:
Seems like my edits on Gerrit disappeared. I have just updated again. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Ian Lance Taylor: Patch Set 9: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
This PR (HEAD: 00b1081) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/192937 to see it. Tip: You can toggle comments from me using the |
Message from Yuichi Nishiwaki: Patch Set 10:
OK, reflected your comment. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Ian Lance Taylor: Patch Set 10: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
The crash occurs when go runtime calls a VDSO function (say __vdso_clock_gettime) and a signal arrives to that thread. Since VDSO functions temporarily destroy the G register (R10), Go functions asynchronously executed in that thread (i.e. Go's signal handler) can try to load data from the destroyed G, which causes segmentation fault.
This PR (HEAD: 28ce42c) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/192937 to see it. Tip: You can toggle comments from me using the |
Message from Ian Lance Taylor: Patch Set 11: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Gobot Gobot: Patch Set 11: TryBots beginning. Status page: https://farmer.golang.org/try?commit=60a69abc Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
Message from Gobot Gobot: Patch Set 11: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
As discussed in #32912, a crash occurs when go runtime calls a VDSO function (say __vdso_clock_gettime) and a signal arrives to that thread. Since VDSO functions temporarily destroy the G register (R10), Go functions asynchronously executed in that thread (i.e. Go's signal handler) can try to load data from the destroyed G, which causes segmentation fault. To fix the issue a guard is inserted in front of sigtrampgo, so that the control escapes from signal handlers without touching G in case the signal occurred in the VDSO context. The test case included in the patch is take from discussion in a relevant thread on github: #32912 (comment). This patch not only fixes the issue on AArch64 but also that on 32bit ARM. Fixes #32912 Change-Id: I657472e54b7aa3c617fabc5019ce63aa4105624a GitHub-Last-Rev: 28ce42c GitHub-Pull-Request: #34030 Reviewed-on: https://go-review.googlesource.com/c/go/+/192937 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Message from Ian Lance Taylor: Patch Set 11: Code-Review+2 Thanks for your patience with this. Please don’t reply on this GitHub thread. Visit golang.org/cl/192937. |
This PR is being closed because golang.org/cl/192937 has been merged. |
This commit fixes issue golang#34391, which is due to an incorrect patch merged in golang#34030. sigtrampgo is modified to record incoming signals in a globally shared atomic bitmask during when the G register is clobbered. When the execution exits from vdso it checks if there is a pending signal it re-raises them to its own process.
This commit fixes issue golang#34391, which is due to an incorrect patch merged in golang#34030. sigtrampgo is modified to record incoming signals in a globally shared atomic bitmask when the G register is clobbered. When the execution exits from vdso it checks if there is a pending signal and in that case it re-raises them to its own process.
As discussed in #32912, a crash occurs when go runtime calls a VDSO function (say
__vdso_clock_gettime) and a signal arrives to that thread.
Since VDSO functions temporarily destroy the G register (R10),
Go functions asynchronously executed in that thread (i.e. Go's signal
handler) can try to load data from the destroyed G, which causes
segmentation fault.
To fix the issue a guard is inserted in front of sigtrampgo, so that the control escapes from
signal handlers without touching G in case the signal occurred in the VDSO context.
The test case included in the patch is take from discussion in a relevant thread on github:
#32912 (comment).
This patch not only fixes the issue on AArch64 but also that on 32bit ARM.
Fixes #32912