Re: [RFC PATCH v2 10/13] entry: Classify ordinary hardirq return-tail work as hardirq context

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] entry: irqentry_exit_to_kernel_mode_after_preempt() breaks preemptirqsoff
- [Medium] softirq: irqentry_hardirq_tail_exit() repeats wakeups with wake_flags = 0
- [Medium] entry: irqentry_hardirq_tail_exit() wakes softirq threads after preempt check
--

commit d2af631b0f352ba4a12e0eb8fa80f5ff66476c74
Author: Karl Mehltretter <kmehltretter@xxxxxxxxx>

entry: Classify ordinary hardirq return-tail work as hardirq context

This commit stages a fresh hardirq-count attempt and restores lockdep
classification across traceable return-tail work on return from ordinary
hardirqs. It adds irqentry_hardirq_tail_enter() and
irqentry_hardirq_tail_exit() around the post-preemption return tail so
return-tail callbacks run as hardirq context.

> diff --git a/include/linux/irq-entry-common.h b/include/linux/irq-entry-common.h
> index 6f649f6613312..f51aafc1b49fd 100644
> --- a/include/linux/irq-entry-common.h
> +++ b/include/linux/irq-entry-common.h
> @@ -383,6 +383,32 @@ irqentry_hardirq_count_state_set(irqentry_state_t *state,
>  		IRQENTRY_HARDIRQ_APPLIED : IRQENTRY_HARDIRQ_REJECTED;
>  }
>  
> +/**
> + * irqentry_hardirq_tail_enter - Stage ordinary-hardirq return-tail state
> + * @state: Pointer to state returned by the matching irqentry_hardirq_enter()
> + *
> + * Make a fresh staged hardirq-count attempt and establish the lockdep
> + * classification which traceable return-tail work needs. Accounting, RCU and
> + * tick handling belong to the completed interrupt and must not be repeated
> + * here.
> + */
> +static __always_inline void
> +irqentry_hardirq_tail_enter(irqentry_state_t *state)
> +{
> +	struct irqentry_hardirq_cookie cookie;
> +
> +	if (!state->hardirq)
> +		return;
> +
> +	cookie = irqentry_hardirq_count_start();
> +	lockdep_hardirq_enter();
> +
> +	instrumentation_begin();
> +	irqentry_hardirq_count_finish(cookie);
> +	instrumentation_end();
> +	irqentry_hardirq_count_state_set(state, cookie);
> +}

[ ... ]

