On Thu, Jun 20, 2019 at 1:43 PM Warner Losh <i...@bsdimp.com> wrote: > > > > On Thu, Jun 20, 2019, 11:44 AM Bruce Evans <b...@optusnet.com.au> wrote: >> >> On Thu, 20 Jun 2019, Alan Somers wrote: >> >> > On Thu, Jun 20, 2019 at 10:43 AM Bruce Evans <b...@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
See https://reviews.freebsd.org/D20715 . _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
