Re: [RFC PATCH 9/9] rv: Test IRQ-exit softirq provenance and handoff

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rv: rq->nr_pinned leak on kunit_skip() under guard(migrate)() in irqstate tests
- [Medium] rv: broken nested IRQ filter in irqstate_inline_hardirq_entry() and exit
- [Medium] rv: missing local_bh_blocked() check and early wait in hardirq exit tests
--

commit 7fb79c597a519150ad675f18bceb7d2417080c8b
Author: Karl Mehltretter <kmehltretter@gmail.com>

rv: Test IRQ-exit softirq provenance and handoff

This commit extends the irqstate KUnit test suite to cover inline softirq
processing during hardirq exit, hardirq provenance tracking, and monitor
synchronization. It also adds tests verifying late work handoff to ksoftirqd
and ktimerd at the softirq and hardirq exit boundaries.

> diff --git a/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c b/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c
> index e3928d10b2825..31e827cdba1c6 100644
> --- a/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c
> +++ b/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c
[ ... ]
> @@ -893,6 +984,39 @@ static bool irqstate_empty_worker_matches(struct irqstate_empty_worker_ctx *empt
>  	return !empty_ctx->real_worker || softirq_kunit_empty_worker_active();
>  }
>  
> +static void irqstate_inline_hardirq_entry(void *data, unsigned int depth)
> +{
> +	struct irqstate_inline_bh_ctx *inline_ctx = data;
> +
> +	/* Inline softirq processing enables IRQs; ignore a nested real IRQ. */
> +	if (!READ_ONCE(inline_ctx->armed) || current != inline_ctx->owner ||
> +	    depth != 1)
> +		return;
> +	inline_ctx->hardirq_entries++;
> +	if (inline_ctx->event_index != 0)
> +		inline_ctx->bad_order = true;
> +	inline_ctx->event_index++;
> +	if (hardirq_count() != HARDIRQ_OFFSET || in_serving_softirq() ||
> +	    !irqs_disabled())
> +		inline_ctx->bad_boundary_context = true;
> +}
> +
> +static void irqstate_inline_hardirq_exit(void *data, unsigned int depth)
> +{
> +	struct irqstate_inline_bh_ctx *inline_ctx = data;
> +
> +	if (!READ_ONCE(inline_ctx->armed) || current != inline_ctx->owner ||
> +	    depth != 1)
> +		return;
> +	inline_ctx->hardirq_exits++;
> +	if (inline_ctx->event_index != 3)
> +		inline_ctx->bad_order = true;
> +	inline_ctx->event_index++;
> +	if (hardirq_count() != HARDIRQ_OFFSET || in_serving_softirq() ||
> +	    !irqs_disabled())
> +		inline_ctx->bad_boundary_context = true;
> +}

[Severity: Medium]
Will checking depth != 1 in irqstate_inline_hardirq_entry() and
irqstate_inline_hardirq_exit() actually filter out a real hardware interrupt
that arrives while inline softirq processing runs with interrupts enabled?

In the rv_test_irqstate_irq_exit_inline() -> irq_exit_rcu() ->
__do_softirq() -> handle_softirqs() path, softirq_handle_begin() replaces
HARDIRQ_OFFSET with SOFTIRQ_OFFSET before local_irq_enable() enables
interrupts:

kernel/softirq.c:softirq_handle_begin() {
	__preempt_count_sub(HARDIRQ_OFFSET - SOFTIRQ_OFFSET);
	lockdep_softirqs_off(_RET_IP_);
	WARN_ON_ONCE(irq_count() != SOFTIRQ_OFFSET);
}

Because HARDIRQ_OFFSET has been cleared, a real hardware interrupt arriving
during the IRQ-enabled softirq window enters irq_enter_rcu() with
hardirq_count() equal to HARDIRQ_OFFSET (so depth is 1, while
in_serving_softirq() is non-zero), and exits irq_exit_rcu() with depth equal
to 1 as well.

As a result, depth != 1 is false for that interrupt in both
irqstate_inline_hardirq_entry() and irqstate_inline_hardirq_exit(), causing
both probes to increment hardirq_entries, hardirq_exits, and event_index and
to set bad_order and bad_boundary_context. Should both callbacks also ignore
events when in_serving_softirq() is true?

