-
Notifications
You must be signed in to change notification settings - Fork 204
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 linux_raw
opt-in backend
#572
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think this seems like a lot of added complexity for a backend that is not enabled by default. I think that we should try to find some other way to solve the issues raised in #433, or just not have a rustix/raw-syscall backend right now.
Getting raw syscalls correct can be very complex if we want to support a range of targets, and I don't think we are well situated to maintain such things.
Yes, it's a reasonable option. We can delay this backend until a later release. But I believe we should eventually have a "raw Linux syscall" backend as it was requested several times (#401, #424).
I do not agree with the "very complex" assessment. As mentioned in the comments, I've mostly copied the battle-tested code from Ideally, I would prefer to have the syscall stuff in a separate lean crate, but, unfortunately, |
As suggested [here](#572 (review)) it may be worth to remove `linux_rustix` from the v0.3.0 release. We probably will either return it in v0.3.1, or will introduce a different `linux_raw` opt-in backend (see #572).
linux_rustix
opt-in backend with linux_raw
linux_raw
opt-in backend
Note that I plan to use this backend to support the |
@josephlr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in reviewing this. I think this is a fine idea, but I'm not sure if we want to bother supporting the more "difficult" targets where syscalls are more complicated than a single instruction.
|
||
pub use crate::util::{inner_u32, inner_u64}; | ||
|
||
#[cfg(not(any(target_os = "android", target_os = "linux")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to figure out what the constraints are around directly invoking syscalls on Android. I can try to check with some people at work, but it's unclear if bionic libc (the libc shipped with Android) does any special handling for syscalls. If the only demand for this is on linux
targets, it might just be best to initially exclude Android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would be nice to verify that raw syscalls work on Android without any surprises.
Here is a relevant rustix
discussion: bytecodealliance/rustix#1095 No one has mentioned any fundamental technical obstacles to using raw syscalls and it looks like some people use them in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since users have to explicitly enable the backend, I think we can leave it as-is and potentially disable it later if it will be found problematic on Android for some reason.
in("a2") flags, | ||
options(nostack, preserves_flags) | ||
); | ||
} else if #[cfg(target_arch = "x86")] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give that there is no i686-unknown-linux-none
target and invoking raw syscalls on x86 is a notorious foot-gun, could we just not support 32-bit x86? It seems easier and then we don't have to have warnings saying "you probably shouldn't use this backend"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC the only problem with int 0x80
is its performance, otherwise it works fine. So I think it's fine to have support for it for completeness sake despite its suboptimality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked the warning a bit. Now it notes that linux_raw
may be slower than libc::getrandom
-based backends in general, not just on x86.
core::arch::asm!( | ||
"mov {tmp}, r7", | ||
"mov r7, {nr}", | ||
"svc 0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the syscall(2)
documentation it seems like there (might) be a difference between EABI / OABI targets for 32-bit arm. Also, that documentation suggests that 32-bit arm should use the swi
instruction.
Personally, I would be fine omitting 32-bit arm support (similar to x86
support). It's more complicated, and there isn't a 32-bit -unknown-linux-none
target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like swi
is the same as svc
(https://developer.arm.com/documentation/ddi0406/cb/Application-Level-Architecture/Instruction-Details/Alphabetical-list-of-instructions/SVC--previously-SWI-?lang=en) I'm still confused about the EABI vs OABI stuff though.
I think this code would be fine provided we simply confirm (via cfg
s) that we are targeting eabi
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is based on rustix
and it does not look like they differentiate between EABI / OABI: https://github.com/bytecodealliance/rustix/blob/main/src/backend/linux_raw/arch/mod.rs#L27-L28
After a brief search I couldn't find any relevant information about OABI support in Rust. Are you sure that Rust supports it in the first place? For example, it looks like GCC has dropped OABI support completely.
This PR replaces the
linux_rustix
opt-in backend introduced in #520 withlinux_raw
opt-in backend based raw syscalls implemented usingasm!
. It noticeably reduces dependency footprint of the crate (see concerns raised in #433). Unfortunately, it means that we have to implement the raw syscalls ourselves.This PR implements
linux_raw
support only for target arches with stableasm!
. Nightly-only support for other targets can be added in later PRs.