Constrained Naked Functions

[npmccallum]

Rendered

Summary

This is a quick attempt at improving the usefulness of naked functions by constraining their use.

Tasks

• Resolve disabling unused arguments lint
• Finalize the RFC
• Implement:
  • warn if calling convention is unspecified - WARN
  • warn if calling convention is extern "Rust" - WARN
  • warn if arguments contain FFI-unsafe arguments - PR
  • warn if return types contain FFI-unsafe arguments - PR
  • move FFI-unsafe warnings to their own lint - PR
  • error if inline attributes are present - WARN
  • error if wrong number of asm blocks - WARN
  • error if asm contains operands other than const or sym - WARN
  • error if asm is missing options(noreturn) - WARN
  • error if options except att_syntax are used - WARN and WARN
  • don't lint for unused function arguments - PR
  • don't emit instructions in the function body before asm!()

CC

rust-lang/rust#32408
rust-lang/rust#72016
#2774

@amluto @comex @programmerjake @Amanieu @haraldh @bjorn3 @joshtriplett

[Amanieu]

Just a few minor nits but otherwise great work!

[Amanieu]

I don't see why we should require such functions to be unsafe. They can't be called by Rust code at all so it shouldn't matter whether they are safe or unsafe.

[Ekleog]

I was originally going to say the same thing, but it actually somehow makes sense: these functions always have a caller requirement (using the right calling convention), so it's kind of logical that passing around extern "custom" fn() pointers should be forbidden to force the use of unsafe extern "custom" fn().

This being said, it is totally possible to imagine a custom calling convention whereby eg. the argument is passed in a global variable and all bitpatterns are valid, so any call would actually be valid, and thus it wouldn't require unsafe.

So, with all the above considered, I also believe that it'd be best to remove this requirement, that AFAIU doesn't bring much additional value.

[npmccallum]

I added that merely to be defensive, though I had the same thought. I'm curious what others think about this.

[amluto]

To me,

that can be used only with naked functions

means that you can't declare an extern "custom" function that is defined in a different language. I assume you intended for this to be allowed, and maybe the wording should change.

As for the unsafe issue, it occurs to me that extern "custom" cheats the type system a bit. It's possible and even likely for multiple extern "custom" functions that appear identically typed to Rust to have different custom calling conventions. This could be awkwardly addressed by something like extern "custom/x86_idt" and extern "custom/x86_syscall", or it could be addressed by encouraging people who pass around pointers to extern "custom" functions to wrap them in more descriptive structs. To that end, perhaps unsafe is the right choice.

As another way of looking at this, I can write a safe function (safe to call and implemented without unsafe) that takes a pointer to a function with a known calling convention and calls it. The called function may itself be problematic, but one might argue that this is a problem with the callee. But I can't write a function that is safe to call and takes an extern "custom" pointer because nothing verifies that the passed pointer is the correct type of custom.

