From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 23489 invoked from network); 27 Jan 2021 15:45:24 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 27 Jan 2021 15:45:24 -0000 Received: (qmail 3497 invoked by uid 550); 27 Jan 2021 15:45:19 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 3478 invoked from network); 27 Jan 2021 15:45:19 -0000 Date: Wed, 27 Jan 2021 10:45:06 -0500 From: Rich Felker To: Andrey Melnikov Cc: musl@lists.openwall.com Message-ID: <20210127154506.GH23432@brightrain.aerifal.cx> References: <20210126172330.GB23432@brightrain.aerifal.cx> <20210126214032.GF23432@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] Re: move __BYTE_ORDER definition to alltypes.h On Wed, Jan 27, 2021 at 11:12:41AM +0300, Andrey Melnikov wrote: > ср, 27 янв. 2021 г. в 00:40, Rich Felker : > > > > On Wed, Jan 27, 2021 at 12:29:05AM +0300, Andrey Melnikov wrote: > > > вт, 26 янв. 2021 г. в 20:23, Rich Felker : > > > > > > > > On Tue, Jan 26, 2021 at 02:55:09PM +0300, Andrey Melnikov wrote: > > > > > Hi. > > > > > > > > > > Your commit 97d35a552ec5b6ddf7923dd2f9a8eb973526acea leads to > > > > > miscompile programs which rely on one of defines __LITTLE_ENDIAN or > > > > > __BIG_ENDIAN. > > > > > Now, both unconditionally defined when included stdarg.h and programs > > > > > which define __(BIG|LITTE)_ENDIAN itself - miscompiled. linux kernel > > > > > for example - it internally uses #if defined __BIG_ENDIAN and defines > > > > > it only for BIGENDAIN arches. > > > > > > > > > > Any ideas? > > > > > > > > The conditionally-defined macros that on some archs tell you the > > > > endianness are __BIG_ENDIAN__ and __LITTLE_ENDIAN__ (note the final > > > > > > Yes, defined by compiller. > > > > > > > __) or other arch-specific macros. __BIG_ENDIAN and __LITTLE_ENDIAN > > > > (without the final __) have always been the possible values for > > > > __BYTE_ORDER from endian.h. > > > > > > >From - its correct headers for this, not from . > > > is for va_* macros. > > > > > > > In any case, all of these are in the > > > > reserved namespace and should not be defined by applications or > > > > inspected in any way other than a manner documented by the > > > > implementation. > > > > > > Where is it reserved? > > > > 7.1.3 Reserved identifiers > > > > "All identifiers that begin with an underscore and either an uppercase > > letter or another underscore are always reserved for any use." > > > > > How did this come up with the Linux kernel? AFAIK it uses -nostdinc > > > > and should not see the libc headers at all. But if that #ifdef is > > > > present in Linux it's probably a bug since it's contrary to all > > > > historical use of __BIG_ENDIAN... > > > > > > Easy. Build an out-of-tree module that includes which > > > is include which is define all variant _ENDIAN macros, > > > later it include which is include > > > in my case, > > > in (read > > > uapi/linux/byteorder/little_endian.h) we have defines: > > > > This also requires -nostdinc. The libc headers are not suitable for > > compiling kernel code. But in this case they should not be breaking > > anything. > > Kernel need va_* macros and definitions for printk(). And > under glibc does not exist, it comes from gcc/llvm distribution > includes. Again, you cannot use libc headers for building the kernel or kernel modules. musl is not glibc. It uses a BSD-like include order and defines all of the headers itself rather than using a mix of gcc headers and libc-provided ones. Add -nostdinc and -I the gcc header path (like the kernel's own build system does) or you will hit obscure problems like this again even if changes are made to the endian macros that make your immediate problem go away. > > > #ifndef __LITTLE_ENDIAN > > > #define __LITTLE_ENDIAN 1234 > > > #endif > > > #ifndef __LITTLE_ENDIAN_BITFIELD > > > #define __LITTLE_ENDIAN_BITFIELD > > > #endif > > > > > > and later, include in module for > > > is_multicast_ether_addr() function: > > > > > > static inline bool is_multicast_ether_addr(const u8 *addr) > > > { > > > #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) > > > u32 a = *(const u32 *)addr; > > > #else > > > u16 a = *(const u16 *)addr; > > > #endif > > > #ifdef __BIG_ENDIAN > > > return 0x01 & (a >> ((sizeof(a) * 8) - 8)); > > > #else > > > return 0x01 & a; > > > #endif1234 > > > } > > > > This code is just wrong. It should be #if __BYTE_ORDER == __BIG_ENDIAN > This code works under glibc/musl-1.1.x and now - broken. This would be a regression if it were doing something violating the reserved namespace, but it's not. musl does not aim for bug-compatibility, and if any application entitled to use libc headers (as opposed to the kernel) were doing sketchy things with the __BIG_ENDIAN macro, it would be an application bug. But since it has potential for silent wrong-code generation it may be appropriate to make changes here anyway. > > or #ifdef __BIG_ENDIAN__. Not #ifdef __BIG_ENDIAN. Where did the code > > come from? > https://github.com/torvalds/linux/blob/2ab38c17aac10bf55ab3efde4c4db3893d8691d2/include/linux/etherdevice.h#L116 Thanks. A quick grep -r __BIG_ENDIAN include/linux shows a good deal of inconsistent usage of this macro (some places with #if __BYTE_ORDER == ..., some with #ifdef). It's not unconditionally wrong for them to use the macro in a different way than libc does, since from the standpoint of building the kernel, Linux is "the implementation" and is the one that owns the entire reserved namespace (apart from what it assumes the compiler might be defining). But it is wrong to mix libc headers in such a context, for this and other reasons. It's not urgent but I think the inconsistency should probably be raised with the kernel folks at some point. It's error-prone and confusing to readers who are used to both endian macros being defined as possible values for __BYTE_ORDER rather than one. > > Whoever wrote it misunderstood the meanings of these macros. > > Even within the kernel they're not intended to be used this way. > Kernel code does not expect random defines coming from random headers. > Remove #include from stdarg.h - it's simply not > needed here. Nothing in this header need to know types/endianness. Or You cannot remove bits/alltypes.h or va_list will not be defined. > remove this header completely - this compiler built-in include. is not compatible with musl. What possibly can be done is removing the __BIG_ENDIAN and __LITTLE_ENDIAN macros and just using the values 1234 and 4321 explicitly. I don't see any strong reason not to do that, and it's probably what I had in an earlier draft of the relevant changes. The reason __BYTE_ORDER is defined here (alltypes.h) is that it's necessary to correctly define some of the types that alltypes.h is capable of exposing. The alltypes.h mechanism itself is used to avoid redundant definitions and guard logic against multiple definition for types which are exposed by more than one header. va_list is such a type because stdio.h, wchar.h, and syslog.h expose it too. Rich