ioctl request arg size for Android aarch64 is wrong?

[lilydjwg]

I was trying to call libc::ioctl but I got type errors with Android aarch64 target. It works fine with x86_64. The second argument request has different size in these targets: c_ulong for x86_64 and c_int for Android aarch64.

I believe it should be c_ulong for Android aarch64 too, because it works (I'm using FFI now, and I get correct data out), and my request 0xC020660B (FS_IOC_FIEMAP) doesn't fit into a c_int.

[alexcrichton]

We are verifying the definition is correct on Android aarch64, so I think this may be an oddity of C!

[drrlvn]

I'm having this same issue but with x86_64-unknown-linux-musl where request is i32 as opposed to u64. What do you mean an oddity of C? Is this not a bug in libc?

[lilydjwg]

I've digged into this a little bit deeper. Technically unless the request is larger than 32 bits, this type difference in C results in the same kernel behaviour (the assembly I got was different but the data passed to kernel was the same).

Android has different header files than glibc's one. Maybe it's the same situation for musl.

[upsuper]

@alexcrichton If you look at definition of ioctl in Android, for example this one, you would see

__BEGIN_DECLS

/**
 * [ioctl(2)](http://man7.org/linux/man-pages/man2/ioctl.2.html) operates on device files.
 */
int ioctl(int __fd, int __request, ...);

/*
 * Work around unsigned -> signed conversion warnings: many common ioctl
 * constants are unsigned.
 *
 * Since this workaround introduces an overload to ioctl, it's possible that it
 * will break existing code that takes the address of ioctl. If such a breakage
 * occurs, you can work around it by either:
 * - specifying a concrete, correct type for ioctl (whether it be through a cast
 *   in `(int (*)(int, int, ...))ioctl`, creating a temporary variable with the
 *   type of the ioctl you prefer, ...), or
 * - defining BIONIC_IOCTL_NO_SIGNEDNESS_OVERLOAD, which will make the
 *   overloading go away.
 */
#if !defined(BIONIC_IOCTL_NO_SIGNEDNESS_OVERLOAD)
/* enable_if(1) just exists to break overloading ties. */
int ioctl(int __fd, unsigned __request, ...) __overloadable __enable_if(1, "") __RENAME(ioctl);
#endif

__END_DECLS

You can see that it explicitly calls out the situation that many common ioctl constants are unsigned, and can't fit into int.

The __overloadable here is a macro defined to be __attribute__((overloadable)), which is an attribute provided by clang and gcc to support overloading in C.

Given the situation that many constants are actually unsigned, and Android header specifically tries to handle that, I think this issue probably shouldn't be closed. I'm not sure how can we solve this, as Rust doesn't support overloading, but this issue is valid and should be discussed.

[gnzlbg]

@upsuper I'm not sure if I understanding the issue properly, but I have one question. Those two "overloads", do they have the same mangled name? If not, then I think we should just be providing two different APIs, with two different type signatures, each pointing at a different overload.

[upsuper]

IIUC the two overloads share the same symbol. It is explicitly done via __RENAME(ioctl) (which expands to __asm__("ioctl")) meaning its name is also ioctl.

[gnzlbg]

I see. The only reasonable way I can think of, of exposing this safely from rust, is to provide two libc APIs, both pointing to the same symbol, but with different type signatures.

[upsuper]

That sounds painful for downstream users, as they may need to use different functions for different platforms. Or are you proposing adding new APIs with consistent signature across platforms?

[CmdrMoozy]

Just adding a +1 since it seems this hasn't got any attention lately. It also affects aarch64-unknown-linux-musl, not just the Android target.

To add some more context:

• The kernel defines ioctl() as taking an unsigned int: https://elixir.bootlin.com/linux/v5.13-rc3/source/fs/ioctl.c#L1055
• musl (and the libc crate) use an int: https://github.com/esmil/musl/blob/master/src/misc/ioctl.c
• The man page ioctl(2) says it takes an unsigned long, though!

This is a weird mismatch, but it turns out to "just work" because the conversion is well defined by the standard and does the (I suppose) intuitive thing (https://stackoverflow.com/questions/46519568/conversion-of-function-parameter-from-signed-int-to-unsigned-int).

Note that maybe surprisingly, sizeof(int) (and likewise sizeof(unsigned int)) is 32 bits on aarch64.

So I think the libc crate is actually doing the right thing. Note that in C when you pass a long int into a function which takes an int, it just implicitly truncates the high bits.

So, I think that no ioctl parameters should actually use > 32 bits, and if they did, they would just get implicitly truncated anyway?

If everything I've said is right, then passing in <value> as libc::c_int should work correctly on all platforms.

Ah, but on x86_64-unknown-linux-gnu the libc crate says it takes an unsigned long. This might match what glibc or something does, but it doesn't match how the kernel actually defines the syscall, so that seems weird. So, maybe it's hopeless and we need to conditionally compile to select the type.