(As an aside, it's not quite clear to me what "memory safety" means in the context of an operating system. Dereferencing a wild pointer is obviously memory unsafe, but what about mis-programming the CPU or associated hardware in a way that causes crashes? I suppose this boils down to the preference of whomever designs the OS.)

[npmccallum]

Yes, I think we should definitely think about the calling convention as a "function type." This is, I think, behind my intuition that these should be marked as unsafe. Basically, extern "custom" is a form of type erasure.

I like the idea of a custom prefix which allows you to declare your own name. This sort of adds type safety. Unless there's a collision, of course.

[programmerjake]

I like the idea of a custom prefix which allows you to declare your own name. This sort of adds type safety. Unless there's a collision, of course.

What about going all out and having non-string ABIs -- similar to generics in that the ABI is a type parameter to the generic class of functions. extern "The-ABI" would be syntax sugar for extern<core::abi::The_ABI> where The-ABI is translated by replacing all - with _. So extern "system-unwind" would be syntax sugar for extern<core::abi::system_unwind>.

// declare a custom ABI
#[derive(ExternABI)]
struct MyABI;

// use a custom ABI
fn f(fn_ptr: unsafe extern<MyABI> fn()) {}

fn generic_over_abi<ABI: ExternABI>(fn_ptr: unsafe extern<ABI> fn()) {}

// both the same type
type ExternCFnPtr = unsafe extern<core::abi::C> fn();
type ExternCFnPtr2 = unsafe extern "C" fn();

[Amanieu]

I feel that this is over-extending the scope of the RFC. Simple newtype wrappers around an extern "custom" function pointer are enough to build type-safe abstractions, there is no need to add complexity to the language for such a niche feature.

Regarding unsafe, it really doesn't make a difference in practice since the only difference between a fn and unsafe fn is that the latter can only be called from within an unsafe block. This doesn't apply to extern "custom" since it can't be called from Rust at all.

[programmerjake]

I feel that this is over-extending the scope of the RFC. Simple newtype wrappers around an extern "custom" function pointer are enough to build type-safe abstractions, there is no need to add complexity to the language for such a niche feature.

Ok, though my proposal is also useful for writing code that is generic over the ABI, which is useful when trying to handle generic fn types. This is more related to the RFC for adding a trait that is implemented by all fn types, which I can't find at the moment.

[Ekleog]

@Amanieu To me, there is some difference in meaning between having the function unsafe or not. If I see:

• extern "C" { fn foo(f: extern "custom" fn()); }, then I expect any non-unsafe extern "custom" function to be possible to put there without memory safety impact — meaning that non-unsafe extern "custom" functions are necessarily functions that never return and that don't assume anything about the caller, to make it possible to have such a signature
• extern "C" { fn foo(f: unsafe extern "custom" fn()); }, then I expect foo to run f in some kind of virtual machine to protect itself against misuse, as it cannot know for sure what ABI to call f in
• extern "C" { unsafe fn foo(f: [unsafe] extern "custom" fn()); }, then I read foo's documentation on what kinds of extern "custom" functions it accepts

So I do believe that there is a semantic difference between unsafe and non-unsafe extern "custom" functions, but the non-unsafe extern "custom" functions are restricted to the set of functions that never return and don't assume anything from their caller (apart from what all rust programs assume by default, eg. “there is a stack and it doesn't overlap with the heap”)

I'd be interested in other views of which extern "custom" functions should or should not be unsafe, that said

[npmccallum]

I am preparing an update which addresses the feedback thus far. Most of the feedback has been straightforward. However, most of the unresolved discussion has centered around defining custom calling conventions.

In the spirit of moving things forward, I propose that we move custom calling conventions to the future possibilities section as an idea for future improvement. Constrained naked functions are useful without custom calling conventions. Further, dealing with custom calling conventions separately eases the path to acceptance and stabilization for constrained naked functions.

[npmccallum]

An update on implementation of this RFC is in order.

I fixed a case where rustc would insert instructions before the asm!() block. I am now using #[naked] quite heavily in various projects and have a degree of confidence that we now conform to the last compiler commitment.

The remaining tasks basically fall to warnings and the generation of the implicit #[inline(never)].

We also still need to resolve the question of implicit #[used] for function arguments.

[Amanieu]

#[used] only applies to globals such as functions and statics. In the case of naked functions all we need to do is suppress the unused argument lint.

[amluto]

Is this RFC actually useful with the extern "custom" part removed?

[npmccallum]

Is this RFC actually useful with the extern "custom" part removed?

To me, yes, very. It means we can stabilize naked sooner rather than later. Custom calling conventions can go through their own design process and added to naked without disruption.

[amluto]

I’m trying to understand how one might create a naked extern “C” in a way that is correct and also accomplishes something useful.

[npmccallum]

I’m trying to understand how one might create a naked extern “C” in a way that is correct and also accomplishes something useful.

https://github.com/enarx/frenetic/blob/master/src/arch/x86_64/sysv.rs#L30-L50

While it might be true that the uses which strictly speaking require naked are not super common, the number of cases where naked can either (1) improve efficiency or (2) offload concerns like stack alignment to the compiler is much larger.

