Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend stack probe support to non-tier-1 platforms, and clarify policy for mitigating LLVM-dependent unsafety #43241

Open
Tracked by #49355
bstrie opened this issue Jul 14, 2017 · 14 comments
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-stack-probe Area: Stack probing and guard pages C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bstrie
Copy link
Contributor

bstrie commented Jul 14, 2017

As of #42816 , all (current) tier-1 platforms are protected against the potential memory unsafety demonstrated in #16012 . However, this protection is dependent upon platform-dependent LLVM support, so AFAIK most of our supported tier-2 and tier-3 platforms ( https://forge.rust-lang.org/platform-support.html ) are still theoretically unprotected.

Proper resolution of this issue will depend on extending LLVM's stack probe support for various platforms, (which should be done regardless of the discussion in the next paragraph). See https://reviews.llvm.org/D34387 for pcwalton's original x86 implementation, https://reviews.llvm.org/D34386 for the implementation of the attribute itself, and #42816 for the work needed on rustc's side.

But here's the more important concern: the list of potential platforms is unbounded, and we need to decide where to draw the line. As minor as the safety hole may be, is it an abrogation of Rust's claim to guarantee memory safety if that safety is only enforced on "popular" platforms? If so, is it worth considering some sort of blanket check that can be implemented in the frontend for use only on platforms where stack probes are unimplemented? For instance, given that we statically know the stack size of all types, could we perhaps enforce an upper bound on the size of a type (perhaps only in debug mode)? I think this discussion may be relevant to #10184 as well.

@bstrie bstrie added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 14, 2017
@bstrie bstrie added A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. labels Jul 14, 2017
@whitequark
Copy link
Member

so AFAIK most of our supported tier-2 and tier-3 platforms

All of them. LLVM only currently has support for x86.

@cuviper
Copy link
Member

cuviper commented Jul 14, 2017

For instance, given that we statically know the stack size of all types, could we perhaps enforce an upper bound on the size of a type (perhaps only in debug mode)?

GCC has -Wframe-larger-than=len, so I could see rustc having something similar. That can't count alloca/VLAs/etc. though. It's also hard to say what a reasonable limit is generally, so I think it would have to be user configurable. e.g. a 1MB stack item might be fine in isolation, but will blow up in mild recursion, or if a few such functions are nested. A kernel probably wants more like ~1KB max.

@bstrie
Copy link
Contributor Author

bstrie commented Jul 14, 2017

@cuviper Would it suffice to give the user the ability to enforce that no given function ever exceeds the size of the guard page? Maybe I'm naive, but I'm assuming that our guard page support is platform-independent, and that we can calculate the maximum size of any given monomorphized function (I can't think of a reason why we couldn't given that we don't support alloca).

@cuviper
Copy link
Member

cuviper commented Jul 14, 2017

Page size is not a static value on some arches, although for this purpose you could probably get away with just assuming a minimum 4KB. Stack probing on those arches will probably have to use 4KB strides as a worst case, even when the real page size is 64KB/etc., unless we want those to query sysconf at runtime.

Also, as we learned in #43052, the Linux kernel now implements a much larger effective guard size, but we have no way to detect that at runtime, unless you tried to probe a fault on purpose.

Anyway, 4KB seems to me like a pretty low stack-frame threshold for userspace programs. I guess some users might be happy with that, but I think a tunable threshold would be more useful.

@cuviper
Copy link
Member

cuviper commented Jul 14, 2017

There's also different behavior between the main thread's growable stack and the fixed stack space of other threads. See pthread_attr_setguardsize -- defaults to one page, which Rust doesn't change.

@bstrie
Copy link
Contributor Author

bstrie commented Jul 14, 2017

I suppose another difficulty is that LLVM can inline functions as it pleases, which would poke a hole in any attempt to statically lint against large functions in the frontend.

@whitequark
Copy link
Member

Would it suffice to give the user the ability to enforce that no given function ever exceeds the size of the guard page?

This is not realistic given the way LLVM is designed. You'd have to plug your pass very late into the MI pipeline, which by itself will need target-specific LLVM patches (that are quite a bit more complex than stack probes themselves).

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 28, 2017
@nikomatsakis
Copy link
Contributor

triage: P-medium

@GreenReaper
Copy link

so AFAIK most of our supported tier-2 and tier-3 platforms

All of them. LLVM only currently has support for x86.

Well, gnux32 (Tier 2) is x86_64, with 32-bit pointers, and theory has support.
But in practice, LLVM is shaky here, too; see #59674.

@whitequark
Copy link
Member

This is not realistic given the way LLVM is designed.

Revisiting: this statement is wrong. There is a way to ask LLVM to emit this information because it gained exactly such a pass plugged very late into MC pipeline. See this.

@cuviper
Copy link
Member

cuviper commented Mar 3, 2020

Right now we implement X86 probes with a call to compiler-builtins' __rust_probestack, but there is support for inline stack-probes coming in LLVM 11:
https://reviews.llvm.org/rGe67cbac81211d40332a79d98c9d5953624cc1202

That's also X86-only so far, but this should be the path forward for other targets.

@Elinvynia Elinvynia added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 9, 2020
@LeSeulArtichaut LeSeulArtichaut removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 9, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 16, 2021
Use probe-stack=inline-asm in LLVM 11+

Fixes (?) rust-lang#74405, related to rust-lang#43241

r? `@cuviper`
@bstrie
Copy link
Contributor Author

bstrie commented Feb 4, 2021

Does #77885 mean that stack probes are now enabled on all LLVM targets? If so, should this be closed?

@cuviper
Copy link
Member

cuviper commented Feb 4, 2021

Not yet -- I would like to test additional architectures natively before enabling them, because we did uncover bugs in X86 codegen. Also, PowerPC and SystemZ (s390x) are the only other targets that have implemented stack probes at all.

@yaahc yaahc added the A-stack-probe Area: Stack probing and guard pages label Jun 1, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 29, 2022
Enable inline stack probes on PowerPC and SystemZ

The LLVM PowerPC and SystemZ targets have both supported `"probe-stack"="inline-asm"` for longer than our current minimum LLVM 13 requirement, so we can turn this on for all `powerpc`, `powerpc64`, `powerpc64le`, and `s390x` targets in Rust. These are all tier-2 or lower, so CI does not run their tests, but I have confirmed that their `linux-gnu` variants do pass on RHEL.

cc rust-lang#43241
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
Enable inline stack probes on PowerPC and SystemZ

The LLVM PowerPC and SystemZ targets have both supported `"probe-stack"="inline-asm"` for longer than our current minimum LLVM 13 requirement, so we can turn this on for all `powerpc`, `powerpc64`, `powerpc64le`, and `s390x` targets in Rust. These are all tier-2 or lower, so CI does not run their tests, but I have confirmed that their `linux-gnu` variants do pass on RHEL.

cc rust-lang#43241
@jackh726 jackh726 added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Oct 11, 2023
@cuviper
Copy link
Member

cuviper commented Mar 7, 2024

As of LLVM 18, "probe-stack"="inline-asm" is supported by AArch64, PowerPC, SystemZ, and X86, and we are using that (StackProbeType::Inline) for almost all related target triples. The exceptions are Windows and UEFI targets that use __chkstk calls, and Fortanix, Illumos, and AIX that might just be missing this on accident.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-stack-probe Area: Stack probing and guard pages C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests