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

On 2020-09-14 09:44, Xin LI wrote:
> 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
>>

Hi,

Using unsigned long is not cross platform compatible, especially when 
you have 32-bit compat shim layers.

On 64-bit platforms long is usually 64-bit and on 32-bit platforms long 
is usually 32-bit.

You've brought up a good question with a good history line.

Maybe we should just "#if 0" the INVARIANTS check and remove that code?

--HPS