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

Support x86_64-unknown-linux-none #424

Closed
briansmith opened this issue May 17, 2024 · 32 comments · Fixed by #520
Closed

Support x86_64-unknown-linux-none #424

briansmith opened this issue May 17, 2024 · 32 comments · Fixed by #520

Comments

@briansmith
Copy link
Contributor

The target was added in rust-lang/rust#125023 and is available in the latest Nightly builds. Use cargo +nightly build --target=x86_64-unknown-linux-none -Zbuild-std.

The libc crate does NOT support this target, so libc::syscall and libc::getrandom cannot be used on this target.

I am planning to support this target in ring soon.

@newpavlov
Copy link
Member

I think #401 is relevant here. We need a way of doing raw syscalls for this. Ideally, it would be handled by the linux-raw-sys crate, but @sunfishcode is hesitant to add raw syscall functions to it.

@josephlr
Copy link
Member

I think if we want to support this target, we should just write the inline assembly to invoke the x86_64 syscall instruction, and hardcode the raw syscall number (318).

I agre with @sunfishcode that supporting a general mechanism for invoking any syscall on any Linux CPU architecture is very complicated (vDSO, some syscalls may or may not return, vfork, inline assembly isn't stable on all architectures supported by Rust, etc...).

But the situation for getrandom(2) + x86_64 specifically is much simpler, Linux only uses syscall on x86_64, and x86_64 inline assembly is stable. So the implementation can just be:

const NR_GETRANDOM: u32 = 318;

pub fn getrandom_syscall(buf: &mut [MaybeUninit<u8>], flags: u32) -> isize {
    unsafe {
        let ret;
        asm!(
            "syscall",
            inlateout("rax") NR_GETRANDOM as isize => ret,
            in("rdi") buf.as_mut_ptr(),
            in("rsi") buf.len(),
            in("rdx") flags,
            lateout("rcx") _,
            lateout("r11") _,
            options(nostack, preserves_flags)
        );
        ret
    }
}

@newpavlov
Copy link
Member

newpavlov commented May 20, 2024

@josephlr
Hm, you are right. If the raw target is limited to x86-64 it would be the easiest way to do this.

We probably can do it for other target arches as well (on x86 we can just use int 0x80 for simplicity) and gate it behind a hypothetical linux_raw_syscall feature.

@sunfishcode
Copy link
Contributor

In the PR, @morr0ne said that x86_64-unknown-linux-none won't have libc, so it's likely that many crates will use rustix for I/O on *-linux-none, so rustix will often be in people's dependency trees already. Perhaps that makes it feasible for getrandom to just use rustix::rand::getrandom_uninit on x86_64-unknown-linux-none.

If it were just getrandom it may seem ok to hard-code a little bit of asm, but if other crates start following this example and we end up with many crates doing their own syscall asms, I think that would be unfortunate. And, it's reasonable to anticipate more linux-none targets than just x86_64.

@morr0ne
Copy link

morr0ne commented May 20, 2024

In the PR, @morr0ne said that x86_64-unknown-linux-none won't have libc, so it's likely that many crates will use rustix for I/O on *-linux-none, so rustix will often be in people's dependency trees already. Perhaps that makes it feasible for getrandom to just use rustix::rand::getrandom_uninit on x86_64-unknown-linux-none.

The *-linux-none targets heavily rely on something like rustix to work; at the moment its basically guaranteed that rustix will already be in the dependency tree.

If it were just getrandom it may seem ok to hard-code a little bit of asm, but if other crates start following this example and we end up with many crates doing their own syscall asms, I think that would be unfortunate. And, it's reasonable to anticipate more linux-none targets than just x86_64.

The x86_64-unknown-linux-none target is just the first of other *-linux-none targets. I plan to implement at the very least aarch64 and riscv64gc targets. Using asm here would mean having to implement such syscall manually for each potential target.

@newpavlov
Copy link
Member

newpavlov commented May 20, 2024

rustix does a lot of things and it's a relatively big dependency (42k LoCs in src/ according to tokei). As I wrote in the other issue, I like rustix and use it in my projects, but it's often prohibitively big for certain things. getrandom is just one of the examples. Another example is the io-uring crate, it needs only 3 syscalls and it handles associated types by itself.

This is why I would strongly prefer to have raw syscalls in the linux-raw-sys crate. In my opinion, the safety arguments against it just don't hold water in the light of the libc::sycall existence. Yes, it's an extremely dangerous unsafe tool and most people should use rustix, but it does not mean it should not be exposed. Arguably, it's similar to how we often expose *-sys crates, while users are directed toward safe wrappers.

If raw syscalls will not be added to linux-raw-sys, then our next best option is to define raw syscalls by ourselves as proposed by @josephlr above. Outside of the x86 targets (if we do not want to use int 0x80), it's a relatively straightforward piece of code.

@briansmith
Copy link
Contributor Author

briansmith commented May 20, 2024

@josephlr wrote:

I think if we want to support this target, we should just write the inline assembly to invoke the x86_64 syscall instruction, and hardcode the raw syscall number (318).

I agree with this suggestion, and this is what I'm planning to do in ring for all x86_64-*-linux-* targets.

getrandom shouldn't add rustix as a dependency for this.

At the most, we could have a feature flag to use rustix instead of the inline assembly support. I suspect there are reason why some rustix users would prefer this, e.g. if they are using rustix to port to a non-Linux OS. But there's a separate issue for rustix already, to track this.

If getrandom is going to have the inline assembly, it should use the inline assembly for all x86_64-*-linux-* targets and eliminate the libc dependency for these targets, so that it gets tested continuously.

@briansmith
Copy link
Contributor Author

This is why I would strongly prefer to have raw syscalls in the linux-raw-sys crate.

I would rather not have getrandom depend on linux-raw-sys crate. Chances are high that there will be bugs in linux-raw-sys crate that will not affect getrandom but which would trigger dependency management tools, causing a lot of churn and wasted effort.

@newpavlov
Copy link
Member

newpavlov commented May 20, 2024

Chances are high that there will be bugs in linux-raw-sys crate that will not affect getrandom but which would trigger dependency management tools, causing a lot of churn and wasted effort.

I don't think it's likely. The crate is a largely automatic conversion of the Linux headers and has almost no issues in its tracker despite being used actively (mostly through rustix). Comparatively, libc is a more problematic dependency and it has caused several breakages in getrandom for various reasons.

I think that maintaining inline assembly for syscalls and syscall numbers (rant: I wish Linux had stable syscall numbers across targets...) for various targets should be outside of the getrandom's area of responsibility. Ideally, we need THE crate for doing raw Linux syscall stuff, which would be used across the crates ecosystem. linux-raw-sys looks like the best candidate for this right now.

@sunfishcode
Copy link
Contributor

sunfishcode commented May 20, 2024

What are the practical problems with using rustix for just *-linux-none, and leaving the rest of getrandom as-is?

@briansmith
Copy link
Contributor Author

briansmith commented May 20, 2024

@josephlr wrote:

But the situation for getrandom(2) + x86_64 specifically is much simpler, Linux only uses syscall on x86_64, and x86_64 inline assembly is stable. So the implementation can just be:

That would support the target when the dev/urandom fallback is disabled. When they added this target, I asked them to commit to a minimum kernel version that would include the getrandom syscall, but they didn't want to. Thus, the minimum kernel version should be assumed to be the same as the Rust project's minimum in general for x86_64-linux; i.e. a dev/urandom fallback would be needed for this target as well.

FYI: In ring, for *-*-linux-*, I am considering requiring the user to opt into a libstd dependency for the dev/urandom fallback, and implementing the syscall much like @josephlr proposed. I would be willing to help getrandom maintain a standalone syscall wrapper.

What are the practical problems with using rustix for just *-linux-none, and leaving the rest of getrandom as-is?

At least for me (ring), I don't want to (in some sense, can't) have divergent codepaths for the x86_64-*-linux-* targets (in configurations for which the dev/urandom fallback is disabled). I expect there will be many rustix-like things in the next few years, unless/until rustix becomes a rust-lang project.

