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

Add reserve-x18 target feature for aarch64 #124323

Closed

Conversation

Darksonn
Copy link
Contributor

This PR resolves #121970 by adding reserve-x18 as a target feature for the aarch64 platform. Enabling the target feature marks the x18 register as reserved so that Rust doesn't use it as a temporary register when generating machine code. This means that passing the -Ctarget-feature=+reserve-x18 flag will no longer result in the following warning:

warning: unknown feature specified for `-Ctarget-feature`: `reserve-x18`
  |
  = note: it is still passed through to the codegen backend
  = help: consider filing a feature request

Typically you will reserve the x18 register when you want to enable SCS (the shadow call stack sanitizer), because it uses x18 to store a pointer to the shadow stack. However, it is important to not conflate reserve-x18 with the shadow call stack sanitizer — the latter depends on the former, but you can enable reserve-x18 without enabling SCS.

ABI compatibility

One concern that was brought up on #121970 is that this flag affects the ABI. However, it does not affect the ABI in a way where it is a problem to mix code with and without the feature. From the ABI spec:

X18 is the platform register and is reserved for the use of platform ABIs. This is an additional temporary register on platforms that don't assign a special meaning to it.

That is to say, the register is either already reserved (this is the case on Android targets), or it is a caller-saved temporary register (this is the case on aarch64-unknown-none). Changing a register from caller-saved temporary register to reserved is not breaking, so selectively enabling reserve-x18 on some compilation targets (or even on specific functions) cannot result in UB.

That said, removing the reserve-x18 target feature from a function can potentially trigger UB under some circumstances. This is because it is UB to link together -Zsanitizer=shadow-call-stack code with code where x18 is a temporary register. So enabling SCS in a binary requires that x18 is reserved globally. However, right now -Zsanitizer=shadow-call-stack can only be used on targets such as Android where x18 is never a temporary register, so this shouldn't be an issue for this PR.

Use in the Linux Kernel

This motivation for this change is use in the Linux Kernel. When compiling Rust code for the kernel, the aarch64-unknown-none target is used, and this is a platform where x18 is a temporary caller-saved register by default. I am proposing to add this target feature so that the Linux Kernel can make x18 into a reserved register when necessary.

The Linux Kernel has some cases where it needs to reserve x18, but does not pass the -Zsanitizer=shadow-call-stack flag. This is due to the dynamic shadow call stack feature, where the Linux Kernel is able to choose whether SCS should be enabled at boot. This works by having the compiler emit PACIASP/AUTIASP instructions instead of SCS_PUSH/SCS_POP. If Linux decides to enable SCS at boot, then it will use the unwind tables to find the PACIASP/AUTIASP instructions, and modify the machine code at runtime by replacing PACIASP/AUTIASP with SCS_PUSH/SCS_POP instructions in all functions.

The transformation from PACIASP/AUTIASP to SCS_PUSH/SCS_POP is only valid if the x18 register is reserved globally.

It is also possible to configure Linux to always use SCS. In this case, it does so using the -fsanitize=shadow-call-stack flag instead.

The Linux Kernel configuration used by Android uses the dynamic shadow call stack feature in production, so reserve-x18 is a prerequisite for using Rust in the Linux Kernel on Android.

Alternatives

I have considered a few different alternatives.

Add a -Cfixed-x18 flag

When compiling C code with clang or gcc, this is configured by passing the -ffixed-x18 flag instead of using the target feature functionality. We could mirror that and add our own -Cfixed-x18 flag to rustc. It would have the same effect as passing -Ctarget-feature=+reserve-x18.

Use a different target

The Rust compiler could provide a version of aarch64-unknown-none where x18 is reserved, and the Linux Kernel build system could switch to that target whenever CONFIG_SHADOW_CALL_STACK is enabled in the Linux build system. However, there are a few disadvantages with using that strategy for this kind of flag:

  • As the number of flags that are configured in this way increases, the number of targets increases exponentially.
  • It complicates the Kernel build system by significantly deviating from clang and gcc on how this can be configured.

My understanding is that the primary reason in favor of using a different target is that compiling the standard library yourself is unstable, so even if this target feature is added, there is no stable way to get a standard library compiled with -Ctarget-feature=+reserve-x18.

However, as outlined in the abi stability section, there is no issue with enabling reserve-x18 in some crates, but not in the standard library.

The Linux Kernel already compiles the standard library manually. Using a prebuilt standard library is pretty unlikely to be the way forward for many other reasons unrelated to this flag.

