[PATCH] Kconfig.debug: disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default

[PATCH] Kconfig.debug: disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default

[Tudor Ambarus]

Building with clang-15 a x86_64_defconfig kernel were CONFIG_KASAN
and CONFIG_KASAN_STACK are enabled resulted in the following errors:

drivers/block/loop.c:1531:12: error: stack frame size (2616) exceeds limit (2048) in 'lo_ioctl'
drivers/gpu/drm/i915/gt/intel_workarounds.c:964:6: error: stack frame size (3032) exceeds limit (2048) in 'intel_engine_init_ctx_wa'
drivers/gpu/drm/i915/gt/intel_workarounds.c:1818:6: error: stack frame size (5496) exceeds limit (2048) in 'intel_gt_init_workarounds'
drivers/gpu/drm/i915/gt/intel_workarounds.c:3153:6: error: stack frame size (5848) exceeds limit (2048) in 'intel_engine_init_workarounds'
drivers/usb/core/devio.c:2801:13: error: stack frame size (2104) exceeds limit (2048) in 'usbdev_ioctl'

With Clang, stack instrumentation has a problem that causes excessive
stack usage, see https://bugs.llvm.org/show_bug.cgi?id=38809.
KASAN_STACK with clang is deemed unsafe and disabled when
compile-testing. However when !COMPILE_TEST the errors are hit because
CONFIG_FRAME_WARN is not disabled.

Looking into the errors, they are indeed caused by compiling with clang
and KASAN_STACK enabled. I determined KASAN_STACK's bloat by lowering
the CONFIG_FRAME_WARN limit. Thus when KASAN and KASAN_STACK are
disabled the same stack frames have the following sizes:

drivers/block/loop.c:1531:12: error: stack frame size (528) exceeds limit (50) in 'lo_ioctl'
drivers/gpu/drm/i915/gt/intel_workarounds.c:964:6: error: stack frame size (72) exceeds limit (50) in 'intel_engine_init_ctx_wa'
drivers/gpu/drm/i915/gt/intel_workarounds.c:1818:6: error: stack frame size (104) exceeds limit (50) in 'intel_gt_init_workarounds'
drivers/gpu/drm/i915/gt/intel_workarounds.c:3153:6: error: stack frame size (88) exceeds limit (50) in 'intel_engine_init_workarounds'
drivers/usb/core/devio.c:2801:13: error: stack frame size (416) exceeds limit (50) in 'usbdev_ioctl'

When KASAN is enabled and KASAN_STACK is disabled the same stack frames
have the following sizes:

drivers/block/loop.c:1531:12: error: stack frame size (600) exceeds limit (50) in 'lo_ioctl'
drivers/gpu/drm/i915/gt/intel_workarounds.c:964:6: error: stack frame size (120) exceeds limit (50) in 'intel_engine_init_ctx_wa'
drivers/gpu/drm/i915/gt/intel_workarounds.c:1818:6: error: stack frame size (136) exceeds limit (50) in 'intel_gt_init_workarounds'
drivers/gpu/drm/i915/gt/intel_workarounds.c:3153:6: error: stack frame size (128) exceeds limit (50) in 'intel_engine_init_workarounds'
drivers/usb/core/devio.c:2801:13: error: stack frame size (480) exceeds limit (50) in 'usbdev_ioctl'

The conclusion is that when KASAN is enabled the stack usage increases a
bit, but nothing unmanageable ~30-70 bytes, whereas when enabling
KASAN_STACK the stack usage is excessive, from ~1.7K to ~5.8K for these
cases.

Disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default.
Adventurers can still override the default value by input prompt or
explicit values in defconfigs in case they feel that some real warnings
are missed.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 lib/Kconfig.debug | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 39d1d93164bd..c5e8b76737af 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -430,6 +430,7 @@ config FRAME_WARN
 	int "Warn for stack frames larger than"
 	range 0 8192
 	default 0 if KMSAN
+	default 0 if KASAN_STACK && CC_IS_CLANG
 	default 2048 if GCC_PLUGIN_LATENT_ENTROPY
 	default 2048 if PARISC
 	default 1536 if (!64BIT && XTENSA)
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH] Kconfig.debug: disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default

[Arnd Bergmann]

On Fri, Apr 21, 2023, at 15:01, Tudor Ambarus wrote:

> The conclusion is that when KASAN is enabled the stack usage increases a
> bit, but nothing unmanageable ~30-70 bytes, whereas when enabling
> KASAN_STACK the stack usage is excessive, from ~1.7K to ~5.8K for these
> cases.
>
> Disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default.
> Adventurers can still override the default value by input prompt or
> explicit values in defconfigs in case they feel that some real warnings
> are missed.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

I think we are still better off with the warning enabled. When someone
really wants to run a kernel with KASAN_STACK enabled, they should have
a chance to see what they are getting into and not report any runtime
bugs that come from stack overflow. 

      Arnd

