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 <imp_at_bsdimp.com> wrote: > > > 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 - 15:46:59 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:25 UTC