[ ... ]
> +/* Work first raised at the final hardirq boundary must reach ksoftirqd. */
> +static void rv_test_irqstate_hardirq_exit_handoff(struct kunit *test)
> +{
> +	struct irqstate_hardirq_exit_ctx *exit_ctx;
> +	struct rv_kunit_ctx *ctx = test->priv;
> +	unsigned long flags;
> +	int ret;
> +
> +	prepare_test(test, &rv_irqstate_ops.mon);
> +	guard(migrate)();
> +	exit_ctx = kunit_kzalloc(test, sizeof(*exit_ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, exit_ctx);
> +	tasklet_setup(&exit_ctx->tasklet, irqstate_hardirq_exit_tasklet);
> +	exit_ctx->owner = current;
> +	exit_ctx->owner_cpu = raw_smp_processor_id();
> +	exit_ctx->softirqd = this_cpu_ksoftirqd();
> +	KUNIT_ASSERT_NOT_NULL(test, exit_ctx->softirqd);
> +	if (local_softirq_pending() ||
> +	    !wait_task_inactive(exit_ctx->softirqd, TASK_INTERRUPTIBLE)) {
> +		kunit_skip(test, "Requires an idle local ksoftirqd");
> +		return;
> +	}

[Severity: High]
Does calling kunit_skip() or failing a KUNIT_ASSERT_*() while guard(migrate)()
is active leak the per-CPU rq->nr_pinned counter?

guard(migrate)() calls migrate_disable(), which increments rq->nr_pinned
(this_rq_pinned()) and relies on the C cleanup attribute to invoke
migrate_enable() at scope exit. However, kunit_skip() and failing
KUNIT_ASSERT_*() macros call kunit_try_catch_throw():

lib/kunit/try-catch.c:kunit_try_catch_throw() {
	try_catch->try_result = -EFAULT;
	kthread_exit(0);
}

Because kunit_try_catch_throw() is noreturn and exits the KUnit runner
kthread directly via kthread_exit(0) -> do_exit(0) without unwinding C stack
frames, the guard(migrate)() cleanup never runs and do_exit() does not call
migrate_enable(). That leaves rq->nr_pinned permanently elevated on the CPU,
which makes rq_has_pinned_tasks(rq) stay true and causes subsequent CPU
hotplug offline operations to hang in balance_hotplug_wait().

The same pattern occurs in rv_test_irqstate_hardirq_exit_timer_handoff()
below, and when force_irqthreads() is enabled on a !CONFIG_PREEMPT_RT kernel
to run rv_test_irqstate_hardirq_exit_timer_handoff(), the following test
rv_test_irqstate_irq_exit_inline() diverts its softirq to ksoftirqd
(ran_inline is false) and deterministically hits kunit_skip() under
guard(migrate)().

> +
> +	ret = register_trace_hardirq_context_exit(
> +		irqstate_hardirq_exit_raise, exit_ctx);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	exit_ctx->attached = true;
> +	KUNIT_ASSERT_EQ(test, 0,
> +			kunit_add_action_or_reset(test,
> +						  irqstate_hardirq_exit_cleanup,
> +						  exit_ctx));
> +	attach_irqstate_test_probes();
> +	KUNIT_ASSERT_EQ(test, 0,
> +			kunit_add_action_or_reset(test, detach_irqstate_test_probes,
> +						  NULL));
> +
> +	local_irq_save(flags);
> +	if (local_softirq_pending() || task_is_running(exit_ctx->softirqd)) {
> +		local_irq_restore(flags);
> +		kunit_skip(test, "Local ksoftirqd stopped being idle");
> +		return;
> +	}

[Severity: Medium]
Could rv_test_irqstate_hardirq_exit_handoff() and
rv_test_irqstate_hardirq_exit_timer_handoff() fail or spuriously skip due to
missing a local_bh_blocked() check on CONFIG_PREEMPT_RT and waiting for
worker inactivity before registering tracepoints?

In __irq_exit_rcu(), waking ksoftirqd for softirq work raised during
trace_hardirq_context_exit() is gated on should_wake_ksoftirqd():

kernel/softirq.c:__irq_exit_rcu() {
	if (irq_count() == HARDIRQ_OFFSET &&
	    unlikely(local_softirq_pending()) &&
	    should_wake_ksoftirqd()) {
		wakeup_softirqd();
		ksoftirqd_wake_requested = true;
	}
}

On CONFIG_PREEMPT_RT, should_wake_ksoftirqd() checks
!this_cpu_read(softirq_ctrl.cnt) (!local_bh_blocked()). Because BH-disabled
sections on PREEMPT_RT are preemptible and can block on RT locks
(TASK_RTLOCK_WAIT), another task on the local CPU (or ksoftirqd itself after
clearing local_softirq_pending() and blocking in TASK_RTLOCK_WAIT, where
task_is_running() is false) can hold softirq_ctrl.cnt non-zero while
local_softirq_pending() is 0 and task_is_running(softirqd) is false. In that
case __irq_exit_rcu() skips wakeup_softirqd() and the assertion on
softirqd_already_runnable fails.

Additionally, unlike rv_test_irqstate_irq_exit_inline(), both handoff tests
call wait_task_inactive() before register_trace_hardirq_context_exit(),
register_trace_sched_waking(), and attach_irqstate_test_probes(), which can
sleep on mutexes and queue RCU work that wakes ksoftirqd or ktimerd after the
wait has already finished. Would it be better to attach the probes before
waiting for the workers to go inactive, and also check local_bh_blocked()?

> +	WRITE_ONCE(exit_ctx->armed, true);
> +	irq_enter_rcu();
> +	irq_exit_rcu();
> +	exit_ctx->tasklet_pending =
> +		!!(local_softirq_pending() & BIT(TASKLET_SOFTIRQ));
> +	exit_ctx->softirqd_already_runnable =
> +		!wake_up_process(exit_ctx->softirqd);
> +	WRITE_ONCE(exit_ctx->armed, false);
> +	local_irq_restore(flags);
[ ... ]

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