Re: [PATCH] Kconfig.debug: disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default

[Tudor Ambarus]

Hi, Arnd,

On 4/21/23 15:30, Arnd Bergmann wrote:
> On Fri, Apr 21, 2023, at 15:01, Tudor Ambarus wrote:
> 
>> The conclusion is that when KASAN is enabled the stack usage increases a
>> bit, but nothing unmanageable ~30-70 bytes, whereas when enabling
>> KASAN_STACK the stack usage is excessive, from ~1.7K to ~5.8K for these
>> cases.
>>
>> Disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default.
>> Adventurers can still override the default value by input prompt or
>> explicit values in defconfigs in case they feel that some real warnings
>> are missed.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> 
> I think we are still better off with the warning enabled. When someone
> really wants to run a kernel with KASAN_STACK enabled, they should have
> a chance to see what they are getting into and not report any runtime
> bugs that come from stack overflow. 
> 

Are such stack overflows warnings reliable when we know that stack
instrumentation causes excessive stack usage? Until clang is fixed one
shall hunt frame-larger-than warnings with KASAN_STACK disabled.

Cheers,
ta

Re: [PATCH] Kconfig.debug: disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default

[Arnd Bergmann]

On Fri, Apr 21, 2023, at 17:10, Tudor Ambarus wrote:
> On 4/21/23 15:30, Arnd Bergmann wrote:
>> On Fri, Apr 21, 2023, at 15:01, Tudor Ambarus wrote:
>> 
>>> The conclusion is that when KASAN is enabled the stack usage increases a
>>> bit, but nothing unmanageable ~30-70 bytes, whereas when enabling
>>> KASAN_STACK the stack usage is excessive, from ~1.7K to ~5.8K for these
>>> cases.
>>>
>>> Disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default.
>>> Adventurers can still override the default value by input prompt or
>>> explicit values in defconfigs in case they feel that some real warnings
>>> are missed.
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> 
>> I think we are still better off with the warning enabled. When someone
>> really wants to run a kernel with KASAN_STACK enabled, they should have
>> a chance to see what they are getting into and not report any runtime
>> bugs that come from stack overflow. 
>> 
>
> Are such stack overflows warnings reliable when we know that stack
> instrumentation causes excessive stack usage? Until clang is fixed one
> shall hunt frame-larger-than warnings with KASAN_STACK disabled.

The main problem with stack frame usage is that the compiler cannot know
during compile time what the other functions are going to use, so it's
always a bit of guesswork, and we just try to make the limit as small
as possible without causing too much work addressing the warnings.

Only if the frame usage grows beyond the actual available stack size,
it's guaranteed to crash (I've seen 80KB for a single function, clearly
larger than 16kb of total stack size), but the larger it gets, the
more problems you'll run into.

We already discussed years ago how clang can improve a lot of the
cases here by reworking the amount of padding between variables
similar to how gcc does it. This won't work for all files, but for
a lot of them, and once clang gets fixed, we can address the
remaining ones by working around them in the kernel.

     Arnd

Re: [PATCH] Kconfig.debug: disable CONFIG_FRAME_WARN for KASAN_STACK && CC_IS_CLANG by default

[Arnd Bergmann]

On Fri, Apr 21, 2023, at 17:28, Arnd Bergmann wrote:
> On Fri, Apr 21, 2023, at 17:10, Tudor Ambarus wrote:
>> On 4/21/23 15:30, Arnd Bergmann wrote:
>
> The main problem with stack frame usage is that the compiler cannot know
> during compile time what the other functions are going to use, so it's
> always a bit of guesswork, and we just try to make the limit as small
> as possible without causing too much work addressing the warnings.

Hi Tudor,

One follow-up here regarding the risk of actually overflowing the
stack on production systems: There is a possible feature that I've
discussed with Ard in the past, if we add a user-configurable stack offset
for syscall entry, it becomes possible to test kernels with an
artificially low stack size to find out the most critical call chain
that one can hit from user space.

You mentioned elsewhere that you are using syzkaller for testing for bugs,
and this would be the perfect way to exercise the modified kernels as
well, since it can hit all kinds of unusual call chains.

We already call add_random_kstack_offset() on four architectures (arm64,
powerpc, s390 and x86), and the same location could add a fixed offset
that is configurable e.g. system-wide using sysctl() or per task using
prctl(), depending on what makes this more useful for testing.

When CONFIG_VMAP_STACK is set, the result should be a process crash
with a full call chain printed to see how it got this bad, similar to
what you get with a KASAN violation. This probably makes most sense
with KASAN_STACK disabled to see which functions are the most critical
in real systems, though testing with KASAN_STACK enabled could also
give some hints at which of the warnings you see are worth fixing first.

If you would like to run tests with this, I might be able to come
up with a prototype patch for it.

       Arnd