Date: Thu, 20 Jun 2019 12:43:26 -0700 From: Warner Losh <imp@bsdimp.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Alan Somers <asomers@freebsd.org>, Ian Lepore <ian@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r349233 - head/sys/sys Message-ID: <CANCZdfqKtEoH7HMT2AESaQ6LtaidodOsdZ6srwhWJ7djxbqNsA@mail.gmail.com> In-Reply-To: <20190621033049.D5823@besplex.bde.org> References: <201906201435.x5KEZTqH021513@repo.freebsd.org> <54f3bc97cbb485cdcc44b81c82c149ac9e46d42f.camel@freebsd.org> <CAOtMX2jt1M6mQNC_JekLMRfxgo2OxeR3VkfuQZHDgx%2B_vzFNWQ@mail.gmail.com> <20190621013236.N5105@besplex.bde.org> <CAOtMX2h3=f_hozsrL7NqGKYafsDMzny3Z-bQh5JJ6n71Y0-9Mg@mail.gmail.com> <20190621033049.D5823@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jun 20, 2019, 11:44 AM Bruce Evans <brde@optusnet.com.au> wrote: > On Thu, 20 Jun 2019, Alan Somers wrote: > > > On Thu, Jun 20, 2019 at 10:43 AM Bruce Evans <brde@optusnet.com.au> > wrote: > >> Summary: <sys/ioctl.h> 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 <sys/types.h> to > > <sys/_types.h> 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 <sys/types.h>. > > 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. > Maybe we should add a __daddr_t define to sys/_types.h? And the usual reshuffling. That would also fix the namespace pollution. Warner >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqKtEoH7HMT2AESaQ6LtaidodOsdZ6srwhWJ7djxbqNsA>