@sunfishcode
Copy link
Contributor

At least for me (ring), I don't want to (in some sense, can't) have divergent codepaths for the x86_64-*-linux-* targets. I expect there will be many rustix-like things in the next few years, unless/until rustix becomes a rust-lang project.

I'm not familiar with ring. What are the practical considerations around using different codepaths for the x86_64-*-linux-* targets?

I expect there will be many rustix-like things in the next few years, unless/until rustix becomes a rust-lang project.

If we only used rustix for *-linux-none, which very few people are using today, how much of a practical concern is this?

@newpavlov
Copy link
Member

newpavlov commented May 20, 2024

What are the practical problems with using rustix for just *-linux-none, and leaving the rest of getrandom as-is?

  1. Complexity and general size. It increases compilation times and makes it harder to review code in security-sensitive settings.
  2. It increases chances of accidentally pulling libc (e.g. by enabling features). You could cfg this out for *-linux-none targets, but it will make rustix less consistent (e.g. it will be strange if the use-libc feature will have no effect).
  3. It has a relatively high MSRV, which is likely to be bumped in future. (This point will be no longer relevant after wide adoption of the MSRV-aware resolver)
  4. It evolves faster than linux-raw-sys and has a much bigger API surface, which increases downstream maintenance burden and probability of introducing new bugs.

