From owner-freebsd-current@freebsd.org Mon Sep 14 17:46:59 2020 Return-Path: Delivered-To: freebsd-current@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 42F603DDC02 for ; Mon, 14 Sep 2020 17:46:59 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qv1-xf2f.google.com (mail-qv1-xf2f.google.com [IPv6:2607:f8b0:4864:20::f2f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Bqv1n68xvz438x for ; Mon, 14 Sep 2020 17:46:53 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qv1-xf2f.google.com with SMTP id cv8so131727qvb.12 for ; Mon, 14 Sep 2020 10:46:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=cU+oNUd0h2uwqA78hS2sGNs2wc7wo6d2l6SUzEDCnTI=; b=U4/hKJv3aXbxZ7ViqcyD1g3WmF+bNzCB8C2EFUXx5dbVR81c4RkE6fzWKES5giVmWF gJ9xjCxzmCshp/tUwfVdpCPMkWIfEUNXazYhHJ52geDUFn6xG/DTQMM19BYlObOVa4V+ cDZ2xmRHUot+E9TTGBXlOOnfeLIjBhNW6TKCyOI/zB/S0aTqP6M3HAhodKx+4VBg1M+q mRa2yPTMZbKvIlCqf298PQk9Op3UBizk9fnd1emB50aFpd6y7io0zxAM+FtUeuv8qypP txNnF96WYl1bWoFLczEsYS0GIH3OIksKpLSSW4o6ZI8HIG74eB3Wxp/cmFBcCmv6aLsY 0SKg== X-Gm-Message-State: AOAM5315YLTr2RtBvtFnSVjNAx7S2TMPPaBdGwWY7BGPHLXLCSPhVwQK JXZnG/sT06zZd70kYvKYmBi7FsUMeeZ3W7rE9IIGbw== X-Google-Smtp-Source: ABdhPJxivjZ1im5XFaMJmgMGq5xlWsr0TEetF9K0vVaEJYKLvQJxn7bSwKOpmKYHF+2WbvzzX8C1ximrvqHP3ZoVLqM= X-Received: by 2002:a0c:e602:: with SMTP id z2mr14969374qvm.69.1600105610004; Mon, 14 Sep 2020 10:46:50 -0700 (PDT) MIME-Version: 1.0 References: <202004151320.03FDKqT7027080@repo.freebsd.org> In-Reply-To: From: Warner Losh Date: Mon, 14 Sep 2020 11:46:39 -0600 Message-ID: Subject: Re: ioctl argument type [Was Re: svn commit: r359968 - head/sys/kern] To: Xin LI Cc: FreeBSD Current , Hans Petter Selasky , Poul-Henning Kamp , jilles@freebsd.org, Doug Rabson , Xin Li X-Rspamd-Queue-Id: 4Bqv1n68xvz438x X-Spamd-Bar: - X-Spamd-Result: default: False [-1.81 / 15.00]; RCVD_TLS_ALL(0.00)[]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com:s=20150623]; NEURAL_HAM_MEDIUM(-0.86)[-0.857]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-0.73)[-0.727]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[freebsd-current@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; NEURAL_HAM_SHORT(-0.23)[-0.229]; RCPT_COUNT_SEVEN(0.00)[7]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::f2f:from]; R_SPF_NA(0.00)[no SPF record]; FREEMAIL_TO(0.00)[gmail.com]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; MIME_TRACE(0.00)[0:+,1:+,2:~]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; MAILMAN_DEST(0.00)[freebsd-current]; RCVD_COUNT_TWO(0.00)[2] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.33 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Sep 2020 17:47:00 -0000 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 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 >> ), 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 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 >> 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 > 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@freebsd.org mailing list >> https://lists.freebsd.org/mailman/listinfo/freebsd-current >> To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org >> " >> >