Re: [PATCH V4 8/8] riscv: Add config of thread stack size

[Arnd Bergmann]

On Mon, Sep 12, 2022, at 6:14 AM, Guo Ren wrote:
> On Mon, Sep 12, 2022 at 2:40 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> On Sun, Sep 11, 2022, at 1:35 AM, Guo Ren wrote:
>> > On Sun, Sep 11, 2022 at 12:07 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> >>
>> >> That sounds like a really bad idea, why would you want to risk
>> >> using such a small stack without CONFIG_VMAP_STACK?
>> >>
>> >> Are you worried about increased memory usage or something else?
>> > The requirement is from [1], and I think disabling CONFIG_VMAP_STACK
>> > would be the last step after serious testing.
>>
>> I still don't see why you need to turn off VMAP_STACK at all
>> if it works. The only downside I can see with using VMAP_STACK
>> on a 64-bit system is that it may expose bugs with device
>> drivers that do DMA to stack data. Once you have tested the
>> system successfully, you can also assume that you have no such
>> drivers.
> 1st, VMAP_STACK could be enabled&disabled in arch/Kconfig. If we don't
> force users to enable VMAP_STACK, why couldn't user adjust
> THREAD_SIZE?

Turning off VMAP_STACK is harmless and may help debug issues
with VMAP_STACK itself. It's also required on architectures
that don't have KASAN_VMALLOC or something else that conflicts
with it.

Changing THREAD_SIZE is also fine, as long as VMAP_STACK catches
the inevitable overflows. I would not object to having an
option that allows setting the stack size larger than the
default without VMAP_STACK, as long as setting it lower requires
using VMAP_STACK. That would however add a lot more complexity
and probably doesn't do what you want either.

> 2nd, VMAP_STACK is not free, we still need 1KB shadow_stack.
> The EXPERT is enough for your concern.

It's actually more than the 1KB: you need both 1KB of shadow
stack and 4KB per CPU for the actual overflow_stack. If you
are micro-optimizing at this level, then a possible option
may be to change the handle_kernel_stack_overflow() function
to not preserve the task stack and just panic() without
showing the backtrace. That way you don't see which code
caused the issue, but at least you avoid corrupting random
data.

     Arnd