I will reverse the question: what are the practical problems with exposing raw syscall functions in linux-raw-sys? As I wrote above and in the rustix issue, I don't consider the safety argument valid. The only stumbling block which I see is the vDSO stuff on x86 targets. One potential solution for this is to use int 0x80 (or potentially sysenter) in linux-raw-sys and implement vDSO-based syscalls in a separate crate or leave it in rustix.

@briansmith
Copy link
Contributor Author

If we only used rustix for *-linux-none, which very few people are using today, how much of a practical concern is this?

I am using *-linux-none without rustix for ring specifically for unspecified reasons.

@morr0ne
Copy link

morr0ne commented May 20, 2024

That would support the target when the dev/urandom fallback is disabled. When they added this target, I asked them to commit to a minimum kernel version that would include the getrandom syscall, but they didn't want to. Thus, the minimum kernel version should be assumed to be the same as the Rust project's minimum in general for x86_64-linux; i.e. a dev/urandom fallback would be needed for this target as well.

When adding the target I specifically did not want to add a minimum version because I felt it was too soon considering I was the only user of the target. Seeing there is interest in adding support for the *-linux-none target(s) in getrandom then it is reasonable to bump the minimum kernel version to support the required syscall.

@morr0ne
Copy link

morr0ne commented May 20, 2024

Regarding the rustix/linux-raw-sys argument I believe we are missing some in-between crate.
Currently rustix bridges the gap between libc and libc-less targets by having both a linux-raw-sys backend and a libc backend. However the linux-raw-sys backend isn't truly a "backend" since it only provides raw definition to kernel headers.

I think a proper solution would be to have a "linux-syscall" crate that depends on linux-raw-sys and provides access to linux syscalls; rustix could then depend on both this new crate and libc to provide a unified api like it does today. Projects like getrandom that cannot or do not want to depend on rustix can depend directly on the new "linux-syscall" crate to talk to the kernel.

@newpavlov
Copy link
Member

newpavlov commented May 20, 2024

However the linux-raw-sys backend isn't truly a "backend" since it only provides raw definition to kernel headers.

This is why I created sunfishcode/linux-raw-sys#116.

I think a proper solution would be to have a "linux-syscall" crate that depends on linux-raw-sys and provides access to linux syscalls

I don't think it makes much sense to introduce a separate crate. You wrote yourself that this hypothetical crate would need to depend on linux-raw-sys and I don't see much use for linux-raw-sys in its current form without syscall functions. It's just that the syscall functions reside in rustix and not exposed in public API. So why not keep them directly in linux-raw-sys?

Drawing parallels with the classic *-sys crates, they provide not only type definitions, but also functions which work with those types. As I see it, the only difference with linux-raw-sys is that instead of wrapping (shared) library functions it should wrap syscall "functions".

@morr0ne
Copy link

morr0ne commented May 20, 2024

I don't think it makes much sense to introduce a separate crate. You wrote yourself that this hypothetical crate would need to depend on linux-raw-sys and I don't see much use for linux-raw-sys in its current form without syscall functions. It's just that the syscall functions reside in rustix and not exposed in public API. So why not keep them directly in linux-raw-sys?

Drawing parallels with the classic *-sys crates, they provide not only type definitions, but also functions which work with those types. As I see it, the only difference with linux-raw-sys is that instead of wrapping (shared) library functions it should wrap syscall "functions".

The linux-raw-sys crate cannot provide a syscall function because such a function does not exist. Currently the api is generated from the kernel userspace api in the same way *-sys crates are generated from C headers. The kernel does not provide any function to call syscalls but just the appropriate definition to create a wrapper manually. Such a wrapper would be outside the scope of a *-sys crate

@newpavlov
Copy link
Member

The linux-raw-sys crate cannot provide a syscall function because such a function does not exist.

Such a wrapper would be outside the scope of a *-sys crate

