On Wed, Jun 15, 2016 at 11:33:32AM +0200, Paul Bolle wrote:

On do, 2016-05-05 at 22:44 -0700, Andrew Morton wrote:

quoted

From: Arnd Bergmann <arnd@arndb.de>
Subject: byteswap: try to avoid __builtin_constant_p gcc bug

This is another attempt to avoid a regression in wwn_to_u64() after that
started using get_unaligned_be64(), which in turn ran into a bug on
gcc-4.9 through 6.1.

The regression got introduced due to the combination of two separate
workarounds (e3bde9568d99 ("include/linux/unaligned: force inlining of
byteswap operations") and ef3fb2422ffe ("scsi: fc: use get/put_unaligned64
for wwn access")) that each try to sidestep distinct problems with gcc
behavior (code growth and increased stack usage).  Unfortunately after
both have been applied, a more serious gcc bug has been uncovered, leading
to incorrect object code that discards part of a function and causes
undefined behavior.

As part of this problem is how __builtin_constant_p gets evaluated on an
argument passed by reference into an inline function, this avoids the use
of __builtin_constant_p() for all architectures that set
CONFIG_ARCH_USE_BUILTIN_BSWAP.  Most architectures do not set
ARCH_SUPPORTS_OPTIMIZED_INLINING, which means they probably do not suffer
from the problem in the qla2xxx driver, but they might still run into it
elsewhere.

Both of the original workarounds were only merged in the 4.6 kernel, and
the bug that is fixed by this patch should only appear if both are there,
so we probably don't need to backport the fix.  On the other hand, it
works by simplifying the code path and should not have any negative
effects.

[arnd@arndb.de: fix older gcc warnings]
  (http://lkml.kernel.org/r/12243652.bxSxEgjgfk@wuerfel)
Link: https://lkml.org/lkml/headers/2016/4/12/1103
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70232
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
Fixes: e3bde9568d99 ("include/linux/unaligned: force inlining of byteswap operations")
Fixes: ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
Link: http://lkml.kernel.org/r/1780465.XdtPJpi8Tt@wuerfel
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Josh Poimboeuf <redacted>
Tested-by: Josh Poimboeuf <redacted> # on gcc-5.3
Tested-by: Quinn Tran <redacted>
Cc: Martin Jambor <redacted>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
Cc: Denys Vlasenko <redacted>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Himanshu Madhani <redacted>
Cc: Jan Hubicka <redacted>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

This became commit 7322dd755e7d ("byteswap: try to avoid
__builtin_constant_p gcc bug"). That commit was included in v4.6-rc7.
Ever since that rc I see this warning when building for x86_64:
    net/netfilter/ipvs/ip_vs_sync.c: In function ‘ip_vs_proc_sync_conn’:
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘opt.init_seq’ may be used uninitialized in this function [-Wmaybe-uninitialized]
      struct ip_vs_sync_conn_options opt;
                                     ^
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘opt.delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘opt.previous_delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘*((void *)&opt+12).init_seq’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘*((void *)&opt+12).delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘*((void *)&opt+12).previous_delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]

(It doesn't show up when building for 32 bits x86. Perhaps there's an
int/long mismatch somewhere in the related code.)

Anyone else seeing this? 

It looks like a false positive. I can make it disappear by making sure
ip_vs_proc_seqopt() isn't inlined. But that's not something I dare to
put into a patch for a false positive. Anyone sitting on a better fix?

Hi Paul,

I agree it looks like a false positive, though the code is a bit
convoluted, so I'm not surprised that gcc might get confused.  How about
initializing opt to 0?

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 803001a..f45496c 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c

@@ -1066,7 +1066,7 @@ static int ip_vs_proc_str(__u8 *p, unsigned int plen, unsigned int *data_len,
  */
 static inline int ip_vs_proc_sync_conn(struct netns_ipvs *ipvs, __u8 *p, __u8 *msg_end)
 {
-	struct ip_vs_sync_conn_options opt;
+	struct ip_vs_sync_conn_options opt = {0};
 	union  ip_vs_sync_conn *s;
 	struct ip_vs_protocol *pp;
 	struct ip_vs_conn_param param;

-- 
Josh