Re: [RFC PATCH 6/9] rv: Add KUnit coverage for IRQ context transitions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rv: kunit: rq->nr_pinned leak on kunit_skip() under guard(migrate)()
--

commit 3183a8d5a8d274f51929b98832f2ad4013dd57a7
Author: Karl Mehltretter <kmehltretter@xxxxxxxxx>

rv: Add KUnit coverage for IRQ context transitions

This commit adds KUnit test cases for the irqstate runtime verification
monitor covering generated state tables, invalid transitions, enable-time
synchronization, nested hardirqs, shared IRQ actions, PREEMPT_RT serving
intervals, and live tracepoint wiring.

> diff --git a/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c b/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c
> new file mode 100644
> index 0000000000000..c5b8834fea3d9
> --- /dev/null
> +++ b/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c
> @@ -0,0 +1,1384 @@

[ ... ]

> +/* Exercise non-RT inline softirq processing from a real hardirq exit. */
> +static void rv_test_irqstate_irq_exit_inline(struct kunit *test)
> +{

[ ... ]

> +	KUNIT_ASSERT_FALSE(test, in_interrupt());
> +	KUNIT_ASSERT_FALSE(test, irqs_disabled());
> +	KUNIT_ASSERT_EQ(test, softirq_count(), 0U);
> +	guard(migrate)();
> +	inline_ctx->owner = current;
> +	inline_ctx->owner_cpu = raw_smp_processor_id();
> +	inline_ctx->softirqd = this_cpu_ksoftirqd();
> +	KUNIT_ASSERT_NOT_NULL(test, inline_ctx->softirqd);
> +	if (local_softirq_pending() ||
> +	    !wait_task_inactive(inline_ctx->softirqd, TASK_INTERRUPTIBLE)) {
> +		kunit_skip(test, "Requires an idle local ksoftirqd");
> +		return;
> +	}
> +	saved_count = preempt_count();
> +	saved_softirq = softirq_count();
> +	local_irq_save(flags);
> +	if (local_softirq_pending() || task_is_running(inline_ctx->softirqd)) {
> +		local_irq_restore(flags);
> +		kunit_skip(test, "Local ksoftirqd stopped being idle");
> +		return;
> +	}
> +	WRITE_ONCE(inline_ctx->armed, true);
> +	WRITE_ONCE(inline_ctx->raise_on_exit, true);
> +	irq_enter_rcu();
> +	tasklet_schedule(&inline_ctx->tasklet);
> +	irq_exit_rcu();
> +	ran_inline = inline_ctx->tasklet_runs != 0;
> +	inline_ctx->exit_tasklet_pending =
> +		!!(local_softirq_pending() & BIT(TASKLET_SOFTIRQ));
> +	/*
> +	 * A zero return means the exit handoff already made ksoftirqd runnable.
> +	 * A one return also makes the failure path safe to drain.
> +	 */
> +	inline_ctx->softirqd_already_runnable =
> +		!wake_up_process(inline_ctx->softirqd);
> +	WRITE_ONCE(inline_ctx->armed, false);
> +	local_irq_restore(flags);
> +
> +	/* Drain a tasklet diverted to ksoftirqd before inspecting or skipping. */
> +	kunit_release_action(test, irqstate_inline_bh_kill_all, inline_ctx);
> +	if (!ran_inline) {
> +		kunit_skip(test, "IRQ exit diverted pending softirq to ksoftirqd");
> +		return;
> +	}

[Severity: High]
Does calling kunit_skip() or KUNIT_ASSERT_*() while guard(migrate)() is
active in kernel/trace/rv/monitors/irqstate/irqstate_kunit.c leak the
runqueue nr_pinned counter?

In rv_test_irqstate_irq_exit_inline(), guard(migrate)() invokes
migrate_disable() -> __migrate_disable() in include/linux/sched.h:

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

Inside that scope, rv_test_irqstate_irq_exit_inline() calls kunit_skip()
when local_softirq_pending() is non-zero, when ksoftirqd is not idle, or
when !ran_inline (for example, when forced IRQ threading diverts the
tasklet to ksoftirqd). However, kunit_skip() in include/kunit/test.h is:

	#define kunit_skip(test, fmt, ...)				\
		do {							\
			kunit_mark_skipped((test), fmt, ##__VA_ARGS__);	\
			kunit_try_catch_throw(&((test)->try_catch));	\
		} while (0)

Both kunit_skip() and failed KUNIT_ASSERT_*() macros call
kunit_try_catch_throw(), which terminates the KUnit runner kthread directly
via kthread_exit(0) -> do_exit(0) without returning to the caller or
unwinding the C stack.

Because scope-based cleanup destructors do not run when kthread_exit(0)
exits the thread, migrate_enable() -> __migrate_enable() never executes to
decrement this_rq_pinned() (rq->nr_pinned). Wouldn't that leave
rq_has_pinned_tasks(rq) permanently true on that CPU and cause subsequent
CPU hotplug offline operations to hang in balance_hotplug_wait()?

The same issue also applies if any KUNIT_ASSERT_*() fails while
guard(migrate)() is held in rv_test_irqstate(),
rv_test_irqstate_shared_actions(), rv_test_irqstate_sync(),
rv_test_irqstate_irq_exit_inline(), rv_test_irqstate_preempt(),
rv_test_irqstate_rt_prepare_irq(), and rv_test_irqstate_rt_prepare_switch().
Could migration disabling be managed with a kunit_add_action_or_reset()
cleanup action, or released before calling kunit_skip() and KUNIT_ASSERT_*()
macros?

[ ... ]

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