> @@ -543,6 +574,31 @@ static __always_inline void irqentry_hrtimer_rearm_deferred(void)
>  static __always_inline void
>  irqentry_exit_to_kernel_mode_after_preempt(struct pt_regs *regs, irqentry_state_t state)
>  {
> +	struct ct_irq_exit_state ct_state = { };
> +
> +	if (state.hardirq) {
> +		if (!regs_irqs_disabled(regs)) {
> +			instrumentation_begin();
> +			hrtimer_rearm_deferred();
> +			/* Tell the tracer that the return will enable IRQs. */
> +			trace_hardirqs_on_prepare();
> +			lockdep_hardirqs_on_prepare_irqtail();
> +			instrumentation_end();
> +		}
> +
> +		if (state.exit_rcu)
> +			ct_state = ct_irq_exit_prepare();
> +
> +		irqentry_hardirq_tail_exit(irqentry_hardirq_count_state(state));

[Severity: High]
Does ordering trace_hardirqs_on_prepare() before irqentry_hardirq_tail_exit()
cause a regression for the preemptirqsoff tracer when returning to
preemptible, IRQ-enabled kernel mode?

In irqentry_exit_to_kernel_mode(), irqentry_hardirq_tail_enter() stages
HARDIRQ_OFFSET in preempt_count() and calls:

irqentry_hardirq_count_finish()
  preempt_latency_start(HARDIRQ_OFFSET)
    trace_preempt_off()

Next, when !regs_irqs_disabled(regs),
irqentry_exit_to_kernel_mode_after_preempt() calls trace_hardirqs_on_prepare()
while HARDIRQ_OFFSET is still set in preempt_count(). In
kernel/trace/trace_irqsoff.c:

void tracer_hardirqs_on(unsigned long a0, unsigned long a1)
{
	if (!preempt_trace(preempt_count()) && irq_trace())
		stop_critical_timing(a0, a1);
}

When the preemptirqsoff tracer is active (both CONFIG_IRQSOFF_TRACER and
CONFIG_PREEMPT_TRACER enabled, so trace_type has both TRACER_IRQS_OFF and
TRACER_PREEMPT_OFF), preempt_trace(preempt_count()) evaluates to true because
preempt_count() contains HARDIRQ_OFFSET. As a result, tracer_hardirqs_on()
skips stop_critical_timing(), while trace_hardirqs_on_prepare() still clears
tracing_irq_cpu.

Immediately after that, irqentry_hardirq_tail_exit() calls:

irqentry_hardirq_exit_prepare()
  irqentry_hardirq_count_sub_prepare()
    preempt_latency_stop(HARDIRQ_OFFSET)
      trace_preempt_on()
        tracer_preempt_on()

In kernel/trace/trace_irqsoff.c:

void tracer_preempt_on(unsigned long a0, unsigned long a1)
{
	if (preempt_trace(preempt_count()) && !irq_trace())
		stop_critical_timing(a0, a1);
}

Because hardware interrupts remain disabled at the CPU until the exception
return instruction, irqs_disabled() is still true, so irq_trace() evaluates to
true and tracer_preempt_on() also skips stop_critical_timing().

Finally, irqentry_hardirq_exit_finish() removes HARDIRQ_OFFSET via
__preempt_count_sub(HARDIRQ_OFFSET). Won't this leave per_cpu(tracing_cpu, cpu)
stuck active after returning to preemptible, IRQ-enabled kernel execution, as
well as emitting extra preempt_disable, irq_enable, and preempt_enable trace
events in inverted order?

> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 169c559832501..85b167db37c29 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -837,6 +837,27 @@ irq_exit_wake_late_softirqs(unsigned int wake_flags)
>  		irq_exit_wake_ksoftirqd();
>  }
>  
> +/**
> + * irqentry_hardirq_tail_exit - Close work and remove return-tail state
> + * @cookie: Staged hardirq-count cookie from irqentry_hardirq_tail_enter()
> + *
> + * Run every instrumentable teardown operation, including the final worker
> + * closure, before conditionally removing any applied raw hardirq count and
> + * ending lockdep nesting.
> + */
> +void noinstr
> +irqentry_hardirq_tail_exit(struct irqentry_hardirq_cookie cookie)
> +{
> +	bool do_sub;
> +
> +	instrumentation_begin();
> +	do_sub = irqentry_hardirq_exit_prepare(cookie);
> +	irq_exit_wake_late_softirqs(0);
> +	instrumentation_end();
> +
> +	irqentry_hardirq_exit_finish(do_sub);
> +}

[Severity: Medium]
Can passing a hardcoded 0 to irq_exit_wake_late_softirqs(0) in
irqentry_hardirq_tail_exit() cause redundant wake_up_process() calls for
ksoftirqd and ktimerd?

During the earlier main hardirq exit in irq_exit_rcu_preempt_counted_prepare(),
__irq_exit_rcu(true) records IRQ_EXIT_WAKE_KSOFTIRQD and IRQ_EXIT_WAKE_KTIMERD
in wake_flags so irq_exit_wake_late_softirqs(wake_flags) avoids waking those
threads a second time. However, local_softirq_pending() or
local_timers_pending_force_th() remains non-zero until the woken thread
actually runs on the CPU and clears the pending bits.

If irqentry_exit_to_kernel_mode_preempt() does not switch to the worker thread
(for example, on any interrupt from an RCU-idle context where state.exit_rcu is
true, when the interrupted kernel context has preemption disabled, when
regs_irqs_disabled(regs) is true, or when current has higher priority),
irqentry_hardirq_tail_exit() runs with those pending bits still set:

irq_exit_wake_late_softirqs(0)
  if (!ksoftirqd_wake_requested)
      ksoftirqd_wake_requested = irq_exit_wake_ksoftirqd();
  if (!ktimerd_wake_requested)
      ktimerd_wake_requested = irq_exit_wake_timersd();

Because wake_flags is 0, won't this call wakeup_softirqd() or wake_timersd()
a second time even when no return-tail callback raised new work, and do so
after trace_hardirqs_on_prepare(), ct_irq_exit_prepare(), and
irqentry_hardirq_exit_prepare() have already completed?

[Severity: Medium]
What happens if a return-tail callback raises softirq or forced-thread timer
work while returning to a preemptible kernel context?

In irqentry_exit_to_kernel_mode():

	instrumentation_begin();
	irqentry_exit_to_kernel_mode_preempt(regs, state);
	instrumentation_end();

	irqentry_hardirq_tail_enter(&state);
	irqentry_exit_to_kernel_mode_after_preempt(regs, state);

the preemption check in irqentry_exit_to_kernel_mode_preempt() runs before the
hardirq return tail is entered.

If a callback inside the return tail (such as during hrtimer_rearm_deferred(),
trace_hardirqs_on_prepare(), lockdep_hardirqs_on_prepare_irqtail(), or
irqentry_hardirq_exit_prepare()) raises softirq or timer work,
irq_exit_wake_late_softirqs(0) in irqentry_hardirq_tail_exit() wakes ksoftirqd
or ktimerd while HARDIRQ_OFFSET is still set in preempt_count(), setting
need_resched() on current. Immediately afterwards,
irqentry_hardirq_exit_finish() drops HARDIRQ_OFFSET via
__preempt_count_sub(HARDIRQ_OFFSET) without checking for rescheduling.

Could this return to a preemptible kernel task with need_resched() set and
miss preempting for the newly woken worker thread (including a higher-priority
SCHED_FIFO ktimerd/ksoftirqd under PREEMPT_RT or force_irqthreads)?

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