linux-hardening.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/6] kunit: build kunit tests without structleak plugin
@ 2021-09-17  6:10 Brendan Higgins
  2021-09-17  6:10 ` [PATCH v1 1/6] gcc-plugins/structleak: add makefile var for disabling structleak Brendan Higgins
                   ` (6 more replies)
  0 siblings, 7 replies; 23+ messages in thread
From: Brendan Higgins @ 2021-09-17  6:10 UTC (permalink / raw)
  To: shuah, davidgow, arnd, keescook, rafael, jic23, lars,
	ulf.hansson, andreas.noever, michael.jamet, mika.westerberg,
	YehezkelShB, masahiroy, michal.lkml, ndesaulniers
  Cc: linux-kselftest, kunit-dev, linux-kernel, torvalds, gregkh,
	linux-iio, linux-mmc, linux-usb, linux-hardening, linux-kbuild,
	Brendan Higgins

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=ELXeoXwunjv0f9frEN6HJODZng@mail.gmail.com/

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
-- 
2.33.0.464.g1972c5931b-goog


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 1/6] gcc-plugins/structleak: add makefile var for disabling structleak
  2021-09-17  6:10 [PATCH v1 0/6] kunit: build kunit tests without structleak plugin Brendan Higgins
@ 2021-09-17  6:10 ` Brendan Higgins
  2021-09-17 15:48   ` Kees Cook
  2021-09-17  6:11 ` [PATCH v1 2/6] iio/test-format: build kunit tests without structleak plugin Brendan Higgins
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Brendan Higgins @ 2021-09-17  6:10 UTC (permalink / raw)
  To: shuah, davidgow, arnd, keescook, rafael, jic23, lars,
	ulf.hansson, andreas.noever, michael.jamet, mika.westerberg,
	YehezkelShB, masahiroy, michal.lkml, ndesaulniers
  Cc: linux-kselftest, kunit-dev, linux-kernel, torvalds, gregkh,
	linux-iio, linux-mmc, linux-usb, linux-hardening, linux-kbuild,
	Brendan Higgins

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
 
-- 
2.33.0.464.g1972c5931b-goog


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 2/6] iio/test-format: build kunit tests without structleak plugin
  2021-09-17  6:10 [PATCH v1 0/6] kunit: build kunit tests without structleak plugin Brendan Higgins
  2021-09-17  6:10 ` [PATCH v1 1/6] gcc-plugins/structleak: add makefile var for disabling structleak Brendan Higgins
@ 2021-09-17  6:11 ` Brendan Higgins
  2021-09-17 15:54   ` Kees Cook
  2021-09-18 15:58   ` Jonathan Cameron
  2021-09-17  6:11 ` [PATCH v1 3/6] device property: " Brendan Higgins
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 23+ messages in thread
From: Brendan Higgins @ 2021-09-17  6:11 UTC (permalink / raw)
  To: shuah, davidgow, arnd, keescook, rafael, jic23, lars,
	ulf.hansson, andreas.noever, michael.jamet, mika.westerberg,
	YehezkelShB, masahiroy, michal.lkml, ndesaulniers
  Cc: linux-kselftest, kunit-dev, linux-kernel, torvalds, gregkh,
	linux-iio, linux-mmc, linux-usb, linux-hardening, linux-kbuild,
	Brendan Higgins

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)
-- 
2.33.0.464.g1972c5931b-goog


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 3/6] device property: build kunit tests without structleak plugin
  2021-09-17  6:10 [PATCH v1 0/6] kunit: build kunit tests without structleak plugin Brendan Higgins
  2021-09-17  6:10 ` [PATCH v1 1/6] gcc-plugins/structleak: add makefile var for disabling structleak Brendan Higgins
  2021-09-17  6:11 ` [PATCH v1 2/6] iio/test-format: build kunit tests without structleak plugin Brendan Higgins
