[1/2] locking/qrwlock: include asm/byteorder.h as needed

Message ID 20180202154104.1522809-1-arnd@arndb.de
State Accepted
Commit ca66e797120fb09b8138623fb4b563e952586ef5
Headers show
Series
  • [1/2] locking/qrwlock: include asm/byteorder.h as needed
Related show

Commit Message

Arnd Bergmann Feb. 2, 2018, 3:40 p.m.
Moving the qrwlock struct definition into a header file introduced
a subtle bug on all little-endian machines, where some files in some
configurations would see the fields in an incorrect order.  This was
found by building with an LTO enabled compiler that warns every time we
try to link together files with incompatible data structures.

A second patch changes linux/kconfig.h to always define the symbols,
but this seems to be the root cause of most of the issues, so I'd suggest
we do both.

On a current linux-next kernel, I verified that this header is
responsible for all type mismatches as a result from the endianess
confusion.

Cc: stable@vger.kernel.org
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Fixes: e0d02285f16e ("locking/qrwlock: Use 'struct qrwlock' instead of 'struct __qrwlock'")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 include/asm-generic/qrwlock_types.h | 1 +
 1 file changed, 1 insertion(+)

-- 
2.9.0

Comments

Masahiro Yamada Feb. 2, 2018, 4:31 p.m. | #1
Hi Arnd,


2018-02-03 0:40 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> Build testing with LTO found a couple of files that get compiled

> differently depending on whether asm/byteorder.h gets included early

> enough or not. In particular, include/asm-generic/qrwlock_types.h is

> affected by this, but there are probably others as well.

>

> The symptom is a series of LTO link time warnings, including these:

>

> net/netlabel/netlabel_unlabeled.h:223: error: type of 'netlbl_unlhsh_add' does not match original declaration [-Werror=lto-type-mismatch]

>  int netlbl_unlhsh_add(struct net *net,

> net/netlabel/netlabel_unlabeled.c:377: note: 'netlbl_unlhsh_add' was previously declared here

>

> include/net/ipv6.h:360: error: type of 'ipv6_renew_options_kern' does not match original declaration [-Werror=lto-type-mismatch]

>  ipv6_renew_options_kern(struct sock *sk,

> net/ipv6/exthdrs.c:1162: note: 'ipv6_renew_options_kern' was previously declared here

>

> net/core/dev.c:761: note: 'dev_get_by_name_rcu' was previously declared here

>  struct net_device *dev_get_by_name_rcu(struct net *net, const char *name)

> net/core/dev.c:761: note: code may be misoptimized unless -fno-strict-aliasing is used

>

> drivers/gpu/drm/i915/i915_drv.h:3377: error: type of 'i915_gem_object_set_to_wc_domain' does not match original declaration [-Werror=lto-type-mismatch]

>  i915_gem_object_set_to_wc_domain(struct drm_i915_gem_object *obj, bool write);

> drivers/gpu/drm/i915/i915_gem.c:3639: note: 'i915_gem_object_set_to_wc_domain' was previously declared here

>

> include/linux/debugfs.h:92:9: error: type of 'debugfs_attr_read' does not match original declaration [-Werror=lto-type-mismatch]

>  ssize_t debugfs_attr_read(struct file *file, char __user *buf,

> fs/debugfs/file.c:318: note: 'debugfs_attr_read' was previously declared here

>

> include/linux/rwlock_api_smp.h:30: error: type of '_raw_read_unlock' does not match original declaration [-Werror=lto-type-mismatch]

>  void __lockfunc _raw_read_unlock(rwlock_t *lock) __releases(lock);

> kernel/locking/spinlock.c:246:26: note: '_raw_read_unlock' was previously declared here

>

> include/linux/fs.h:3308:5: error: type of 'simple_attr_open' does not match original declaration [-Werror=lto-type-mismatch]

>  int simple_attr_open(struct inode *inode, struct file *file,

> fs/libfs.c:795: note: 'simple_attr_open' was previously declared here

>

> All of the above are caused by include/asm-generic/qrwlock_types.h failing

> to include asm/byteorder.h after commit e0d02285f16e ("locking/qrwlock:

> Use 'struct qrwlock' instead of 'struct __qrwlock'") in linux-4.15.

>

> Similar bugs may or may not exist in older kernels as well, but there

> is no easy way to test those with link-time optimizations, and kernels

> before 4.14 are harder to fix because they don't have Babu's patch series



Is this patch only for stable kernel,
not for the mainline?


> We had similar issues with CONFIG_ symbols in the past and ended up

> always including the configuration headers though linux/kconfig.h.

> This works around the issue through that same file, defining either

> __BIG_ENDIAN or __LITTLE_ENDIAN depending on CONFIG_CPU_BIG_ENDIAN,

> which is now always set on all architectures since commit 4c97a0c8fee3

> ("arch: define CPU_BIG_ENDIAN for all fixed big endian archs").

>

> Cc: stable@vger.kernel.org

> Cc: Babu Moger <babu.moger@oracle.com>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

>  include/linux/kconfig.h | 6 ++++++

>  1 file changed, 6 insertions(+)

>

> diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h

> index fec5076eda91..cc8fa109cfa3 100644

> --- a/include/linux/kconfig.h

> +++ b/include/linux/kconfig.h

> @@ -4,6 +4,12 @@

>

>  #include <generated/autoconf.h>

>

> +#ifdef CONFIG_CPU_BIG_ENDIAN

> +#define __BIG_ENDIAN 4321

> +#else

> +#define __LITTLE_ENDIAN 1234

> +#endif

> +

>  #define __ARG_PLACEHOLDER_1 0,

>  #define __take_second_arg(__ignored, val, ...) val

>

> --

> 2.9.0

>


If all architectures define
CONFIG_CPU_BIG_ENDIAN or CONFIG_CPU_LITTLE_ENDIAN,
is it possible to use this for endian test?

i.e.

Is it possible to replace like this?
   #ifdef __BIG_ENDIAN       ->  #ifdef CONFIG_CPU_BIG_ENDIAN

   #ifdef __LITTLE_ENDIAN     -> #ifdef CONFIG_CPU_LITTLE_ENDIAN






-- 
Best Regards
Masahiro Yamada

Patch

diff --git a/include/asm-generic/qrwlock_types.h b/include/asm-generic/qrwlock_types.h
index 137ecdd16daa..c36f1d5a2572 100644
--- a/include/asm-generic/qrwlock_types.h
+++ b/include/asm-generic/qrwlock_types.h
@@ -3,6 +3,7 @@ 
 #define __ASM_GENERIC_QRWLOCK_TYPES_H
 
 #include <linux/types.h>
+#include <asm/byteorder.h>
 #include <asm/spinlock_types.h>
 
 /*