On Mon, Sep 14, 2020 at 1:45 AM Xin LI <delphij_at_gmail.com> 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. 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 0x40000000 /* copy out parameters */ #define IOC_IN 0x80000000 /* 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_VOID 0x20000000 /* no parameters */ #define IOC_OUT 0x40000000 /* copy out parameters */ #define IOC_IN 0x80000000 /* copy in parameters */ #define IOC_INOUT (IOC_IN|IOC_OUT) I had expected to find a 0x80000000ul 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&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 == 300003) to detect this change. Indeed, NetBSD also uses unsigned long in how I'd expect: #define IOC_IN (unsigned long)0x80000000 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/00005/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 > <https://svnweb.freebsd.org/base?view=revision&revision=140406>), and this > nrecent 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? > 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 4.4Lite2 was released, but I can't find anything more specific than the CSRG commit (or the NetBSD one that preceded it which had similar verbiage). I honestly think, though, that our best bet is to use the NetBSD definitions for IOC_* we see above. This would mean that the values wouldn't be sign extended (unless someone hardcodes a bare 0x80000000 somewhere) and I'll bet the warnings from Chrome would just disappear. They properly cast the raw int to an unsigned long (though I suppose one could use UL at the end, though bde would have objected in the code review that followed). It would also reduce the diffs with NetBSD in an area that don't need to be different from the 4.4Lite2 code. I'd be tempted to leave the printf live and not turn it off like hps did. I'm guessing they used the IOC_ stuff and hand-rolled the shifts without the cast we have. But I'm not up for diving into that pool tonight... It's too deep to chase down the define that leads to this. As to why we clip it at 32-bits, I'm less sure. It is handy to have it clipped to 32-bits for 32-bit on 64-bit code situations, but I'll leave it to Brooks to say if that's legit or not. I think we rely on it really only having 32-bits of significance to reduce the number of changes we need in drivers to support 32-bit IOCTL in COMPAT32 mode. Warner 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 > > > _______________________________________________ > freebsd-current_at_freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-current > To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org" >Received on Mon Sep 14 2020 - 07:59:12 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:25 UTC