Re: [PATCH v2 06/12] irq: Add KUnit test for refcounted interrupt enable/disable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] KUnit test assertions are executed while hardware interrupts are disabled, causing a `GFP_KERNEL` allocation and kernel BUG if a test expectation fails.
- [High] The test suite lacks robust teardown and initialization to protect the kernel from leaking disabled interrupt states.
- [Low] The KUnit test is unconditionally compiled when CONFIG_KUNIT is enabled, bypassing subsystem-specific test configs.
- [Low] Missing closing parenthesis in a code comment.
--

commit ad24a2af47c3509344d4c410cf8820cb3d8e2b06
Author: Lyude Paul <lyude@redhat.com>

irq: Add KUnit test for refcounted interrupt enable/disable

Adds a KUnit test suite to verify the refcounted interrupt enable and
disable functionality.

Link: https://patch.msgid.link/20260121223933.1568682-7-lyude@redhat.com

> diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile
> index 86a2e5ae08f9a..44c4d6fc502ac 100644
> --- a/kernel/irq/Makefile
> +++ b/kernel/irq/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_SMP) += affinity.o
>  obj-$(CONFIG_GENERIC_IRQ_DEBUGFS) += debugfs.o
>  obj-$(CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR) += matrix.o
>  obj-$(CONFIG_IRQ_KUNIT_TEST) += irq_test.o
> +obj-$(CONFIG_KUNIT) += refcount_interrupt_test.o

[Severity: Low]
Should this test be guarded by a subsystem-specific configuration option like
CONFIG_IRQ_KUNIT_TEST instead of CONFIG_KUNIT?

Building it unconditionally when CONFIG_KUNIT is enabled means the test
will automatically execute on any kernel with KUnit globally enabled, which
might bypass subsystem test configs and increase test execution times.

> diff --git a/kernel/irq/refcount_interrupt_test.c b/kernel/irq/refcount_interrupt_test.c
> new file mode 100644
> index 0000000000000..b4f224595f261
> --- /dev/null
> +++ b/kernel/irq/refcount_interrupt_test.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for refcounted interrupt enable/disables.
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/interrupt_rc.h>
> +
> +#define TEST_IRQ_ON() KUNIT_EXPECT_FALSE(test, irqs_disabled())
> +#define TEST_IRQ_OFF() KUNIT_EXPECT_TRUE(test, irqs_disabled())
> +
> +/* ===== Test cases ===== */
> +static void test_single_irq_change(struct kunit *test)
> +{
> +	local_interrupt_disable();
> +	TEST_IRQ_OFF();

[Severity: High]
Could this trigger a sleeping in atomic context BUG if the test expectation
fails?

If TEST_IRQ_OFF() or TEST_IRQ_ON() fails, KUnit invokes kunit_fail(), which
uses GFP_KERNEL to allocate memory for the failure message. Since GFP_KERNEL
implies __GFP_DIRECT_RECLAIM, doing this while hardware interrupts are
disabled violates atomic context rules.

Would it be safer to save the interrupt state to a local variable inside the
atomic section and evaluate the KUnit assertion after interrupts are safely
re-enabled?

[ ... ]

> +static struct kunit_case test_cases[] = {
> +	KUNIT_CASE(test_single_irq_change),
> +	KUNIT_CASE(test_nested_irq_change),
> +	KUNIT_CASE(test_multiple_irq_change),
> +	KUNIT_CASE(test_irq_save),
> +	{},
> +};
> +
> +/* (init and exit are the same */

[Severity: Low]
This isn't a bug, but it looks like there is a missing closing parenthesis
at the end of this comment.

> +static int test_init(struct kunit *test)
> +{
> +	TEST_IRQ_ON();
> +
> +	return 0;
> +}
> +
> +static void test_exit(struct kunit *test)
> +{
> +	TEST_IRQ_ON();
> +}

[Severity: High]
Does this test suite need more robust teardown to protect the kernel from
leaking disabled interrupt states?

If a test case fails to balance local_interrupt_disable() and
local_interrupt_enable(), it will return with hardware interrupts permanently
disabled. Because test_exit() only checks the state but does not repair it,
the KUnit kthread will continue executing with interrupts disabled, which can
lead to a system hang, RCU stall, or softlockup.

Should test_exit() explicitly restore a safe interrupt environment, and
should test_init() fail the test suite if it detects interrupts are already
disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526152148.30514-1-boqun@kernel.org?part=6