Here are four patches to fix some of the problems with endianness-related
preprocessor conditionals I have found in the UAPI header files.
The problem is that the way that preprocessor conditionals are used to
determine endianness when building for Linux userspace (as defined by the
predominant use of glibc) is not compatible with the way that the kernel build
does things. The problem revolves around how __BIG_ENDIAN and __LITTLE_ENDIAN
are defined in each environment.
When building for Linux userspace, __BIG_ENDIAN and __LITTLE_ENDIAN are always
defined - so the kernel's preferred:
if defined(__xxx_ENDIAN)
is always true in userspace builds, no matter which endianness your check
employs - whereas only one is defined in the kernel builds - meaning
#if __BYTE_ORDER == __xxx_ENDIAN
gives a warning with -Wundef if you select the undefined endianness for your
check.
Unfortunately, the UAPI header files _must_ employ the userspace variant of
these conditionals outside of __KERNEL__-conditionalised regions as these
conditionals get exposed to userspace and userspace _cannot_ be changed.
These can be fixed in the UAPI headers by changing:
#if defined(__BIG_ENDIAN)
#define foo 1234
#elif defined(__LITTLE_ENDIAN)
#define foo 4321
#else
#error endianness unspecified
#endif
to:
#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
#define foo 1234
#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
#define foo 4321
#else
#error endianness unspecified
#endif
as it appears gcc's cpp doesn't complain about macros that aren't evaluated.
[!!!] NOTE [!!!] These patches may adversely change the userspace API. Since
the userspace API appears to be wrong under some circumstances due to incorrect
conditionals, it may be necessary to make an alternate fix whereby the we
select the first variant unconditionally in all cases as we would otherwise be
changing the actual userspace API.
David
---
David Howells (4):
UAPI: Fix endianness conditionals in linux/aio_abi.h
UAPI: Fix endianness conditionals in linux/acct.h
UAPI: Fix endianness conditionals in linux/raid/md_p.h
UAPI: Fix endianness conditionals in M32R's asm/stat.h
arch/m32r/include/uapi/asm/stat.h | 4 ++--
include/uapi/linux/acct.h | 6 ++++--
include/uapi/linux/aio_abi.h | 4 ++--
include/uapi/linux/raid/md_p.h | 6 ++++--
4 files changed, 12 insertions(+), 8 deletions(-)
In the UAPI header files, __BIG_ENDIAN and __LITTLE_ENDIAN must be compared
against __BYTE_ORDER in preprocessor conditionals where these are exposed to
userspace (that is they're not inside __KERNEL__ conditionals).
However, in the main kernel the norm is to check for "defined(__XXX_ENDIAN)"
rather than comparing against __BYTE_ORDER and this has incorrectly leaked
into the userspace headers.
The definition of PADDED() in linux/aio_abi.h is wrong in this way. Note that
userspace will likely interpret this and thus the order of fields in struct
iocb incorrectly as the little-endian variant on big-endian machines -
depending on header inclusion order.
[!!!] NOTE [!!!] This patch may adversely change the userspace API. It might
be better to fix the ordering of aio_key and aio_reserved1 in struct iocb.
Signed-off-by: David Howells <[email protected]>
cc: Benjamin LaHaise <[email protected]>
cc: [email protected]
---
include/uapi/linux/aio_abi.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index 86fa7a7..bb2554f 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -62,9 +62,9 @@ struct io_event {
__s64 res2; /* secondary result */
};
-#if defined(__LITTLE_ENDIAN)
+#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
#define PADDED(x,y) x, y
-#elif defined(__BIG_ENDIAN)
+#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
#define PADDED(x,y) y, x
#else
#error edit for your odd byteorder.
In the UAPI header files, __BIG_ENDIAN and __LITTLE_ENDIAN must be compared
against __BYTE_ORDER in preprocessor conditionals where these are exposed to
userspace (that is they're not inside __KERNEL__ conditionals).
However, in the main kernel the norm is to check for "defined(__XXX_ENDIAN)"
rather than comparing against __BYTE_ORDER and this has incorrectly leaked
into the userspace headers.
The definition of ACCT_BYTEORDER in linux/acct.h is wrong in this way. Note
that userspace will likely interpret this incorrectly as the big-endian variant
on little-endian machines - depending on header inclusion order.
[!!!] NOTE [!!!] This patch may adversely change the userspace API. It might
be better to fix the value of ACCT_BYTEORDER.
Signed-off-by: David Howells <[email protected]>
---
include/uapi/linux/acct.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/acct.h b/include/uapi/linux/acct.h
index 11b6ca3..df2f9a0 100644
--- a/include/uapi/linux/acct.h
+++ b/include/uapi/linux/acct.h
@@ -107,10 +107,12 @@ struct acct_v3
#define ACORE 0x08 /* ... dumped core */
#define AXSIG 0x10 /* ... was killed by a signal */
-#ifdef __BIG_ENDIAN
+#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
#define ACCT_BYTEORDER 0x80 /* accounting file is big endian */
-#else
+#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
#define ACCT_BYTEORDER 0x00 /* accounting file is little endian */
+#else
+#error unspecified endianness
#endif
#ifndef __KERNEL__
In the UAPI header files, __BIG_ENDIAN and __LITTLE_ENDIAN must be compared
against __BYTE_ORDER in preprocessor conditionals where these are exposed to
userspace (that is they're not inside __KERNEL__ conditionals).
However, in the main kernel the norm is to check for "defined(__XXX_ENDIAN)"
rather than comparing against __BYTE_ORDER and this has incorrectly leaked
into the userspace headers.
The definition of struct stat64 in M32R's asm/stat.h is wrong in this way.
Note that userspace will likely interpret the field order incorrectly as the
big-endian variant on little-endian machines - depending on header inclusion
order.
[!!!] NOTE [!!!] This patch may adversely change the userspace API. It might
be better to fix the ordering of st_blocks and __pad4 in struct stat64.
Signed-off-by: David Howells <[email protected]>
cc: Hirokazu Takata <[email protected]>
cc: [email protected]
cc: [email protected]
---
arch/m32r/include/uapi/asm/stat.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/m32r/include/uapi/asm/stat.h b/arch/m32r/include/uapi/asm/stat.h
index da4518f..98470fe 100644
--- a/arch/m32r/include/uapi/asm/stat.h
+++ b/arch/m32r/include/uapi/asm/stat.h
@@ -63,10 +63,10 @@ struct stat64 {
long long st_size;
unsigned long st_blksize;
-#if defined(__BIG_ENDIAN)
+#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
unsigned long __pad4; /* future possible st_blocks high bits */
unsigned long st_blocks; /* Number 512-byte blocks allocated. */
-#elif defined(__LITTLE_ENDIAN)
+#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
unsigned long st_blocks; /* Number 512-byte blocks allocated. */
unsigned long __pad4; /* future possible st_blocks high bits */
#else
In the UAPI header files, __BIG_ENDIAN and __LITTLE_ENDIAN must be compared
against __BYTE_ORDER in preprocessor conditionals where these are exposed to
userspace (that is they're not inside __KERNEL__ conditionals).
However, in the main kernel the norm is to check for "defined(__XXX_ENDIAN)"
rather than comparing against __BYTE_ORDER and this has incorrectly leaked
into the userspace headers.
The definition of struct mdp_superblock_s in linux/raid/md_p.h is wrong in this
way. Note that userspace will likely interpret the ordering of the fields
incorrectly as the big-endian variant on a little-endian machines - depending
on header inclusion order.
[!!!] NOTE [!!!] This patch may adversely change the userspace API. It might
be better to fix the ordering of events_hi, events_lo, cp_events_hi and
cp_events_lo in struct mdp_superblock_s / typedef mdp_super_t.
Signed-off-by: David Howells <[email protected]>
cc: Neil Brown <[email protected]>
cc: [email protected]
---
include/uapi/linux/raid/md_p.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
index ee75353..fe1a540 100644
--- a/include/uapi/linux/raid/md_p.h
+++ b/include/uapi/linux/raid/md_p.h
@@ -145,16 +145,18 @@ typedef struct mdp_superblock_s {
__u32 failed_disks; /* 4 Number of failed disks */
__u32 spare_disks; /* 5 Number of spare disks */
__u32 sb_csum; /* 6 checksum of the whole superblock */
-#ifdef __BIG_ENDIAN
+#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
__u32 events_hi; /* 7 high-order of superblock update count */
__u32 events_lo; /* 8 low-order of superblock update count */
__u32 cp_events_hi; /* 9 high-order of checkpoint update count */
__u32 cp_events_lo; /* 10 low-order of checkpoint update count */
-#else
+#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
__u32 events_lo; /* 7 low-order of superblock update count */
__u32 events_hi; /* 8 high-order of superblock update count */
__u32 cp_events_lo; /* 9 low-order of checkpoint update count */
__u32 cp_events_hi; /* 10 high-order of checkpoint update count */
+#else
+#error unspecified endianness
#endif
__u32 recovery_cp; /* 11 recovery checkpoint sector count */
/* There are only valid for minor_version > 90 */
On Wed, 06 Mar 2013 20:47:51 +0000 David Howells <[email protected]> wrote:
> In the UAPI header files, __BIG_ENDIAN and __LITTLE_ENDIAN must be compared
> against __BYTE_ORDER in preprocessor conditionals where these are exposed to
> userspace (that is they're not inside __KERNEL__ conditionals).
>
> However, in the main kernel the norm is to check for "defined(__XXX_ENDIAN)"
> rather than comparing against __BYTE_ORDER and this has incorrectly leaked
> into the userspace headers.
>
> The definition of struct mdp_superblock_s in linux/raid/md_p.h is wrong in this
> way. Note that userspace will likely interpret the ordering of the fields
> incorrectly as the big-endian variant on a little-endian machines - depending
> on header inclusion order.
>
> [!!!] NOTE [!!!] This patch may adversely change the userspace API. It might
> be better to fix the ordering of events_hi, events_lo, cp_events_hi and
> cp_events_lo in struct mdp_superblock_s / typedef mdp_super_t.
Thanks David - looks good to me.
Changing the ordering of fields isn't really an option at this stage - over
10 years too late :-(.
I think any user-space tools that use this data structure have their own copy
of the include file.
Acked-by: NeilBrown <[email protected]>
Thanks,
NeilBrown
>
> Signed-off-by: David Howells <[email protected]>
> cc: Neil Brown <[email protected]>
> cc: [email protected]
> ---
>
> include/uapi/linux/raid/md_p.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
> index ee75353..fe1a540 100644
> --- a/include/uapi/linux/raid/md_p.h
> +++ b/include/uapi/linux/raid/md_p.h
> @@ -145,16 +145,18 @@ typedef struct mdp_superblock_s {
> __u32 failed_disks; /* 4 Number of failed disks */
> __u32 spare_disks; /* 5 Number of spare disks */
> __u32 sb_csum; /* 6 checksum of the whole superblock */
> -#ifdef __BIG_ENDIAN
> +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
> __u32 events_hi; /* 7 high-order of superblock update count */
> __u32 events_lo; /* 8 low-order of superblock update count */
> __u32 cp_events_hi; /* 9 high-order of checkpoint update count */
> __u32 cp_events_lo; /* 10 low-order of checkpoint update count */
> -#else
> +#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
> __u32 events_lo; /* 7 low-order of superblock update count */
> __u32 events_hi; /* 8 high-order of superblock update count */
> __u32 cp_events_lo; /* 9 low-order of checkpoint update count */
> __u32 cp_events_hi; /* 10 high-order of checkpoint update count */
> +#else
> +#error unspecified endianness
> #endif
> __u32 recovery_cp; /* 11 recovery checkpoint sector count */
> /* There are only valid for minor_version > 90 */
On Wed, Mar 06, 2013 at 08:47:33PM +0000, David Howells wrote:
> In the UAPI header files, __BIG_ENDIAN and __LITTLE_ENDIAN must be compared
> against __BYTE_ORDER in preprocessor conditionals where these are exposed to
> userspace (that is they're not inside __KERNEL__ conditionals).
>
> However, in the main kernel the norm is to check for "defined(__XXX_ENDIAN)"
> rather than comparing against __BYTE_ORDER and this has incorrectly leaked
> into the userspace headers.
>
> The definition of PADDED() in linux/aio_abi.h is wrong in this way. Note that
> userspace will likely interpret this and thus the order of fields in struct
> iocb incorrectly as the little-endian variant on big-endian machines -
> depending on header inclusion order.
>
> [!!!] NOTE [!!!] This patch may adversely change the userspace API. It might
> be better to fix the ordering of aio_key and aio_reserved1 in struct iocb.
It is unlikely that anyone has used the existing kernel headers and hit this
issue given that most existing users use the libaio.h include (which does not
get the endianness tests wrong). Given that the kernel has always used the
correct endian mappings, this change is correct.
Acked-by: Benjamin LaHaise <[email protected]>
-ben
> Signed-off-by: David Howells <[email protected]>
> cc: Benjamin LaHaise <[email protected]>
> cc: [email protected]
> ---
>
> include/uapi/linux/aio_abi.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
> index 86fa7a7..bb2554f 100644
> --- a/include/uapi/linux/aio_abi.h
> +++ b/include/uapi/linux/aio_abi.h
> @@ -62,9 +62,9 @@ struct io_event {
> __s64 res2; /* secondary result */
> };
>
> -#if defined(__LITTLE_ENDIAN)
> +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
> #define PADDED(x,y) x, y
> -#elif defined(__BIG_ENDIAN)
> +#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
> #define PADDED(x,y) y, x
> #else
> #error edit for your odd byteorder.
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-aio' in
> the body to [email protected]. For more info on Linux AIO,
> see: http://www.kvack.org/aio/
> Don't email: <a href=mailto:"[email protected]">[email protected]</a>
--
"Thought is the essence of where you are now."
Benjamin LaHaise <[email protected]> writes:
> On Wed, Mar 06, 2013 at 08:47:33PM +0000, David Howells wrote:
>> In the UAPI header files, __BIG_ENDIAN and __LITTLE_ENDIAN must be compared
>> against __BYTE_ORDER in preprocessor conditionals where these are exposed to
>> userspace (that is they're not inside __KERNEL__ conditionals).
>>
>> However, in the main kernel the norm is to check for "defined(__XXX_ENDIAN)"
>> rather than comparing against __BYTE_ORDER and this has incorrectly leaked
>> into the userspace headers.
>>
>> The definition of PADDED() in linux/aio_abi.h is wrong in this way. Note that
>> userspace will likely interpret this and thus the order of fields in struct
>> iocb incorrectly as the little-endian variant on big-endian machines -
>> depending on header inclusion order.
>>
>> [!!!] NOTE [!!!] This patch may adversely change the userspace API. It might
>> be better to fix the ordering of aio_key and aio_reserved1 in struct iocb.
>
> It is unlikely that anyone has used the existing kernel headers and hit this
> issue given that most existing users use the libaio.h include (which does not
> get the endianness tests wrong). Given that the kernel has always used the
> correct endian mappings, this change is correct.
Agreed.
Acked-by: Jeff Moyer <[email protected]>