@ 2021-09-17  6:11 ` Brendan Higgins
  2021-09-17 15:54   ` Kees Cook
  2021-09-17  6:11 ` [PATCH v1 4/6] thunderbolt: " Brendan Higgins
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 23+ messages in thread
From: Brendan Higgins @ 2021-09-17  6:11 UTC (permalink / raw)
  To: shuah, davidgow, arnd, keescook, rafael, jic23, lars,
	ulf.hansson, andreas.noever, michael.jamet, mika.westerberg,
	YehezkelShB, masahiroy, michal.lkml, ndesaulniers
  Cc: linux-kselftest, kunit-dev, linux-kernel, torvalds, gregkh,
	linux-iio, linux-mmc, linux-usb, linux-hardening, linux-kbuild,
	Brendan Higgins

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)
-- 
2.33.0.464.g1972c5931b-goog


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 4/6] thunderbolt: build kunit tests without structleak plugin
  2021-09-17  6:10 [PATCH v1 0/6] kunit: build kunit tests without structleak plugin Brendan Higgins
                   ` (2 preceding siblings ...)
  2021-09-17  6:11 ` [PATCH v1 3/6] device property: " Brendan Higgins
@ 2021-09-17  6:11 ` Brendan Higgins
  2021-09-17 10:16   ` Mika Westerberg
  2021-09-17 15:55   ` Kees Cook
  2021-09-17  6:11 ` [PATCH v1 5/6] mmc: sdhci-of-aspeed: " Brendan Higgins
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 23+ messages in thread
From: Brendan Higgins @ 2021-09-17  6:11 UTC (permalink / raw)
  To: shuah, davidgow, arnd, keescook, rafael, jic23, lars,
	ulf.hansson, andreas.noever, michael.jamet, mika.westerberg,
	YehezkelShB, masahiroy, michal.lkml, ndesaulniers
  Cc: linux-kselftest, kunit-dev, linux-kernel, torvalds, gregkh,
	linux-iio, linux-mmc, linux-usb, linux-hardening, linux-kbuild,
	Brendan Higgins

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
-- 
2.33.0.464.g1972c5931b-goog


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 5/6] mmc: sdhci-of-aspeed: build kunit tests without structleak plugin
  2021-09-17  6:10 [PATCH v1 0/6] kunit: build kunit tests without structleak plugin Brendan Higgins
                   ` (3 preceding siblings ...)
  2021-09-17  6:11 ` [PATCH v1 4/6] thunderbolt: " Brendan Higgins
@ 2021-09-17  6:11 ` Brendan Higgins
  2021-09-17 15:56   ` Kees Cook
  2021-09-17  6:11 ` [PATCH v1 6/6] bitfield: " Brendan Higgins
  2021-09-17  7:38 ` [PATCH v1 0/6] kunit: " Arnd Bergmann
  6 siblings, 1 reply; 23+ messages in thread
From: Brendan Higgins @ 2021-09-17  6:11 UTC (permalink / raw)
  To: shuah, davidgow, arnd, keescook, rafael, jic23, lars,
	ulf.hansson, andreas.noever, michael.jamet, mika.westerberg,
	YehezkelShB, masahiroy, michal.lkml, ndesaulniers
  Cc: linux-kselftest, kunit-dev, linux-kernel, torvalds, gregkh,
	linux-iio, linux-mmc, linux-usb, linux-hardening, linux-kbuild,
	Brendan Higgins

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
-- 
2.33.0.464.g1972c5931b-goog


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 6/6] bitfield: build kunit tests without structleak plugin
  2021-09-17  6:10 [PATCH v1 0/6] kunit: build kunit tests without structleak plugin Brendan Higgins
                   ` (4 preceding siblings ...)
  2021-09-17  6:11 ` [PATCH v1 5/6] mmc: sdhci-of-aspeed: " Brendan Higgins
@ 2021-09-17  6:11 ` Brendan Higgins
  2021-09-17  7:22   ` Arnd Bergmann
  2021-09-17  7:38 ` [PATCH v1 0/6] kunit: " Arnd Bergmann
  6 siblings, 1 reply; 23+ messages in thread