One important point is that I'm working on this because I need naked and I want to ensure that there is a clear path to stable for the benefit of my users. I deferred the question of extern "custom" because I don't want the perfect to become the enemy of the good.

[npmccallum]

#[used] only applies to globals such as functions and statics. In the case of naked functions all we need to do is suppress the unused argument lint.

Resolved in the latest commit.

[haraldh]

I would like to use it for the syscall kernel function.

https://github.com/enarx/enarx/blob/master/enarx-keep/shims/shim-sev/src/syscall.rs#L26-L86

[roblabla]

In my case, I'd like to use it for a low-level context switching function (saving and restoring register states) in my kernel. My current implementation doesn't use naked function (because naked functions have historically been too buggy to use), and takes a slight performance penalty as a result (the compiler inserts a useless prelude). I have no need for extern "custom", as my function is meant to be C ABI compliant.

It's generally possible to replace naked functions with an extern block and a global_asm!()/external asm file, but that comes at the cost of creating a global symbol, potentially leading to symbol conflicts if the same name is used twice.

[amluto]

I’m trying to understand how one might create a naked extern “C” in a way that is correct and also accomplishes something useful.

https://github.com/enarx/frenetic/blob/master/src/arch/x86_64/sysv.rs#L30-L50

While it might be true that the uses which strictly speaking require naked are not super common, the number of cases where naked can either (1) improve efficiency or (2) offload concerns like stack alignment to the compiler is much larger.

I can see how this type of context switching would make sense in a naked, extern "C" context, but I have a question about your implementation. As far as I can see, your save function can return more than once. This seems like something that rustc would not expect, and I'm surprised to see that functionality show up in a non-unsafe function.

[amluto]

I would like to use it for the syscall kernel function.

https://github.com/enarx/enarx/blob/master/enarx-keep/shims/shim-sev/src/syscall.rs#L26-L86

This would need extern "custom" to really be correct, right?

Also, I find it surprising that you are swapgs-ing back to user GSBASE before running Rust code. You also seem to be leaking whatever Rust sticks in rcx and r11 to userspace. And you're returning with iret, which is painfully slow but at least avoids the return-to-noncanonical-address SYSRET issue on Intel CPUs.

[roblabla]

As far as I can see, your save function can return more than once. This seems like something that rustc would not expect, and I'm surprised to see that functionality show up in a non-unsafe function.

load is unsafe though, and is ultimately what triggers the "second return". But, yes, this pattern is unlikely to be sound, just like how setjmp/longjmp is currently unsound

[haraldh]

I would like to use it for the syscall kernel function.
https://github.com/enarx/enarx/blob/master/enarx-keep/shims/shim-sev/src/syscall.rs#L26-L86

This would need extern "custom" to really be correct, right?

Yes, I guess so.

Also, I find it surprising that you are swapgs-ing back to user GSBASE before running Rust code.

yeah, maybe I'll change that.

You also seem to be leaking whatever Rust sticks in rcx and r11 to userspace.

Thanks for the heads-up.

And you're returning with iret, which is painfully slow but at least avoids the return-to-noncanonical-address SYSRET issue on Intel CPUs.

Yeah, that was the intention. When we will do performance optimizations, we will turn on sysret for AMD.

[josephlr]

Could we extend this to allow multiple asm! blocks, to support #[cfg(...)]? Right now I have code that looks like:

#[naked]
pub unsafe extern "C" fn start64() {
    #[cfg(feature = "init_data")]
    asm!(
        "movabs  rsi, offset __init_data",
        "movabs  rdi, offset __start_data",
        "movabs  rcx, offset __stop_data",
        "sub     rcx, rdi",
        "rep movsb [rdi], [rsi]",
    );
    #[cfg(feature = "zero_bss")]
    asm!(
        "movabs  rdi, offset __start_bss",
        "movabs  rcx, offset __stop_bss",
        "sub     rcx, rdi",
        "xor     eax, eax",
        "rep stosb [rdi], al",
    );
    asm!(
        "movabs  rsp, offset __rust_stack_top",
        "movabs  rax, offset __rust_start",
        "jmp     rax",
        options(noreturn)
    )
}

