Re: [RFC PATCH 7/9] rv: Add KUnit coverage for IRQ-exit scheduling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rv: leaked rq->nr_pinned on assertion abort in rv_test_irqexit_live()
--

commit 69d8a9063a6f40de41bbbc2fc85512c36f8a0acb
Author: Karl Mehltretter <kmehltretter@gmail.com>

rv: Add KUnit coverage for IRQ-exit scheduling

This commit adds KUnit test cases for the irqexit runtime verification
monitor, covering its state transitions, synchronization, and live
invocations of preempt_schedule_irq() including task switching and CPU
migration.

> diff --git a/kernel/trace/rv/monitors/irqexit/irqexit_kunit.c b/kernel/trace/rv/monitors/irqexit/irqexit_kunit.c
> new file mode 100644
> index 0000000000000..56a9442e2f704
> --- /dev/null
> +++ b/kernel/trace/rv/monitors/irqexit/irqexit_kunit.c
[ ... ]
> +static void rv_test_irqexit_live(struct kunit *test)
> +{
> +	struct rv_monitor *mon = rv_irqexit_ops.mon.rv_this;
> +	struct rv_kunit_ctx *ctx = test->priv;
> +	struct irqexit_live_ctx *live = (void *)test->param_value;
> +	struct task_struct *worker;
> +	unsigned int entries, exits, switches_out, switches_in;
> +	unsigned int first_entries, first_switches;
> +	unsigned long flags;
> +	bool worker_done;
> +
> +	KUNIT_ASSERT_FALSE(test, mon->enabled);
> +	live->owner = current;
> +	get_task_struct(live->owner);
> +	init_completion(&live->worker_done);
> +
> +	active_ctx = ctx;
> +	mon->react = rv_kunit_mock_react;
> +	rv_mock_current(NULL);
> +	KUNIT_ASSERT_EQ(test, 0,
> +			kunit_add_action_or_reset(test, irqexit_live_cleanup, live));
> +	/* The worker must run on this CPU inside the explicit scheduling loop. */
> +	guard(migrate)();
> +	worker = kthread_create(irqexit_live_worker, live, "rv-irqexit-test");
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, worker);
> +	live->worker = worker;
> +	kthread_bind(worker, smp_processor_id());
> +	sched_set_fifo(worker);
> +	KUNIT_ASSERT_EQ(test, 0,
> +			register_trace_event_irqexit(irqexit_observe_event, live));
> +	live->observer_attached = true;
> +	KUNIT_ASSERT_EQ(test, irqexit_observe_boundaries(live), 0);
> +
> +	mon->enabled = 1;
> +
> +	KUNIT_ASSERT_EQ(test, preempt_count(), 0);

[Severity: Medium]
Does this leak the per-CPU rq->nr_pinned counter if any of the KUNIT_ASSERT_*
checks inside the guard(migrate)() scope fail?

In kernel/trace/rv/monitors/irqexit/irqexit_kunit.c, rv_test_irqexit_live()
enters guard(migrate)(), which calls migrate_disable() -> __migrate_disable()
in include/linux/sched.h:

	this_rq_pinned()++;
	p->migration_disabled = 1;

If a subsequent assertion fails (such as KUNIT_ASSERT_NOT_ERR_OR_NULL() after
kthread_create(), or KUNIT_ASSERT_EQ() after register_trace_event_irqexit(),
irqexit_observe_boundaries(), or preempt_count()), KUnit aborts the test case
via __kunit_abort() -> kunit_try_catch_throw() in lib/kunit/try-catch.c:

void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
{
	try_catch->try_result = -EFAULT;
	kthread_exit(0);
}

Because kthread_exit(0) terminates the KUnit runner kthread directly via
do_exit(0) without unwinding the C stack, the scope cleanup destructor for
guard(migrate)() (__migrate_enable()) never runs. Neither kthread_exit() nor
do_exit() decrements this_rq_pinned(), leaving rq->nr_pinned permanently
elevated on that CPU and causing any later CPU hotplug offline operation to
hang in balance_hotplug_wait() waiting for !rq_has_pinned_tasks(rq).

Could migration disabling be narrowed with scoped_guard(migrate) or explicit
migrate_disable() / migrate_enable() around the non-asserting section, or
cleaned up via a KUnit action when current->migration_disabled is set?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022050712.75035-1-kmehltretter@gmail.com?part=7