From: Brendan Higgins @ 2021-09-17  6:11 UTC (permalink / raw)
  To: shuah, davidgow, arnd, keescook, rafael, jic23, lars,
	ulf.hansson, andreas.noever, michael.jamet, mika.westerberg,
	YehezkelShB, masahiroy, michal.lkml, ndesaulniers
  Cc: linux-kselftest, kunit-dev, linux-kernel, torvalds, gregkh,
	linux-iio, linux-mmc, linux-usb, linux-hardening, linux-kbuild,
	Brendan Higgins

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
-- 
2.33.0.464.g1972c5931b-goog


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 6/6] bitfield: build kunit tests without structleak plugin
  2021-09-17  6:11 ` [PATCH v1 6/6] bitfield: " Brendan Higgins
@ 2021-09-17  7:22   ` Arnd Bergmann
  2021-09-17 15:57     ` Kees Cook
  2021-09-29 21:04     ` Brendan Higgins
  0 siblings, 2 replies; 23+ messages in thread
From: Arnd Bergmann @ 2021-09-17  7:22 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: Shuah Khan, David Gow, Arnd Bergmann, Kees Cook, Rafael Wysocki,
	Jonathan Cameron, Lars-Peter Clausen, Ulf Hansson,
	andreas.noever, michael.jamet, Mika Westerberg, YehezkelShB,
	Masahiro Yamada, Michal Marek, Nick Desaulniers,
	open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
	Linux Kernel Mailing List, Linus Torvalds, gregkh, linux-iio,
	linux-mmc, USB list, linux-hardening, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 0/6] kunit: build kunit tests without structleak plugin
  2021-09-17  6:10 [PATCH v1 0/6] kunit: build kunit tests without structleak plugin Brendan Higgins
                   ` (5 preceding siblings ...)
  2021-09-17  6:11 ` [PATCH v1 6/6] bitfield: " Brendan Higgins
@ 2021-09-17  7:38 ` Arnd Bergmann
  2021-09-29 20:46   ` Brendan Higgins
  6 siblings, 1 reply; 23+ messages in thread
From: Arnd Bergmann @ 2021-09-17  7:38 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: Shuah Khan, David Gow, Arnd Bergmann, Kees Cook, Rafael Wysocki,
	Jonathan Cameron, Lars-Peter Clausen, Ulf Hansson,
	andreas.noever, michael.jamet, Mika Westerberg, YehezkelShB,
	Masahiro Yamada, Michal Marek, Nick Desaulniers,
	open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
	Linux Kernel Mailing List, Linus Torvalds, gregkh, linux-iio,
	linux-mmc, USB list, linux-hardening, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 4/6] thunderbolt: build kunit tests without structleak plugin
  2021-09-17  6:11 ` [PATCH v1 4/6] thunderbolt: " Brendan Higgins
@ 2021-09-17 10:16   ` Mika Westerberg
  2021-09-17 15:55   ` Kees Cook
  1 sibling, 0 replies; 23+ messages in thread
From: Mika Westerberg @ 2021-09-17 10:16 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: shuah, davidgow, arnd, keescook, rafael, jic23, lars,
	ulf.hansson, andreas.noever, michael.jamet, YehezkelShB,
	masahiroy, michal.lkml, ndesaulniers, linux-kselftest, kunit-dev,
	linux-kernel, torvalds, gregkh, linux-iio, linux-mmc, linux-usb,
	linux-hardening, linux-kbuild

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>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 1/6] gcc-plugins/structleak: add makefile var for disabling structleak
  2021-09-17  6:10 ` [PATCH v1 1/6] gcc-plugins/structleak: add makefile var for disabling structleak Brendan Higgins
@ 2021-09-17 15:48   ` Kees Cook
  2021-09-29 20:25     ` Brendan Higgins
  0 siblings, 1 reply; 23+ messages in thread
