linux-kernel - Re: [PATCH v2 08/12] lib/crc_kunit.c: add KUnit test suite for CRC library functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMj1kXHAktbQ-605wfqXCWtn8bP-yEv8sYKWAykajeAX2m1hEA@mail.gmail.com>
Date: Sun, 23 Mar 2025 16:35:29 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Eric Biggers <ebiggers@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-arch@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-crypto@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, x86@...nel.org, 
	Zhihang Shao <zhihang.shao.iscas@...il.com>, Vinicius Peixoto <vpeixoto@...amp.dev>, 
	"Martin K. Petersen" <martin.petersen@...cle.com>
Subject: Re: [PATCH v2 08/12] lib/crc_kunit.c: add KUnit test suite for CRC
 library functions

On Sat, 22 Mar 2025 at 15:33, Guenter Roeck <linux@...ck-us.net> wrote:
>
> Hi,
>
> On Sun, Dec 01, 2024 at 05:20:52PM -0800, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@...gle.com>
> >
> > Add a KUnit test suite for the crc16, crc_t10dif, crc32_le, crc32_be,
> > crc32c, and crc64_be library functions.  It avoids code duplication by
> > sharing most logic among all CRC variants.  The test suite includes:
> >
> > - Differential fuzz test of each CRC function against a simple
> >   bit-at-a-time reference implementation.
> > - Test for CRC combination, when implemented by a CRC variant.
> > - Optional benchmark of each CRC function with various data lengths.
> >
> > This is intended as a replacement for crc32test and crc16_kunit, as well
> > as a new test for CRC variants which didn't previously have a test.
> >
> > Reviewed-by: Ard Biesheuvel <ardb@...nel.org>
> > Reviewed-by: Martin K. Petersen <martin.petersen@...cle.com>
> > Cc: Vinicius Peixoto <vpeixoto@...amp.dev>
> > Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> > ---
> ...
> > +
> > +             nosimd = rand32() % 8 == 0;
> > +
> > +             /*
> > +              * Compute the CRC, and verify that it equals the CRC computed
> > +              * by a simple bit-at-a-time reference implementation.
> > +              */
> > +             expected_crc = crc_ref(v, init_crc, &test_buffer[offset], len);
> > +             if (nosimd)
> > +                     local_irq_disable();
> > +             actual_crc = v->func(init_crc, &test_buffer[offset], len);
> > +             if (nosimd)
> > +                     local_irq_enable();
>
> This triggers a traceback on some arm systems.
>
> [    7.810000]     ok 2 crc16_benchmark # SKIP not enabled
> [    7.810000] ------------[ cut here ]------------
> [    7.810000] WARNING: CPU: 0 PID: 1145 at kernel/softirq.c:369 __local_bh_enable_ip+0x118/0x194
> [    7.810000] Modules linked in:
> [    7.810000] CPU: 0 UID: 0 PID: 1145 Comm: kunit_try_catch Tainted: G                 N 6.14.0-rc7-00196-g88d324e69ea9 #1
> [    7.810000] Tainted: [N]=TEST
> [    7.810000] Hardware name: NPCM7XX Chip family
> [    7.810000] Call trace:
> [    7.810000]  unwind_backtrace from show_stack+0x10/0x14
> [    7.810000]  show_stack from dump_stack_lvl+0x7c/0xac
> [    7.810000]  dump_stack_lvl from __warn+0x7c/0x1b8
> [    7.810000]  __warn from warn_slowpath_fmt+0x19c/0x1a4
> [    7.810000]  warn_slowpath_fmt from __local_bh_enable_ip+0x118/0x194
> [    7.810000]  __local_bh_enable_ip from crc_t10dif_arch+0xd4/0xe8
> [    7.810000]  crc_t10dif_arch from crc_t10dif_wrapper+0x14/0x1c
> [    7.810000]  crc_t10dif_wrapper from crc_main_test+0x178/0x360
> [    7.810000]  crc_main_test from kunit_try_run_case+0x78/0x1e0
> [    7.810000]  kunit_try_run_case from kunit_generic_run_threadfn_adapter+0x1c/0x34
> [    7.810000]  kunit_generic_run_threadfn_adapter from kthread+0x118/0x254
> [    7.810000]  kthread from ret_from_fork+0x14/0x28
> [    7.810000] Exception stack(0xe3651fb0 to 0xe3651ff8)
> [    7.810000] 1fa0:                                     00000000 00000000 00000000 00000000
> [    7.810000] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    7.810000] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    7.810000] irq event stamp: 29
> [    7.810000] hardirqs last  enabled at (27): [<c037875c>] __local_bh_enable_ip+0xb4/0x194
> [    7.810000] hardirqs last disabled at (28): [<c0b09684>] crc_main_test+0x2e4/0x360
> [    7.810000] softirqs last  enabled at (26): [<c032a3ac>] kernel_neon_end+0x0/0x1c
> [    7.810000] softirqs last disabled at (29): [<c032a3c8>] kernel_neon_begin+0x0/0x70
> [    7.810000] ---[ end trace 0000000000000000 ]---
> [    8.050000]     # crc_t10dif_test: pass:1 fail:0 skip:0 total:1
>
> kernel_neon_end() calls local_bh_enable() which apparently conflicts with
> the local_irq_disable() in above code.
>

This seems to be an oversight on my part. Can you try the below please?

diff --git a/arch/arm/include/asm/simd.h b/arch/arm/include/asm/simd.h
index 82191dbd7e78..56ddbd3c4997 100644
--- a/arch/arm/include/asm/simd.h
+++ b/arch/arm/include/asm/simd.h
@@ -4,5 +4,6 @@

 static __must_check inline bool may_use_simd(void)
 {
-       return IS_ENABLED(CONFIG_KERNEL_MODE_NEON) && !in_hardirq();
+       return IS_ENABLED(CONFIG_KERNEL_MODE_NEON) &&
+              !in_hardirq() && !irqs_disabled();
 }

However, this test code also appears to assume that SIMD is forbidden
on any architecture when IRQs are disabled, but this not guaranteed.