Use a target.json in the kernel

The Linux Kernel is already using a target.json file for x86 targets due to #116852, which is a similar issue with a different target feature.

    if cfg.has("ARM64") {
        panic!("arm64 uses the builtin rustc aarch64-unknown-none target");
    } else if cfg.has("X86_64") {
        ts.push("arch", "x86_64");
        ts.push(
            "data-layout",
            "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128",
        );
        let mut features = "-3dnow,-3dnowa,-mmx,+soft-float".to_string();
        if cfg.has("MITIGATION_RETPOLINE") {
            features += ",+retpoline-external-thunk";
        }
        ts.push("features", features);
        ts.push("llvm-target", "x86_64-linux-gnu");
        ts.push("target-pointer-width", "64");

link

However, Linux is trying to move away from target.json targets because Rust considers target.json to be permanently unstable.

Future possibilities

We could make it possible to use -Zsanitizer=shadow-call-stack together with -Ctarget-feature=+reserve-x18 to enable SCS on targets where x18 is normally a temporary caller-saved register. This could be done similarly to required_panic_strategy, which enforces that all compilation units have a shared understanding of the panic strategy. That is, if -Zsanitizer=shadow-call-stack is passed, then fail compilation unless

  1. the target is one where x18 is always reserved, or
  2. -Ctarget-feature=+reserve-x18 is passed as an argument to all crates in the crate graph.

This lets us avoid adding any compiler flags combinations that trigger UB.

References

  1. Discussion in the t-compiler stream on zulip.
  2. Discussion on the Linux Kernel mailing list.
  3. General issue on unrecognized target features.
  4. List of wanted Rust for Linux features.

Fixes #121970
r? rust-lang/compiler

This PR resolves issue 121970 [1] by adding `reserve-x18` as a target
feature for the aarch64 platform. Enabling the target feature marks the
x18 register as reserved so that Rust doesn't use it as a temporary
register when generating machine code. This means that passing the
`-Ctarget-feature=+reserve-x18` flag will no longer result in the
following warning:

warning: unknown feature specified for `-Ctarget-feature`: `reserve-x18`
  |
  = note: it is still passed through to the codegen backend
  = help: consider filing a feature request

Typically you will reserve the x18 register when you want to enable SCS
(the shadow call stack sanitizer [9]), because it uses x18 to store a
pointer to the shadow stack. However, it is important to not conflate
`reserve-x18` with `-Zsanitizer=shadow-call-stack` — the latter depends
on the former, but you can enable `reserve-x18` without enabling SCS.

# ABI compatibility

One concern that was brought up on issue 121970 [1] is that this flag
affects the ABI. However, it does not affect the ABI in a way where it
is a problem to mix code with and without the feature. From the ABI
spec [2]:

> X18 is the platform register and is reserved for the use of platform
> ABIs. This is an additional temporary register on platforms that don't
> assign a special meaning to it.

That is to say, the register is either already reserved (this is the
case on Android targets), or it is a caller-saved temporary register
(this is the case on `aarch64-unknown-none`). Changing a register from
caller-saved temporary register to reserved is not breaking, so
selectively enabling `reserve-x18` on some compilation targets (or even
on specific functions) cannot result in UB.

That said, *removing* the `reserve-x18` target feature from a function
can potentially trigger UB under some circumstances. This is because it
is UB to link together `-Zsanitizer=shadow-call-stack` code with code
where x18 is a temporary register. So enabling SCS in a binary requires
that x18 is reserved globally. However, right now
`-Zsanitizer=shadow-call-stack` can only be used on targets such as
Android where x18 is never a temporary register, so this shouldn't be an
issue for this PR.

# Use in the Linux Kernel

This motivation for this change is use in the Linux Kernel. When
compiling Rust code for the kernel, the `aarch64-unknown-none` target is
used, and this is a platform where x18 is a temporary caller-saved
register by default. I am proposing to add this target feature so that
the Linux Kernel can make x18 into a reserved register when necessary.

The Linux Kernel has some cases where it needs to reserve x18, but does
not pass the `-Zsanitizer=shadow-call-stack` flag. This is due to the
dynamic shadow call stack feature [3], where the Linux Kernel is
able to choose whether SCS should be enabled at boot. This works by
having the compiler emit PACIASP/AUTIASP instructions instead of
SCS_PUSH/SCS_POP. If Linux decides to enable SCS at boot, then it will
use the unwind tables to find the PACIASP/AUTIASP instructions, and
modify the machine code at runtime by replacing PACIASP/AUTIASP with
SCS_PUSH/SCS_POP instructions in all functions.

The transformation from PACIASP/AUTIASP to SCS_PUSH/SCS_POP is only
valid if the x18 register is reserved globally.

It is also possible to configure Linux to always use SCS. In this case,
it does so using the `-fsanitize=shadow-call-stack` flag instead.

The Linux Kernel configuration used by Android uses the dynamic shadow
call stack feature in production, so `reserve-x18` is a prerequisite for
using Rust in the Linux Kernel on Android.

# Alternatives

I have considered a few different alternatives.

## Add a `-Cfixed-x18` flag

When compiling C code with clang or gcc, this is configured by passing
the `-ffixed-x18` flag instead of using the target feature
functionality. We could mirror that and add our own `-Cfixed-x18` flag
to rustc. It would have the same effect as passing
`-Ctarget-feature=+reserve-x18`.

## Use a different target

The Rust compiler could provide a version of `aarch64-unknown-none`
where x18 is reserved, and the Linux Kernel build system could switch to
that target whenever `CONFIG_SHADOW_CALL_STACK` is enabled in the Linux
build system. However, there are a few disadvantages with using that
strategy for this kind of flag:

* As the number of flags that are configured in this way increases, the
  number of targets increases exponentially.
* It complicates the Kernel build system by significantly deviating from
  both clang and gcc on how this can be configured.

My understanding is that the primary reason in favor of using a
different target is that compiling the standard library yourself is
unstable, so even if this target feature is added, there is no stable
way to get a standard library compiled with
`-Ctarget-feature=+reserve-x18`.

However, as outlined in the abi stability section, there is no issue
with enabling `reserve-x18` in some crates, but not in the standard
library.

The Linux Kernel already compiles the standard library manually. Using a
prebuilt standard library is pretty unlikely to be the way forward for
many other reasons unrelated to this flag.

## Use a `target.json` in the kernel

The Linux Kernel is already using a `target.json` file for x86 targets
due to issue 116852 [4], which is a similar issue with a different
target feature.

    if cfg.has("ARM64") {
        panic!("arm64 uses the builtin rustc aarch64-unknown-none target");
    } else if cfg.has("X86_64") {
        ts.push("arch", "x86_64");
        ts.push(
            "data-layout",
            "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128",
        );
        let mut features = "-3dnow,-3dnowa,-mmx,+soft-float".to_string();
        if cfg.has("MITIGATION_RETPOLINE") {
            features += ",+retpoline-external-thunk";
        }
        ts.push("features", features);
        ts.push("llvm-target", "x86_64-linux-gnu");
        ts.push("target-pointer-width", "64");

However, Linux is trying to move away from `target.json` targets because
Rust considers `target.json` to be permanently unstable.

# Future possibilities

We could make it possible to use `-Zsanitizer=shadow-call-stack`
together with `-Ctarget-feature=+reserve-x18` to enable SCS on targets
where x18 is normally a temporary caller-saved register. This could be
done similarly to `required_panic_strategy`, which enforces that all
compilation units have a shared understanding of the panic strategy.
That is, if `-Zsanitizer=shadow-call-stack` is passed, then fail
compilation unless

1. the target is one where x18 is always reserved, or
2. `-Ctarget-feature=+reserve-x18` is passed as an argument to all crates
   in the crate graph.

This lets us avoid adding any compiler flags combinations that trigger
UB.

# References

1. Discussion in the t-compiler stream on zulip. [5]
2. Discussion on the Linux Kernel mailing list. [6]
3. General issue on unrecognized target features. [7]
4. List of wanted Rust for Linux features. [8]

Link: https://www.github.com/rust-lang/rust/issues/121970 [1]
Link: https://developer.arm.com/documentation/den0024/a/The-ABI-for-ARM-64-bit-Architecture/Register-use-in-the-AArch64-Procedure-Call-Standard/Parameters-in-general-purpose-registers [2]
Link: https://lore.kernel.org/all/[email protected]/ [3]
Link: https://www.github.com/rust-lang/rust/issues/116852 [4]
Link: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/-ffixed-x18/near/430864291 [5]
Link: https://lore.kernel.org/rust-for-linux/[email protected]/ [6]
Link: https://www.github.com/rust-lang/rust/issues/96472 [7]
Link: https://www.github.com/Rust-for-Linux/linux/issues/355 [8]
Link: https://www.github.com/rust-lang/rust/pull/98208 [9]
Signed-off-by: Alice Ryhl <[email protected]>
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 24, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 24, 2024

Some changes occurred in tests/ui/check-cfg

cc @Urgau

@joshtriplett
Copy link
Member

Seems like a reasonable addition. 👍

@traviscross
Copy link
Contributor

@rustbot labels +T-lang +I-lang-nominated

Let's discuss this as this is important for RfL work and as we need to sign off on new target features.

@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 29, 2024
@Urgau
Copy link
Member

Urgau commented Apr 29, 2024

Let's discuss this as this is important for RfL work and as we need to sign off on new target features.

This new target feature is currently being added as insta-stable, but new target features can be unstably added, without requiring T-lang sign off before hand.

If RfL wants this to be merged quickly or simply experiment before hand, this PR could merged quickly as unstable.

@Darksonn
Copy link
Contributor Author

Darksonn commented Apr 29, 2024

I think it's fine to go straight to stabilization. The flag already works today since unknown target features are still passed through to LLVM, so this isn't a blocker for us to begin experimenting with it. I am using it in the Android Kernel build today, and it works great.

@RalfJung
Copy link
Member

RalfJung commented Apr 30, 2024

Wow, what a nightmare mess.^^

However, right now -Zsanitizer=shadow-call-stack can only be used on targets such as Android where x18 is never a temporary register, so this shouldn't be an issue for this PR.

This motivation for this change is use in the Linux Kernel. When compiling Rust code for the kernel, the aarch64-unknown-none target is used, and this is a platform where x18 is a temporary caller-saved register by default. I am proposing to add this target feature so that the Linux Kernel can make x18 into a reserved register when necessary.

The Linux Kernel has some cases where it needs to reserve x18, but does not pass the -Zsanitizer=shadow-call-stack flag.

So this SCS flag is only for Android targets but Linux also uses it for aarch64-unknown-none -- that seems to contradict each other?

It seems further that this target feature is only useful for "SCS sanitizing without -Zsanitizer=shadow-call-stack"? And that using SCS sanitizing (with or without the flag) requires x18 to be reserved in all compilation units? I suppose -Zsanitizer=shadow-call-stack will hence set the require-x18 flag, so this amounts to saying "sanitizer-enabled code can only be linked with other sanitizer-enabled code" (else UB)? Is that a common requirement for sanitizers? It sounds like a big footgun.

TBH this does not at all feel like a target feature to me; target features are usually for things like "does the target support certain instructions". This here is an ABI degree of freedom, with subtle sanitizer interactions. Not sure how we usually handle them; I saw proposals for "ABI variants" in other sanitizer work, and this seems like a possible use-case for that?

@Darksonn
Copy link
Contributor Author

Darksonn commented Apr 30, 2024

So this SCS flag is only for Android targets but Linux also uses it for aarch64-unknown-none -- that seems to contradict each other?

Android is just an example here. Right now, some aarch64 targets reserve x18 and some don't. The sanitizer is only listed as supported on targets that do, and Android is one such target (but not the only one, see #121966).

The Linux kernel supports SCS on all aarch64 builds. It reserves x18 iff you enable SCS.

It seems further that this target feature is only useful for "SCS sanitizing without -Zsanitizer=shadow-call-stack"? And that using SCS sanitizing (with or without the flag) requires x18 to be reserved in all compilation units? I suppose -Zsanitizer=shadow-call-stack will hence set the require-x18 flag, so this amounts to saying "sanitizer-enabled code can only be linked with other sanitizer-enabled code" (else UB)? Is that a common requirement for sanitizers? It sounds like a big footgun.

I'm not aware of any other uses than SCS, no.

I think it's pretty common for sanitizers to require that you enable them globally, though I don't think SCS does. As long as x18 is reserved globally, it works even if you mix-and-match SCS. In that case, it has the same behavior as-if the functions without SCS were inlined into the SCS function that calls them.

TBH this does not at all feel like a target feature to me; target features are usually for things like "does the target support certain instructions". This here is an ABI degree of freedom, with subtle sanitizer interactions. Not sure how we usually handle them; I saw proposals for "ABI variants" in other sanitizer work, and this seems like a possible use-case for that?

I believe the answer is that we don't handle the problem right now.

Also, in gcc and clang, this is configured with a flag called -ffixed-x18 rather than a target feature, so I don't think you're alone in thinking it doesn't feel like a target feature.

Ultimately, I have no attachment to this being a target feature. I just want some way to enable my use-case. For example, something along these lines would also make sense to me:

  1. Declare that mixing sanitizers is unsupported and UB.
  2. Add a -Cfixed-x18 flag for the kernel's use-case. Adding this flag on its own can never trigger UB like explained in my original post.
  3. List the SCS sanitizer as supported on aarch64 targets even if they don't reserve x18 normally. (And when you enable it, that implicitly reserves x18. Or we could require that you pass -Cfixed-x18.)
  4. Fail compilation if sanitizers are not applied globally. (And maybe we have a -Zallow-mixed-sanitizers, though I don't need that.)

@RalfJung
Copy link
Member

RalfJung commented May 1, 2024

Android is just an example here. Right now, some aarch64 targets reserve x18 and some don't. The sanitizer is only listed as supported on targets that do, and Android is one such target (but not the only one, see #121966).

This proposal is about supporting the sanitizer on targets that do not reserve x18, right? Specifically, aarch64-unknown-none. So then the argument that there is no incompatibility no longer applies, since the entire point of this proposal is to enable using the sanitizer in a regime where there is an incompatibility. That's why you need the flag in the first place.

Ultimately, I have no attachment to this being a target feature.

To be clear, I don't have a strong opinion here either -- but it just doesn't "feel" like a target feature.

One proposal may be to describe the intended effect instead of the mechanism. So, basically, a flag that says "build this in a way that is compatible with some sanitizer" (but without actually adding the sanitizer support itself). Not sure if that makes sense -- it would make it easier to describe what we actually guarantee about the resulting binary, and makes it very obvious that when linking with code that does not use the sanitizer, the presence of the flag changes nothing (except for performance, I guess).

@Darksonn
Copy link
Contributor Author

Darksonn commented May 2, 2024

I'm open to flags that just say "compatible with X sanitizer", but I still think that -Cfixed-x18 is also reasonable. It's not too hard to explain what we guarantee: x18 will not be used as a temporary register in the generated machine code.

And -Cfixed-x18 has the advantage that it is consistent with gcc and clang.

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024 via email

@nbdd0121
Copy link
Contributor

nbdd0121 commented May 2, 2024

It seems further that this target feature is only useful for "SCS sanitizing without -Zsanitizer=shadow-call-stack"? And that using SCS sanitizing (with or without the flag) requires x18 to be reserved in all compilation units? I suppose -Zsanitizer=shadow-call-stack will hence set the require-x18 flag, so this amounts to saying "sanitizer-enabled code can only be linked with other sanitizer-enabled code" (else UB)? Is that a common requirement for sanitizers? It sounds like a big footgun.

We already have cases where one compilation unit enforces some compilation option being used for all other compilation units, namely the panic strategy. It doesn't feel too bad to me that if a crate is compiled with SCS enabled then we require all other crates requiring SCS enabled or reserve-x18 target feature set. I agree it'll be a big footgun if a mismatching use causes UB, but I think if we add checks to ensure the constraint is met (else compiler error) then it should be fine.

@RalfJung
Copy link
Member

RalfJung commented May 3, 2024

We still need to also carefully document this -- the checks only work when rustc is doing the (static) linking; there's also the case of building .so files with rustc and then linking them together externally.

@Darksonn Darksonn mentioned this pull request May 3, 2024
@Darksonn
Copy link
Contributor Author

Darksonn commented May 3, 2024

I opened a PR that adds a -Zfixed-x18 flag instead: #124655. Please let me know if that approach is preferable.

@Darksonn
Copy link
Contributor Author

I am closing this in favor of #124655.

@Darksonn Darksonn closed this May 15, 2024
@Darksonn Darksonn mentioned this pull request May 15, 2024
3 tasks
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 29, 2024
Add `-Zfixed-x18`

This PR is a follow-up to rust-lang#124323 that proposes a different implementation. Please read the description of that PR for motivation.

See the equivalent flag in [the clang docs](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffixed-x18).

MCP: rust-lang/compiler-team#748
Fixes rust-lang#121970
r? rust-lang/compiler
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2024
Rollup merge of rust-lang#124655 - Darksonn:fixed-x18, r=lqd,estebank

Add `-Zfixed-x18`

This PR is a follow-up to rust-lang#124323 that proposes a different implementation. Please read the description of that PR for motivation.

See the equivalent flag in [the clang docs](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffixed-x18).

MCP: rust-lang/compiler-team#748
Fixes rust-lang#121970
r? rust-lang/compiler
@Darksonn Darksonn deleted the target-feature-reserve-x18 branch September 3, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler does not recognize the reserve-x18 target feature
8 participants