On Sun, 23 Mar 2025 at 18:12, Eric Biggers <ebigg...@kernel.org> wrote:
>
> On Sun, Mar 23, 2025 at 04:35:29PM +0100, Ard Biesheuvel wrote:
> > On Sat, 22 Mar 2025 at 15:33, Guenter Roeck <li...@roeck-us.net> wrote:
> > >
> > > Hi,
> > >
> > > On Sun, Dec 01, 2024 at 05:20:52PM -0800, Eric Biggers wrote:
> > > > From: Eric Biggers <ebigg...@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 <a...@kernel.org>
> > > > Reviewed-by: Martin K. Petersen <martin.peter...@oracle.com>
> > > > Cc: Vinicius Peixoto <vpeix...@lkcamp.dev>
> > > > Signed-off-by: Eric Biggers <ebigg...@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.
>
Sure.
On other architectures, we might just turn this logic around, and only
disable softirqs when IRQs are enabled, as otherwise, there is no
need: we don't care about whether or not IRQs are disabled, only the
softirq plumbing that we need to call into does, and no softirqs can
be delivered over the back of a hard IRQ when those are disabled to
begin with.
> > 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.
>
Sounds good.
