Support ioctl!(none, ...) with an argument

[GabrielMajeri]

The KVM ioctl KVM_GET_API_VERSION is of type none / _IO. I've tried defining it as:

const KVMIO: u8 = 0xAE;
ioctl!(none get_api_version with KVMIO, 0x00);
let version = unsafe { get_api_version(self.fd)? };

However, this fails with EINVAL, since the code in the kernel still expects one i32 parameter, which should be 0.

nix should have some support for an additional parameter with none ioctls. Right now I am manually using libc::ioctl for this, but the macro should support something like

let version = unsafe { get_api_version(self.fd, 0)? };

Or perhaps add a new ioctl type, which is the same as none, but called none_int, which takes the second parameter.

[Susurrus]

According to the kernel docs, there should be no argument to this ioctl. This agrees with the ioctl number definition _IO(KVMIO, 0x00) in /usr/include/linux/kvm.h.

I think you have a misunderstanding of the ioctl logic here, because the 0 in this context is not an argument. As in it's not data being passed to the ioctl call, it's actually part of the name. The modern way to define IOCTLs is to use a namespace (KVMIO in this case) and then the specific ioctl within that namespace. So in this case you want to use the 0th ioctl within the KVMIO namespace, which is an ioctl that returns an int and takes no argument.

So as far as I can tell your declaration is correct. Maybe try a ioctl!(bad none get_api_version with XXX) where XXX is extracted by a small C program that prints the value of KVM_GET_API_VERSION. Let me know if that works, because then it's our ioctl id generating code that's broken somewhere.

[GabrielMajeri]

According to the kernel docs, there should be no argument to this ioctl. This agrees with the ioctl number definition _IO(KVMIO, 0x00) in /usr/include/linux/kvm.h.

I know, unfortunately the actual code does check for the argument.

The same goes for KVM_CHECK_EXTENSION, defined as _IO(KVMIO, 0x03), which nevertheless requires a first parameter.

I tried bad none too, however using bad none did not allow me to pass the one argument I needed.

Changing the kernel ABI to properly label ioctls is beyond my powers, so I just wondered if nix was able to workaround this problem. Right now I am building the ioctl code with io! and manually calling the ioctl function.

[posborne]

I'm a little confused here. The actual system call to the kernel will always have a value for the "arg" (e.g. https://github.com/idunham/musl/blob/7e10f209fbc26a5179a4c0817c986e7c7bd327c9/src/misc/ioctl.c). I would expect the libc implementation that we call to result in a 0x00 for this argument if the C ioctl function is called without any additional arguments.

You seem to indicate that this is not the case in your tests? Please confirm in the debugger or strace the actual syscall parameters going to the kernel.

[GabrielMajeri]

@posborne The problem isn't with the argument being set to 0 or not, the problem is that KVM's ioctls require the mandatory argument to be something specific.

E.g. a normal none ioctl would be called like this in C:

#define SOME_IOCTL (_IO(0, 0))
ioctl(fd, SOME_IOCTL); // not passing extra argument is OK, set to 0 automatically

KVM's ioctl is special:

#define KVM_CREATE_VM _IO(KVMIO, 0x03);
ioctl(fd, KVM_CREATE_VM, some_flags); // extra argument is required, and it's not 0, it's some relevant value.

[posborne]

@GabrielMajeri I was thrown off by the link to the kernel for handling KVM_GET_API_VERSION which you also reference in your initial post. For this one, the kernel is checking the arg but just checking that it is 0 which is what it should be by default. I see no reason the generated code should not work in that case (but maybe I'm missing something).

I think I understand the basic problem here but not sure if it is worthwhile to change nix to support kernel subsystems/drivers which do not follow the conventions properly (this is the first I have seen doing this specifically). Not being able to specify an argument for the bad seems like a problem here. I think adding a bad readwrite_int would solve your problem, correct? The case where you don't care about the result value could be handled fine with bad write_int as-is today. You could use bad write_int now and just ignore the result if you choose as well (not ideal).

[GabrielMajeri]

For this one, the kernel is checking the arg but just checking that it is 0 which is what it should be by default. I see no reason the generated code should not work in that case (but maybe I'm missing something).

Not sure if this should be reported as a bug, but it does not work if I do not pass a 0 explicitly. Indeed, the C code properly sets the default argument to 0, but Rust does not.

See this Rust playground's disassembly: Rust does not clear the edx register if I do not pass an argument, meaning it will contain a random / uninitialized value.

I think I understand the basic problem here but not sure if it is worthwhile to change nix to support kernel subsystems/drivers which do not follow the conventions properly

It's alright, I ended up wrapping the ioctl! macro anyways, and it is not nix's fault for bad API design.

The case where you don't care about the result value could be handled fine with bad write_int as-is today.

I am using that as a workaround, and have no problems.

---

What I initially hoped for is for nix to add a new "convenience" none_with_arg ioctl in the macro, which would end up being just like none (i.e. the I/O code would be defined with io!(...) instead of iow!(...)) but also take a i32 parameter (like write_int) so I didn't have to write wrappers around everything.

I don't think this is a good idea anymore, kvm seems to be a special case and this change would complicate nix's macro needlessly.

[jethrogb]

Same issue with NBD ioctl API:

https://github.com/torvalds/linux/blob/9aa0d2dde6ebd14e9d16e28081a24721d5b41cc8/drivers/block/nbd.c#L1151
https://github.com/torvalds/linux/blob/9aa0d2dde6ebd14e9d16e28081a24721d5b41cc8/include/uapi/linux/nbd.h#L20

[jethrogb]

@posborne

I would expect the libc implementation that we call to result in a 0x00 for this argument if the C ioctl function is called without any additional arguments.

Why? You even linked to code that explicitly doesn't do that.

[GabrielMajeri]

@jethrogb @posborne It looks like this is not an isolated issue, I am reopening this issue until we determine how nix should handle this.

[jethrogb]

Looking at the original commit that added _IO to Linux, it's not clear to me that _IO with an integer argument is wrong. According to that comment, the encoded size is that of the user buffer area, which obviously doesn't exist when passing an integer argument.

[Susurrus]

How is this not covered by the bad write_int ioctl variant that nix exports? This definitely seems to only be applicable to "bad" ioctls, so those variants should be used here.

[jethrogb]

It's not covered because the io! macro is not documented. Also as I mentioned in my previous comment, I don't think it's “bad”.

[Susurrus]

Okay, that didn't really help me understand anything. I'm unclear if people in this conversation know what the correct solution is here. It sounds like we understand the problem plenty well enough, so does anyone have a proposed solution? My main question is what's wrong with the solution of specifying 0 as the 3rd argument to libc::ioctl for both none and bad none ioctl types?

[GabrielMajeri]

Passing a 0 would fix my issue and @jethrogb' (see below), but we don't know if it will work for all ioctls. In the future someone might need to pass a 1, what will we do then?

It would be great if Rust had function overloading (so we could have a function with an extra parameter for ioctl!(none)), or default arguments (so we don't break existing code), but since it doesn't, I don't see an elegant solution in this case.

Adding a none_int variant to the macro is possible, but extra work on nix's part and confusing for people who don't need it. The alternative of using the libc::ioctl function in crates which use nix is also a lot of work.

[jethrogb]

@GabrielMajeri passing 0 would not fix my issue. I need to pass an argument.

@Susurrus I don't think that helps anyone. ioctls that don't require an integer argument don't care about the value passed. ioctls that do are not going to be satisfied with passing only 0. ioctls that “don't” require an integer argument but explicitly check it's 0 are really ioctls that do take an integer argument.

The issue is there appear to be two ways to specify ioctls that take an integer argument (as opposed to a pointer argument).

1. _IO(...)
2. _IOW(..., int)

Both ways appear to be widely used in the Linux kernel. I personally think that the _IOW(..., int) encoding for an integer argument is wrong and that it actually specifies a pointer argument to an integer in user space.

I think having a none_int variant of the macro would be a fine solution.

[Susurrus]

I still don't get why bad write_int isn't appropriate for all the use cases that have been described in this issue. These are seem "bad" to me. So for all these use cases nix's exported APIs are suitable, though not as convenient, but that's always been the case for ioctls that don't follow the modern conventions.

[jethrogb]

I'm trying to make the argument that they're not bad and it is what's currently supported through write_int that is bad.

Also, the io! macro is not documented so it's not even clear how to use the current API.

[Susurrus]

Ignoring whether these are bad or not, you're saying that you can't use write_int? Why not?

[jethrogb]

write_int uses _IOW with a type of int (which is incorrect, as that encodes the size of an int*). These ioctls need _IO.

[Susurrus]

No, I meant bad write_int, which is what I thought you were referring to above. Of course write_int won't be suitable here.

[jethrogb]

Because the bad set of macro invocations require you to specify the raw ioctl number, whereas these ioctls are specified using the standard _IOx macros.

[Susurrus]

Which you can get by writing a little C program and printing it out, or you can use the io! macro just like the equivalent _IO() C macros.

[Susurrus]

This does have me thinking again about how we use a single ioctl! macro, which makes the docs really bad. I'm looking to improve our docs right now and I think it makes sense to refactor all this into each ioctl type having it's own macro.

[Susurrus]

I'm going to go ahead and close this because I don't believe that ioctl!(none, ...) should take an argument, you're looking for ioctl!(bad write_int, ...) for your use case here. I'll also be CCing everyone here on a PR I'm filing today that will clean up the API a little and provide better docs to make this more clear.