From: Kees Cook @ 2021-09-17 15:48 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: shuah, davidgow, arnd, rafael, jic23, lars, ulf.hansson,
	andreas.noever, michael.jamet, mika.westerberg, YehezkelShB,
	masahiroy, michal.lkml, ndesaulniers, linux-kselftest, kunit-dev,
	linux-kernel, torvalds, gregkh, linux-iio, linux-mmc, linux-usb,
	linux-hardening, linux-kbuild

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
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/6] iio/test-format: build kunit tests without structleak plugin
  2021-09-17  6:11 ` [PATCH v1 2/6] iio/test-format: build kunit tests without structleak plugin Brendan Higgins
@ 2021-09-17 15:54   ` Kees Cook
  2021-09-29 20:50     ` Brendan Higgins
  2021-09-18 15:58   ` Jonathan Cameron
  1 sibling, 1 reply; 23+ messages in thread
From: Kees Cook @ 2021-09-17 15:54 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: shuah, davidgow, arnd, rafael, jic23, lars, ulf.hansson,
	andreas.noever, michael.jamet, mika.westerberg, YehezkelShB,
	masahiroy, michal.lkml, ndesaulniers, linux-kselftest, kunit-dev,
	linux-kernel, torvalds, gregkh, linux-iio, linux-mmc, linux-usb,
	linux-hardening, linux-kbuild

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
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 3/6] device property: build kunit tests without structleak plugin
  2021-09-17  6:11 ` [PATCH v1 3/6] device property: " Brendan Higgins
@ 2021-09-17 15:54   ` Kees Cook
  0 siblings, 0 replies; 23+ messages in thread
From: Kees Cook @ 2021-09-17 15:54 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: shuah, davidgow, arnd, rafael, jic23, lars, ulf.hansson,
	andreas.noever, michael.jamet, mika.westerberg, YehezkelShB,
	masahiroy, michal.lkml, ndesaulniers, linux-kselftest, kunit-dev,
	linux-kernel, torvalds, gregkh, linux-iio, linux-mmc, linux-usb,
	linux-hardening, linux-kbuild

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>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 4/6] thunderbolt: build kunit tests without structleak plugin
  2021-09-17  6:11 ` [PATCH v1 4/6] thunderbolt: " Brendan Higgins
  2021-09-17 10:16   ` Mika Westerberg
@ 2021-09-17 15:55   ` Kees Cook
  1 sibling, 0 replies; 23+ messages in thread
From: Kees Cook @ 2021-09-17 15:55 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: shuah, davidgow, arnd, rafael, jic23, lars, ulf.hansson,
	andreas.noever, michael.jamet, mika.westerberg, YehezkelShB,
	masahiroy, michal.lkml, ndesaulniers, linux-kselftest, kunit-dev,
	linux-kernel, torvalds, gregkh, linux-iio, linux-mmc, linux-usb,
	linux-hardening, linux-kbuild

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>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 5/6] mmc: sdhci-of-aspeed: build kunit tests without structleak plugin
  2021-09-17  6:11 ` [PATCH v1 5/6] mmc: sdhci-of-aspeed: " Brendan Higgins
@ 2021-09-17 15:56   ` Kees Cook
  2021-09-17 18:40     ` Linus Torvalds
  0 siblings, 1 reply; 23+ messages in thread
From: Kees Cook @ 2021-09-17 15:56 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: shuah, davidgow, arnd, rafael, jic23, lars, ulf.hansson,
	andreas.noever, michael.jamet, mika.westerberg, YehezkelShB,
	masahiroy, michal.lkml, ndesaulniers, linux-kselftest, kunit-dev,
	linux-kernel, torvalds, gregkh, linux-iio, linux-mmc, linux-usb,
	linux-hardening, linux-kbuild

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
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 6/6] bitfield: build kunit tests without structleak plugin
  2021-09-17  7:22   ` Arnd Bergmann
@ 2021-09-17 15:57     ` Kees Cook
  2021-09-29 21:04     ` Brendan Higgins
  1 sibling, 0 replies; 23+ messages in thread
From: Kees Cook @ 2021-09-17 15:57 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Brendan Higgins, Shuah Khan, David Gow, Rafael Wysocki,
	Jonathan Cameron, Lars-Peter Clausen, Ulf Hansson,
	andreas.noever, michael.jamet, Mika Westerberg, YehezkelShB,
	Masahiro Yamada, Michal Marek, Nick Desaulniers,
	open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
	Linux Kernel Mailing List, Linus Torvalds, gregkh, linux-iio,
	linux-mmc, USB list, linux-hardening, Linux Kbuild mailing list

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>


-- 
Kees Cook

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 5/6] mmc: sdhci-of-aspeed: build kunit tests without structleak plugin
  2021-09-17 15:56   ` Kees Cook