which seems to meet all the requirements we want to have (only asm!, ends with noreturn). But this code will currently trigger the lint despite being "safe".

[roblabla]

@josephlr this code block doesn't meet this requirement:

must have a body which contains only a single asm!() statement

As if one of the feature is enabled, there are multiple asm statements.

[josephlr]

As if one of the feature is enabled, there are multiple asm statements.

Exactly. That's why I was wondering about the feasibility of (now or as a future extension) allowing multiple asm! blocks in a naked function.

Otherwise, you would need weird macros to get the above code to compile without warnings.

[Amanieu]

I've been using macro_rules which expand to string literals in my code. For example you can do this:

#[cfg(feature = "zero_bss")]
macro_rules! zero_bss {
    () => {
        r#"
            movabs  rdi, offset __start_bss
            movabs  rcx, offset __stop_bss
            sub     rcx, rdi
            xor     eax, eax
            rep stosb [rdi], al
        "#
    }
}
#[cfg(not(feature = "zero_bss"))]
macro_rules! zero_bss {
    () => { "" }
}

asm!(
    zero_bss!(),
    "movabs  rsp, offset __rust_stack_top",
    "movabs  rax, offset __rust_start",
    "jmp     rax",
    options(noreturn)
);

[npmccallum]

I'd like to propose one small modification to this RFC for consideration. Currently, the RFC defines that the constrained naked functions "must not specify the #[inline] or #[inline(always)] attribute" and in turn the complier will emit the equivalent of #[inline(never)]. This means, however, that we can never change this behavior in the future.

I'd like to change the wording instead to define that constrained naked functions "must additionally define the #[inline(never)] attribute. This has several benefits. First, the complier behavior is explicit rather than implicit. Second, if we desire to change this behavior in the future we can loosen the requirement. Third, the code becomes self-documenting.

Thoughts?

[joshtriplett]

@npmccallum That sounds reasonable. In general, I think "requires specifying" seems preferable to "implies" unless we're absolutely certain that it'll never ever make sense to do otherwise.

And I can imagine wanting to inline a naked function that follows the calling convention.

[joshtriplett]

This seems reasonable, it doesn't have any apparent outstanding issues, and it's actually implemented in nightly.

Thus, I would propose we accept this RFC

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[mark-i-m]

This is a truly excellent RFC. I have wanted this feature for a long time, and this RFC resolves a lot of open discussions and questions in a simple and flexible way. 🎉

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[roblabla]

Sorry, I'm a little late to the party, but there's a concern that I just raised in the Tracking issue: currently, naked functions have an extra ud2 instruction inserted after the asm block due by the TrapUnreachable LLVM option. According to Amanieu:

I did a bit of searching in LLVM's code and it seems that TrapUnreachable is actually required on some targets because the unwinder requires that call return addresses point within the bounds of the caller function. This requires adding an ud2 (or equivalent) at the end of a function to ensure the byte after a noreturn call is still inside the function.

In conclusion I don't think that we can actually guarantee the lack of any instructions in the epilogue with the current implementation in LLVM.

See this comment.

To workaround this, we may want to ensure naked functions are additionally marked nounwind. I don't think I've ever seen a naked function that called an unwinding function, and if it did, I'd expect it to deal with the unwinding itself, through assembler directives (that's rather the point of naked functions - they are supposed to deal with everything).

I'm not sure if this would be enough to allow naked functions to guarantee to only generated the exact asm block (and have no suffix instructions) though.

[npmccallum]

If @roblabla 's suggestion actually allows us to discard the trailing ud2, I'm in favor of the suggestion and would be willing to delay the RFC to implement the requirement if necessary. However, it may not be necessary to block the RFC on this, pending the input of others.

I propose that we accept the RFC as it stands and explore this option as an additional requirement after the RFC lands. If we do this, we could simply enforce this with a test. This would delay the stabilization of naked functions, but not necessarily this RFC.

Thoughts?

[nikomatsakis]

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

Tracking issue: rust-lang/rust#90957

If you'd like to keep following the development of this feature, please subscribe to that issue, thanks! :)