* [PATCH] s390: disable -Warray-bounds @ 2022-04-22 13:43 Sven Schnelle 2022-04-22 17:54 ` Kees Cook 0 siblings, 1 reply; 17+ messages in thread From: Sven Schnelle @ 2022-04-22 13:43 UTC (permalink / raw) To: Heiko Carstens Cc: Vasily Gorbik, Alexander Gordeev, Linus Torvalds, Kees Cook, Linux Kernel Mailing List gcc-12 shows a lot of array bound warnings on s390. This is caused by our S390_lowcore macro: which uses an hardcoded address of 0. Wrapping that with absolute_pointer() works, but gcc no longer knows that a 12 bit instruction is sufficient to access lowcore. So it emits instructions like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a single load/store instruction. As s390 stores variables often read/written in lowcore, this is considered problematic. Therefore disable -Warray-bounds on s390 for now until there is a better real solution. Signed-off-by: Sven Schnelle <svens@linux.ibm.com> --- arch/s390/Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/s390/Makefile b/arch/s390/Makefile index e441b60b1812..aff0f66e25fb 100644 --- a/arch/s390/Makefile +++ b/arch/s390/Makefile @@ -14,6 +14,7 @@ KBUILD_AFLAGS_MODULE += -fPIC KBUILD_CFLAGS_MODULE += -fPIC KBUILD_AFLAGS += -m64 KBUILD_CFLAGS += -m64 +KBUILD_CFLAGS += $(call cc-disable-warning, array-bounds) ifeq ($(CONFIG_RELOCATABLE),y) KBUILD_CFLAGS += -fPIE LDFLAGS_vmlinux := -pie @@ -28,6 +29,7 @@ KBUILD_CFLAGS_DECOMPRESSOR += -fno-asynchronous-unwind-tables KBUILD_CFLAGS_DECOMPRESSOR += -ffreestanding KBUILD_CFLAGS_DECOMPRESSOR += -fno-stack-protector KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning, address-of-packed-member) +KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning, array-bounds) KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),-g) KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO_DWARF4), $(call cc-option, -gdwarf-4,)) UTS_MACHINE := s390x -- 2.35.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-04-22 13:43 [PATCH] s390: disable -Warray-bounds Sven Schnelle @ 2022-04-22 17:54 ` Kees Cook 2022-04-25 9:13 ` Heiko Carstens 2022-06-08 20:07 ` Linus Torvalds 0 siblings, 2 replies; 17+ messages in thread From: Kees Cook @ 2022-04-22 17:54 UTC (permalink / raw) To: Sven Schnelle Cc: Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linus Torvalds, Linux Kernel Mailing List On Fri, Apr 22, 2022 at 03:43:08PM +0200, Sven Schnelle wrote: > gcc-12 shows a lot of array bound warnings on s390. This is caused > by our S390_lowcore macro: > > which uses an hardcoded address of 0. Wrapping that with > absolute_pointer() works, but gcc no longer knows that a 12 bit > instruction is sufficient to access lowcore. So it emits instructions > like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a single load/store > instruction. As s390 stores variables often read/written in lowcore, > this is considered problematic. Therefore disable -Warray-bounds on > s390 for now until there is a better real solution. > > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> It looks like the source of this problem (the literal-values-treated-as-NULL) is gcc-12 specific. From the discussions, it sounded like Jacob was going to fix this "correctly" in gcc-13. It might be a good idea to make this version-checked? (i.e. only disable on gcc-12) Either way: Reviewed-by: Kees Cook <keescook@chromium.org> -- Kees Cook ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-04-22 17:54 ` Kees Cook @ 2022-04-25 9:13 ` Heiko Carstens 2022-06-08 20:07 ` Linus Torvalds 1 sibling, 0 replies; 17+ messages in thread From: Heiko Carstens @ 2022-04-25 9:13 UTC (permalink / raw) To: Kees Cook Cc: Sven Schnelle, Vasily Gorbik, Alexander Gordeev, Linus Torvalds, Linux Kernel Mailing List On Fri, Apr 22, 2022 at 10:54:09AM -0700, Kees Cook wrote: > On Fri, Apr 22, 2022 at 03:43:08PM +0200, Sven Schnelle wrote: > > gcc-12 shows a lot of array bound warnings on s390. This is caused > > by our S390_lowcore macro: > > > > which uses an hardcoded address of 0. Wrapping that with > > absolute_pointer() works, but gcc no longer knows that a 12 bit > > instruction is sufficient to access lowcore. So it emits instructions > > like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a single load/store > > instruction. As s390 stores variables often read/written in lowcore, > > this is considered problematic. Therefore disable -Warray-bounds on > > s390 for now until there is a better real solution. > > > > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > > It looks like the source of this problem (the literal-values-treated-as-NULL) > is gcc-12 specific. From the discussions, it sounded like Jacob was > going to fix this "correctly" in gcc-13. It might be a good idea to make > this version-checked? (i.e. only disable on gcc-12) That makes sense, so we still get at least some coverage for compilers < gcc 12; and also latest clang still seems to do the right thing. Sven, could you either send an updated patch, or an addon patch, please? Whatever you prefer. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-04-22 17:54 ` Kees Cook 2022-04-25 9:13 ` Heiko Carstens @ 2022-06-08 20:07 ` Linus Torvalds 2022-06-08 21:33 ` Kees Cook 1 sibling, 1 reply; 17+ messages in thread From: Linus Torvalds @ 2022-06-08 20:07 UTC (permalink / raw) To: Kees Cook Cc: Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List Coming back to this, because my rc2 week tends to be quiet as people take a breather, and as such a good time for me to do system upgrades. And gcc-12 dropped in Fedora 36, and shows problems on x86 too. So I suspect we'll have to disable -Warray-bounds globally on gcc-12, not just on s390. Unless Kees has patches ready to go already. Some of the warnings look potentially simple, ie struct mbus_dram_target_info; in <linux/mbus.h> has the comment * [..] Peripherals are * required to support at least 4 decode windows. and then as a result has int num_cs; struct mbus_dram_window { [..] } cs[4]; and that "cs[4]" looks just bogus - it can be a much larger array, the '4' is just a minimum. The real limit is that 'num_cs' one. But there's a *lot* of warnings, and many of them are due to this, and while some are obvious, others aren't. There are other things too in gcc-12 that seem half-baked. I was interested to see the new '-Wdangling-pointer' thing, but then when I looked at it, the two cases I looked at were just bogus, so .. Linus On Fri, Apr 22, 2022 at 10:54 AM Kees Cook <keescook@chromium.org> wrote: > > On Fri, Apr 22, 2022 at 03:43:08PM +0200, Sven Schnelle wrote: > > gcc-12 shows a lot of array bound warnings on s390. This is caused > > by our S390_lowcore macro: > > > > which uses an hardcoded address of 0. Wrapping that with > > absolute_pointer() works, but gcc no longer knows that a 12 bit > > instruction is sufficient to access lowcore. So it emits instructions > > like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a single load/store > > instruction. As s390 stores variables often read/written in lowcore, > > this is considered problematic. Therefore disable -Warray-bounds on > > s390 for now until there is a better real solution. > > > > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > > It looks like the source of this problem (the literal-values-treated-as-NULL) > is gcc-12 specific. From the discussions, it sounded like Jacob was > going to fix this "correctly" in gcc-13. It might be a good idea to make > this version-checked? (i.e. only disable on gcc-12) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-08 20:07 ` Linus Torvalds @ 2022-06-08 21:33 ` Kees Cook 2022-06-08 23:59 ` Linus Torvalds 0 siblings, 1 reply; 17+ messages in thread From: Kees Cook @ 2022-06-08 21:33 UTC (permalink / raw) To: Linus Torvalds Cc: Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List On Wed, Jun 08, 2022 at 01:07:05PM -0700, Linus Torvalds wrote: > Coming back to this, because my rc2 week tends to be quiet as people > take a breather, and as such a good time for me to do system upgrades. > > And gcc-12 dropped in Fedora 36, and shows problems on x86 too. > > So I suspect we'll have to disable -Warray-bounds globally on gcc-12, > not just on s390. > > Unless Kees has patches ready to go already. 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. > Some of the warnings look potentially simple, ie > > struct mbus_dram_target_info; > > in <linux/mbus.h> has the comment > > * [..] Peripherals are > * required to support at least 4 decode windows. > > and then as a result has > > int num_cs; > struct mbus_dram_window { > [..] > } cs[4]; > > and that "cs[4]" looks just bogus - it can be a much larger array, the > '4' is just a minimum. The real limit is that 'num_cs' one. > > But there's a *lot* of warnings, and many of them are due to this, and > while some are obvious, others aren't. When I did a count in -next 2 weeks ago, there were 182 warnings (x86 allmodconfig) from GCC 12 where 153 were from -Warray-bounds. Today we're now down to 80 total (61 from -Warray-bounds), so we've solved over half of them. > There are other things too in gcc-12 that seem half-baked. I was > interested to see the new '-Wdangling-pointer' thing, but then when I > looked at it, the two cases I looked at were just bogus, so .. Yes, GCC 12 is very odd in places. Besides the literal-as-pointer issue that still causes problems for s390[2], there seem to be at least a few other bugs associated with the internal diagnostics infrastructure that informs -Warray-bounds, -Wstringop-overflow, etc. I narrowed down one recently with UBSAN_BOUNDS[3] (which therefore impacts all*config builds), but there is no GCC fix yet. :( So, it's unclear to me if we want to try to get back to 0 warnings (where we were with v5.18 and GCC 11) in the next couple weeks, or if we need to just disable it for GCC 12 until everything is fixed again. -Kees [1] https://github.com/KSPP/linux/issues/190 [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578 [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679 -- Kees Cook ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-08 21:33 ` Kees Cook @ 2022-06-08 23:59 ` Linus Torvalds 2022-06-09 0:39 ` Kees Cook ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Linus Torvalds @ 2022-06-08 23:59 UTC (permalink / raw) To: Kees Cook, Philipp Zabel, Shawn Guo, Sascha Hauer, Fabio Estevam, David Howells Cc: Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List [-- 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 # ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-08 23:59 ` Linus Torvalds @ 2022-06-09 0:39 ` Kees Cook 2022-06-09 1:22 ` Linus Torvalds ` (2 more replies) 2022-06-09 9:56 ` Philipp Zabel 2022-06-09 14:55 ` Nathan Chancellor 2 siblings, 3 replies; 17+ messages in thread From: Kees Cook @ 2022-06-09 0:39 UTC (permalink / raw) To: Linus Torvalds, Philipp Zabel, Shawn Guo, Sascha Hauer, Fabio Estevam, David Howells Cc: Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List On June 8, 2022 4:59:29 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote: >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. Yup! :) > >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 I'll take a look; thanks! Should I send them back as a pull request? >but that still leaves the netfs_i_context games, which gcc-12 is very >unhappy about: Yeah. Happily, this has already been solved, but it looks like David didn't do a pull yet for it? https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-next >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? For gcc's UBSAN_SHIFT (I typoed this in my first reply) bug, netdev has been moving it to W=1 builds on a per-source basis for the moment: https://git.kernel.org/linus/e95032988053c17baf6c7e27024f5103a19a5f4a Some discussion: https://lore.kernel.org/lkml/202205231229.CF6B8471@keescook/ Perhaps these could be even more carefully limited to GCC 12 only, using the Kconfig you suggested? -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-09 0:39 ` Kees Cook @ 2022-06-09 1:22 ` Linus Torvalds 2022-06-09 9:56 ` Philipp Zabel 2022-06-09 14:14 ` David Howells 2 siblings, 0 replies; 17+ messages in thread From: Linus Torvalds @ 2022-06-09 1:22 UTC (permalink / raw) To: Kees Cook Cc: Philipp Zabel, Shawn Guo, Sascha Hauer, Fabio Estevam, David Howells, Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List On Wed, Jun 8, 2022 at 5:39 PM Kees Cook <keescook@chromium.org> wrote: > > I'll take a look; thanks! Should I send them back as a pull request? That would be good. > Yeah. Happily, this has already been solved, but it looks like David didn't do a pull yet for it? > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-next Good. > For gcc's UBSAN_SHIFT (I typoed this in my first reply) bug, netdev has been moving it to W=1 builds on a per-source basis for the moment: > > https://git.kernel.org/linus/e95032988053c17baf6c7e27024f5103a19a5f4a Ugh. That's sad. Since now the gcc-12 misfeature ends up biting everybody else too. > Perhaps these could be even more carefully limited to GCC 12 only, using the Kconfig you suggested? Yeah, I'd rather just say "gcc-12 gets this thing entirely wrong, let's disable it there" than disable it for compilers that get it right. In fact, I'd rather have that global "gcc-12 is broken, disable it", than marking "this file shouldn't get checked" kind of logic. It's wrong blaming the C code, when the compiler is doing bad sh*t. Linus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-09 0:39 ` Kees Cook 2022-06-09 1:22 ` Linus Torvalds @ 2022-06-09 9:56 ` Philipp Zabel 2022-06-09 13:02 ` Kees Cook 2022-06-09 14:14 ` David Howells 2 siblings, 1 reply; 17+ messages in thread From: Philipp Zabel @ 2022-06-09 9:56 UTC (permalink / raw) To: Kees Cook, Linus Torvalds, Shawn Guo, Sascha Hauer, Fabio Estevam, David Howells Cc: Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List Hi Kees, On Mi, 2022-06-08 at 17:39 -0700, Kees Cook wrote: [...] > > 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 > > I'll take a look; thanks! Should I send them back as a pull request? Does this refer to the whole patch, including (a) and (b), or am I to pick up the ipuv3-crtc.c fix? regards Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-09 9:56 ` Philipp Zabel @ 2022-06-09 13:02 ` Kees Cook 0 siblings, 0 replies; 17+ messages in thread From: Kees Cook @ 2022-06-09 13:02 UTC (permalink / raw) To: Philipp Zabel, Linus Torvalds, Shawn Guo, Sascha Hauer, Fabio Estevam, David Howells Cc: Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List On June 9, 2022 2:56:47 AM PDT, Philipp Zabel <p.zabel@pengutronix.de> wrote: >Hi Kees, > >On Mi, 2022-06-08 at 17:39 -0700, Kees Cook wrote: >[...] >> > 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 >> >> I'll take a look; thanks! Should I send them back as a pull request? > >Does this refer to the whole patch, including (a) and (b), or am I to >pick up the ipuv3-crtc.c fix? Go ahead and grab that one please; that's more "normal" :) Thanks! -- Kees Cook ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-09 0:39 ` Kees Cook 2022-06-09 1:22 ` Linus Torvalds 2022-06-09 9:56 ` Philipp Zabel @ 2022-06-09 14:14 ` David Howells 2022-06-09 18:20 ` Linus Torvalds 2 siblings, 1 reply; 17+ messages in thread From: David Howells @ 2022-06-09 14:14 UTC (permalink / raw) To: Linus Torvalds Cc: dhowells, dchinner, Kees Cook, Philipp Zabel, Shawn Guo, Sascha Hauer, Fabio Estevam, Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Yeah. Happily, this has already been solved, but it looks like David didn't do a pull yet for it? > > > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-next > > Good. Do you want it tagging and a pull req generating, even though it's a single patch? Note that Dave Chinner would rather I converted code like: struct myfs_inode *myfsinode = xyz; myfsinode->netfs.inode.i_ino = 123; to something like: struct myfs_inode *myfsinode = xyz; struct inode *inode = VFS_I(myfsinode); inode->i_ino = 123; where the translation is wrapped inside a VFS_I() macro in every filesystem and wants this across all filesystems. I think the former looks cleaner, but he has a point about how to deal with yet another layer of wrapping being inserted in the future. Do you have a preference? David ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-09 14:14 ` David Howells @ 2022-06-09 18:20 ` Linus Torvalds 2022-06-09 23:59 ` Dave Chinner 0 siblings, 1 reply; 17+ messages in thread From: Linus Torvalds @ 2022-06-09 18:20 UTC (permalink / raw) To: David Howells Cc: Dave Chinner, Kees Cook, Philipp Zabel, Shawn Guo, Sascha Hauer, Fabio Estevam, Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List On Thu, Jun 9, 2022 at 7:14 AM David Howells <dhowells@redhat.com> wrote: > > Note that Dave Chinner would rather I converted code like: > > struct myfs_inode *myfsinode = xyz; > myfsinode->netfs.inode.i_ino = 123; > > to something like: > > struct myfs_inode *myfsinode = xyz; > struct inode *inode = VFS_I(myfsinode); > inode->i_ino = 123; > > where the translation is wrapped inside a VFS_I() macro in every filesystem > and wants this across all filesystems. What? No. That's absolutely disgusting. Maybe I'm mis-undestanding. The usual way filesystems should handle this is that they have their own inode information that contains a 'struct inode', and then they have an inline function to go from that generic VFS inode to their one using "container_of()". And yeah, maybe they call that container_of() thing MYINODE() or something, although I think an inline function without the ugly all-uppercase is right. But the way they go the other way is literally to just dereference the inode that they have, ie they just use a if (S_ISREG(inode->vfs_inode.i_mode)) .. kind pattern. There's no reason or excuse to try to "wrap" that, and it would be a big step backwards to introduce some kind of VFS_I() macro. There's also no reason to make that generic. At no point should you ever go from "random filesystem inode" to "actual generic VFS inode" in some uncontrolled manner. But maybe Dave is talking about something else, and I'm missing the point. Linus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-09 18:20 ` Linus Torvalds @ 2022-06-09 23:59 ` Dave Chinner 2022-06-10 1:18 ` Linus Torvalds 0 siblings, 1 reply; 17+ messages in thread From: Dave Chinner @ 2022-06-09 23:59 UTC (permalink / raw) To: Linus Torvalds Cc: David Howells, Kees Cook, Philipp Zabel, Shawn Guo, Sascha Hauer, Fabio Estevam, Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List On Thu, Jun 09, 2022 at 11:20:02AM -0700, Linus Torvalds wrote: > On Thu, Jun 9, 2022 at 7:14 AM David Howells <dhowells@redhat.com> wrote: > > > > Note that Dave Chinner would rather I converted code like: > > > > struct myfs_inode *myfsinode = xyz; > > myfsinode->netfs.inode.i_ino = 123; > > > > to something like: > > > > struct myfs_inode *myfsinode = xyz; > > struct inode *inode = VFS_I(myfsinode); > > inode->i_ino = 123; > > > > where the translation is wrapped inside a VFS_I() macro in every filesystem > > and wants this across all filesystems. > > What? No. That's absolutely disgusting. > > Maybe I'm mis-undestanding. Perhaps, because I think what I said looks very different when taken out of context. I saw a heap of different implementations of the same thing with no consistency across them (i.e. inode container definitions) and a mess of a patch to convert them without solving the problem that there's no consistent convention for doing filesystem inode -> VFS inode container conversion > The usual way filesystems should handle this is that they have their > own inode information that contains a 'struct inode', and then they > have an inline function to go from that generic VFS inode to their one > using "container_of()". > > And yeah, maybe they call that container_of() thing MYINODE() or > something, although I think an inline function without the ugly > all-uppercase is right. Right, BTRFS_I(), EXT4_I(), F2FS_I(), AFS_FS_I(), P9FS_I(), etc. It's a convention, it dates back to macro days (hence upper case even though most are static inlines these days), and it obvious no matter what filesystem code I read that when I see this XXX_I(inode) convention I know the code is accessing the filesystem inode in the container, not the VFS indoe. > But the way they go the other way is literally to just dereference the > inode that they have, ie they just use a > > if (S_ISREG(inode->vfs_inode.i_mode)) .. The problem with this is that we have very similar names in both the VFS inode and the filesysetm inodes (e.g. i_flags), and without a clear demarcation of which inode is being referenced it can lead to confusion and bugs. > kind pattern. There's no reason or excuse to try to "wrap" that, and > it would be a big step backwards to introduce some kind of VFS_I() > macro. If the result of adding a helper convention is that every reverse inode container resolution looks identical across all filesystems, then we no longer have to know the details of the fs specific container to get the conversion right. All the code across all the filesystems would look the same, even though the wrapper would be different. We do helper conversions like this all the time to make the code easier to read, understand and maintain, so I really don't see why this would be considered a step backwards.... > There's also no reason to make that generic. At no point should you > ever go from "random filesystem inode" to "actual generic VFS inode" > in some uncontrolled manner. We never do any conversions in an uncontrolled manner. We often need to go from fs inode to vfs inode because we are deep in filesystem implementation code passing around filesystem inodes, but the piece of information we need to access is stored in the VFS inode (e.g. uid, gid, etc). That's what this netfs inode container was requiring in the patchset... > But maybe Dave is talking about something else, and I'm missing the point. Perhaps - my comment was not about the VFS_I() name or implementation; I used it simply because I can point at code that uses it as an example of having a symmetric, easily recognisable convention. My point was that the fs inode to vfs inode conversion is a common operation performed across all filesystems that lacks any consistency in implementation. Some filesystems use a symmetric API for these container conversions and so I was simply suggesting that converting them all to use a common symmetric convention would simplify the maintenance of filesystem code in future and make it easier for other people to understand... Cheers, Dave. -- Dave Chinner dchinner@redhat.com ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-09 23:59 ` Dave Chinner @ 2022-06-10 1:18 ` Linus Torvalds 0 siblings, 0 replies; 17+ messages in thread From: Linus Torvalds @ 2022-06-10 1:18 UTC (permalink / raw) To: Dave Chinner Cc: David Howells, Kees Cook, Philipp Zabel, Shawn Guo, Sascha Hauer, Fabio Estevam, Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List On Thu, Jun 9, 2022 at 4:59 PM Dave Chinner <dchinner@redhat.com> wrote: > > I saw a heap of different implementations of the same thing with no > consistency across them (i.e. inode container definitions) and a > mess of a patch to convert them without solving the problem that > there's no consistent convention for doing filesystem inode -> VFS > inode container conversion Sure. But the thing is, they aren't actually all the same. We do have a pattern - embed the generic vfs inode inside the filesystem-specific one - but the exact details of how you do it isn't fixed in stone. And this netfs thing is actually an example of why it *shouldn't* be fixed in stone, exactly because a netfs user doesn't want to just "embed the vfs inode" - it wants to embed something *else* that in turn embeds the vfs inode. So yes, most filesystems do similar things, but they aren't exactly the same. And they *could* be more different than they actually are (there's nothing that says you *have* to embed the generic VFS inode in the filesystem-specific one, it's just that we make it easy and it's a pattern that has been copied because it works really well) And yes, we could just enforce naming, and force everybody do use #define VFS_I(myino) (&(myino)->vfs_inode) but then we really would have been in trouble with this whole netfs embedded struct. And no, it wouldn't be some kind of insurmountable issue, using an unnamed union (so that "vfs_inode" would be the inode, and "netfs_inode" would be the bigger netfs inode+context) would have made it all work out. But I really don't see the point of trying to just force everybody to use the same name, and force people to use a common macro that doesn't really *buy* you anything. I think just writing 'inode->vfs_inode.i_mode' is very clear, and is particularly obvious in that there's no costly translation. 'VFS_I(inode)->i_mode' might be shorter to write, but that's mainly because of the ugly shortened macro name. If you want ugly short names, you could have called the inode member just 'vfs_i' in the first place. And yes, we could go even further, and just make the rule be that everybody should actually put the generic VFS inode struct at the beginning of the filesystem inode. I think people do that already in practice. Then we could maybe use some language tricks to make the filesystems get their own inode pointer directly as arguments, instead of getting a 'struct inode *" and having to do that struct ext4_inode_info *ei = EXT4_I(inode); at all. I suspect we'd have to use a macro with a cast at the op assignment time, which would be really ugly, though, but maybe there's some gcc language extension that allows that kind of thing. Anyway, my point is that yes, we could enforce tighter rules here, and make everybody match some particular pattern. But I don't think we'd actually benefit from it, and I think it would have just caused more pain in this situation, for example. Linus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-08 23:59 ` Linus Torvalds 2022-06-09 0:39 ` Kees Cook @ 2022-06-09 9:56 ` Philipp Zabel 2022-06-09 14:55 ` Nathan Chancellor 2 siblings, 0 replies; 17+ messages in thread From: Philipp Zabel @ 2022-06-09 9:56 UTC (permalink / raw) To: Linus Torvalds, Kees Cook, Shawn Guo, Sascha Hauer, Fabio Estevam, David Howells Cc: Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List Hi Linus, On Mi, 2022-06-08 at 16:59 -0700, Linus Torvalds wrote: > 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? Thank you, this fix clearly matches the original intention. Acked-by: Philipp Zabel <p.zabel@pengutronix.de> The mistake had no adverse effect since the following condition doesn't actually dereference the NULL pointer, but given the compiler warning this Fixes: eb8c88808c83 ("drm/imx: add deferred plane disabling") regards Philipp ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-08 23:59 ` Linus Torvalds 2022-06-09 0:39 ` Kees Cook 2022-06-09 9:56 ` Philipp Zabel @ 2022-06-09 14:55 ` Nathan Chancellor 2022-06-09 18:51 ` Linus Torvalds 2 siblings, 1 reply; 17+ messages in thread From: Nathan Chancellor @ 2022-06-09 14:55 UTC (permalink / raw) To: Linus Torvalds Cc: Kees Cook, Philipp Zabel, Shawn Guo, Sascha Hauer, Fabio Estevam, David Howells, Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List On Wed, Jun 08, 2022 at 04:59:29PM -0700, Linus Torvalds wrote: Just one small drive by comment in case this ends up going in in one form or another. > 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) I think this should be $(CONFIG_CC_NO_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 > # Cheers, Nathan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390: disable -Warray-bounds 2022-06-09 14:55 ` Nathan Chancellor @ 2022-06-09 18:51 ` Linus Torvalds 0 siblings, 0 replies; 17+ messages in thread From: Linus Torvalds @ 2022-06-09 18:51 UTC (permalink / raw) To: Nathan Chancellor Cc: Kees Cook, Philipp Zabel, Shawn Guo, Sascha Hauer, Fabio Estevam, David Howells, Sven Schnelle, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Linux Kernel Mailing List On Thu, Jun 9, 2022 at 7:55 AM Nathan Chancellor <nathan@kernel.org> wrote: > > I think this should be $(CONFIG_CC_NO_ARRAY_BOUNDS)? Thanks, fixed. Anyway, in order to deal with the (few - the rc2 week does tend to be small) pull requests I have pending, I have basically worked around all the new warnings I see. Some of the workarounds are the proper fixes, but mostly it's a pretty harsh "just shut that warning up". That includes for things that have proper fixes pending (ie the netfs issue), where I just did a pretty ugly but very localized #pragma GCC diagnostic ignored "-Wattribute-warning" in the affected files. End result: I have a clean 'allmodconfig' build again, and hopefully most of these workarounds can either be tightened up or removed entirely at some point. It's this in my tree now: 507160f46c55 ("netfs: gcc-12: temporarily disable '-Wattribute-warning' for now") f0be87c42cbd ("gcc-12: disable '-Warray-bounds' universally for now") 842c3b3ddc5f ("mellanox: mlx5: avoid uninitialized variable warning with gcc-12") 49beadbd47c2 ("gcc-12: disable '-Wdangling-pointer' warning for now") 7aefd8b53815 ("drm: imx: fix compiler warning with gcc-12") 6bfb56e93bce ("cert host tools: Stop complaining about deprecated OpenSSL functions") in case people care. Linus ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-06-10 1:19 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-22 13:43 [PATCH] s390: disable -Warray-bounds Sven Schnelle 2022-04-22 17:54 ` Kees Cook 2022-04-25 9:13 ` Heiko Carstens 2022-06-08 20:07 ` Linus Torvalds 2022-06-08 21:33 ` Kees Cook 2022-06-08 23:59 ` Linus Torvalds 2022-06-09 0:39 ` Kees Cook 2022-06-09 1:22 ` Linus Torvalds 2022-06-09 9:56 ` Philipp Zabel 2022-06-09 13:02 ` Kees Cook 2022-06-09 14:14 ` David Howells 2022-06-09 18:20 ` Linus Torvalds 2022-06-09 23:59 ` Dave Chinner 2022-06-10 1:18 ` Linus Torvalds 2022-06-09 9:56 ` Philipp Zabel 2022-06-09 14:55 ` Nathan Chancellor 2022-06-09 18:51 ` Linus Torvalds
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.