@ 2021-09-17 18:40     ` Linus Torvalds
  2021-09-29 20:59       ` Brendan Higgins
  0 siblings, 1 reply; 23+ messages in thread
From: Linus Torvalds @ 2021-09-17 18:40 UTC (permalink / raw)
  To: Kees Cook
  Cc: Brendan Higgins, Shuah Khan, David Gow, Arnd Bergmann,
	Rafael Wysocki, Jonathan Cameron, Lars-Peter Clausen,
	Ulf Hansson, andreas.noever, michael.jamet, Mika Westerberg,
	YehezkelShB, Masahiro Yamada, Michal Marek, Nick Desaulniers,
	open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
	Linux Kernel Mailing List, Greg Kroah-Hartman, linux-iio,
	linux-mmc, linux-usb, linux-hardening, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/6] iio/test-format: build kunit tests without structleak plugin
  2021-09-17  6:11 ` [PATCH v1 2/6] iio/test-format: build kunit tests without structleak plugin Brendan Higgins
  2021-09-17 15:54   ` Kees Cook
@ 2021-09-18 15:58   ` Jonathan Cameron
  1 sibling, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2021-09-18 15:58 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: shuah, davidgow, arnd, keescook, rafael, lars, ulf.hansson,
	andreas.noever, michael.jamet, mika.westerberg, YehezkelShB,
	masahiroy, michal.lkml, ndesaulniers, linux-kselftest, kunit-dev,
	linux-kernel, torvalds, gregkh, linux-iio, linux-mmc, linux-usb,
	linux-hardening, linux-kbuild

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)


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 1/6] gcc-plugins/structleak: add makefile var for disabling structleak
  2021-09-17 15:48   ` Kees Cook
@ 2021-09-29 20:25     ` Brendan Higgins
  0 siblings, 0 replies; 23+ messages in thread
From: Brendan Higgins @ 2021-09-29 20:25 UTC (permalink / raw)
  To: Kees Cook
  Cc: shuah, davidgow, arnd, rafael, jic23, lars, ulf.hansson,
	andreas.noever, michael.jamet, mika.westerberg, yehezkelshb,
	masahiroy, michal.lkml, ndesaulniers, linux-kselftest, kunit-dev,
	linux-kernel, torvalds, gregkh, linux-iio, linux-mmc, linux-usb,
	linux-hardening, linux-kbuild

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!

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 0/6] kunit: build kunit tests without structleak plugin
  2021-09-17  7:38 ` [PATCH v1 0/6] kunit: " Arnd Bergmann
@ 2021-09-29 20:46   ` Brendan Higgins
  0 siblings, 0 replies; 23+ messages in thread
From: Brendan Higgins @ 2021-09-29 20:46 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Shuah Khan, David Gow, Kees Cook, Rafael Wysocki,
	Jonathan Cameron, Lars-Peter Clausen, Ulf Hansson,
	andreas.noever, michael.jamet, Mika Westerberg, yehezkelshb,
	Masahiro Yamada, Michal Marek, Nick Desaulniers,
	open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
	Linux Kernel Mailing List, Linus Torvalds, gregkh, linux-iio,
	linux-mmc, USB list, linux-hardening, Linux Kbuild mailing list

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!

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/6] iio/test-format: build kunit tests without structleak plugin
  2021-09-17 15:54   ` Kees Cook
