[go] runtime/secret: implement new secret package
Daniel Morsing (Gerrit)
Daniel Morsing voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Morsing
| Commit-Queue | +1 |
1 comment
This is ready for review. The trybots are unhappy about the tests using gdb to get the core dump, but I've asked on go-dev about what can be done to fix it. I would very much prefer that the core tests aren't skipped since leakage is a hard problem to detect within the process itself.
Does anything need to happen since Keith wrote most of this code? what's the policy on co-authorship?
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
This is ready for review. The trybots are unhappy about the tests using gdb to get the core dump, but I've asked on go-dev about what can be done to fix it. I would very much prefer that the core tests aren't skipped since leakage is a hard problem to detect within the process itself.
Does anything need to happen since Keith wrote most of this code? what's the policy on co-authorship?
Co-authorship is fine. We've all signed the CLA, which is what matters.
I do think we still need to come to agreement on the pointer leak thing. Maybe it is ok, but I don't have the expertise to make that call.
I will take a look at this CL this week. Sounds like I might be reviewing some of my own code. I'm sure it is awesome 😊
Related details
- Daniel Morsing
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Daniel Morsing
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Filippo Valsorda (Gerrit)
Filippo Valsorda added 1 comment![]( "Open in Gerrit")
Keith RandallThis is ready for review. The trybots are unhappy about the tests using gdb to get the core dump, but I've asked on go-dev about what can be done to fix it. I would very much prefer that the core tests aren't skipped since leakage is a hard problem to detect within the process itself.
Does anything need to happen since Keith wrote most of this code? what's the policy on co-authorship?
Co-authorship is fine. We've all signed the CLA, which is what matters.
I do think we still need to come to agreement on the pointer leak thing. Maybe it is ok, but I don't have the expertise to make that call.
I will take a look at this CL this week. Sounds like I might be reviewing some of my own code. I'm sure it is awesome 😊
I'm ok with leaking pointers if it makes this viable.
The primary use case for runtime/secret is cryptographic keys involved in forward secrecy mechanisms, and cryptographic code already never ever encodes secrets in pointers and offsets, because there is just no way to stop the CPU from leaking them through cache side-channels.
Related details
- Daniel Morsing
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 2 comments![]( "Open in Gerrit")
Keith RandallThis is ready for review. The trybots are unhappy about the tests using gdb to get the core dump, but I've asked on go-dev about what can be done to fix it. I would very much prefer that the core tests aren't skipped since leakage is a hard problem to detect within the process itself.
Does anything need to happen since Keith wrote most of this code? what's the policy on co-authorship?
Filippo ValsordaCo-authorship is fine. We've all signed the CLA, which is what matters.
I do think we still need to come to agreement on the pointer leak thing. Maybe it is ok, but I don't have the expertise to make that call.
I will take a look at this CL this week. Sounds like I might be reviewing some of my own code. I'm sure it is awesome 😊
I'm ok with leaking pointers if it makes this viable.
The primary use case for runtime/secret is cryptographic keys involved in forward secrecy mechanisms, and cryptographic code already never ever encodes secrets in pointers and offsets, because there is just no way to stop the CPU from leaking them through cache side-channels.
That about matches with my expectation. I'm gonna go rack my brain to see if I can come up with a succinct way of describing the secrecy guarantees (hopefully also get some better terminology. Reading the code back, I must have used a dozen different synonyms for "secret" in the comments)
Rebased to resolve the conflict with GC leak detection
Related details
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 9 comments![]( "Open in Gerrit")
Looks pretty good, thanks for taking this up.
// If in secret mode, erase registers on transitionIt seems unfortunate to add this overhead to every systemstack call. We use systemstack a lot in the runtime.
// if we're running secret code. Set a flag to clear them our byout
if xRegs.cache != nil {
memclrNoHeapPointers(unsafe.Pointer(xRegs.cache), unsafe.Sizeof(xRegState{}))
}
xRegs.secret = falseShould this be inside the `if` above?
secretStack stack // the shadow stack for returning into the kernelI was wondering if we could keep a pool of these somewhere else, like in a P or M.
Only the last one allocated per M might still be in use, right? So maybe 2 per M would be enough.
secretStoreAddr := uint64(uintptr(unsafe.Pointer(&secretStore[0])))Will ASLR break this?
func Do(f func()) {We should probably mention the pointer leak thing here if we're going to allow it.
TEXT ·secretEraseRegistersMcall(SB),NOSPLIT|NOFRAME,$0-0Maybe a comment here that we need to avoid clobbering args of mcall.
CMPL AX, AX //eflagsI don't think it really matters, but to be safe use BX here, we know it is cleared.
Related details
- Daniel Morsing
- Filippo Valsorda
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Daniel Morsing
- Filippo Valsorda
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Morsing
| Commit-Queue | +1 |
2 comments
Going to see if I can't get all the trybots happy about the core dump before I address the other comments
secretStoreAddr := uint64(uintptr(unsafe.Pointer(&secretStore[0])))Daniel MorsingWill ASLR break this?
Rewrote the test so that it no longer relies on the address of the secretStore.
Related details
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted Commit-Queue+1![]( "Open in Gerrit")
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted Commit-Queue+1![]( "Open in Gerrit")
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted and added 8 comments![]( "Open in Gerrit")
Votes added by Daniel Morsing
| Commit-Queue | +1 |
8 comments
It seems unfortunate to add this overhead to every systemstack call. We use systemstack a lot in the runtime.
I was working off the old CL, but thinking about it now, systemstack is only called with other go code that should have no reason to spill confidential values onto the stack or into any buffers. Removed
// if we're running secret code. Set a flag to clear them our byDaniel Morsingout
Removed the xregs code because it wasn't needed, Done
if xRegs.cache != nil {
memclrNoHeapPointers(unsafe.Pointer(xRegs.cache), unsafe.Sizeof(xRegState{}))
}
xRegs.secret = falseShould this be inside the `if` above?
Done
secretStack stack // the shadow stack for returning into the kernelI was wondering if we could keep a pool of these somewhere else, like in a P or M.
Only the last one allocated per M might still be in use, right? So maybe 2 per M would be enough.
I think that brings us back to a situation where we'd have to track which M executed secret code, plus now that I've removed the async xRegs code, heap allocations are the only thing that relies on a garbage collection having run after returning from secret code.
If shadow secret stack allocation cost is a factor, there are ways around it. Create a goroutine that sets up a secret and then receives requests for encryption/decryption. That way, we only have to allocate once. That scheme could definitely work for TLS and SSH which are the motivating uses for creating the package.
We should probably mention the pointer leak thing here if we're going to allow it.
Done
TEXT ·secretEraseRegistersMcall(SB),NOSPLIT|NOFRAME,$0-0Maybe a comment here that we need to avoid clobbering args of mcall.
Done
I don't think it really matters, but to be safe use BX here, we know it is cleared.
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
Related details
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jes Cok (Gerrit)
Jes Cok added 1 comment![]( "Open in Gerrit")
CL600635. I have added arm64 support, signal handling, preemptionCL 600635, please add space character. Also I noticed that in this CL, there are some `Copyright 2024` headers which come from that CL, should they be `Copyright 2025`?
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
CL600635. I have added arm64 support, signal handling, preemptionCL 600635, please add space character. Also I noticed that in this CL, there are some `Copyright 2024` headers which come from that CL, should they be `Copyright 2025`?
Done.
As for the copyright notices, those are the files that were created by the previous CL. I'm not sure what the policy is here, but I left them as is.
Related details
- Filippo Valsorda
- Jes Cok
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Filippo Valsorda (Gerrit)
Filippo Valsorda added 1 comment![]( "Open in Gerrit")
Friendly ping, I think this is ready for another round of review?
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Filippo Valsorda (Gerrit)
Filippo Valsorda added 1 comment![]( "Open in Gerrit")
CL600635. I have added arm64 support, signal handling, preemptionDaniel MorsingCL 600635, please add space character. Also I noticed that in this CL, there are some `Copyright 2024` headers which come from that CL, should they be `Copyright 2025`?
Done.
As for the copyright notices, those are the files that were created by the previous CL. I'm not sure what the policy is here, but I left them as is.
The copyright year is the year the file was first mailed to Gerrit, not when it lands.
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 2 comments![]( "Open in Gerrit")
Is it feasible to land this before the go1.26 freeze? Maybe behind an experiment flag if that makes a difference
if sizeSpecializedMallocEnabled && heapBitsInSpan(size) && gp.secret == 0 {Just realised that the specialized mallocs being called directly from the compiler now means that this check doesn't do anything. I'll take a stab at updating mkmalloc to fix this, but I might need some help from @mat...@golang.org
Related details
- Keith Randall
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
if sizeSpecializedMallocEnabled && heapBitsInSpan(size) && gp.secret == 0 {Just realised that the specialized mallocs being called directly from the compiler now means that this check doesn't do anything. I'll take a stab at updating mkmalloc to fix this, but I might need some help from @mat...@golang.org
Hi, I haven't read up on the details of the proposal, but (not having the secret-specific context) I want to mention that not all specialized mallocs are called from the compiler: only those where the compiler knows the size at compile time. So we still need this code path.
Related details
- Daniel Morsing
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
if sizeSpecializedMallocEnabled && heapBitsInSpan(size) && gp.secret == 0 {Michael MatloobJust realised that the specialized mallocs being called directly from the compiler now means that this check doesn't do anything. I'll take a stab at updating mkmalloc to fix this, but I might need some help from @mat...@golang.org
Hi, I haven't read up on the details of the proposal, but (not having the secret-specific context) I want to mention that not all specialized mallocs are called from the compiler: only those where the compiler knows the size at compile time. So we still need this code path.
Ah, the check I am referring to is the `gp.secret == 0` part, not the entire `if` statement
Essentially, the tiny allocator has to be disabled when code is running in secret mode. I tried modifying `malloc_stubs.go` to call into `smallNoScanStub` from the `tinyStub`, but I'm hitting a couple of codegen bugs that I'm not sure how to resolve
Related details
- Keith Randall
- Michael Knyszek
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
if sizeSpecializedMallocEnabled && heapBitsInSpan(size) && gp.secret == 0 {Michael MatloobJust realised that the specialized mallocs being called directly from the compiler now means that this check doesn't do anything. I'll take a stab at updating mkmalloc to fix this, but I might need some help from @mat...@golang.org
Daniel MorsingHi, I haven't read up on the details of the proposal, but (not having the secret-specific context) I want to mention that not all specialized mallocs are called from the compiler: only those where the compiler knows the size at compile time. So we still need this code path.
Ah, the check I am referring to is the `gp.secret == 0` part, not the entire `if` statement
Essentially, the tiny allocator has to be disabled when code is running in secret mode. I tried modifying `malloc_stubs.go` to call into `smallNoScanStub` from the `tinyStub`, but I'm hitting a couple of codegen bugs that I'm not sure how to resolve
Alright, I managed to make switchable allocators work, but I had to rewrite the allocator generator to do so :) CL 719580
I'm unsure whether I should land the allocator change and rebase this CL on top of it, or if I should just roll it into this one. Any thoughts?
Filippo Valsorda (Gerrit)
Filippo Valsorda added 3 comments![]( "Open in Gerrit")
Chatted with @rol...@golang.org, let's hide the exported package (and any dangerous behavior if it's easy) behind a GOEXPERIMENT.
// platforms Do will immediately panic.On unsupported platforms this should just invoke f. We can't litter every call site with build tags.
// - Protection does not extend to any new goroutines made by f.Let's panic if a new goroutine is started. We can always relax it later.
Related details
- Daniel Morsing
Michael Knyszek (Gerrit)
Michael Knyszek added 4 comments![]( "Open in Gerrit")
if sizeSpecializedMallocEnabled && heapBitsInSpan(size) && gp.secret == 0 {Michael MatloobJust realised that the specialized mallocs being called directly from the compiler now means that this check doesn't do anything. I'll take a stab at updating mkmalloc to fix this, but I might need some help from @mat...@golang.org
Daniel MorsingHi, I haven't read up on the details of the proposal, but (not having the secret-specific context) I want to mention that not all specialized mallocs are called from the compiler: only those where the compiler knows the size at compile time. So we still need this code path.
Daniel MorsingAh, the check I am referring to is the `gp.secret == 0` part, not the entire `if` statement
Essentially, the tiny allocator has to be disabled when code is running in secret mode. I tried modifying `malloc_stubs.go` to call into `smallNoScanStub` from the `tinyStub`, but I'm hitting a couple of codegen bugs that I'm not sure how to resolve
Alright, I managed to make switchable allocators work, but I had to rewrite the allocator generator to do so :) CL 719580
I'm unsure whether I should land the allocator change and rebase this CL on top of it, or if I should just roll it into this one. Any thoughts?
allocator bits LGTM, but yeah you will need to add a fallback case in the directly-called-and-generated tiny allocator routines.
func getsp() uintptr {maybe just add this to internal/runtime/sys as GetCurrentSP?
// - Pointer addresses may leak into data buffers used by the runtime
// to perform garbage collection. Users should not encode confidential
// information into pointers.this feels like way overkill to state... it is hard to construct a pointer field that's valid, you don't have very many bits to encode sensitive data into, so this would need be extremely elaborate to not cause random GC crashes.
like:
```
x := new([1<<20]byte)
x = (*[1<<20]byte)(unsafe.Pointer(uintptr(unsafe.Pointer(x))|twentyBitsOfSensitiveData))
```
unless you mean something like, a data structure's actual layout somehow encodes sensitive information? in that case it may be worth stating that explicitly. or perhaps I'm not being creative enough.
// R14 later, but the function is ABI0 here and elsewhere: trailing whitespace
Related details
- Daniel Morsing
- Keith Randall
- Michael Matloob
Cherry Mui (Gerrit)
Cherry Mui added 12 comments![]( "Open in Gerrit")
Thank you for the CL! Not a complete review, just some drive-by comments. Thanks.
// allow any conservative scanning of stacks, as that may leadIt seems reasonable not to conservatively scan the stack containing secrets. However, we could preempt the goroutine for other reasons, like scheduling purposes. I think we can still allow those, just not async preemption for stack scanning.
// (Assembled with gcc. The Go assembler doesn't know these instructions.)
// 62 e1 fd 48 28 c0 vmovapd %zmm0,%zmm16Go assembler does support VMOVAPD Z0, Z16.
// (Assembled with gcc. The Go assembler doesn't know these instructions.)
// c4 e1 fc 47 c0 kxorq %k0,%k0,%k0Go assembler does support KXORQ instruction.
// If we're running secret code, the state on the signal stack might be
// confidential. In which case, we copy the signal stack, and return the stacksize
// and the SP to switch to. sigtramp can then clear the stack and switch to our
// new context stackSo the idea here is that we switch to a different stack, and clear the old signal stack (which could contain secrets), and return to the kernel from a new stack? This might work but it seems weird that we are returning from a different stack. What if there are stack pointers on the stack? What if we are called from another signal handler (probably from non-Go code) that does signal forwarding?
Another approach I can think of is that when we enter secret.Do, we allocate a new signal stack, and clear it when we return out of secret.Do. We don't have to clear it at every signal. Or are we concerned that other, non-Go signal handler could copy things out of our signal stack? Or concerned that the code in secret.Do may call some non-Go code that installs its own signal stack?
runningSecret := atomic.Loadint32(&gp.secret) > 0Why this needs to be atomic?
// TODO(dmo): what is the ABI guarantee here? we use
// R14 later, but the function is ABI0 Good question. I think the assumption here is that it is called from Go, not assembly, so R14 is set. Perhaps we should make this function ABIInternal (at some point).
Remove trailing space in the comment.
BL ·secretEraseRegisters(SB)Nit: too many tabs.
Related details
- Daniel Morsing
- Keith Randall
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Daniel Morsing
- Keith Randall
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted and added 18 comments![]( "Open in Gerrit")
Votes added by Daniel Morsing
| Commit-Queue | +1 |
Chatted with @rol...@golang.org, let's hide the exported package (and any dangerous behavior if it's easy) behind a GOEXPERIMENT.
Done. Will need a new set of builders to test this CL going forward, I guess gotip-linux-amd64-runtimesecret, gotip-linux-arm64-runtimesecret @dmit...@golang.org
if sizeSpecializedMallocEnabled && heapBitsInSpan(size) && gp.secret == 0 {Michael MatloobJust realised that the specialized mallocs being called directly from the compiler now means that this check doesn't do anything. I'll take a stab at updating mkmalloc to fix this, but I might need some help from @mat...@golang.org
Daniel MorsingHi, I haven't read up on the details of the proposal, but (not having the secret-specific context) I want to mention that not all specialized mallocs are called from the compiler: only those where the compiler knows the size at compile time. So we still need this code path.
Daniel MorsingAh, the check I am referring to is the `gp.secret == 0` part, not the entire `if` statement
Essentially, the tiny allocator has to be disabled when code is running in secret mode. I tried modifying `malloc_stubs.go` to call into `smallNoScanStub` from the `tinyStub`, but I'm hitting a couple of codegen bugs that I'm not sure how to resolve
Michael KnyszekAlright, I managed to make switchable allocators work, but I had to rewrite the allocator generator to do so :) CL 719580
I'm unsure whether I should land the allocator change and rebase this CL on top of it, or if I should just roll it into this one. Any thoughts?
allocator bits LGTM, but yeah you will need to add a fallback case in the directly-called-and-generated tiny allocator routines.
Since performance testing on CL 719580 showed improvement, I spun it out as its own CL and then rebased this CL on top of it.
// allow any conservative scanning of stacks, as that may leadIt seems reasonable not to conservatively scan the stack containing secrets. However, we could preempt the goroutine for other reasons, like scheduling purposes. I think we can still allow those, just not async preemption for stack scanning.
Preemption for other reasons might leak registers into the preemption buffers. Added to the comment to explain
maybe just add this to internal/runtime/sys as GetCurrentSP?
Done
On unsupported platforms this should just invoke f. We can't litter every call site with build tags.
Done
// - Protection does not extend to any new goroutines made by f.Let's panic if a new goroutine is started. We can always relax it later.
Done
// - Pointer addresses may leak into data buffers used by the runtime
// to perform garbage collection. Users should not encode confidential
// information into pointers.this feels like way overkill to state... it is hard to construct a pointer field that's valid, you don't have very many bits to encode sensitive data into, so this would need be extremely elaborate to not cause random GC crashes.
like:
```
x := new([1<<20]byte)
x = (*[1<<20]byte)(unsafe.Pointer(uintptr(unsafe.Pointer(x))|twentyBitsOfSensitiveData))
```unless you mean something like, a data structure's actual layout somehow encodes sensitive information? in that case it may be worth stating that explicitly. or perhaps I'm not being creative enough.
This is referring to the data structures actual layout. I've expanded the wording a bit to explain the requirement and why it is usually implicitly fulfilled by the intended users of this code.
// (Assembled with gcc. The Go assembler doesn't know these instructions.)
// 62 e1 fd 48 28 c0 vmovapd %zmm0,%zmm16Go assembler does support VMOVAPD Z0, Z16.
Done
// (Assembled with gcc. The Go assembler doesn't know these instructions.)
// c4 e1 fc 47 c0 kxorq %k0,%k0,%k0Go assembler does support KXORQ instruction.
Done
// integer registers Daniel MorsingRemove trailing space.
Done
// If we're running secret code, the state on the signal stack might be
// confidential. In which case, we copy the signal stack, and return the stacksize
// and the SP to switch to. sigtramp can then clear the stack and switch to our
// new context stackSo the idea here is that we switch to a different stack, and clear the old signal stack (which could contain secrets), and return to the kernel from a new stack? This might work but it seems weird that we are returning from a different stack. What if there are stack pointers on the stack? What if we are called from another signal handler (probably from non-Go code) that does signal forwarding?
Another approach I can think of is that when we enter secret.Do, we allocate a new signal stack, and clear it when we return out of secret.Do. We don't have to clear it at every signal. Or are we concerned that other, non-Go signal handler could copy things out of our signal stack? Or concerned that the code in secret.Do may call some non-Go code that installs its own signal stack?
The signal stack is tied to the M while the "secretness" of the machine context that gets put onto the signal stack is tied to the G. The shadow stack is a way of tying the place with confidential information directly to the G, so that we don't have to track which Ms ran which Gs.
[Keith's comment](https://github.com/golang/go/issues/21865#issuecomment-2292354685) and [my followup](https://github.com/golang/go/issues/21865#issuecomment-3327040593) on the issue about the problem are worth a read.
Why this needs to be atomic?
The secret stack is allocated by the code that calls secret.Do, while the signal handler uses it. This access is atomic to act as a fence between before the secret stack is allocated and after.
here and elsewhere: trailing whitespace
Done
// TODO(dmo): what is the ABI guarantee here? we use
// R14 later, but the function is ABI0 Good question. I think the assumption here is that it is called from Go, not assembly, so R14 is set. Perhaps we should make this function ABIInternal (at some point).
Remove trailing space in the comment.
Done
BL ·secretEraseRegisters(SB)Daniel MorsingNit: too many tabs.
Done
Related details
- Cherry Mui
- Dmitri Shuralyov
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
#ifdef GOEXPERIMENT_runtimesecretI've reenabled this check in this patchset, I'm sure there are places where registers can leak from code running in g0 to cgo or some other execution environment that I'm not considering and since this is an experiment now, we shouldn't see any performance impact.
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- Cherry Mui
- Daniel Morsing
- Dmitri Shuralyov
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted Commit-Queue+1![]( "Open in Gerrit")
- Cherry Mui
- Dmitri Shuralyov
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
secretStack stack // the shadow stack for returning into the kernelDaniel MorsingI was wondering if we could keep a pool of these somewhere else, like in a P or M.
Only the last one allocated per M might still be in use, right? So maybe 2 per M would be enough.
I think that brings us back to a situation where we'd have to track which M executed secret code, plus now that I've removed the async xRegs code, heap allocations are the only thing that relies on a garbage collection having run after returning from secret code.
If shadow secret stack allocation cost is a factor, there are ways around it. Create a goroutine that sets up a secret and then receives requests for encryption/decryption. That way, we only have to allocate once. That scheme could definitely work for TLS and SSH which are the motivating uses for creating the package.
oh wait, I read this suggestion wrong, you were talking about a simple allocation cache.
An arbitrary number of secretStacks can be live at the same time, so I don't think there's much to be gained by having it cached. A better avenue is probably using a different allocator that isn't the stack one, but I think I need to figure out what the performance numbers are before I put in the work for that. The remarks I made about amortizing the cost above are still valid, so I believe we can work around it for the first prototype users of this.
Related details
- Cherry Mui
- Dmitri Shuralyov
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 11 comments![]( "Open in Gerrit")
get_tls(CX)We can probably get rid of these two instructions. Or put them just after the secretEraseRegisters call.
That way there's no extra instructions if the experiment is off.
const stubSigStackSize = 4096Where does this number come from?
secretStack stack // the shadow stack for returning into the kernelDaniel MorsingI was wondering if we could keep a pool of these somewhere else, like in a P or M.
Only the last one allocated per M might still be in use, right? So maybe 2 per M would be enough.
I think that brings us back to a situation where we'd have to track which M executed secret code, plus now that I've removed the async xRegs code, heap allocations are the only thing that relies on a garbage collection having run after returning from secret code.
If shadow secret stack allocation cost is a factor, there are ways around it. Create a goroutine that sets up a secret and then receives requests for encryption/decryption. That way, we only have to allocate once. That scheme could definitely work for TLS and SSH which are the motivating uses for creating the package.
Acknowledged
// TODO: linux only?I think this has been resolved? The code that uses it has been commented out.
BYTE $0x62; BYTE $0x71; BYTE $0xfd; BYTE $0x48; BYTE $0x10; BYTE $0x30These instructions exist in the assembler now.
// TODO(dmo): more substantial dirtying hereWe should at least add F0-F31 here.
// the race detector does not like our pointer shenanigansThe race detector is such a buzzkill.
// TODO(dmo): vdso calls while running secret code can spill secrets onto the stack.I would think secret code is exactly the place where they would want vgetrandom to work.
Do we really need to disable it in secret mode?
Related details
- Cherry Mui
- Daniel Morsing
- Dmitri Shuralyov
- Filippo Valsorda
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Cherry Mui
- Daniel Morsing
- Dmitri Shuralyov
- Filippo Valsorda
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 9 comments![]( "Open in Gerrit")
get_tls(CX)We can probably get rid of these two instructions. Or put them just after the secretEraseRegisters call.
That way there's no extra instructions if the experiment is off.
Daniel Morsing
Done
Where does this number come from?
It was gathered experimentally. Added to the comment explaining the rationale
I think this has been resolved? The code that uses it has been commented out.
Done
BYTE $0x62; BYTE $0x71; BYTE $0xfd; BYTE $0x48; BYTE $0x10; BYTE $0x30These instructions exist in the assembler now.
Done
Is there a plan to enable these?
This is left over from your original CL and I honestly haven't given it that much thought. We're probably in a better spot to do runtime checking of SIMD features now, but I'm going to admit ignorance on what that actually looks like.
These we could do now.
Done
We should at least add F0-F31 here.
Done
// TODO(dmo): vdso calls while running secret code can spill secrets onto the stack.I would think secret code is exactly the place where they would want vgetrandom to work.
Do we really need to disable it in secret mode?
I must have done this code right after I did a bunch of assembly where the register erasing can only happen from inside the function, because the compiler will happily generate the right spills for me if I just call `secretEraseRegisters`. Done
Related details
- Cherry Mui
- Dmitri Shuralyov
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov added 1 comment![]( "Open in Gerrit")
Daniel MorsingChatted with @rol...@golang.org, let's hide the exported package (and any dangerous behavior if it's easy) behind a GOEXPERIMENT.
Done. Will need a new set of builders to test this CL going forward, I guess gotip-linux-amd64-runtimesecret, gotip-linux-arm64-runtimesecret @dmit...@golang.org
A new builder is typically appropriate when the goal is to run all of the Go tests under a different mode. For example, if the GOEXPERIMENT modifies runtime behavior.
If the change of behavior here is more contained and the goal is to run tests inside the runtime/secret package, tests that require that GOEXPERIMENT environment variable to be set, that can be arranged by registering a custom test with said environment variable in cmd/dist/test.go. See https://cs.opensource.google/go/go/+/master:src/cmd/dist/test.go;l=747-754;drc=4bfc3a9d14c0b3bfcfe4ce987e47cda6720785a2 for example. An upside of this approach is that it means other people running all.bash locally can get signal from runtime/secret tests without needing to do anything differently.
If you think it's still better to go with the dedicated builder testing approach, please file a tracking issue for it and add a new-builder label. Thanks.
Related details
- Cherry Mui
- Daniel Morsing
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Morsing
| Commit-Queue | +1 |
Daniel MorsingChatted with @rol...@golang.org, let's hide the exported package (and any dangerous behavior if it's easy) behind a GOEXPERIMENT.
Dmitri ShuralyovDone. Will need a new set of builders to test this CL going forward, I guess gotip-linux-amd64-runtimesecret, gotip-linux-arm64-runtimesecret @dmit...@golang.org
A new builder is typically appropriate when the goal is to run all of the Go tests under a different mode. For example, if the GOEXPERIMENT modifies runtime behavior.
If the change of behavior here is more contained and the goal is to run tests inside the runtime/secret package, tests that require that GOEXPERIMENT environment variable to be set, that can be arranged by registering a custom test with said environment variable in cmd/dist/test.go. See https://cs.opensource.google/go/go/+/master:src/cmd/dist/test.go;l=747-754;drc=4bfc3a9d14c0b3bfcfe4ce987e47cda6720785a2 for example. An upside of this approach is that it means other people running all.bash locally can get signal from runtime/secret tests without needing to do anything differently.
If you think it's still better to go with the dedicated builder testing approach, please file a tracking issue for it and add a new-builder label. Thanks.
Well, the runtime changes are supposed to be dormant when the secret code isn't used, so I guess a dist package scoped test would cover most the bases. But then again, testing that the runtime changes are dormant is part of the test 😊
I've added the dist test suggested and we can revisit the builder once internal users start appearing. I think Filippo is considering using this package to harden the `crypto/tls` package for forward-secrecy capable ciphers in the near future.
Related details
- Cherry Mui
- Dmitri Shuralyov
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 6 comments![]( "Open in Gerrit")
// allow any conservative scanning of stacks, as that may leadDaniel MorsingIt seems reasonable not to conservatively scan the stack containing secrets. However, we could preempt the goroutine for other reasons, like scheduling purposes. I think we can still allow those, just not async preemption for stack scanning.
Preemption for other reasons might leak registers into the preemption buffers. Added to the comment to explain
What is the preemption buffer? g.sched? That contains only the PC, SP, FP, etc., no user data.
// Q: what does "all" mean in this context? Do we need to
// worry about just Go-allocatable registers, or do we need
// to consider any register potentially used by assembly?I think we should erase all registers, except the ones that are "reserved", like SP, LR, and G. One can call assembly code to handle some secret data, like hashing or encrpytion.
// Figure out the regions of stack we need to zero.
// Our frame layout is as diagrammed below. (With memclr*
// the sole function we're going to call.)
// +----------------------+
// | secret.Do |
// +----------------------+ <- csp (caller stack pointer)
// | secret_eraseSecrets |
// +----------------------+ <- sp (stack pointer)
// | memclrNoHeapPointers |
// +----------------------+This depends on the implementation detail of memclrNoHeapPointers. Would it be simpler and more reliable to switch to systemstack to erase from sp? Maybe we could even mcall an erase function which directly returns to our caller, so we don't need to worry about the current frame as well.
// If we're running secret code, the state on the signal stack might be
// confidential. In which case, we copy the signal stack, and return the stacksize
// and the SP to switch to. sigtramp can then clear the stack and switch to our
// new context stackDaniel MorsingSo the idea here is that we switch to a different stack, and clear the old signal stack (which could contain secrets), and return to the kernel from a new stack? This might work but it seems weird that we are returning from a different stack. What if there are stack pointers on the stack? What if we are called from another signal handler (probably from non-Go code) that does signal forwarding?
Another approach I can think of is that when we enter secret.Do, we allocate a new signal stack, and clear it when we return out of secret.Do. We don't have to clear it at every signal. Or are we concerned that other, non-Go signal handler could copy things out of our signal stack? Or concerned that the code in secret.Do may call some non-Go code that installs its own signal stack?
The signal stack is tied to the M while the "secretness" of the machine context that gets put onto the signal stack is tied to the G. The shadow stack is a way of tying the place with confidential information directly to the G, so that we don't have to track which Ms ran which Gs.
[Keith's comment](https://github.com/golang/go/issues/21865#issuecomment-2292354685) and [my followup](https://github.com/golang/go/issues/21865#issuecomment-3327040593) on the issue about the problem are worth a read.
Thanks for the pointer. I'm not a fan of switching the stack where we return from the signal handler. If the signal handler is invoked by the kernel and the only thing on the signal handler is the signal context, that is probably fine. But if it is invoked by some user C code, which does happen in cgo binaries, I don't think it is going to work very well.
We could pin the G to the M. It is heavy weight, but other parts of secret.Do is sort of heavy weight, too, so perhaps not too bad?
Related details
- Dmitri Shuralyov
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
// If we're running secret code, the state on the signal stack might be
// confidential. In which case, we copy the signal stack, and return the stacksize
// and the SP to switch to. sigtramp can then clear the stack and switch to our
// new context stackDaniel MorsingSo the idea here is that we switch to a different stack, and clear the old signal stack (which could contain secrets), and return to the kernel from a new stack? This might work but it seems weird that we are returning from a different stack. What if there are stack pointers on the stack? What if we are called from another signal handler (probably from non-Go code) that does signal forwarding?
Another approach I can think of is that when we enter secret.Do, we allocate a new signal stack, and clear it when we return out of secret.Do. We don't have to clear it at every signal. Or are we concerned that other, non-Go signal handler could copy things out of our signal stack? Or concerned that the code in secret.Do may call some non-Go code that installs its own signal stack?
Cherry MuiThe signal stack is tied to the M while the "secretness" of the machine context that gets put onto the signal stack is tied to the G. The shadow stack is a way of tying the place with confidential information directly to the G, so that we don't have to track which Ms ran which Gs.
[Keith's comment](https://github.com/golang/go/issues/21865#issuecomment-2292354685) and [my followup](https://github.com/golang/go/issues/21865#issuecomment-3327040593) on the issue about the problem are worth a read.
Thanks for the pointer. I'm not a fan of switching the stack where we return from the signal handler. If the signal handler is invoked by the kernel and the only thing on the signal handler is the signal context, that is probably fine. But if it is invoked by some user C code, which does happen in cgo binaries, I don't think it is going to work very well.
We could pin the G to the M. It is heavy weight, but other parts of secret.Do is sort of heavy weight, too, so perhaps not too bad?
We could also have the scheduler track which M the secret G has run on and received a signal. Then the signal stack could be zeroed when the scheduler switches the G off it or when secret.Do returns.
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 4 comments![]( "Open in Gerrit")
// allow any conservative scanning of stacks, as that may leadDaniel MorsingIt seems reasonable not to conservatively scan the stack containing secrets. However, we could preempt the goroutine for other reasons, like scheduling purposes. I think we can still allow those, just not async preemption for stack scanning.
Cherry MuiPreemption for other reasons might leak registers into the preemption buffers. Added to the comment to explain
What is the preemption buffer? g.sched? That contains only the PC, SP, FP, etc., no user data.
I'm referring to `src/runtime/preempt_xreg.go`, the extended register state that preemption stores in off-stack buffers.
// Q: what does "all" mean in this context? Do we need to
// worry about just Go-allocatable registers, or do we need
// to consider any register potentially used by assembly?I think we should erase all registers, except the ones that are "reserved", like SP, LR, and G. One can call assembly code to handle some secret data, like hashing or encrpytion.
I think this is the status quo, for a decent value of "all". I've changed this comment to reflect this.
VMOVUPD (AX), Z14Nit: tab here, also below.
Done
VMOVAPD Z0, Z16Nit: tab here, also below.
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
// allow any conservative scanning of stacks, as that may leadDaniel MorsingIt seems reasonable not to conservatively scan the stack containing secrets. However, we could preempt the goroutine for other reasons, like scheduling purposes. I think we can still allow those, just not async preemption for stack scanning.
Cherry MuiPreemption for other reasons might leak registers into the preemption buffers. Added to the comment to explain
Daniel MorsingWhat is the preemption buffer? g.sched? That contains only the PC, SP, FP, etc., no user data.
I'm referring to `src/runtime/preempt_xreg.go`, the extended register state that preemption stores in off-stack buffers.
Good point on that. That buffer is associated to the G, which we can clear. There is also a very temporary scratch space on the P, which we can also clear when we copy the data to the per-G buffer (in `xRegSave`). The one on the P that is used on the return path (`xRegRestore` and `asyncPreempt`), is a bit tricky, where we need the data to reload the registers. Maybe we can write an assembly code that clears it using fewer registers (the integer registers are still available when we're done with that buffer). Or maybe we can have the scheduler clears it when it moves the G to a different P.
(Sounds nontrivial. Probably fine as a followup CL.)
Related details
- Daniel Morsing
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Morsing
| Commit-Queue | +1 |
2 comments
// allow any conservative scanning of stacks, as that may leadDaniel MorsingIt seems reasonable not to conservatively scan the stack containing secrets. However, we could preempt the goroutine for other reasons, like scheduling purposes. I think we can still allow those, just not async preemption for stack scanning.
Cherry MuiPreemption for other reasons might leak registers into the preemption buffers. Added to the comment to explain
Daniel MorsingWhat is the preemption buffer? g.sched? That contains only the PC, SP, FP, etc., no user data.
Cherry MuiI'm referring to `src/runtime/preempt_xreg.go`, the extended register state that preemption stores in off-stack buffers.
Good point on that. That buffer is associated to the G, which we can clear. There is also a very temporary scratch space on the P, which we can also clear when we copy the data to the per-G buffer (in `xRegSave`). The one on the P that is used on the return path (`xRegRestore` and `asyncPreempt`), is a bit tricky, where we need the data to reload the registers. Maybe we can write an assembly code that clears it using fewer registers (the integer registers are still available when we're done with that buffer). Or maybe we can have the scheduler clears it when it moves the G to a different P.
(Sounds nontrivial. Probably fine as a followup CL.)
I did actually attempt this on an earlier patchset (patch 19), but I pulled that code because I was seeing random failures.
I'll leave a TODO.
// Figure out the regions of stack we need to zero.
// Our frame layout is as diagrammed below. (With memclr*
// the sole function we're going to call.)
// +----------------------+
// | secret.Do |
// +----------------------+ <- csp (caller stack pointer)
// | secret_eraseSecrets |
// +----------------------+ <- sp (stack pointer)
// | memclrNoHeapPointers |
// +----------------------+This depends on the implementation detail of memclrNoHeapPointers. Would it be simpler and more reliable to switch to systemstack to erase from sp? Maybe we could even mcall an erase function which directly returns to our caller, so we don't need to worry about the current frame as well.
Done and the code is so much more simpler for it. Keith made these lovely diagrams, feels almost rude to remove them 😊
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
// Figure out the regions of stack we need to zero.
// Our frame layout is as diagrammed below. (With memclr*
// the sole function we're going to call.)
// +----------------------+
// | secret.Do |
// +----------------------+ <- csp (caller stack pointer)
// | secret_eraseSecrets |
// +----------------------+ <- sp (stack pointer)
// | memclrNoHeapPointers |
// +----------------------+Daniel MorsingThis depends on the implementation detail of memclrNoHeapPointers. Would it be simpler and more reliable to switch to systemstack to erase from sp? Maybe we could even mcall an erase function which directly returns to our caller, so we don't need to worry about the current frame as well.
Done and the code is so much more simpler for it. Keith made these lovely diagrams, feels almost rude to remove them 😊
Nice!
I think we still want to erase the registers?
On ARM64, we probably still want to skip the word below SP for the frame pointer. The epilogue will load the frame pointer from it. (Sorry, that layout is unfortunate...)
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 3 comments![]( "Open in Gerrit")
removed some more churn from files that we no longer need to touch
// Figure out the regions of stack we need to zero.
// Our frame layout is as diagrammed below. (With memclr*
// the sole function we're going to call.)
// +----------------------+
// | secret.Do |
// +----------------------+ <- csp (caller stack pointer)
// | secret_eraseSecrets |
// +----------------------+ <- sp (stack pointer)
// | memclrNoHeapPointers |
// +----------------------+Daniel MorsingThis depends on the implementation detail of memclrNoHeapPointers. Would it be simpler and more reliable to switch to systemstack to erase from sp? Maybe we could even mcall an erase function which directly returns to our caller, so we don't need to worry about the current frame as well.
Cherry MuiDone and the code is so much more simpler for it. Keith made these lovely diagrams, feels almost rude to remove them 😊
Nice!
I think we still want to erase the registers?
On ARM64, we probably still want to skip the word below SP for the frame pointer. The epilogue will load the frame pointer from it. (Sorry, that layout is unfortunate...)
We clear the registers on entry into systemstack, this cleanup code runs with a non-zero gp.secret
Adjusted the SP as suggested
Daniel MorsingIs there a plan to enable these?
This is left over from your original CL and I honestly haven't given it that much thought. We're probably in a better spot to do runtime checking of SIMD features now, but I'm going to admit ignorance on what that actually looks like.
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
// If we're running secret code, the state on the signal stack might be
// confidential. In which case, we copy the signal stack, and return the stacksize
// and the SP to switch to. sigtramp can then clear the stack and switch to our
// new context stackDaniel MorsingSo the idea here is that we switch to a different stack, and clear the old signal stack (which could contain secrets), and return to the kernel from a new stack? This might work but it seems weird that we are returning from a different stack. What if there are stack pointers on the stack? What if we are called from another signal handler (probably from non-Go code) that does signal forwarding?
Another approach I can think of is that when we enter secret.Do, we allocate a new signal stack, and clear it when we return out of secret.Do. We don't have to clear it at every signal. Or are we concerned that other, non-Go signal handler could copy things out of our signal stack? Or concerned that the code in secret.Do may call some non-Go code that installs its own signal stack?
Cherry MuiThe signal stack is tied to the M while the "secretness" of the machine context that gets put onto the signal stack is tied to the G. The shadow stack is a way of tying the place with confidential information directly to the G, so that we don't have to track which Ms ran which Gs.
[Keith's comment](https://github.com/golang/go/issues/21865#issuecomment-2292354685) and [my followup](https://github.com/golang/go/issues/21865#issuecomment-3327040593) on the issue about the problem are worth a read.
Cherry MuiThanks for the pointer. I'm not a fan of switching the stack where we return from the signal handler. If the signal handler is invoked by the kernel and the only thing on the signal handler is the signal context, that is probably fine. But if it is invoked by some user C code, which does happen in cgo binaries, I don't think it is going to work very well.
We could pin the G to the M. It is heavy weight, but other parts of secret.Do is sort of heavy weight, too, so perhaps not too bad?
We could also have the scheduler track which M the secret G has run on and received a signal. Then the signal stack could be zeroed when the scheduler switches the G off it or when secret.Do returns.
I think the last sticking point here is the signal handling code, and I do not blame you too much. It is a gnarly hack, borne out of 2 facts:
1) the signal machine context is read by the kernel off the stack pointer. Essentially, the `rt_sigreturn` syscall takes the stack pointer as an argument. Hence the confidential information has to stay in on the stack until after control has been handed to the kernel.
2) we cannot clear an Ms signal stack from anywhere other than from inside the M, lest we smash the stack while it is handling a signal. This complicates matters since sleeping or blocked Ms might have confidential information in their signal stack, but no way of guaranteeing that we can run them in a timely manner.
Doing the clearing while handling the signal solves these 2 problems with minimal runtime involvement (although you could argue that ship sailed when I started adding things to systemstack :) )
Note that switching to the shadow signal stack and executing code on it was done for simplicity's sake. We could clear out the bottom of the signal stack with the confidential information, then switch the stack pointer right before we trap into the OS with the `rt_sigreturn`, and never run code with the adjusted stack pointer.
I have a couple of other ideas on how to handle this, but it's gonna take a bit of exploratory testing to see if they are feasible and I don't know if they will be ready before the 1.26 freeze
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 2 comments![]( "Open in Gerrit")
Alright, a lot of things changed in this last patchset, and for the better:
1) the signal stack switch has been replaced by a method where we use the kernels propensity to write register values onto the signal stack to our advantage.
2) less invasive mkmalloc implementation, no longer requires the previous CL in this stack
3) more testing hooks for things covered by TestCore. We now explicitly check for the signal stacks being cleared.
PTAL
// If we're running secret code, the state on the signal stack might be
// confidential. In which case, we copy the signal stack, and return the stacksize
// and the SP to switch to. sigtramp can then clear the stack and switch to our
// new context stackDaniel MorsingSo the idea here is that we switch to a different stack, and clear the old signal stack (which could contain secrets), and return to the kernel from a new stack? This might work but it seems weird that we are returning from a different stack. What if there are stack pointers on the stack? What if we are called from another signal handler (probably from non-Go code) that does signal forwarding?
Another approach I can think of is that when we enter secret.Do, we allocate a new signal stack, and clear it when we return out of secret.Do. We don't have to clear it at every signal. Or are we concerned that other, non-Go signal handler could copy things out of our signal stack? Or concerned that the code in secret.Do may call some non-Go code that installs its own signal stack?
Cherry MuiThe signal stack is tied to the M while the "secretness" of the machine context that gets put onto the signal stack is tied to the G. The shadow stack is a way of tying the place with confidential information directly to the G, so that we don't have to track which Ms ran which Gs.
[Keith's comment](https://github.com/golang/go/issues/21865#issuecomment-2292354685) and [my followup](https://github.com/golang/go/issues/21865#issuecomment-3327040593) on the issue about the problem are worth a read.
Cherry MuiThanks for the pointer. I'm not a fan of switching the stack where we return from the signal handler. If the signal handler is invoked by the kernel and the only thing on the signal handler is the signal context, that is probably fine. But if it is invoked by some user C code, which does happen in cgo binaries, I don't think it is going to work very well.
We could pin the G to the M. It is heavy weight, but other parts of secret.Do is sort of heavy weight, too, so perhaps not too bad?
Daniel MorsingWe could also have the scheduler track which M the secret G has run on and received a signal. Then the signal stack could be zeroed when the scheduler switches the G off it or when secret.Do returns.
I think the last sticking point here is the signal handling code, and I do not blame you too much. It is a gnarly hack, borne out of 2 facts:
1) the signal machine context is read by the kernel off the stack pointer. Essentially, the `rt_sigreturn` syscall takes the stack pointer as an argument. Hence the confidential information has to stay in on the stack until after control has been handed to the kernel.
2) we cannot clear an Ms signal stack from anywhere other than from inside the M, lest we smash the stack while it is handling a signal. This complicates matters since sleeping or blocked Ms might have confidential information in their signal stack, but no way of guaranteeing that we can run them in a timely manner.Doing the clearing while handling the signal solves these 2 problems with minimal runtime involvement (although you could argue that ship sailed when I started adding things to systemstack :) )
Note that switching to the shadow signal stack and executing code on it was done for simplicity's sake. We could clear out the bottom of the signal stack with the confidential information, then switch the stack pointer right before we trap into the OS with the `rt_sigreturn`, and never run code with the adjusted stack pointer.
I have a couple of other ideas on how to handle this, but it's gonna take a bit of exploratory testing to see if they are feasible and I don't know if they will be ready before the 1.26 freeze
Alright, I implemented a way that is much simpler, faster and easier to port to other platforms, PTAL
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 2 comments![]( "Open in Gerrit")
{subBasicLit, "isTiny_", str(0)},Can we make isTiny_ a bool instead and use str(false)/str(true) instead? I think that would make the code a little more readable?
if goexperiment.RuntimeSecret && isTiny && gp.secret > 0 {
return mallocgcSmallNoScanSC2(size, typ, needzero)
}Can we put this code into another function that's inlined in the tiny case, (and inlined to an empty bodied function in the alternate case)? then we won't have to insert it into the generated code for the non-tiny functions.
Related details
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek added 1 comment![]( "Open in Gerrit")
Alright, a lot of things changed in this last patchset, and for the better:
1) the signal stack switch has been replaced by a method where we use the kernels propensity to write register values onto the signal stack to our advantage.
2) less invasive mkmalloc implementation, no longer requires the previous CL in this stack
3) more testing hooks for things covered by TestCore. We now explicitly check for the signal stacks being cleared.PTAL
the mkmalloc stuff LGTM, modulo Michael's comments. thanks!
Related details
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Daniel Morsing (Gerrit)
Daniel Morsing added 2 comments![]( "Open in Gerrit")
{subBasicLit, "isTiny_", str(0)},Can we make isTiny_ a bool instead and use str(false)/str(true) instead? I think that would make the code a little more readable?
Doing so hits a bug in the mkmalloc generator. It expects an `ast.BasicLit` and `true` or `false` are `ast.Ident`s
if goexperiment.RuntimeSecret && isTiny && gp.secret > 0 {
return mallocgcSmallNoScanSC2(size, typ, needzero)
}Can we put this code into another function that's inlined in the tiny case, (and inlined to an empty bodied function in the alternate case)? then we won't have to insert it into the generated code for the non-tiny functions.
It's possible, but because it has to return the function early depending on `gp.secret`, the enclosing function has to have an if statement and the empty inlined function turns into
```
avoid := nil
if avoid != nil {
return avoid
}
```
in the non-tiny case. Worse, the entirety of the specialized tiny function gets inlined yet again because the enclosing function now includes both terminating and non-terminating returns. It's not pretty and given the results I got from CL 719580, I suspect there's also a performance penaltyBetween the 2 options, I think a superfluous if statement that gets constant folded away is a lot more readable
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Michael Matloob (Gerrit)
Michael Matloob added 2 comments![]( "Open in Gerrit")
{subBasicLit, "isTiny_", str(0)},Daniel MorsingCan we make isTiny_ a bool instead and use str(false)/str(true) instead? I think that would make the code a little more readable?
Doing so hits a bug in the mkmalloc generator. It expects an `ast.BasicLit` and `true` or `false` are `ast.Ident`s
Got it. I'll try to fix that after this CL is in.
if goexperiment.RuntimeSecret && isTiny && gp.secret > 0 {
return mallocgcSmallNoScanSC2(size, typ, needzero)
}Daniel MorsingCan we put this code into another function that's inlined in the tiny case, (and inlined to an empty bodied function in the alternate case)? then we won't have to insert it into the generated code for the non-tiny functions.
It's possible, but because it has to return the function early depending on `gp.secret`, the enclosing function has to have an if statement and the empty inlined function turns into
```
avoid := nilif avoid != nil {
return avoid
}
```
in the non-tiny case. Worse, the entirety of the specialized tiny function gets inlined yet again because the enclosing function now includes both terminating and non-terminating returns. It's not pretty and given the results I got from CL 719580, I suspect there's also a performance penaltyBetween the 2 options, I think a superfluous if statement that gets constant folded away is a lot more readable
Yeah I realized the return issue after writing the comment... We absolutely don't want the rest of the tiny function to be inlined again. I still want to try to see if there's something we can do here, but it shouldn't block this CL. I'll take a look to see if there's anything we can do with mkmalloc after this CL is submitted if there's time before the freeze.
Related details
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted and added 13 comments![]( "Open in Gerrit")
Votes added by Cherry Mui
| Code-Review | +1 |
13 comments
LGTM mostly. Thank you!
(I haven't looked carefully at the GC special stuff and the tests. Perhaps others have them covered already.)
offsetX86HasAVX512F = unsafe.Offsetof(cpu.X86.HasAVX512F)Just use HasAVX512. In other places like preempt_amd64.s, we don't really distinguish F vs. other "always have" features.
if goexperiment.RuntimeSecret && gp.secret > 0 {Do we need to reset gp.secret to 0 here? (If we are leaving secret.Do without calling dec.)
// Copyright 2024 The Go Authors. All rights reserved.Nit: 2025. (Apply to all new files.)
// we're potentially racing with a signal being delivered,
// so do it atomically
atomic.Xaddint32(&gp.secret, 1)If the signal is delivered to this g, it cannot at the same time executing code (as it is running the signal handler). What potential race could occur?
// Use the sigPerThreadSyscall signal but do not set m.needPerThreadSyscall.
// This turns into a no-op that just puts new context onto the signal stackIn a cgo binary, the signal may have some meaning in the C side (e.g. used by glibc). It may not always be happy to receive an additional signal.
I think it is safer to use a preemption signal. If there is no preemption request, it is no-op.
// These functions cannot live in test files in the secret package
// because we need to compile the crashing binary as non-test.
// Otherwise, dist will pick up the crashing binary as a crashing test.I'm not sure I follow this comment. Can this be in runtime/secret/testdata? I don't think dist does anything special with files in testdata (and we have a bunch of crashing programs in runtime/testdata). Then we don't need to edit deps_test.go. (Generally, I hope we don't have too many test-only packages.)
// unsupported, just invoke f directly.
f()
returnShould it panic?
// Note: if f calls runtime.Goexit, step 3 and above will not
// happen, as Goexit is unrecoverable. We handle that case in
// runtime/proc.go:goexit0.If we call eraseSecrets in a deferred function, it will run even if f calls Goexit. Can we do it this way? I guess this depends on the frame size of this function is smaller than doHelper?
func count() int32
func inc()
func dec()
func eraseSecrets()Would be good to make them "handshake" linknames. I.e. add linkname directives here like
```
//go:linkname count
func count() int
```
(Doesn't need to specify the full name, which is inferred with the package prefix.)
Nice! Yeah, I was thinking clearing the signal stack by just sending a dummy signal. (It may not even need to be in STW. The M should be able to handle signals even if it is executing. Anyway, doing it during STW is fine.)
secretEraseRegisters()Why here instead of doing this at the entry?
BL ·secretEraseRegisters(SB)Nit: too many tabs. Lines above have both a space and a tab, remove the space.
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
// Note: if f calls runtime.Goexit, step 3 and above will not
// happen, as Goexit is unrecoverable. We handle that case in
// runtime/proc.go:goexit0.If we call eraseSecrets in a deferred function, it will run even if f calls Goexit. Can we do it this way? I guess this depends on the frame size of this function is smaller than doHelper?
I guess this depends on the frame size of this function is smaller than doHelper?
(Clarify: I mean, if we do it in a defer function, I guess it depends on the deferred function's frame size is not larger than doHelper.)
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +1 by Cherry Mui
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 11 comments![]( "Open in Gerrit")
offsetX86HasAVX512F = unsafe.Offsetof(cpu.X86.HasAVX512F)Just use HasAVX512. In other places like preempt_amd64.s, we don't really distinguish F vs. other "always have" features.
Done
Do we need to reset gp.secret to 0 here? (If we are leaving secret.Do without calling dec.)
Done, put clearing it in gdestroy so we should never have a stale value
aside: we've always given out old partially uncleared Gs? Seems risky for very little benefit.
// Copyright 2024 The Go Authors. All rights reserved.Nit: 2025. (Apply to all new files.)
Those are from Keith's original implementation. Following [Filippo's](https://go-review.googlesource.com/c/go/+/704615/comment/ae58d592_da7e6c13/) lead here. Don't know if we have a "co-authorship copyright headers spanning multiple years" policy but I wouldn't be surprised if there isn't one 😂
// we're potentially racing with a signal being delivered,
// so do it atomically
atomic.Xaddint32(&gp.secret, 1)If the signal is delivered to this g, it cannot at the same time executing code (as it is running the signal handler). What potential race could occur?
Previously, `gp.secret` acted as a fence on whether the `gp.secretStack` field would have a defined value. Now, it's not necessary. Done.
Aside: Do we define our memory model WRT. signals anywhere? I can't believe we'd ever support an architecture that doesn't flush memory on signal delivery and have non-atomic int32 assigns.
// Use the sigPerThreadSyscall signal but do not set m.needPerThreadSyscall.
// This turns into a no-op that just puts new context onto the signal stackIn a cgo binary, the signal may have some meaning in the C side (e.g. used by glibc). It may not always be happy to receive an additional signal.
I think it is safer to use a preemption signal. If there is no preemption request, it is no-op.
Done. I was initially thinking of doing this, but I saw some bookkeeping code that made me think the runtime needed to be updated to work around being sent preemption signals with no request backing them, but thinking about it again, I guess they have handle them if someone sends them explicitly from outside the environment.
// These functions cannot live in test files in the secret package
// because we need to compile the crashing binary as non-test.
// Otherwise, dist will pick up the crashing binary as a crashing test.I'm not sure I follow this comment. Can this be in runtime/secret/testdata? I don't think dist does anything special with files in testdata (and we have a bunch of crashing programs in runtime/testdata). Then we don't need to edit deps_test.go. (Generally, I hope we don't have too many test-only packages.)
I think I managed to conflate 2 different facts and not explain either of them in the process.
1) the original tests built a version of themselves to test the crashing behavior. This interacted badly with dist when running on the builders.
2) the tests needed access to internal packages like internal/cpu, hence they had to live in-tree.
I've re-written the crash test so it can live as simple testdata file.
// unsupported, just invoke f directly.
f()
returnDaniel MorsingShould it panic?
Went with Filippo's suggestion earlier from an earlier patchset. I think this is a better interface since users don't have to special case their code depending on platform support
func count() int32
func inc()
func dec()
func eraseSecrets()Would be good to make them "handshake" linknames. I.e. add linkname directives here like
```
//go:linkname count
func count() int
```
(Doesn't need to specify the full name, which is inferred with the package prefix.)
Done
func count() int32
func inc()
func dec()
func eraseSecrets()Would be good to make them "handshake" linknames. I.e. add linkname directives here like
```
//go:linkname count
func count() int
```
(Doesn't need to specify the full name, which is inferred with the package prefix.)
Done
Why here instead of doing this at the entry?
Not seeing a reason why not. Done
Nit: too many tabs. Lines above have both a space and a tab, remove the space.
- Cherry Mui
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Morsing
| Commit-Queue | +1 |
2 comments
// Note: if f calls runtime.Goexit, step 3 and above will not
// happen, as Goexit is unrecoverable. We handle that case in
// runtime/proc.go:goexit0.If we call eraseSecrets in a deferred function, it will run even if f calls Goexit. Can we do it this way? I guess this depends on the frame size of this function is smaller than doHelper?
I guess this depends on the frame size of this function is smaller than doHelper?
(Clarify: I mean, if we do it in a defer function, I guess it depends on the deferred function's frame size is not larger than doHelper.)
Tried implementing this and realised that erasing registers isn't needed because mcall already does it for us by the time we get down this path. Left a comment to that effect and added a best-effort test
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 3 comments![]( "Open in Gerrit")
// Copyright 2024 The Go Authors. All rights reserved.Daniel MorsingNit: 2025. (Apply to all new files.)
Those are from Keith's original implementation. Following [Filippo's](https://go-review.googlesource.com/c/go/+/704615/comment/ae58d592_da7e6c13/) lead here. Don't know if we have a "co-authorship copyright headers spanning multiple years" policy but I wouldn't be surprised if there isn't one 😂
Alright. I don't think it matters too much.
// we're potentially racing with a signal being delivered,
// so do it atomically
atomic.Xaddint32(&gp.secret, 1)If the signal is delivered to this g, it cannot at the same time executing code (as it is running the signal handler). What potential race could occur?
Previously, `gp.secret` acted as a fence on whether the `gp.secretStack` field would have a defined value. Now, it's not necessary. Done.
Aside: Do we define our memory model WRT. signals anywhere? I can't believe we'd ever support an architecture that doesn't flush memory on signal delivery and have non-atomic int32 assigns.
I'm not sure we explicitly mentioned that. I'd think we treat the signal handler as part of the program running sequentially. That is,
instructions before the signal PC, signal handler, instructions after the signal PC, run sequentially as part of the program order. They run on the same OS thread anyway, so the memory accesses should follow the sequential order.
// These functions cannot live in test files in the secret package
// because we need to compile the crashing binary as non-test.
// Otherwise, dist will pick up the crashing binary as a crashing test.Daniel MorsingI'm not sure I follow this comment. Can this be in runtime/secret/testdata? I don't think dist does anything special with files in testdata (and we have a bunch of crashing programs in runtime/testdata). Then we don't need to edit deps_test.go. (Generally, I hope we don't have too many test-only packages.)
I think I managed to conflate 2 different facts and not explain either of them in the process.
1) the original tests built a version of themselves to test the crashing behavior. This interacted badly with dist when running on the builders.
2) the tests needed access to internal packages like internal/cpu, hence they had to live in-tree.I've re-written the crash test so it can live as simple testdata file.
files in testdata directory could import internal packages. I think we can move the test-only functions to testdata as well, instead of copying them in crash_test.go. So we don't have to build them in regular, non-test builds.
There could be multiple packages in testdata as well, which are importable from other testdata packages.
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
// These functions cannot live in test files in the secret package
// because we need to compile the crashing binary as non-test.
// Otherwise, dist will pick up the crashing binary as a crashing test.Daniel MorsingI'm not sure I follow this comment. Can this be in runtime/secret/testdata? I don't think dist does anything special with files in testdata (and we have a bunch of crashing programs in runtime/testdata). Then we don't need to edit deps_test.go. (Generally, I hope we don't have too many test-only packages.)
Cherry MuiI think I managed to conflate 2 different facts and not explain either of them in the process.
1) the original tests built a version of themselves to test the crashing behavior. This interacted badly with dist when running on the builders.
2) the tests needed access to internal packages like internal/cpu, hence they had to live in-tree.I've re-written the crash test so it can live as simple testdata file.
files in testdata directory could import internal packages. I think we can move the test-only functions to testdata as well, instead of copying them in crash_test.go. So we don't have to build them in regular, non-test builds.
There could be multiple packages in testdata as well, which are importable from other testdata packages.
I think we want to move all test-only code to testdata. And on crash_test.go, just build the test program without copying. Just do a "go build" with cmd.Dir as `testdata` (or `testdata/some_package` if that makes sense) and put the output binary in a temporary directory, and run it. (See the runtime's [buildTestProg](https://cs.opensource.google/go/go/+/master:src/runtime/crash_test.go;l=128). It has more complex configuration handling which we don't need.)
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
// These functions cannot live in test files in the secret package
// because we need to compile the crashing binary as non-test.
// Otherwise, dist will pick up the crashing binary as a crashing test.Daniel MorsingI'm not sure I follow this comment. Can this be in runtime/secret/testdata? I don't think dist does anything special with files in testdata (and we have a bunch of crashing programs in runtime/testdata). Then we don't need to edit deps_test.go. (Generally, I hope we don't have too many test-only packages.)
Cherry MuiI think I managed to conflate 2 different facts and not explain either of them in the process.
1) the original tests built a version of themselves to test the crashing behavior. This interacted badly with dist when running on the builders.
2) the tests needed access to internal packages like internal/cpu, hence they had to live in-tree.I've re-written the crash test so it can live as simple testdata file.
Cherry Muifiles in testdata directory could import internal packages. I think we can move the test-only functions to testdata as well, instead of copying them in crash_test.go. So we don't have to build them in regular, non-test builds.
There could be multiple packages in testdata as well, which are importable from other testdata packages.
I think we want to move all test-only code to testdata. And on crash_test.go, just build the test program without copying. Just do a "go build" with cmd.Dir as `testdata` (or `testdata/some_package` if that makes sense) and put the output binary in a temporary directory, and run it. (See the runtime's [buildTestProg](https://cs.opensource.google/go/go/+/master:src/runtime/crash_test.go;l=128). It has more complex configuration handling which we don't need.)
`secret_test.go` also uses these assembly files. I could pull them out into their own package so they can be shared, but it'd have to be internal and then I'm back to square one with having to have a `dirtysecret` and `crashsecret` package live in the source tree itself
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
// These functions cannot live in test files in the secret package
// because we need to compile the crashing binary as non-test.
// Otherwise, dist will pick up the crashing binary as a crashing test.Daniel MorsingI'm not sure I follow this comment. Can this be in runtime/secret/testdata? I don't think dist does anything special with files in testdata (and we have a bunch of crashing programs in runtime/testdata). Then we don't need to edit deps_test.go. (Generally, I hope we don't have too many test-only packages.)
Cherry MuiI think I managed to conflate 2 different facts and not explain either of them in the process.
1) the original tests built a version of themselves to test the crashing behavior. This interacted badly with dist when running on the builders.
2) the tests needed access to internal packages like internal/cpu, hence they had to live in-tree.I've re-written the crash test so it can live as simple testdata file.
Cherry Muifiles in testdata directory could import internal packages. I think we can move the test-only functions to testdata as well, instead of copying them in crash_test.go. So we don't have to build them in regular, non-test builds.
There could be multiple packages in testdata as well, which are importable from other testdata packages.
Daniel MorsingI think we want to move all test-only code to testdata. And on crash_test.go, just build the test program without copying. Just do a "go build" with cmd.Dir as `testdata` (or `testdata/some_package` if that makes sense) and put the output binary in a temporary directory, and run it. (See the runtime's [buildTestProg](https://cs.opensource.google/go/go/+/master:src/runtime/crash_test.go;l=128). It has more complex configuration handling which we don't need.)
`secret_test.go` also uses these assembly files. I could pull them out into their own package so they can be shared, but it'd have to be internal and then I'm back to square one with having to have a `dirtysecret` and `crashsecret` package live in the source tree itself
Could you put functions in secret_test.go in testdata as well? It could be Go tests, or just a main executable with multiple entry points (like, crash if the check fails). And have a test driver in runtime/secret to build and run the actual tests in testdata.
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
// These functions cannot live in test files in the secret package
// because we need to compile the crashing binary as non-test.
// Otherwise, dist will pick up the crashing binary as a crashing test.Daniel MorsingI'm not sure I follow this comment. Can this be in runtime/secret/testdata? I don't think dist does anything special with files in testdata (and we have a bunch of crashing programs in runtime/testdata). Then we don't need to edit deps_test.go. (Generally, I hope we don't have too many test-only packages.)
Cherry MuiI think I managed to conflate 2 different facts and not explain either of them in the process.
1) the original tests built a version of themselves to test the crashing behavior. This interacted badly with dist when running on the builders.
2) the tests needed access to internal packages like internal/cpu, hence they had to live in-tree.I've re-written the crash test so it can live as simple testdata file.
Cherry Muifiles in testdata directory could import internal packages. I think we can move the test-only functions to testdata as well, instead of copying them in crash_test.go. So we don't have to build them in regular, non-test builds.
There could be multiple packages in testdata as well, which are importable from other testdata packages.
Daniel MorsingI think we want to move all test-only code to testdata. And on crash_test.go, just build the test program without copying. Just do a "go build" with cmd.Dir as `testdata` (or `testdata/some_package` if that makes sense) and put the output binary in a temporary directory, and run it. (See the runtime's [buildTestProg](https://cs.opensource.google/go/go/+/master:src/runtime/crash_test.go;l=128). It has more complex configuration handling which we don't need.)
Cherry Mui`secret_test.go` also uses these assembly files. I could pull them out into their own package so they can be shared, but it'd have to be internal and then I'm back to square one with having to have a `dirtysecret` and `crashsecret` package live in the source tree itself
Could you put functions in secret_test.go in testdata as well? It could be Go tests, or just a main executable with multiple entry points (like, crash if the check fails). And have a test driver in runtime/secret to build and run the actual tests in testdata.
We'd lose dashboard functionality like watchflakes picking up the right tests, and I think it would make the organization more muddled.
As kludge-y as `crash_test.go` is, it is at least miles better than what it was previously.
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
// These functions cannot live in test files in the secret package
// because we need to compile the crashing binary as non-test.
// Otherwise, dist will pick up the crashing binary as a crashing test.Daniel MorsingI'm not sure I follow this comment. Can this be in runtime/secret/testdata? I don't think dist does anything special with files in testdata (and we have a bunch of crashing programs in runtime/testdata). Then we don't need to edit deps_test.go. (Generally, I hope we don't have too many test-only packages.)
Cherry MuiI think I managed to conflate 2 different facts and not explain either of them in the process.
1) the original tests built a version of themselves to test the crashing behavior. This interacted badly with dist when running on the builders.
2) the tests needed access to internal packages like internal/cpu, hence they had to live in-tree.I've re-written the crash test so it can live as simple testdata file.
Cherry Muifiles in testdata directory could import internal packages. I think we can move the test-only functions to testdata as well, instead of copying them in crash_test.go. So we don't have to build them in regular, non-test builds.
There could be multiple packages in testdata as well, which are importable from other testdata packages.
Daniel MorsingI think we want to move all test-only code to testdata. And on crash_test.go, just build the test program without copying. Just do a "go build" with cmd.Dir as `testdata` (or `testdata/some_package` if that makes sense) and put the output binary in a temporary directory, and run it. (See the runtime's [buildTestProg](https://cs.opensource.google/go/go/+/master:src/runtime/crash_test.go;l=128). It has more complex configuration handling which we don't need.)
Cherry Mui`secret_test.go` also uses these assembly files. I could pull them out into their own package so they can be shared, but it'd have to be internal and then I'm back to square one with having to have a `dirtysecret` and `crashsecret` package live in the source tree itself
Daniel MorsingCould you put functions in secret_test.go in testdata as well? It could be Go tests, or just a main executable with multiple entry points (like, crash if the check fails). And have a test driver in runtime/secret to build and run the actual tests in testdata.
We'd lose dashboard functionality like watchflakes picking up the right tests, and I think it would make the organization more muddled.
As kludge-y as `crash_test.go` is, it is at least miles better than what it was previously.
We'd lose dashboard functionality like watchflakes picking up the right tests
We can still achieve that by having multiple tests crash_test.go, each invokes a different entry point of the test binary. The runtime's testdata/testprog does this.
If you think that is too annoying and you want to keep the current layout, it would better for the test-only code be in separate files, and with a clear a comment on each file (including the assembly files) saying that this is test-only.
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Morsing
| Commit-Queue | +1 |
2 comments
Rebased onto master to fix the dev.simd conflicts
// These functions cannot live in test files in the secret package
// because we need to compile the crashing binary as non-test.
// Otherwise, dist will pick up the crashing binary as a crashing test.Daniel MorsingI'm not sure I follow this comment. Can this be in runtime/secret/testdata? I don't think dist does anything special with files in testdata (and we have a bunch of crashing programs in runtime/testdata). Then we don't need to edit deps_test.go. (Generally, I hope we don't have too many test-only packages.)
Cherry MuiI think I managed to conflate 2 different facts and not explain either of them in the process.
1) the original tests built a version of themselves to test the crashing behavior. This interacted badly with dist when running on the builders.
2) the tests needed access to internal packages like internal/cpu, hence they had to live in-tree.I've re-written the crash test so it can live as simple testdata file.
Cherry Muifiles in testdata directory could import internal packages. I think we can move the test-only functions to testdata as well, instead of copying them in crash_test.go. So we don't have to build them in regular, non-test builds.
There could be multiple packages in testdata as well, which are importable from other testdata packages.
Daniel MorsingI think we want to move all test-only code to testdata. And on crash_test.go, just build the test program without copying. Just do a "go build" with cmd.Dir as `testdata` (or `testdata/some_package` if that makes sense) and put the output binary in a temporary directory, and run it. (See the runtime's [buildTestProg](https://cs.opensource.google/go/go/+/master:src/runtime/crash_test.go;l=128). It has more complex configuration handling which we don't need.)
Cherry Mui`secret_test.go` also uses these assembly files. I could pull them out into their own package so they can be shared, but it'd have to be internal and then I'm back to square one with having to have a `dirtysecret` and `crashsecret` package live in the source tree itself
Daniel MorsingCould you put functions in secret_test.go in testdata as well? It could be Go tests, or just a main executable with multiple entry points (like, crash if the check fails). And have a test driver in runtime/secret to build and run the actual tests in testdata.
Cherry MuiWe'd lose dashboard functionality like watchflakes picking up the right tests, and I think it would make the organization more muddled.
As kludge-y as `crash_test.go` is, it is at least miles better than what it was previously.
We'd lose dashboard functionality like watchflakes picking up the right tests
We can still achieve that by having multiple tests crash_test.go, each invokes a different entry point of the test binary. The runtime's testdata/testprog does this.
If you think that is too annoying and you want to keep the current layout, it would better for the test-only code be in separate files, and with a clear a comment on each file (including the assembly files) saying that this is test-only.
I think leave the structure as is. Added breadcrumb comments to make it more obvious what it is.
Related details
- Cherry Mui
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted Code-Review+2![]( "Open in Gerrit")
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek added 2 comments![]( "Open in Gerrit")
for mp := allm; mp != nil; mp = mp.alllink {an O(M) operation during the STW is worrying to me in terms of latency. we've made a lot of progress over the years reducing pause time to essentially just be O(P) (there are some exceptions, but they're intentionally made as cheap as possible; sending a signal does not seem as cheap).
I think this could represent a real regression in applications with a lot of idle threads.
it's OK for the experiment, but dealing with this is IMO important before enabling the experiment by default.
please add a TODO here.
// addSecret records the fact that we need to zero p immediately
// when it is freed.
func addSecret(p unsafe.Pointer) {
lock(&mheap_.speciallock)
s := (*specialSecret)(mheap_.specialSecretAlloc.alloc())
s.special.kind = _KindSpecialSecret
unlock(&mheap_.speciallock)
addspecial(p, &s.special, false)
}how big are secret contexts supposed to be, generally?
if they're relatively large (an entire goroutine's lifetime), then I suspect this will really hurt allocation performance at scale because of the lock access. also, if you allocate in a loop, you'll get O(n^2) behavior up to the number of objects in a span because of how addspecial traverses the linked list. (we should make specials better, but that's a bigger job.)
again, fine for an experiment, but I think we should try to resolve this before we turn the experiment on by default.
for the scalability bit we can add a per-P cache of secret specials. for the O(n^2) problem with specials, we could consider adding specials from the _back_ since that's the general order of allocation.
please add a TODO.
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing uploaded new patchset![]( "Open in Gerrit")
- Code-Review: +2 by Cherry Mui
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
- Cherry Mui
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 2 comments![]( "Open in Gerrit")
an O(M) operation during the STW is worrying to me in terms of latency. we've made a lot of progress over the years reducing pause time to essentially just be O(P) (there are some exceptions, but they're intentionally made as cheap as possible; sending a signal does not seem as cheap).
I think this could represent a real regression in applications with a lot of idle threads.
it's OK for the experiment, but dealing with this is IMO important before enabling the experiment by default.
please add a TODO here.
Added a TODO with some ideas how to handle this
// addSecret records the fact that we need to zero p immediately
// when it is freed.
func addSecret(p unsafe.Pointer) {
lock(&mheap_.speciallock)
s := (*specialSecret)(mheap_.specialSecretAlloc.alloc())
s.special.kind = _KindSpecialSecret
unlock(&mheap_.speciallock)
addspecial(p, &s.special, false)
}how big are secret contexts supposed to be, generally?
if they're relatively large (an entire goroutine's lifetime), then I suspect this will really hurt allocation performance at scale because of the lock access. also, if you allocate in a loop, you'll get O(n^2) behavior up to the number of objects in a span because of how addspecial traverses the linked list. (we should make specials better, but that's a bigger job.)
again, fine for an experiment, but I think we should try to resolve this before we turn the experiment on by default.
for the scalability bit we can add a per-P cache of secret specials. for the O(n^2) problem with specials, we could consider adding specials from the _back_ since that's the general order of allocation.
please add a TODO.
- Cherry Mui
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted![]( "Open in Gerrit")
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roland Shoemaker (Gerrit)
Roland Shoemaker voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Code-Review | +1 |
Related details
- Daniel Morsing
- Filippo Valsorda
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Filippo Valsorda (Gerrit)
Filippo Valsorda added 1 comment![]( "Open in Gerrit")
Resolved a trivial conflict.
```
diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index b44b3f5..1653809 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -4489,12 +4489,8 @@ func gdestroy(gp *g) {
gp.labels = nil
gp.timer = nil
gp.bubble = nil
-<<<<<<< Side #1 (Conflict 1 of 1)
gp.fipsOnlyBypass = false
-||||||| Base
-=======
gp.secret = 0
->>>>>>> Side #2 (Conflict 1 of 1 ends)
if gcBlackenEnabled != 0 && gp.gcAssistBytes > 0 {
// Flush assist credit to the global pool. This gives
```Related details
- Daniel Morsing
- Keith Randall
- Michael Knyszek
- Roland Shoemaker
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Filippo Valsorda (Gerrit)
Filippo Valsorda uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Daniel Morsing
- Keith Randall
- Michael Knyszek
- Roland Shoemaker
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
45 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/runtime/proc.go
Insertions: 1, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
runtime/secret: implement new secret package
Implement secret.Do.
- When secret.Do returns:
- Clear stack that is used by the argument function.
- Clear all the registers that might contain secrets.
- On stack growth in secret mode, clear the old stack.
- When objects are allocated in secret mode, mark them and then zero
the marked objects immediately when they are freed.
- If the argument function panics, raise that panic as if it originated
from secret.Do. This removes anything about the secret function
from tracebacks.
For now, this is only implemented on linux for arm64 and amd64.
This is a rebased version of Keith Randalls initial implementation at
CL 600635. I have added arm64 support, signal handling, preemption
handling and dealt with vDSOs spilling into system stacks.
Fixes #21865
- A doc/next/6-stdlib/1-secret.md
- M src/cmd/dist/test.go
- M src/go/build/deps_test.go
- A src/internal/goexperiment/exp_runtimesecret_off.go
- A src/internal/goexperiment/exp_runtimesecret_on.go
- M src/internal/goexperiment/flags.go
- M src/runtime/_mkmalloc/mkmalloc.go
- M src/runtime/asm_amd64.s
- M src/runtime/asm_arm64.s
- M src/runtime/malloc.go
- M src/runtime/malloc_generated.go
- M src/runtime/malloc_stubs.go
- M src/runtime/mgc.go
- M src/runtime/mheap.go
- M src/runtime/preempt.go
- M src/runtime/proc.go
- M src/runtime/runtime2.go
- A src/runtime/secret.go
- A src/runtime/secret/asm_amd64.s
- A src/runtime/secret/asm_arm64.s
- A src/runtime/secret/crash_test.go
- A src/runtime/secret/export.go
- A src/runtime/secret/secret.go
- A src/runtime/secret/secret_test.go
- A src/runtime/secret/stubs.go
- A src/runtime/secret/stubs_noasm.go
- A src/runtime/secret/testdata/crash.go
- A src/runtime/secret_amd64.s
- A src/runtime/secret_arm64.s
- A src/runtime/secret_asm.go
- A src/runtime/secret_noasm.go
- A src/runtime/secret_nosecret.go
- M src/runtime/signal_linux_amd64.go
- M src/runtime/signal_linux_arm64.go
- M src/runtime/signal_unix.go
- M src/runtime/sizeof_test.go
- M src/runtime/stack.go
- M src/runtime/sys_linux_amd64.s
- M src/runtime/sys_linux_arm64.s
- M src/runtime/time_linux_amd64.s
- M src/runtime/vgetrandom_linux.go
- M src/syscall/asm_linux_amd64.s
Code-Review: +2 by Cherry Mui, +1 by Roland Shoemaker
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Chase (Gerrit)
David Chase has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Chase (Gerrit)
David Chase voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Chase
| Commit-Queue | +1 |
1 comment
This breaks amd64 linux longtest, specifically
cmd/cgo/internal/testshared$ go test -run TestStd .
There is a revert created, that depends on the revert of another CL.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Morsing (Gerrit)
Daniel Morsing added 1 comment![]( "Open in Gerrit")
This breaks amd64 linux longtest, specifically
cmd/cgo/internal/testshared$ go test -run TestStd .
There is a revert created, that depends on the revert of another CL.
I have a fix up at CL 724001
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 1 comment![]( "Open in Gerrit")
Bsaed on the build dashboard, this appears to have caused https://go.dev/issue/76586, a failure on the gotip-linux-amd64-asan-clang15 builder.