I disagree. For most practical intents and purposes syscalls are functions, they just have a "weird" ABI, which can not be properly described in C headers (well, I guess technically you could do it? but historically people don't do it for various reasons). The items defined in the Linux headers are intended to by used with syscalls, so, arguably, it makes sense to have them in one crate.

That said, I am fine with having a separate syscall crate when compared to the current status quo. I guess such crate could be relatively stable, while linux-raw-sys may need new releases on future Linux kernel releases which introduce new stuff in public API.

@morr0ne
Copy link

morr0ne commented May 20, 2024

I disagree. For most practical intents and purposes syscalls are functions, they just have a "weird" ABI, which can not be properly described in C headers (well, I guess technically you could do it? but historically people don't do it for various reasons). The items defined in the Linux headers are intended to by used with syscalls, so, arguably, it makes sense to have them in one crate.

Perhaps I didn't explain myself properly. What I mean to say is that syscalls are not something we can automatically generate but need to be manually implemented. linux-raw-sys is fully generated code.

That said, I am fine with having a separate syscall crate when compared to the current status quo. I guess such crate could be relatively stable, while linux-raw-sys may need new releases on future Linux kernel releases which introduce new stuff in public API.

That is somewhat the idea however we really need @sunfishcode input on this

@josephlr
Copy link
Member

Perhaps I didn't explain myself properly. What I mean to say is that syscalls are not something we can automatically generate but need to be manually implemented. linux-raw-sys is fully generated code.

I think it would be possible for the linux-raw-sys crate to generate the syscall wrapper functions, as all their information is present in the various Linux headers, so they could (in theory) be auto-generated. For example:

  • include/uapi/asm-generic/unistd.h (and arch-specific files) defines __NR_getrandom and references sys_getrandom
    #define __NR_getrandom 278
    __SYSCALL(__NR_getrandom, sys_getrandom)
  • include/linux/syscalls.h defines the prototype for sys_getrandom
    asmlinkage long sys_getrandom(char __user *buf, size_t count, unsigned int flags);
  • linux_raw_sys::general::__NR_getrandom would still exist as a constant
  • linux_raw_sys::syscalls::sys_getrandom would be a function whose implementation could be generated based on the "ABI" value in the syscall table:
    #[inline]
    pub unsafe fn sys_getrandom(buf: *mut c_char, count: c_size_t, flags: c_uint) -> c_long
  • There would not be a generic syscall() function. I agree with the above points that a function which can invoke any syscall is impossible to implement reliably.

However, I definitely recognize that such auto-generation is quite complex, compared to what linux-raw-sys currently does.

Zooming out, I think it would be fine for this crate to depend on rustix/linux-raw-sys on either all Linux targets or just the linux-none targets, provided that adoption is sufficiently widespread. A good litmus test for this would be libstd itself depending on rustix/linux-raw-sys on some/all Linux targets.

Until that point, having our own syscall wrapper seems like the least bad option. It will work with our without rustix, and we can always move to rustix/linux-raw-sys when there is consensus.

@briansmith
Copy link
Contributor Author

Zooming out, I think it would be fine for this crate to depend on rustix/linux-raw-sys on either all Linux targets or just the linux-none targets, provided that adoption is sufficiently widespread. A good litmus test for this would be libstd itself depending on rustix/linux-raw-sys on some/all Linux targets.

Until that point, having our own syscall wrapper seems like the least bad option. It will work with our without rustix, and we can always move to rustix/linux-raw-sys when there is consensus.

I agree with this.

@briansmith
Copy link
Contributor Author

One question is, how will we test this? My understanding is that libtest and the other things we use in our tests aren't available for this target (yet?). If that is true, then we may need to get a little creative.

I propose the following:

  • We use the exact same code paths in all cases for all(target_os = "linux", any(target_env = "none", all(feature = "linux_disable_fallback", not(feature = "std"))).
  • We rely on the cargo test --features=linux_disable_fallback without --features=std to approximate this target.

The implication here is we'd need to remove all the libc usage from the all(target_os = "linux", feature = "linux_disable_fallback", not(feature = "std")) case.

Ideally, we'd change Cargo.toml so that libc isn't a dependency for target_os = "linux" at all, or at least when the linux_disable_fallback feature is activated. But, I think that would require us to reverse in this sense:

- linux_disable_fallback = []
+ linux_enable_dev_urandom_fallback = ["libc"]

(The use_file implementation uses util_libc::{open_readonly, sys_fill_exact}; libc_util uses, of course, many things from libc including libc::open.)

@josephlr
Copy link
Member

josephlr commented May 30, 2024

I propose the following:

  • We use the exact same code paths in all cases for all(target_os = "linux", any(target_env = "none", all(feature = "linux_disable_fallback", not(feature = "std"))).

  • We rely on the cargo test --features=linux_disable_fallback without --features=std to approximate this target.

That testing strategy sounds good to me. I think that provided we don't explicitly check for target_env = "none" in the code and x86_64-unknown-linux-none builds, we can be reasonably sure the implementation is properly tested.

The implication here is we'd need to remove all the libc usage from the all(target_os = "linux", feature = "linux_disable_fallback", not(feature = "std")) case.

The main other uses of libc on Linux are:

  • The errno constants (needed for checking libc::EINTR)
  • The use of last_os_error() inside of sys_fill_exact

The ideal approach might be to have the main looping functionality of sys_fill_exact be moved to util.rs or lib.rs.

@newpavlov
Copy link
Member

My opinion is that we should add new linux_raw feature which would imply linux_disable_fallback. It would use the same backend as the none targets. It would make testing easier and would allows using raw getrandom syscall on the "classic" Linux targets.

@briansmith
Copy link
Contributor Author

Does anybody want to take a stab at the inline assembly for invoking the syscall?

@briansmith
Copy link
Contributor Author

The errno constants (needed for checking libc::EINTR)

It looks like it doesn't vary by architecture; it's always 4; https://github.com/torvalds/linux/blob/2df0193e62cf887f373995fb8a91068562784adc/include/uapi/asm-generic/errno-base.h#L8. Though it would be good to find a definitive promise of that.

The use of last_os_error() inside of sys_fill_exact

We could change the signature of sys_fill_exact like so:

- sys_fill: impl Fn(&mut [MaybeUninit<u8>]) -> libc::ssize_t,
+ sys_fill: impl Fn(&mut [MaybeUninit<u8>]) -> Result<usize, Error>

where any short read, including now Ok(0), indicates that we should keep going. Then the raw syscall code can do something like:

usize::try_from(res).map_err(|_| res.unsigned_abs())

Regardless, I think it is manageable.

@briansmith
Copy link
Contributor Author

My opinion is that we should add new linux_raw feature which would imply linux_disable_fallback. It would use the same backend as the none targets. It would make testing easier and would allows using raw getrandom syscall on the "classic" Linux targets.

Why would we want to keep the current code path for using libc::syscall though? I think we should use the same backend for all linux targets, since that will ensure that it gets the most testing and review.

@briansmith
Copy link
Contributor Author

Does anybody want to take a stab at the inline assembly for invoking the syscall?

nvm. Giving it a go now.

@briansmith
Copy link
Contributor Author

briansmith commented Jun 6, 2024

OK, there is now a sketch of this, including building for --target=x86_64-unknown-linux-none, and also ensuring the feature="linux_disable_fallback" and target_env="" share a code path, in PR #46.

@newpavlov
Copy link
Member

newpavlov commented Jun 6, 2024

Why would we want to keep the current code path for using libc::syscall though? I think we should use the same backend for all linux targets, since that will ensure that it gets the most testing and review.

Personally, I would be fine with unconditionally using raw syscalls for calling getrandom, but it may be controversial with some users. Though we already use libc::syscall, so it's probably less of an issue if we were using libc::getrandom.

@josephlr
Copy link
Member

josephlr commented Jun 7, 2024

Out of curiosity, I was trying to figure out if it would work to only use either libc::getrandom or directly invoke a syscall via asm!, avoiding any use of libc::syscall. This would make our logic simpler and consistent:

if has libc::getrandom:
  call libc::getrandom
else:
  raw syscall

x86-64-unknown-linux-none then just becomes a target where has libc::getrandom is always false. This approch would only require asm! support for targets that either lack a libc or have an old minimum libc version. Using libc::getrandom would also allow us to avoid needing unpoision calls as part of #463 (by basically saying "use a not-ancient libc if you want to use sanitizers" which is fine for me).

Looking at the supported Android/Linux targets:

  • All MUSL targets are fine
  • uClibc has libc::getrandom since v1.0.2 (2015) (before glibc)
    • A minimum version isn't specified for the targets, but I'd be fine assuming we are good here.
  • OHOS seems to have it in all publicly available releases (3.0 - 4.1)
  • Added in Android API Level 28 (Android 9 / Android P / Andorid Pie)
    • Rust supports NDK 25 (soon to be 26), so has a min API Level of 19 (soon to be 21)
    • So we need direct syscalls on aarch64, arm, x86, x86_64, all of which have stable inline asm.
    • It's not clear if Android Apps are permitted to make raw system calls (my guess is yes but I can check at work).
  • Added in glibc 2.25 (see https://lwn.net/Articles/711013/)
    • Rust supports a min glibc of 2.17 on aarch64, arm, powerpc, powerpc64, powerpc64le, s390x, x86, x86_64
    • Rust supports a min glibc of 2.23 on armv5te, sparc64, thumbv7neon, mips, mipsel, mips64, mips64el
    • We would need direct syscalls on all these architectures.

Given this, it's probably not possible to remove use of libc::syscall for quite some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants