Fix the 3rd argument type of ioctl_write_int_bad

[Lencerf]

This patch follows 5dad660 'Correct the third argument to ioctl on appropriate platforms', where only ioctl_write_int was fixed.

What does this PR do

Checklist:

• I have read CONTRIBUTING.md
• I have written necessary tests and rustdoc comments
• A change log has been added if this PR modifies nix's API

[SteveLauC]

From our code, it seems that this type ioctl_param_type is c_ulong on all Linux targets, including musl

[Lencerf]

Fixed. Thank you!

[SteveLauC]

I am not familiar with ioctl(), would you like to provide any documentation on what the proper type should be?

The example ioctl provided in the Nix doc, TCSBRK, seems to require the argument to be of type int:

ioctl_write_int_bad!(tcsbrk, libc::TCSBRK);

Sending a break
TCSBRK Argument: int arg

The other example, request_code_none!(KVMIO, 0x03), KVM_CHECK_EXTENSION, requires its third argument to be an integer identifying the extension to check, which is represented by a u32 in the kvm_ioctl crate

So this argument seems to change depending on the actual ioctl it is used with.

[Lencerf]

• Based on the discussions in ioctl write_int doesn't support passing long integers #824 and 5dad660, on the Linux targets, the 3rd parameter passed to ioctl is an unsigned long, which is 64 bits on 64-bit machines.

• From Linux man page: https://man7.org/linux/man-pages/man2/ioctl.2.html

  The third argument is an untyped pointer to memory. It's traditionally char *argp (from the days before void * was valid C), and will be so named for this discussion.

  Both char *argp and unsigned long has 64 bits on 64-bit machines.

• Basically, for some ioctls, their parameters are complex, for example, KVM_SET_REGS's parameter is struct kvm_regs, therefore the kernel interprets the 3rd parameter passed in ioctl as the pointer to a struct kvm_regs, see kvm_main.c;

• On the other hand, some ioctls only need one integer as the parameter, therefore these ioctls just interpret the 3rd parameter as the required integer, for example, KVM_SET_TSS_ADDR, in kvm/x86.c, we can see arg is passed directly to kvm_vm_ioctl_set_tss_addr().

• For sure, there are also ioctls that need an integer as its parameter, but also require the user-space to pass a pointer to the integer as its 3rd parameter, for example, KVM_SET_IDENTITY_MAP_ADDR. In the kernel source code kvm/x86.c, we can see that different from KVM_SET_TSS_ADDR, kernel calls copy_from_user to fetch the integer value from the pointer stored in arg.

• In summary, on Linux targets, the 3rd parameter of ioctl should be 64 bits on 64-bit machines. Therefore the ioctl_write_int and ioctl_write_int_bad macros should generates a function that accepts an unsigned long. 5dad660 has fixed it for ioctl_write_int, but forgot ioctl_write_int_bad.

• Without this PR, for ioctls like KVM_SET_TSS_ADDR, the functions generated by ioctl_write_int_bad accepts an c_int and we cannot pass a value larger that 0xffffffff as reported by ioctl write_int doesn't support passing long integers #824.

[Lencerf]

Ping! Any feedback about this PR?

If this change is not acceptable, how about adding another macro that allows specifying the type of the 3rd parameter?

For example,

ioctl_write_num_bad!(kvm_set_tss_addr, request_code_none!(KVMIO, 0x47), libc::c_ulong);

This generates a function of the following signature

pub unsafe fn kvm_set_tss_addr(fd: nix::libc::c_int, data: libc::c_long) -> nix::Result<nix::libc::c_int>

and will pass data as the third parameter to libc::ioctl.

[asomers]

@Lencerf from your explanation, on Linux no ioctl should ever use an int for the third parameter. Yet TCSBRK does. Why?

[Lencerf]

@asomers Thanks for the feedback! My argument is, at the ABI level, on linux targets, the third argument should be something that has the same size as a pointer void*. But at the API level, the kernel implementation of these ioctl functions may interpret this void* in different ways. As you pointed out TCSBRK doc says it accepts an int, that means on 64bit machines, the kernel ignores the upper 32bit and only takes the lower 32bit.

To support my argument

the third argument should be something that has the same size as a pointer void*

let's look at glibc's wrapper of ioctl on linux

int
__ioctl (int fd, unsigned long int request, ...)
{
  va_list args;
  va_start (args, request);
  void *arg = va_arg (args, void *);   //  <== check this line
  va_end (args);

  int r;
  if (!__ioctl_arch (&r, fd, request, arg))
    {
      r = INTERNAL_SYSCALL_CALL (ioctl, fd, request, arg);
      if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (r)))
	{
	  __set_errno (-r);
	  return -1;
	}
    }
  return r;
}

it's quite clear that ioctl takes a void* from the va_list and assigns it to arg, and arg is passed to INTERNAL_SYSCALL_CALL. As a result, even if we call ioctl with an int as its 3rd parameter, thisint is still extended to 64 bits before it is passed to kernel.

Coming back to the original topic, since nix is aimed at providing type safe wrappers for rust, I agree that for ioctls like TCSBRK, we should provide a rust function that accepts a c_int, instead of c_ulong. But with current nix ioctl_* macros, there is no way to create a wrapper that accepts a c_ulong as the third parameter, therefore in my last comment, I proposed that we can add a new macro ioctl_write_num_bad that allows users to specify the type of the third parameter. What do you think?

[asomers]

I proposed that we can add a new macro ioctl_write_num_bad that allows users to specify the type of the third parameter. What do you think?

So for this macro, the user would supply a type, and the macro would cast from that type to *mut c_void? That might be the least bad option.