Skip to content

Handle 64 bit syscall arguments properly on 32-bit systems #17

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JonSeverinsson
Copy link

This adds support for the keyword 'u64' to the syscall! macro, which causes the next argument to be treated as a 64-bit value rather than an usize. On 64-bit platforms, using the 'u64' keyword does nothing, while on 32-bit platforms it will correctly split it over two 32-bit registers.

This is fully backwards compatible, as current behaviour is retained in absence of the u64 keyword.

NB: This mostly helps for syscalls that have the same API but different ABI on different platforms. If the API differs as well, you still need conditionals in the call site.

For example, readahed becomes trivial to implement:
syscall!(READAHEAD, fd, u64, offset, count)
compared to the current:
syscall!(READAHEAD, fd, offset, count) on aarch64, mips64, powerpc64, sparc64, x86_64
syscall!(READAHEAD, fd, 0, (offset >> 32), offset, count) on mips, powerpc
syscall!(READAHEAD, fd, 0, offset, (offset >> 32), count) on armeabi, mipsel
syscall!(READAHEAD, fd, offset, (offset >> 32), count) on x86
but posix_fadvise only becomes slighly easier:
syscall!(FADVISE64, fd, u64, offset, u64, len, advise) on aarch64, mips64, powerpc64, x86_64, mips, mipsel
syscall!(FADVISE64_64, fd, u64, offset, u64, len, advise) on sparc64, x86
syscall!(FADVISE64_64, fd, advise, u64, offset, u64, len) on powerpc
syscall!(ARM_FADVISE64_64, fd, advise, u64, offset, u64, len) on armeabi
compared to the current:
syscall!(FADVISE64, fd, offset, len, advise) on aarch64, mips64, powerpc64, x86_64
syscall!(FADVISE64, fd, 0, (offset >> 32), offset, (len >> 32), len, advise) on mips
syscall!(FADVISE64, fd, 0, offset, (offset >> 32), len, (len >> 32), advise) on mipsel
syscall!(FADVISE64_64, fd, offset, len, advise) on sparc64
syscall!(FADVISE64_64, fd, offset, (offset >> 32), len, (len >> 32), advise) on x86
syscall!(FADVISE64_64, fd, advise, (offset >> 32), offset, (len >> 32), len) on powerpc
syscall!(ARM_FADVISE64_64, fd, advise, offset, (offset >> 32), len, (len >> 32)) on armeabi

This adds support for the keyword 'u64' to the syscall! macro, which
causes the next argument to be treated as a 64-bit value rather than
an usize. On 64-bit platforms, using the 'u64' keyword does nothing,
while on 32-bit platforms it will correctly split it over two 32-bit
registers.
@japaric
Copy link
Owner

japaric commented Jun 10, 2017

The first two commits LGTM.

@tbu- what do you think about the change about handling 64 bit arguments on 32-bit systems? Would it simplify some of the work you have done on steed?

@tbu-
Copy link

tbu- commented Jun 12, 2017

From man 2 syscall, I get

The affected system calls are fadvise64_64(2), ftruncate64(2), posix_fadvise(2), pread64(2), pwrite64(2), readahead(2), sync_file_range(2), and truncate64(2).

We currently use ftruncate64, pread64 and pwrite64, taking up a total of ~100 LOC, not tested for 64 bit integers. This PR adds ~2500 LOC, also not tested for 64 bit integers, with some unused code paths.

Personally, I think this PR would be better if it generated the macro code (using some Python script or so), because otherwise it's not unlikely that there's a copy-and-paste mistake somewhere. It would reduce the complexity of writing the above-mentioned functions in steed, however, I think that the current situation of hardcoding this conversion is not that bad. There's a total of 7 syscalls in the kernel that need this which you could extrapolate to ~200 LOC if written manually. One upside of the approach of this PR is that other projects implementing the same syscalls don't have to write this code as well – although there are other, similar pitfalls.

All in all, I don't think that adding ~2500 LOC (a lot copy-pasted) for ~100 LOC (also copy-paste-style) is not worth it.

@JonSeverinsson
Copy link
Author

From man 2 syscall, I get

The affected system calls are fadvise64_64(2), ftruncate64(2), posix_fadvise(2), pread64(2), pwrite64(2), readahead(2), sync_file_range(2), and truncate64(2).

@tbu: That list only covers what is affected by alignment issues (eg the difference between 32be.rs and 32be-align.rs, or between 32le.rs and 32le-align.rs). The difference between 64 bit, 32 bit little endian, and 32 bit big endian affects a lot more syscalls.

That said, it would be trivial to generate all the other 32 bit source files form either 32be-align.rs or 32le-align.rs using sed or similar (in fact, I did create them using regular expression find-and-replace in my text editor), and I could integrate that into the build system if you would prefer. Procedurally generating all variants (32 and 64 bit) would be a lot trickier, but I could make an attempt if you insisted.

This PR adds ~2500 LOC, also not tested for 64 bit integers, with some unused code paths.

I have tested several syscalls (including truncate/truncate64, which uses an unaligned 64 bit integer) on aarch64, arm, i686, mips, mipsel, mips64, mips64el, powerpc, powerpc64, powerpc64el and x86_64 (all but i686 and x86_64 using qemu-user), which together tests most, if not all, codepaths.

@japaric
Copy link
Owner

japaric commented Sep 14, 2017

@JonSeverinsson sorry, this PR fell off my radar.

The first two commits LGTM.

(Err, I actually meant the two last commits: the commits that didn't change the macros.)

I think this is certainly an improvement when dealing with syscalls that take 64 bit arguments on 32 bit architectures, but I would fell more comfortable if the macros were generated by a script (Python or build script) then the chance of errors in the macros would be less, or at least the errors should be easier to spot (in theory).

I'm also wondering if it would make sense to type check the 64-bit argument when the u64 variants are used. Something like:

#[macro_export]
macro_rules! syscall {
    ($nr:ident, u64, $a1:expr) => {
        let a1: u64 = $a1; // <- type check

        ::sc::syscall2(
            ::sc::nr::$nr,
            (a1 >> 32) as usize, a1 as usize)
    };
    // ..
}

Though I'm not sure if this sanity check would end up increasing the number of #[cfg] attributes required.

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 this pull request may close these issues.

3 participants