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

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

[Arnd Bergmann]

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(+)

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>
 
 /*
-- 
2.9.0

[PATCH 2/2] Kbuild: always define endianess in kconfig.h

[Arnd Bergmann]

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

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

Re: [PATCH 2/2] Kbuild: always define endianess in kconfig.h

[Masahiro Yamada]

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

Re: [PATCH 2/2] Kbuild: always define endianess in kconfig.h

[Arnd Bergmann]

On Fri, Feb 2, 2018 at 5:31 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2018-02-03 0:40 GMT+09:00 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

I believe we don't always define CONFIG_CPU_LITTLE_ENDIAN,
but it would work in principle. The main issue here is changing so
many files:

$ git grep -wl '__\(BIG\|LITTLE\)_ENDIAN' | wc -l
418

$ git grep -wl 'CONFIG_CPU_\(BIG\|LITTLE\)_ENDIAN' | wc -l
145

      Arnd

Re: [PATCH 2/2] Kbuild: always define endianess in kconfig.h

[Andrew Morton]

Thanks.  I've queued both for 4.16-rc2 (or maybe -rc3).

[tip:locking/urgent] locking/qrwlock: include asm/byteorder.h as needed

[tip-bot for Arnd Bergmann]

Commit-ID:  ca66e797120fb09b8138623fb4b563e952586ef5
Gitweb:     https://git.kernel.org/tip/ca66e797120fb09b8138623fb4b563e952586ef5
Author:     Arnd Bergmann <arnd@arndb.de>
AuthorDate: Fri, 2 Feb 2018 16:40:44 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 6 Feb 2018 10:28:58 +0100

locking/qrwlock: include asm/byteorder.h as needed

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.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Babu Moger <babu.moger@oracle.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Fixes: e0d02285f16e ("locking/qrwlock: Use 'struct qrwlock' instead of 'struct __qrwlock'")
Link: http://lkml.kernel.org/r/20180202154104.1522809-1-arnd@arndb.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/asm-generic/qrwlock_types.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/asm-generic/qrwlock_types.h b/include/asm-generic/qrwlock_types.h
index 137ecdd..c36f1d5 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>
 
 /*