Re: [PATCH] s390: disable -Warray-bounds

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]

On Wed, Jun 8, 2022 at 2:33 PM Kees Cook <keescook@chromium.org> wrote:
>
> I and others have been working through a bunch of them, though yes,
> they're not all fixed yet. I've been trying to track it here[1], but
> many of those fixes are only in -next.

Hmm. Even with that disabled, I get a few warnings I *really* would
want to get rid of.

The one in ipuv3-crtc.c seems valid about "address used as boolean is
always true".

The 'dangling-pointer' warning does seem interesting, but not when the
compiler does as bad a job as gcc seems to do.

See the attached patch for

 (a) make the s390 "use -Wno-array-bounds for gcc-12" be generic

 (b) fix the ipuv3-crtc.c one. IMX people?

 (c) disable -Wdangling-pointer entirely for now

but that still leaves the netfs_i_context games, which gcc-12 is very
unhappy about:

  In function ‘fortify_memset_chk’,
      inlined from ‘netfs_i_context_init’ at ./include/linux/netfs.h:327:2,
      inlined from ‘afs_set_netfs_context’ at fs/afs/inode.c:61:2,
      inlined from ‘afs_inode_init_from_status’ at fs/afs/inode.c:139:2:
  ./include/linux/fortify-string.h:258:25: error: call to
‘__write_overflow_field’ declared with attribute warning: detected
write beyond size of field (1st parameter); maybe use struct_group()?
[-Werror=attribute-warning]

and I do kind of agree with the compiler in that case. That code
should have some kind of

  struct container {
        struct inode inode;
        struct netfs_i_context ctx;
   };

thing, and aim to do that instead of the pointer arithmetic games.

Ceph seems to trigger the exact same thing.

There's also an annoying mlx5 issue, with gcc apparently not tracking
the usage of

        struct lag_tracker tracker;

well enough (it's never used if do_bond is false, but probably some
inlining change means that gcc doesn't see that).

DavidH - mind looking at the netfs_i_context_init() thing?

I'd like to use something more surgical than
CONFIG_CC_NO_ARRAY_BOUNDS, but considering the s390 issues, it may not
even be worth it. Kees, just how far away are we from that being ok on
x86-64?

I did consider making CONFIG_CC_NO_ARRAY_BOUNDS be more akin to

 config CC_NO_ARRAY_BOUNDS
        bool
        depends on CC_IS_GCC
        depends on GCC_VERSION >= 120000 && GCC_VERSION < 130000
        default GCC12_NO_ARRAY_BOUNDS

and then s390 and any subsystem that triggers the -Warray-bounds problem can do

        select GCC12_NO_ARRAY_BOUNDS

to show that they have issues with the new gcc12 rules.

That would be at least a bit more surgical..

                  Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 3073 bytes --]

 Makefile                         |  4 ++++
 arch/s390/Makefile               | 10 +---------
 drivers/gpu/drm/imx/ipuv3-crtc.c |  2 +-
 init/Kconfig                     |  5 +++++
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index c43d825a3c4c..b2e93c1a8021 100644
--- a/Makefile
+++ b/Makefile
@@ -788,6 +788,7 @@ stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG)      := -fstack-protector-strong
 KBUILD_CFLAGS += $(stackp-flags-y)
 
 KBUILD_CFLAGS-$(CONFIG_WERROR) += -Werror
+KBUILD_CFLAGS-$(CONFIG_CC_NO_ARRAY_BOUNDS) += -Wno-array-bounds
 KBUILD_CFLAGS += $(KBUILD_CFLAGS-y) $(CONFIG_CC_IMPLICIT_FALLTHROUGH)
 
 ifdef CONFIG_CC_IS_CLANG
@@ -805,6 +806,9 @@ endif
 KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
 KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
 
+# These result in bogus false positives
+KBUILD_CFLAGS += $(call cc-disable-warning, dangling-pointer)
+
 ifdef CONFIG_FRAME_POINTER
 KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
 else
diff --git a/arch/s390/Makefile b/arch/s390/Makefile
index d73611b35164..e1abb0d03824 100644
--- a/arch/s390/Makefile
+++ b/arch/s390/Makefile
@@ -32,15 +32,7 @@ KBUILD_CFLAGS_DECOMPRESSOR += -fno-stack-protector
 KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning, address-of-packed-member)
 KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),-g)
 KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO_DWARF4), $(call cc-option, -gdwarf-4,))
-
-ifdef CONFIG_CC_IS_GCC
-	ifeq ($(call cc-ifversion, -ge, 1200, y), y)
-		ifeq ($(call cc-ifversion, -lt, 1300, y), y)
-			KBUILD_CFLAGS += $(call cc-disable-warning, array-bounds)
-			KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning, array-bounds)
-		endif
-	endif
-endif
+KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CC_NO_ARRAY_BOUNDS),-Wno-array-bounds)
 
 UTS_MACHINE	:= s390x
 STACK_SIZE	:= $(if $(CONFIG_KASAN),65536,16384)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 9c8829f945b2..f7863d6dea80 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -69,7 +69,7 @@ static void ipu_crtc_disable_planes(struct ipu_crtc *ipu_crtc,
 	drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) {
 		if (plane == &ipu_crtc->plane[0]->base)
 			disable_full = true;
-		if (&ipu_crtc->plane[1] && plane == &ipu_crtc->plane[1]->base)
+		if (ipu_crtc->plane[1] && plane == &ipu_crtc->plane[1]->base)
 			disable_partial = true;
 	}
 
diff --git a/init/Kconfig b/init/Kconfig
index c984afc489de..ccb1302d6edd 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -885,6 +885,11 @@ config CC_IMPLICIT_FALLTHROUGH
 	default "-Wimplicit-fallthrough=5" if CC_IS_GCC && $(cc-option,-Wimplicit-fallthrough=5)
 	default "-Wimplicit-fallthrough" if CC_IS_CLANG && $(cc-option,-Wunreachable-code-fallthrough)
 
+config CC_NO_ARRAY_BOUNDS
+	bool
+	depends on CC_IS_GCC
+	default y if GCC_VERSION >= 120000 && GCC_VERSION < 130000
+
 #
 # For architectures that know their GCC __int128 support is sound
 #