diff mbox series

crypto/ecc: increase frame warning limit

Message ID 20240712113656.30422-1-flyingpeng@tencent.com (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series crypto/ecc: increase frame warning limit | expand

Commit Message

Hao Peng July 12, 2024, 11:36 a.m. UTC 
From: Peng Hao <flyingpeng@tencent.com>

When building kernel with clang, which will typically
have sanitizers enabled, there is a warning about a large stack frame.

crypto/ecc.c:1129:13: error: stack frame size (2136) exceeds limit (2048) in 'ecc_point_double_jacobian' [-Werror,-Wframe-larger-than]
static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 *z1,
            ^

Since many arrays are defined in ecc_point_double_jacobian, they occupy a
lot of stack space, but are difficult to adjust. just increase the limit
for configurations that have KASAN or KCSAN enabled.

Signed-off-by: Peng Hao <flyingpeng@tencent.com>
---
 crypto/Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Ard Biesheuvel July 13, 2024, 10:23 a.m. UTC | #1 
(cc Arnd, Nathan)

On Fri, 12 Jul 2024 at 13:37, <flyingpenghao@gmail.com> wrote:
>
> From: Peng Hao <flyingpeng@tencent.com>
>
> When building kernel with clang, which will typically
> have sanitizers enabled, there is a warning about a large stack frame.
>
> crypto/ecc.c:1129:13: error: stack frame size (2136) exceeds limit (2048) in 'ecc_point_double_jacobian' [-Werror,-Wframe-larger-than]
> static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 *z1,
>             ^
>
> Since many arrays are defined in ecc_point_double_jacobian, they occupy a
> lot of stack space, but are difficult to adjust. just increase the limit
> for configurations that have KASAN or KCSAN enabled.
>
> Signed-off-by: Peng Hao <flyingpeng@tencent.com>
> ---
>  crypto/Makefile | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/crypto/Makefile b/crypto/Makefile
> index edbbaa3ffef5..ab7bebaa7218 100644
> --- a/crypto/Makefile
> +++ b/crypto/Makefile
> @@ -190,6 +190,12 @@ obj-$(CONFIG_CRYPTO_ECC) += ecc.o
>  obj-$(CONFIG_CRYPTO_ESSIV) += essiv.o
>  obj-$(CONFIG_CRYPTO_CURVE25519) += curve25519-generic.o
>
> +ifneq ($(CONFIG_FRAME_WARN),0)
> +ifeq ($(filter y,$(CONFIG_KASAN)$(CONFIG_KCSAN)),y)
> +CFLAGS_ecc.o = -Wframe-larger-than=2776
> +endif
> +endif
> +

I don't think this is an acceptable workaround - this applies to all
functions in the file, which call into each other as well.

It would be better if we could figure out why the stack blows up like
this on clang, which could be related to inlining behavior or other
compiler heuristics that prevent stack allocations from being reused.

Arnd, Nathan: any insights?
Nathan Chancellor July 13, 2024, 11:31 p.m. UTC | #2 
Hi Ard,

On Sat, Jul 13, 2024 at 12:23:24PM +0200, Ard Biesheuvel wrote:
> On Fri, 12 Jul 2024 at 13:37, <flyingpenghao@gmail.com> wrote:
> >
> > From: Peng Hao <flyingpeng@tencent.com>
> >
> > When building kernel with clang, which will typically
> > have sanitizers enabled, there is a warning about a large stack frame.
> >
> > crypto/ecc.c:1129:13: error: stack frame size (2136) exceeds limit (2048) in 'ecc_point_double_jacobian' [-Werror,-Wframe-larger-than]
> > static void ecc_point_double_jacobian(u64 *x1, u64 *y1, u64 *z1,
> >             ^
> >
> > Since many arrays are defined in ecc_point_double_jacobian, they occupy a
> > lot of stack space, but are difficult to adjust. just increase the limit
> > for configurations that have KASAN or KCSAN enabled.
> >
> > Signed-off-by: Peng Hao <flyingpeng@tencent.com>
> > ---
> >  crypto/Makefile | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/crypto/Makefile b/crypto/Makefile
> > index edbbaa3ffef5..ab7bebaa7218 100644
> > --- a/crypto/Makefile
> > +++ b/crypto/Makefile
> > @@ -190,6 +190,12 @@ obj-$(CONFIG_CRYPTO_ECC) += ecc.o
> >  obj-$(CONFIG_CRYPTO_ESSIV) += essiv.o
> >  obj-$(CONFIG_CRYPTO_CURVE25519) += curve25519-generic.o
> >
> > +ifneq ($(CONFIG_FRAME_WARN),0)
> > +ifeq ($(filter y,$(CONFIG_KASAN)$(CONFIG_KCSAN)),y)
> > +CFLAGS_ecc.o = -Wframe-larger-than=2776
> > +endif
> > +endif
> > +
> 
> I don't think this is an acceptable workaround - this applies to all
> functions in the file, which call into each other as well.

Agreed, as I stated on a review of another patch this same submitter
sent for a similar warning in infiniband:

https://lore.kernel.org/20240709212608.GA1649561@thelio-3990X/

Unfortunately, this appears to be getting copy and pasted from AMDGPU,
which needed it for their horrible DCN code that comes from some other
upstream source.

> It would be better if we could figure out why the stack blows up like
> this on clang, which could be related to inlining behavior or other
> compiler heuristics that prevent stack allocations from being reused.

I asked for further details like configuration in my second message in
that thread and it never materialized:

https://lore.kernel.org/20240709212608.GA1649561@thelio-3990X/

I suspect this might just be a KASAN_STACK warning, which is known to
generate high stack usage warnings (as noted in the Kconfig text),we
will have to wait for confirmation:

https://github.com/ClangBuiltLinux/linux/issues/39

Cheers,
Nathan
diff mbox series

Patch

diff --git a/crypto/Makefile b/crypto/Makefile
index edbbaa3ffef5..ab7bebaa7218 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -190,6 +190,12 @@  obj-$(CONFIG_CRYPTO_ECC) += ecc.o
 obj-$(CONFIG_CRYPTO_ESSIV) += essiv.o
 obj-$(CONFIG_CRYPTO_CURVE25519) += curve25519-generic.o
 
+ifneq ($(CONFIG_FRAME_WARN),0)
+ifeq ($(filter y,$(CONFIG_KASAN)$(CONFIG_KCSAN)),y)
+CFLAGS_ecc.o = -Wframe-larger-than=2776
+endif
+endif
+
 ecdh_generic-y += ecdh.o
 ecdh_generic-y += ecdh_helper.o
 obj-$(CONFIG_CRYPTO_ECDH) += ecdh_generic.o