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

[Warner Losh]

Sorry for top posting. I think that https://reviews.freebsd.org/D26423 is
the proper fix to catchup to NetBSD/4.4BSD Lite-2.

Warner

On Mon, Sep 14, 2020 at 3:58 AM Warner Losh  wrote:

>
>
> On Mon, Sep 14, 2020 at 1:45 AM 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
>>  with a
>> follow up commit of r38517
>>  , possibly
>> because ioctl was defined to take an unsigned long command before FreeBSD.
>
>
> These commits were for the Alpha port. I think for two reasons. (1) long
> was the size of a register there. (2) unsigned because BSD encoded types in
> the cmd number:
>
> Prior to 4.2BSD, ioctls cmd weren't encoded and had simple int defines. Or
> at least defines that fit into ints. For example:
> #define TIOCGETD(('t'<<8)|0)/* get line discipline */
> which reflected the PDP-11 past where int was 16 bits and signed vs
> unsigned as at best a hint to the reader.
>
> 4.2BSD introduced the encoding we have today with defines like:
>
> #define IOC_OUT 0x4000  /* copy out parameters */
> #define IOC_IN  0x8000  /* copy in parameters */
> #define IOC_INOUT   (IOC_IN|IOC_OUT)
>
> which filled all 32-bits of the ioctl cmd and introduced the concept of
> automatic copyin/copyout so the drivers wouldn't have to cope. The IOC_IN
> being especially troublesome because it's not an int, but unsigned. But
> that troublesome bit is still with us today:
>
> #define IOC_VOID0x2000  /* no parameters */
> #define IOC_OUT 0x4000  /* copy out parameters */
> #define IOC_IN  0x8000  /* copy in parameters */
> #define IOC_INOUT   (IOC_IN|IOC_OUT)
>
> I had expected to find a 0x8000ul or something similar there to get
> around the warnings, but that was never undertaken. However, I think this
> is the cause of our problems, see below for NetBSD's solution.
>
> That's a long way of saying, I'd expect it was to fix signedness warnings,
> but I'm not seeing where the fix I expected to find for that is applied. :(
> I do see we have a cast here:
> #define _IOC(inout,group,num,len)   ((unsigned long) \
> which quiets all the type mismatch warnings that would otherwise result in
> if/case statements. If you trace it back through a line wrap obrien did,
> you find it originate at
> https://svnweb.freebsd.org/base?view=revision=36735 which
> justified the change as
> This commit fixes various 64bit portability problems required for
> FreeBSD/alpha.  The most significant item is to change the command
> argument to ioctl functions from int to u_long.  This change brings us
> inline with various other BSD versions.  Driver writers may like to
> use (__FreeBSD_version == 33) to detect this change.
>
> Indeed, NetBSD also uses unsigned long in how I'd expect:
> #define IOC_IN  (unsigned long)0x8000
> and doesn't have the icky cast we have above in their _IOC definition.
> This was changed quite early in NetBSD's history (1994 by cgd) and is
> reflected in all the post 4.4BSD variants of BSD except 386BSD as far as I
> can tell from a quick peek. cgd later committed this into 4.4Lite2, the
> SCCS files of which have:
>
> D 8.3 95/01/09 18:16:30 cgd 3 2 00010/5/00033
> MRs:
> COMMENTS:
> 64-bit changes: ioctl cmd -> u_long, some protos.  some style, return vals.
>
> I also suspect that since register_t == long (or unsigned long)  this
> helped with the calling conventions of some 64-bit architecture, while
> hurting the 32-bit ones in NetBSD. But I can't find documentation of that.
>
> Internally, we have truncated it to 32-bit since 2005 (r140406
>> ), and
>> this
>> nrecent change made it a silent behavior.  POSIX, on the other hand,
>> defined
>> 
>> ioctl as taking an int as its second parameter, but neither Linux (glibc
>> in
>> particular, despite its documentation says
>> 
>> differently) nor macOS appear to define it that way, but Solaris seems
>>  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?
>>
>
> Given that this was changed 25 years ago, I think the ship 

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

[Warner Losh]

On Mon, Sep 14, 2020 at 1:45 AM 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
>  with a
> follow up commit of r38517
>  , possibly
> because ioctl was defined to take an unsigned long command before FreeBSD.


These commits were for the Alpha port. I think for two reasons. (1) long
was the size of a register there. (2) unsigned because BSD encoded types in
the cmd number:

Prior to 4.2BSD, ioctls cmd weren't encoded and had simple int defines. Or
at least defines that fit into ints. For example:
#define TIOCGETD(('t'<<8)|0)/* get line discipline */
which reflected the PDP-11 past where int was 16 bits and signed vs
unsigned as at best a hint to the reader.

4.2BSD introduced the encoding we have today with defines like:

