svn commit: r349233 - head/sys/sys

[Alan Somers]

On Thu, Jun 20, 2019 at 1:43 PM Warner Losh <imp at bsdimp.com> wrote:
>
>
>
> On Thu, Jun 20, 2019, 11:44 AM Bruce Evans <brde at optusnet.com.au> wrote:
>>
>> On Thu, 20 Jun 2019, Alan Somers wrote:
>>
>> > On Thu, Jun 20, 2019 at 10:43 AM Bruce Evans <brde at 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 .