@ 2021-09-29 20:50     ` Brendan Higgins
  0 siblings, 0 replies; 23+ messages in thread
From: Brendan Higgins @ 2021-09-29 20:50 UTC (permalink / raw)
  To: Kees Cook
  Cc: shuah, davidgow, arnd, rafael, jic23, lars, ulf.hansson,
	andreas.noever, michael.jamet, mika.westerberg, yehezkelshb,
	masahiroy, michal.lkml, ndesaulniers, linux-kselftest, kunit-dev,
	linux-kernel, torvalds, gregkh, linux-iio, linux-mmc, linux-usb,
	linux-hardening, linux-kbuild

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 5/6] mmc: sdhci-of-aspeed: build kunit tests without structleak plugin
  2021-09-17 18:40     ` Linus Torvalds
@ 2021-09-29 20:59       ` Brendan Higgins
  0 siblings, 0 replies; 23+ messages in thread
From: Brendan Higgins @ 2021-09-29 20:59 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Kees Cook, Shuah Khan, David Gow, Arnd Bergmann, Rafael Wysocki,
	Jonathan Cameron, Lars-Peter Clausen, Ulf Hansson,
	andreas.noever, michael.jamet, Mika Westerberg, yehezkelshb,
	Masahiro Yamada, Michal Marek, Nick Desaulniers,
	open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
	Linux Kernel Mailing List, Greg Kroah-Hartman, linux-iio,
	linux-mmc, linux-usb, linux-hardening, Linux Kbuild mailing list

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!

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 6/6] bitfield: build kunit tests without structleak plugin
  2021-09-17  7:22   ` Arnd Bergmann
  2021-09-17 15:57     ` Kees Cook
@ 2021-09-29 21:04     ` Brendan Higgins
  1 sibling, 0 replies; 23+ messages in thread
From: Brendan Higgins @ 2021-09-29 21:04 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Shuah Khan, David Gow, Kees Cook, Rafael Wysocki,
	Jonathan Cameron, Lars-Peter Clausen, Ulf Hansson,
	andreas.noever, michael.jamet, Mika Westerberg, yehezkelshb,
	Masahiro Yamada, Michal Marek, Nick Desaulniers,
	open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
	Linux Kernel Mailing List, Linus Torvalds, gregkh, linux-iio,
	linux-mmc, USB list, linux-hardening, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2021-09-29 21:04 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17  6:10 [PATCH v1 0/6] kunit: build kunit tests without structleak plugin Brendan Higgins
2021-09-17  6:10 ` [PATCH v1 1/6] gcc-plugins/structleak: add makefile var for disabling structleak Brendan Higgins
2021-09-17 15:48   ` Kees Cook
2021-09-29 20:25     ` Brendan Higgins
2021-09-17  6:11 ` [PATCH v1 2/6] iio/test-format: build kunit tests without structleak plugin Brendan Higgins
2021-09-17 15:54   ` Kees Cook
2021-09-29 20:50     ` Brendan Higgins
2021-09-18 15:58   ` Jonathan Cameron
2021-09-17  6:11 ` [PATCH v1 3/6] device property: " Brendan Higgins
2021-09-17 15:54   ` Kees Cook
2021-09-17  6:11 ` [PATCH v1 4/6] thunderbolt: " Brendan Higgins
2021-09-17 10:16   ` Mika Westerberg
2021-09-17 15:55   ` Kees Cook
2021-09-17  6:11 ` [PATCH v1 5/6] mmc: sdhci-of-aspeed: " Brendan Higgins
2021-09-17 15:56   ` Kees Cook
2021-09-17 18:40     ` Linus Torvalds
2021-09-29 20:59       ` Brendan Higgins
2021-09-17  6:11 ` [PATCH v1 6/6] bitfield: " Brendan Higgins
2021-09-17  7:22   ` Arnd Bergmann
2021-09-17 15:57     ` Kees Cook
2021-09-29 21:04     ` Brendan Higgins
2021-09-17  7:38 ` [PATCH v1 0/6] kunit: " Arnd Bergmann
2021-09-29 20:46   ` Brendan Higgins

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).