#define IOC_OUT 0x4000  /* copy out parameters */
#define IOC_IN  0x8000  /* copy in parameters */
#define IOC_INOUT   (IOC_IN|IOC_OUT)

which filled all 32-bits of the ioctl cmd and introduced the concept of
automatic copyin/copyout so the drivers wouldn't have to cope. The IOC_IN
being especially troublesome because it's not an int, but unsigned. But
that troublesome bit is still with us today:

#define IOC_VOID0x2000  /* no parameters */
#define IOC_OUT 0x4000  /* copy out parameters */
#define IOC_IN  0x8000  /* copy in parameters */
#define IOC_INOUT   (IOC_IN|IOC_OUT)

I had expected to find a 0x8000ul or something similar there to get
around the warnings, but that was never undertaken. However, I think this
is the cause of our problems, see below for NetBSD's solution.

That's a long way of saying, I'd expect it was to fix signedness warnings,
but I'm not seeing where the fix I expected to find for that is applied. :(
I do see we have a cast here:
#define _IOC(inout,group,num,len)   ((unsigned long) \
which quiets all the type mismatch warnings that would otherwise result in
if/case statements. If you trace it back through a line wrap obrien did,
you find it originate at
https://svnweb.freebsd.org/base?view=revision=36735 which
justified the change as
This commit fixes various 64bit portability problems required for
FreeBSD/alpha.  The most significant item is to change the command
argument to ioctl functions from int to u_long.  This change brings us
inline with various other BSD versions.  Driver writers may like to
use (__FreeBSD_version == 33) to detect this change.

Indeed, NetBSD also uses unsigned long in how I'd expect:
#define IOC_IN  (unsigned long)0x8000
and doesn't have the icky cast we have above in their _IOC definition. This
was changed quite early in NetBSD's history (1994 by cgd) and is reflected
in all the post 4.4BSD variants of BSD except 386BSD as far as I can tell
from a quick peek. cgd later committed this into 4.4Lite2, the SCCS files
of which have:

D 8.3 95/01/09 18:16:30 cgd 3 2 00010/5/00033
MRs:
COMMENTS:
64-bit changes: ioctl cmd -> u_long, some protos.  some style, return vals.

I also suspect that since register_t == long (or unsigned long)  this
helped with the calling conventions of some 64-bit architecture, while
hurting the 32-bit ones in NetBSD. But I can't find documentation of that.

Internally, we have truncated it to 32-bit since 2005 (r140406
> ), and this
> nrecent change made it a silent behavior.  POSIX, on the other hand,
> defined
> 
> ioctl as taking an int as its second parameter, but neither Linux (glibc in
> particular, despite its documentation says
> 
> differently) nor macOS appear to define it that way, but Solaris seems
>  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?
>

Given that this was changed 25 years ago, I think the ship has sailed on
changing the prototype now since it reflects down the stack into the
drivers. And there's likely a lot of code that would grow a signed vs
unsigned warning, or worse.

As for the actual motivation, I suspect that it has to do with the register
size on the first 64-bit ports that NetBSD was doing and contributing back
to CSRG before 

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

[Hans Petter Selasky]

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
 with a
follow up commit of r38517
 , possibly
because ioctl was defined to take an unsigned long command before FreeBSD.

Internally, we have truncated it to 32-bit since 2005 (r140406
), and this
recent change made it a silent behavior.  POSIX, on the other hand, defined

ioctl as taking an int as its second parameter, but neither Linux (glibc in
particular, despite its documentation says

differently) nor macOS appear to define it that way, but Solaris seems
 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 
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)
@@ -652,18 +652,19 @@ 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 > 0x) {
 printf(
 "WARNING pid %d (%s): ioctl sign-extension ioctl
%lx\n",
 td->td_proc->p_pid, td->td_name, uap->com);
-   uap->com &= 0x;
 }
-   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
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

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

[Xin LI]

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
 with a
follow up commit of r38517
 , possibly
because ioctl was defined to take an unsigned long command before FreeBSD.

Internally, we have truncated it to 32-bit since 2005 (r140406
), and this
recent change made it a silent behavior.  POSIX, on the other hand, defined

ioctl as taking an int as its second parameter, but neither Linux (glibc in
particular, despite its documentation says

differently) nor macOS appear to define it that way, but Solaris seems
 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 
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)
> @@ -652,18 +652,19 @@ 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 > 0x) {
> printf(
> "WARNING pid %d (%s): ioctl sign-extension ioctl
> %lx\n",
> td->td_proc->p_pid, td->td_name, uap->com);
> -   uap->com &= 0x;
> }
> -   com = uap->com;
> +#endif
> +   com = (uint32_t)uap->com;
>
> /*
>  * Interpret high order word to find amount of data to be
>
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"