ioctl argument type [Was Re: svn commit: r359968 - head/sys/kern]

Hi,

I have seen Chromium trigger the warning (I run -CURRENT with INVARIANTS)
and looked into the code history a little bit.

It seems that the command was changed to u_long in r36846
<https://svnweb.freebsd.org/base?view=revision&revision=36846> with a
follow up commit of r38517
<https://svnweb.freebsd.org/base?view=revision&revision=38517> , possibly
because ioctl was defined to take an unsigned long command before FreeBSD.

Internally, we have truncated it to 32-bit since 2005 (r140406
<https://svnweb.freebsd.org/base?view=revision&revision=140406>), and this
recent change made it a silent behavior.  POSIX, on the other hand, defined
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/ioctl.html>
ioctl as taking an int as its second parameter, but neither Linux (glibc in
particular, despite its documentation says
<https://www.gnu.org/software/libc/manual/html_node/IOCTLs.html>
differently) nor macOS appear to define it that way, but Solaris seems
<https://docs.oracle.com/cd/E23824_01/html/821-1463/ioctl-2.html> to be
defining it as an int.

What was the motivation to keep the prototype definition as

     int
     ioctl(int fd, unsigned long request, ...);

instead of:

     int
     ioctl(int fd, int request, ...);

Other than to make existing code happy?  Alternatively, will it be a good
idea to give compiler some hints (e.g. by using __attribute__(enable_if))
to emit errors, if we insist keeping the existing signature?


On Wed, Apr 15, 2020 at 6:21 AM Hans Petter Selasky <hselasky_at_freebsd.org>
wrote:

> Author: hselasky
> Date: Wed Apr 15 13:20:51 2020
> New Revision: 359968
> URL: https://svnweb.freebsd.org/changeset/base/359968
>
> Log:
>   Cast all ioctl command arguments through uint32_t internally.
>
>   Hide debug print showing use of sign extended ioctl command argument
>   under INVARIANTS. The print is available to all and can easily fill
>   up the logs.
>
>   No functional change intended.
>
>   MFC after:    1 week
>   Sponsored by: Mellanox Technologies
>
> Modified:
>   head/sys/kern/sys_generic.c
>
> Modified: head/sys/kern/sys_generic.c
>
> ==============================================================================
> --- head/sys/kern/sys_generic.c Wed Apr 15 13:13:46 2020        (r359967)
> +++ head/sys/kern/sys_generic.c Wed Apr 15 13:20:51 2020        (r359968)
> _at__at_ -652,18 +652,19 _at__at_ int
>  sys_ioctl(struct thread *td, struct ioctl_args *uap)
>  {
>         u_char smalldata[SYS_IOCTL_SMALL_SIZE]
> __aligned(SYS_IOCTL_SMALL_ALIGN);
> -       u_long com;
> +       uint32_t com;
>         int arg, error;
>         u_int size;
>         caddr_t data;
>
> +#ifdef INVARIANTS
>         if (uap->com > 0xffffffff) {
>                 printf(
>                     "WARNING pid %d (%s): ioctl sign-extension ioctl
> %lx\n",
>                     td->td_proc->p_pid, td->td_name, uap->com);
> -               uap->com &= 0xffffffff;
>         }
> -       com = uap->com;
> +#endif
> +       com = (uint32_t)uap->com;
>
>         /*
>          * Interpret high order word to find amount of data to be
>