Byteorder conditional compilation problems

Byteorder conditional compilation problems

[David Howells]

Hi Linus,

Looking at commit 13da9e200fe4740b02cd51e07ab454627e228920, I suspect the
answer will be no, but can we please revisit the use of __BYTE_ORDER in the
kernel?


The reason for this is that I've noticed a number of places in the UAPI
headers where __LITTLE_ENDIAN and __BIG_ENDIAN are assumed to act as in the
main kernel: they are tested for definedness, which is incorrect in UAPI
headers unless guarded by __KERNEL__.  In UAPI headers, they _must_ be
compared to __BYTE_ORDER in !__KERNEL__ conditions and <endian.h> should
perhaps be #included as well.

The following headers are all incorrect in their usage in this manner:

	include/uapi/linux/aio_abi.h - definition of PADDED()
	include/uapi/linux/acct.h - definition of ACCT_BYTEORDER
	include/uapi/linux/raid/md_p.h - order of members in mdp_superblock_s
	arch/m32r/include/uapi/asm/stat.h - order of members in stat64

The last two are the most worrying as these are visible from userspace.  Both
cases will likely use the __BIG_ENDIAN version in userspace (depending on
header inclusion ordering) - and it wouldn't surprise me if there's a nasty
bug in the RAID API that no one has spotted yet.  M32R will likely have a
problem too if anyone uses that in LE mode (the default is "no" on that one,
though).

And then there are:

	arch/arc/include/uapi/asm/byteorder.h - uses CONFIG_CPU_BIG_ENDIAN
	arch/c6x/include/uapi/asm/byteorder.h - uses _BIG_ENDIAN
	arch/c6x/include/uapi/asm/ptrace.h - uses _BIG_ENDIAN
	arch/mips/include/uapi/asm/msgbuf.h - uses CONFIG_CPU_LITTLE_ENDIAN

which all look like they might be wrong.

And finally, there are:

	include/{uapi/,}linux/patchkey.h - macro _PATCHKEY
	include/{uapi/,}linux/soundcard.h - macro AFMT_S16_NE

which have two separate bits (one UAPI, one KAPI) defining the same macro in
the same way because the macro is endianness-dependent, but the logic to
select the variant cannot be shared between UAPI and KAPI.

The following:

	include/uapi/sound/asound.h
	include/uapi/sound/sfnt_info.h

seem to dodge the problem by using SNDRV_{LITTLE,BIG}_ENDIAN instead - though
this doesn't seem to be defined in any headers under /usr/include, so I
presume it is required to be set manually by anyone using those headers.


Whilst the following opinion:

	The kernel has never had that crazy "__BYTE_ORDER == __LITTLE_ENDIAN"
	model.  It's not how we do things, and it isn't how we _should_ do
	things.  So don't go there.

may be a valid, it is incompatible with userspace.  We cannot change how
userspace works, but we could change how the kernel works to match it, thus
reducing the confusion.


Looking at commit b3b77c8caef1750ebeea1054e39e358550ea9f55, I think the
problems with it are simple to resolve:

 (1) linux/byteorder/big_endian.h didn't define __LITTLE_ENDIAN and
     .../little_endian.h didn't define __BIG_ENDIAN.  These should be placed
     in a common header, and only __BYTE_ORDER defined in the separate
     headers.

 (2) Tests for the definedness of __BIG_ENDIAN and __LITTLE_ENDIAN must be
     replaced en mass with comparisons against __BYTE_ORDER.

Sadly, (1) and (2) would have to be done in the same commit to avoid problems
since __BIG_ENDIAN and __LITTLE_ENDIAN must both be defined to for (1):-/


Throughout the whole kernel:

 - There are 703 instances of "defined(__XXX_ENDIAN)" scattered over 68
   files.  None of these appear to be in comments.

 - There are 6 instances of "def __XXX_ENDIAN[^_]" scattered over 4 files and
   these are all in comments.

 - There are a lot of "__XXX_ENDIAN__" - but these are not a problem.
   Possibly these should be converted to "__XXX_ENDIAN" and the relevant -D
   preprocessor flags removed.

 - There are a lot of "__XXX_ENDIAN_BITFIELD" - which aren't a problem either.


Scripting a change from "defined(__XXX_ENDIAN)" to "(__XXX_ENDIAN==__BYTEORDER)"
should be easy to script.

David

Re: Byteorder conditional compilation problems

[Linus Torvalds]

On Wed, Mar 6, 2013 at 8:43 AM, David Howells <dhowells@redhat.com> wrote:
>
> Scripting a change from "defined(__XXX_ENDIAN)" to "(__XXX_ENDIAN==__BYTEORDER)"
> should be easy to script.

How about we just make the rule be that we shouldn't test __xyz_ENDIAN
at all, and instead always use CONFIG_xyz_ENDIAN, which isn't
ambiguous. And then have the exporter of the uapi header files just
sed-script that into __xyz_ENDIAN == __BYTEORDER.

The whole __xyz_ENDIAN == __BYTE_ORDER syntax is moronic, and the
standard under BSD seems to be single-underscore "_BYTE_ORDER", while
glibc uses double underscore __BYTE_ORDER. The whole thing is a
f*cking mess, and I really don't want to get the idiotic mess into the
kernel any more than I have to.

In fact, I'd prefer to let it just be. Changing it in the kernel to
the idiotic and non-standard user mode format is pointless. Just fix
up the (few) entries in the uapi header files.

                  Linus

Re: Byteorder conditional compilation problems

[Geert Uytterhoeven]

On Wed, Mar 6, 2013 at 6:37 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Mar 6, 2013 at 8:43 AM, David Howells <dhowells@redhat.com> wrote:
>> Scripting a change from "defined(__XXX_ENDIAN)" to "(__XXX_ENDIAN==__BYTEORDER)"
>> should be easy to script.
>
> How about we just make the rule be that we shouldn't test __xyz_ENDIAN
> at all, and instead always use CONFIG_xyz_ENDIAN, which isn't
> ambiguous. And then have the exporter of the uapi header files just
> sed-script that into __xyz_ENDIAN == __BYTEORDER.

I was going to suggest a checkpatch test, but your suggestion is a
better solution.
Still, we may want the checkpatch test, to make sure no one explicitly does the
moronic byte order check.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds