From owner-svn-src-head@freebsd.org Thu Jun 20 18:44:30 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 0623915C47BA; Thu, 20 Jun 2019 18:44:30 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 7690E9119B; Thu, 20 Jun 2019 18:44:29 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 34CFE14A4A8; Fri, 21 Jun 2019 04:44:20 +1000 (AEST) Date: Fri, 21 Jun 2019 04:44:18 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Alan Somers cc: Ian Lepore , src-committers , svn-src-all , svn-src-head Subject: Re: svn commit: r349233 - head/sys/sys In-Reply-To: Message-ID: <20190621033049.D5823@besplex.bde.org> References: <201906201435.x5KEZTqH021513@repo.freebsd.org> <54f3bc97cbb485cdcc44b81c82c149ac9e46d42f.camel@freebsd.org> <20190621013236.N5105@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=D+Q3ErZj c=1 sm=1 tr=0 cx=a_idp_d a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=qHZTYdl3w4KMAS8C2bwA:9 a=fzeUE3LNdtxtRXMc:21 a=jRwNM3MqSicG2Lbx:21 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: 7690E9119B X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.98 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.996,0]; NEURAL_HAM_SHORT(-0.99)[-0.987,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Jun 2019 18:44:30 -0000 On Thu, 20 Jun 2019, Alan Somers wrote: > On Thu, Jun 20, 2019 at 10:43 AM Bruce Evans wrote: >> Summary: and the headers that it includes should declare >> minimal types to compile (so __int64_t is enough). Most uses of this >> header require including domain-specific headers which declare the >> relevant data structures. > > Bruce, would you be satisfied by switching from to > and from int64_t to __int64_t? Not quite. The kernel block number type is daddr_t, and [__]int64_t is a hard coding of that. Hard-coding of typedefs is good for reducing namespace pollution, but it is not done for the nearby use of off_t. Unfortunately, daddr_t is only declared in . The type daddr_t seems to be correct, but ffs conflates it with ufs2_daddr_t. It is only for blocks of size DEV_BSIZE, while fs blocks are usually larger. The conflation is just a style bug because 64 bits should be large enough for anyone and ffs does the necessary conversions between DEV_BSIZE-blocks and fs-blocks. ext2fs seems to be more broken in this area. It doesn't have ext2fs_daddr_t, but hard-codes this as int32_t or int64_t. int32_t became very broken when ext2fs started attempting to support unsigned block numbers. Now, ext2_bmap() corrupts "daddr_t a_bn" to "int32_t bn" when it calls ext4_bmapext(). This has a chance of working via unsign-extension bugs provided a_bn fits in uint64_t. It is sure to fit for a valid a_bn, but perhaps your new ioctl allows probing for panics using invalid a_bn. ext2_bmap() also calls ext2_bmaparray(). That used to have essentially the same style bugs as ffs (with the ext2fs type corresponding to ufs2_daddr_t spelled int32_t), but it now correctly uses daddr_t. Internally, it misuses daddr_t for ext2fs block numbers, and there is an ext2fs block number type after all (e2fs_lbn_t = int64_t) which it uses only for metalbn. It looks like the older ext2fs code was fixed, especially if older ext2fs is limited to int32_t block numbers, but the ext4 case is quite broken since unsign extension bugs don't help it. a_bn starts as int64_t, then is truncated to the function arg "int32_t bn", then the function assigns bn to "int64_t lbn" and doesn't use bn again. Using a generic int64_t type in all interfaces would avoid some of these bugs, so I don't mind using it for the API. Just add a note that it must be large enough to represent all useful values of daddr_t. Bruce