The structleak plugin causes the stack frame size to grow immensely when used with KUnit; this is caused because KUnit allocates lots of moderately sized structs on the stack as part of its assertion macro implementation. For most tests with small to moderately sized tests cases there are never enough KUnit assertions to be an issue at all; even when a single test cases has many KUnit assertions, the compiler should never put all these struct allocations on the stack at the same time since the scope of the structs is so limited; however, the structleak plugin does not seem to respect the compiler doing the right thing and will still warn of excessive stack size in some cases.
These patches are not a permanent solution since new tests can be added with huge test cases, but this serves as a stop gap to stop structleak from being used on KUnit tests which will currently result in excessive stack size.
Of the following patches, I think the thunderbolt patch may be unnecessary since Linus already fixed that test. Additionally, I was not able to reproduce the error on the sdhci-of-aspeed test. Nevertheless, I included these tests cases for completeness. Please see my discussion with Arnd for more context[1].
NOTE: Arnd did the legwork for most of these patches, but did not actually share code for some of them, so I left his Signed-off-by off of those patches as I don't want to misrepresent him. Arnd, please sign off on those patches at your soonest convenience.
[1] https://lore.kernel.org/linux-arm-kernel/CAFd5g44udqkDiYBWh+VeDVJ=ELXeoXwunj...
Arnd Bergmann (1): bitfield: build kunit tests without structleak plugin
Brendan Higgins (5): gcc-plugins/structleak: add makefile var for disabling structleak iio/test-format: build kunit tests without structleak plugin device property: build kunit tests without structleak plugin thunderbolt: build kunit tests without structleak plugin mmc: sdhci-of-aspeed: build kunit tests without structleak plugin
drivers/base/test/Makefile | 2 +- drivers/iio/test/Makefile | 1 + drivers/mmc/host/Makefile | 1 + drivers/thunderbolt/Makefile | 1 + lib/Makefile | 2 +- scripts/Makefile.gcc-plugins | 4 ++++ 6 files changed, 9 insertions(+), 2 deletions(-)
base-commit: 316346243be6df12799c0b64b788e06bad97c30b
KUnit and structleak don't play nice, so add a makefile variable for enabling structleak when it complains.
Co-developed-by: Kees Cook keescook@chromium.org Signed-off-by: Brendan Higgins brendanhiggins@google.com --- scripts/Makefile.gcc-plugins | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins index 952e46876329a..4aad284800355 100644 --- a/scripts/Makefile.gcc-plugins +++ b/scripts/Makefile.gcc-plugins @@ -19,6 +19,10 @@ gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF) \ += -fplugin-arg-structleak_plugin-byref gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL) \ += -fplugin-arg-structleak_plugin-byref-all +ifdef CONFIG_GCC_PLUGIN_STRUCTLEAK + DISABLE_STRUCTLEAK_PLUGIN += -fplugin-arg-structleak_plugin-disable +endif +export DISABLE_STRUCTLEAK_PLUGIN gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK) \ += -DSTRUCTLEAK_PLUGIN
On Thu, Sep 16, 2021 at 11:10:59PM -0700, Brendan Higgins wrote:
KUnit and structleak don't play nice, so add a makefile variable for enabling structleak when it complains.
Co-developed-by: Kees Cook keescook@chromium.org
For a C-d-b, also include a S-o-b:
Signed-off-by: Kees Cook keescook@chromium.org
But otherwise, yes, this is good. :)
-Kees
Signed-off-by: Brendan Higgins brendanhiggins@google.com
scripts/Makefile.gcc-plugins | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins index 952e46876329a..4aad284800355 100644 --- a/scripts/Makefile.gcc-plugins +++ b/scripts/Makefile.gcc-plugins @@ -19,6 +19,10 @@ gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF) \ += -fplugin-arg-structleak_plugin-byref gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL) \ += -fplugin-arg-structleak_plugin-byref-all +ifdef CONFIG_GCC_PLUGIN_STRUCTLEAK
- DISABLE_STRUCTLEAK_PLUGIN += -fplugin-arg-structleak_plugin-disable
+endif +export DISABLE_STRUCTLEAK_PLUGIN gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK) \ += -DSTRUCTLEAK_PLUGIN -- 2.33.0.464.g1972c5931b-goog
On Fri, Sep 17, 2021 at 8:48 AM Kees Cook keescook@chromium.org wrote:
On Thu, Sep 16, 2021 at 11:10:59PM -0700, Brendan Higgins wrote:
KUnit and structleak don't play nice, so add a makefile variable for enabling structleak when it complains.
Co-developed-by: Kees Cook keescook@chromium.org
For a C-d-b, also include a S-o-b:
Signed-off-by: Kees Cook keescook@chromium.org
But otherwise, yes, this is good. :)
Yeah, I know that's necessary for the patch to be accepted, but in this case, I don't think your original version of this (it wasn't actually a patch) had a S-o-b on it, so I didn't want to say that you had signed off on something that you didn't.
I have run into this situation before and handled it this way - letting the co-developer sign off on the list. Is this something I should avoid in the future?
In any case, I will resubmit this now that I have your S-o-b.
Thanks!
The structleak plugin causes the stack frame size to grow immensely when used with KUnit:
../drivers/iio/test/iio-test-format.c: In function ‘iio_test_iio_format_value_fixedpoint’: ../drivers/iio/test/iio-test-format.c:98:1: warning: the frame size of 2336 bytes is larger than 2048 bytes [-Wframe-larger-than=]
Turn it off in this file.
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com --- drivers/iio/test/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile index f1099b4953014..467519a2027e5 100644 --- a/drivers/iio/test/Makefile +++ b/drivers/iio/test/Makefile @@ -5,3 +5,4 @@
# Keep in alphabetical order obj-$(CONFIG_IIO_TEST_FORMAT) += iio-test-format.o +CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN)
On Thu, Sep 16, 2021 at 11:11:00PM -0700, Brendan Higgins wrote:
The structleak plugin causes the stack frame size to grow immensely when used with KUnit:
../drivers/iio/test/iio-test-format.c: In function ‘iio_test_iio_format_value_fixedpoint’: ../drivers/iio/test/iio-test-format.c:98:1: warning: the frame size of 2336 bytes is larger than 2048 bytes [-Wframe-larger-than=]
Turn it off in this file.
Given that these are all for KUnit tests, is it possible there are going to be other CONFIGs that will interact poorly (e.g. KASAN)? Maybe there needs to be a small level of indirection with something like:
DISABLE_UNDER_KUNIT := $(DISABLE_STRUCTLEAK_PLUGIN) export DISABLE_UNDER_KUNIT
then all of these become:
+CFLAGS_iio-test-format.o += $(DISABLE_UNDER_KUNIT)
Either way, I think these are fine to add.
Reviewed-by: Kees Cook keescook@chromium.org
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com
drivers/iio/test/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile index f1099b4953014..467519a2027e5 100644 --- a/drivers/iio/test/Makefile +++ b/drivers/iio/test/Makefile @@ -5,3 +5,4 @@ # Keep in alphabetical order obj-$(CONFIG_IIO_TEST_FORMAT) += iio-test-format.o
+CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN)
2.33.0.464.g1972c5931b-goog
On Fri, Sep 17, 2021 at 8:54 AM Kees Cook keescook@chromium.org wrote:
On Thu, Sep 16, 2021 at 11:11:00PM -0700, Brendan Higgins wrote:
The structleak plugin causes the stack frame size to grow immensely when used with KUnit:
../drivers/iio/test/iio-test-format.c: In function ‘iio_test_iio_format_value_fixedpoint’: ../drivers/iio/test/iio-test-format.c:98:1: warning: the frame size of 2336 bytes is larger than 2048 bytes [-Wframe-larger-than=]
Turn it off in this file.
Given that these are all for KUnit tests, is it possible there are going to be other CONFIGs that will interact poorly (e.g. KASAN)? Maybe there needs to be a small level of indirection with something like:
I don't think so. KASAN actually uses KUnit to test it, and we have experimented running KASAN alongside other KUnit tests for added protection and results have looked good.
I would be surprised if there are other CONFIGs other than things dealing with stack size that don't like KUnit.
DISABLE_UNDER_KUNIT := $(DISABLE_STRUCTLEAK_PLUGIN) export DISABLE_UNDER_KUNIT
then all of these become:
+CFLAGS_iio-test-format.o += $(DISABLE_UNDER_KUNIT)
Either way, I think these are fine to add.
I like your suggestion if we find other configs that don't like KUnit, but I don't think we have seen any others so far, so I think I will keep it as it is for now.
Reviewed-by: Kees Cook keescook@chromium.org
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com
drivers/iio/test/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile index f1099b4953014..467519a2027e5 100644 --- a/drivers/iio/test/Makefile +++ b/drivers/iio/test/Makefile @@ -5,3 +5,4 @@
# Keep in alphabetical order obj-$(CONFIG_IIO_TEST_FORMAT) += iio-test-format.o
+CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN)
2.33.0.464.g1972c5931b-goog
-- Kees Cook
On Thu, 16 Sep 2021 23:11:00 -0700 Brendan Higgins brendanhiggins@google.com wrote:
The structleak plugin causes the stack frame size to grow immensely when used with KUnit:
../drivers/iio/test/iio-test-format.c: In function ‘iio_test_iio_format_value_fixedpoint’: ../drivers/iio/test/iio-test-format.c:98:1: warning: the frame size of 2336 bytes is larger than 2048 bytes [-Wframe-larger-than=]
Turn it off in this file.
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com
Acked-by: Jonathan Cameron Jonathan.Cameron@huawei.com
drivers/iio/test/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/iio/test/Makefile b/drivers/iio/test/Makefile index f1099b4953014..467519a2027e5 100644 --- a/drivers/iio/test/Makefile +++ b/drivers/iio/test/Makefile @@ -5,3 +5,4 @@ # Keep in alphabetical order obj-$(CONFIG_IIO_TEST_FORMAT) += iio-test-format.o +CFLAGS_iio-test-format.o += $(DISABLE_STRUCTLEAK_PLUGIN)
The structleak plugin causes the stack frame size to grow immensely when used with KUnit:
../drivers/base/test/property-entry-test.c:492:1: warning: the frame size of 2832 bytes is larger than 2048 bytes [-Wframe-larger-than=] ../drivers/base/test/property-entry-test.c:322:1: warning: the frame size of 2080 bytes is larger than 2048 bytes [-Wframe-larger-than=] ../drivers/base/test/property-entry-test.c:250:1: warning: the frame size of 4976 bytes is larger than 2048 bytes [-Wframe-larger-than=] ../drivers/base/test/property-entry-test.c:115:1: warning: the frame size of 3280 bytes is larger than 2048 bytes [-Wframe-larger-than=]
Turn it off in this file.
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com --- drivers/base/test/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile index 64b2f3d744d51..7f76fee6f989d 100644 --- a/drivers/base/test/Makefile +++ b/drivers/base/test/Makefile @@ -2,4 +2,4 @@ obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o
obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o -CFLAGS_REMOVE_property-entry-test.o += -fplugin-arg-structleak_plugin-byref -fplugin-arg-structleak_plugin-byref-all +CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
On Thu, Sep 16, 2021 at 11:11:01PM -0700, Brendan Higgins wrote:
The structleak plugin causes the stack frame size to grow immensely when used with KUnit:
../drivers/base/test/property-entry-test.c:492:1: warning: the frame size of 2832 bytes is larger than 2048 bytes [-Wframe-larger-than=] ../drivers/base/test/property-entry-test.c:322:1: warning: the frame size of 2080 bytes is larger than 2048 bytes [-Wframe-larger-than=] ../drivers/base/test/property-entry-test.c:250:1: warning: the frame size of 4976 bytes is larger than 2048 bytes [-Wframe-larger-than=] ../drivers/base/test/property-entry-test.c:115:1: warning: the frame size of 3280 bytes is larger than 2048 bytes [-Wframe-larger-than=]
Turn it off in this file.
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com
Reviewed-by: Kees Cook keescook@chromium.org
The structleak plugin causes the stack frame size to grow immensely when used with KUnit:
drivers/thunderbolt/test.c:1529:1: error: the frame size of 1176 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
Turn it off in this file.
Linus already split up tests in this file, so this change *should* be redundant now.
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com --- drivers/thunderbolt/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile index da19d7987d005..78fd365893c13 100644 --- a/drivers/thunderbolt/Makefile +++ b/drivers/thunderbolt/Makefile @@ -7,6 +7,7 @@ thunderbolt-objs += usb4_port.o nvm.o retimer.o quirks.o thunderbolt-${CONFIG_ACPI} += acpi.o thunderbolt-$(CONFIG_DEBUG_FS) += debugfs.o thunderbolt-${CONFIG_USB4_KUNIT_TEST} += test.o +CFLAGS_test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
thunderbolt_dma_test-${CONFIG_USB4_DMA_TEST} += dma_test.o obj-$(CONFIG_USB4_DMA_TEST) += thunderbolt_dma_test.o
On Thu, Sep 16, 2021 at 11:11:02PM -0700, Brendan Higgins wrote:
The structleak plugin causes the stack frame size to grow immensely when used with KUnit:
drivers/thunderbolt/test.c:1529:1: error: the frame size of 1176 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
Turn it off in this file.
Linus already split up tests in this file, so this change *should* be redundant now.
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com
Acked-by: Mika Westerberg mika.westerberg@linux.intel.com
On Thu, Sep 16, 2021 at 11:11:02PM -0700, Brendan Higgins wrote:
The structleak plugin causes the stack frame size to grow immensely when used with KUnit:
drivers/thunderbolt/test.c:1529:1: error: the frame size of 1176 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
Turn it off in this file.
Linus already split up tests in this file, so this change *should* be redundant now.
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Arnd Bergmann arnd@arndb.de
Reviewed-by: Kees Cook keescook@chromium.org
The structleak plugin causes the stack frame size to grow immensely when used with KUnit.
Turn it off.
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com --- drivers/mmc/host/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile index 14004cc09aaad..2ab083931f8fd 100644 --- a/drivers/mmc/host/Makefile +++ b/drivers/mmc/host/Makefile @@ -85,6 +85,7 @@ obj-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o obj-$(CONFIG_MMC_SDHCI_TEGRA) += sdhci-tegra.o obj-$(CONFIG_MMC_SDHCI_OF_ARASAN) += sdhci-of-arasan.o obj-$(CONFIG_MMC_SDHCI_OF_ASPEED) += sdhci-of-aspeed.o +CFLAGS_sdhci-of-aspeed.o += $(DISABLE_STRUCTLEAK_PLUGIN) obj-$(CONFIG_MMC_SDHCI_OF_AT91) += sdhci-of-at91.o obj-$(CONFIG_MMC_SDHCI_OF_ESDHC) += sdhci-of-esdhc.o obj-$(CONFIG_MMC_SDHCI_OF_HLWD) += sdhci-of-hlwd.o
On Thu, Sep 16, 2021 at 11:11:03PM -0700, Brendan Higgins wrote:
The structleak plugin causes the stack frame size to grow immensely when used with KUnit.
Turn it off.
Co-developed-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com
drivers/mmc/host/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile index 14004cc09aaad..2ab083931f8fd 100644 --- a/drivers/mmc/host/Makefile +++ b/drivers/mmc/host/Makefile @@ -85,6 +85,7 @@ obj-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o obj-$(CONFIG_MMC_SDHCI_TEGRA) += sdhci-tegra.o obj-$(CONFIG_MMC_SDHCI_OF_ARASAN) += sdhci-of-arasan.o obj-$(CONFIG_MMC_SDHCI_OF_ASPEED) += sdhci-of-aspeed.o +CFLAGS_sdhci-of-aspeed.o += $(DISABLE_STRUCTLEAK_PLUGIN)
This isn't a stand-alone test object, so I'm less excited about disabling STRUCTLEAK here.
obj-$(CONFIG_MMC_SDHCI_OF_AT91) += sdhci-of-at91.o obj-$(CONFIG_MMC_SDHCI_OF_ESDHC) += sdhci-of-esdhc.o obj-$(CONFIG_MMC_SDHCI_OF_HLWD) += sdhci-of-hlwd.o -- 2.33.0.464.g1972c5931b-goog
On Fri, Sep 17, 2021 at 8:57 AM Kees Cook keescook@chromium.org wrote:
This isn't a stand-alone test object, so I'm less excited about disabling STRUCTLEAK here.
Yeah, please don't do this for things that aren't pure tests. You're now disabling security measures (even if I hate the gcc plugins and hope they will go away).
Linus
On Fri, Sep 17, 2021 at 11:40 AM Linus Torvalds torvalds@linux-foundation.org wrote:
On Fri, Sep 17, 2021 at 8:57 AM Kees Cook keescook@chromium.org wrote:
This isn't a stand-alone test object, so I'm less excited about disabling STRUCTLEAK here.
Yeah, please don't do this for things that aren't pure tests. You're now disabling security measures (even if I hate the gcc plugins and hope they will go away).
Oh, whoops, yeah, I shouldn't do that. I am just going to drop this patch entirely, as I wasn't able to reproduce the stack frame size issue on qemu anyway (as I mentioned on the cover letter).
Thanks for catching this.
Sorry!
From: Arnd Bergmann arnd@arndb.de
The structleak plugin causes the stack frame size to grow immensely:
lib/bitfield_kunit.c: In function 'test_bitfields_constants': lib/bitfield_kunit.c:93:1: error: the frame size of 7440 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
Turn it off in this file.
Signed-off-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com --- lib/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/Makefile b/lib/Makefile index 5efd1b435a37c..c93c4b59af969 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -351,7 +351,7 @@ obj-$(CONFIG_OBJAGG) += objagg.o obj-$(CONFIG_PLDMFW) += pldmfw/
# KUnit tests -CFLAGS_bitfield_kunit.o := $(call cc-option,-Wframe-larger-than=10240) +CFLAGS_bitfield_kunit.o := $(call cc-option,-Wframe-larger-than=10240) $(DISABLE_STRUCTLEAK_PLUGIN) obj-$(CONFIG_BITFIELD_KUNIT) += bitfield_kunit.o obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
On Fri, Sep 17, 2021 at 8:11 AM Brendan Higgins brendanhiggins@google.com wrote:
From: Arnd Bergmann arnd@arndb.de
The structleak plugin causes the stack frame size to grow immensely:
lib/bitfield_kunit.c: In function 'test_bitfields_constants': lib/bitfield_kunit.c:93:1: error: the frame size of 7440 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
Turn it off in this file.
Signed-off-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com
lib/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/Makefile b/lib/Makefile index 5efd1b435a37c..c93c4b59af969 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -351,7 +351,7 @@ obj-$(CONFIG_OBJAGG) += objagg.o obj-$(CONFIG_PLDMFW) += pldmfw/
# KUnit tests -CFLAGS_bitfield_kunit.o := $(call cc-option,-Wframe-larger-than=10240) +CFLAGS_bitfield_kunit.o := $(call cc-option,-Wframe-larger-than=10240) $(DISABLE_STRUCTLEAK_PLUGIN)
I think the $(call cc-option,-Wframe-larger-than=10240) needs to be dropped here. This was not in my original patch and it is definitely broken on all architectures with 8KB stack size or less if the function needs that much. What is the amount of actual stack usage you observe without this? If we still get a warning, then I think this needs to be fixed in the code.
Arnd
On Fri, Sep 17, 2021 at 09:22:08AM +0200, Arnd Bergmann wrote:
On Fri, Sep 17, 2021 at 8:11 AM Brendan Higgins brendanhiggins@google.com wrote:
From: Arnd Bergmann arnd@arndb.de
The structleak plugin causes the stack frame size to grow immensely:
lib/bitfield_kunit.c: In function 'test_bitfields_constants': lib/bitfield_kunit.c:93:1: error: the frame size of 7440 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
Turn it off in this file.
Signed-off-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com
lib/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/Makefile b/lib/Makefile index 5efd1b435a37c..c93c4b59af969 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -351,7 +351,7 @@ obj-$(CONFIG_OBJAGG) += objagg.o obj-$(CONFIG_PLDMFW) += pldmfw/
# KUnit tests -CFLAGS_bitfield_kunit.o := $(call cc-option,-Wframe-larger-than=10240) +CFLAGS_bitfield_kunit.o := $(call cc-option,-Wframe-larger-than=10240) $(DISABLE_STRUCTLEAK_PLUGIN)
I think the $(call cc-option,-Wframe-larger-than=10240) needs to be dropped here. This was not in my original patch and it is definitely broken on all architectures with 8KB stack size or less if the function needs that much. What is the amount of actual stack usage you observe without this? If we still get a warning, then I think this needs to be fixed in the code.
With the frame-larger-than dropped:
Reviewed-by: Kees Cook keescook@chromium.org
On Fri, Sep 17, 2021 at 12:22 AM Arnd Bergmann arnd@arndb.de wrote:
On Fri, Sep 17, 2021 at 8:11 AM Brendan Higgins brendanhiggins@google.com wrote:
From: Arnd Bergmann arnd@arndb.de
The structleak plugin causes the stack frame size to grow immensely:
lib/bitfield_kunit.c: In function 'test_bitfields_constants': lib/bitfield_kunit.c:93:1: error: the frame size of 7440 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
Turn it off in this file.
Signed-off-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Brendan Higgins brendanhiggins@google.com
lib/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/Makefile b/lib/Makefile index 5efd1b435a37c..c93c4b59af969 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -351,7 +351,7 @@ obj-$(CONFIG_OBJAGG) += objagg.o obj-$(CONFIG_PLDMFW) += pldmfw/
# KUnit tests -CFLAGS_bitfield_kunit.o := $(call cc-option,-Wframe-larger-than=10240) +CFLAGS_bitfield_kunit.o := $(call cc-option,-Wframe-larger-than=10240) $(DISABLE_STRUCTLEAK_PLUGIN)
I think the $(call cc-option,-Wframe-larger-than=10240) needs to be dropped here. This was not in my original patch and it is definitely broken on
Ah, someone else put that there, so I just left it, but I can drop it.
all architectures with 8KB stack size or less if the function needs that much. What is the amount of actual stack usage you observe without this?
Well STRUCTLEAK claims 7440 bytes, but I don't entirely believe that. Regardless, it is definitely less than 8KB.
If we still get a warning, then I think this needs to be fixed in the code.
Arnd
Cheers
On Fri, Sep 17, 2021 at 8:10 AM Brendan Higgins brendanhiggins@google.com wrote:
The structleak plugin causes the stack frame size to grow immensely when used with KUnit; this is caused because KUnit allocates lots of moderately sized structs on the stack as part of its assertion macro implementation. For most tests with small to moderately sized tests cases there are never enough KUnit assertions to be an issue at all; even when a single test cases has many KUnit assertions, the compiler should never put all these struct allocations on the stack at the same time since the scope of the structs is so limited; however, the structleak plugin does not seem to respect the compiler doing the right thing and will still warn of excessive stack size in some cases.
These patches are not a permanent solution since new tests can be added with huge test cases, but this serves as a stop gap to stop structleak from being used on KUnit tests which will currently result in excessive stack size.
Of the following patches, I think the thunderbolt patch may be unnecessary since Linus already fixed that test. Additionally, I was not able to reproduce the error on the sdhci-of-aspeed test. Nevertheless, I included these tests cases for completeness. Please see my discussion with Arnd for more context[1].
NOTE: Arnd did the legwork for most of these patches, but did not actually share code for some of them, so I left his Signed-off-by off of those patches as I don't want to misrepresent him. Arnd, please sign off on those patches at your soonest convenience.
Thanks a lot for picking up this work where I dropped the ball.
Patches 1-5 look good to me, and I replied on one remaining issue I see with patch 6. I think you did more work on these that I did, by doing a nice write-up and splitting them into separate patches with useful changelogs, you should keep authorship, and just change my S-o-b to Suggested-by.
If you prefer to keep me as the author, then the correct way would be to commit them with --author= to ensure that the author and first s-o-b match.
Arnd
On Fri, Sep 17, 2021 at 12:38 AM Arnd Bergmann arnd@arndb.de wrote:
On Fri, Sep 17, 2021 at 8:10 AM Brendan Higgins brendanhiggins@google.com wrote:
The structleak plugin causes the stack frame size to grow immensely when used with KUnit; this is caused because KUnit allocates lots of moderately sized structs on the stack as part of its assertion macro implementation. For most tests with small to moderately sized tests cases there are never enough KUnit assertions to be an issue at all; even when a single test cases has many KUnit assertions, the compiler should never put all these struct allocations on the stack at the same time since the scope of the structs is so limited; however, the structleak plugin does not seem to respect the compiler doing the right thing and will still warn of excessive stack size in some cases.
These patches are not a permanent solution since new tests can be added with huge test cases, but this serves as a stop gap to stop structleak from being used on KUnit tests which will currently result in excessive stack size.
Of the following patches, I think the thunderbolt patch may be unnecessary since Linus already fixed that test. Additionally, I was not able to reproduce the error on the sdhci-of-aspeed test. Nevertheless, I included these tests cases for completeness. Please see my discussion with Arnd for more context[1].
NOTE: Arnd did the legwork for most of these patches, but did not actually share code for some of them, so I left his Signed-off-by off of those patches as I don't want to misrepresent him. Arnd, please sign off on those patches at your soonest convenience.
Thanks a lot for picking up this work where I dropped the ball.
Patches 1-5 look good to me, and I replied on one remaining issue I see with patch 6. I think you did more work on these that I did, by doing a nice write-up and splitting them into separate patches with useful changelogs, you should keep authorship, and just change my S-o-b to Suggested-by.
If you prefer to keep me as the author, then the correct way would be to commit them with --author= to ensure that the author and first s-o-b match.
Sounds good. I will keep the one that has you as the author since I just rebased it, but I will move you to Suggested-by on the others.
Thanks!
linux-kselftest-mirror@lists.linaro.org
-
Arnd Bergmann -
Brendan Higgins -
Jonathan Cameron -
Kees Cook -
Linus Torvalds -
Mika Westerberg