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

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Mar 23, 2025 at 04:35:29PM +0100, Ard Biesheuvel wrote:
> On Sat, 22 Mar 2025 at 15:33, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > Hi,
> >
> > On Sun, Dec 01, 2024 at 05:20:52PM -0800, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.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@kernel.org>
> > > Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
> > > Cc: Vinicius Peixoto <vpeixoto@lkcamp.dev>
> > > Signed-off-by: Eric Biggers <ebiggers@google.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();
>  }

Thanks Ard, you beat me to it.  Yes, may_use_simd() needs to be consistent with
kernel_neon_begin().  On x86 there is a case where the equivalent function is
expected to work when irqs_disabled(), but if there is no such case on arm this
fix looks good.  Can you send it out as a formal patch?  Presumably for the arm
maintainer to pick up.

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

Yes, to reliably test the no-SIMD code paths, I need to finish refactoring the
crypto_simd_disabled_for_test stuff to be disentangled from the crypto subsystem
so that crc_kunit.c can use it.  It's on my list of things to do, and I'm
planning to get it done in 6.16.  Disabling hardirqs is just a trick to get
there more easily on some architectures.  But as this shows it's a useful test
to have anyway, so we'll want to keep that too.  The CRC functions need to work
in any context, and any context that we can easily test we should do so.

- Eric