Re: [PATCH v2] softirq: Preserve interrupt context during IRQ exit

[Sebastian Andrzej Siewior]

On 2026-09-05 04:32:10 [+0200], Karl Mehltretter wrote:
> __irq_exit_rcu() drops HARDIRQ_OFFSET before deferred hrtimer rearm,
> entry into softirq dispatch, and timersd wakeup. The rearm, wakeup, and
> softirq entry code before softirq_handle_begin() are still on the IRQ
> return path, but in_task() reports task context.
> 
> Context-sensitive code called from this window therefore sees task
> context. ftrace records normal-context flags and selects its normal
> recursion slot. KCSAN attributes IRQ-exit accesses to the interrupted
> task, while KMSAN can select and modify that task's metadata.

The breakage is limited to KCSAN & friends within the window during
transition to softirq and out. There is nothing else? Well, the timer
wake looks wrong in trace, noted.

…
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -481,15 +481,35 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt)
>  }
>  EXPORT_SYMBOL(__local_bh_enable_ip);
>  
> -static inline void softirq_handle_begin(void)
> +static inline bool softirq_handle_begin(void)
>  {
> -	__local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);
> +	bool from_hardirq = in_hardirq();
> +
> +	if (!from_hardirq) {
> +		__local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);
> +		return false;
> +	}
> +
> +	/* Replace the retained hardirq context with normal softirq context. */

/*
 * This is only entered on return from interrupt. Preemption disabled
 * locations remains unchanged, the context (-HARDIRQ +SOFTIRQ) is
 * updated and lockdep is let known.
 */

> +	__preempt_count_add((int)SOFTIRQ_OFFSET - (int)HARDIRQ_OFFSET);
The casts look odd. We need this? It is defined as long, yes, but
preempt_count accepts an int only so it will throw the upper bits away.

> +	WARN_ON_ONCE(softirq_count() != SOFTIRQ_OFFSET);

I think we want to ensure that irq_count() == SOFTIRQ_OFFSET

at this point. Only the lower preemption bits may differ, everything
else should be as we expect it. softirq_count() would drop the HARDIRQ
bits.

> +	lockdep_softirqs_off(_RET_IP_);
> +
> +	return true;
>  }
>  
> -static inline void softirq_handle_end(void)
> +static inline void softirq_handle_end(bool from_hardirq)
>  {
> -	__local_bh_enable(SOFTIRQ_OFFSET);
> -	WARN_ON_ONCE(in_interrupt());
> +	if (!from_hardirq) {
> +		__local_bh_enable(SOFTIRQ_OFFSET);
> +		WARN_ON_ONCE(in_interrupt());
> +		return;
> +	}
> +
> +	WARN_ON_ONCE(softirq_count() != SOFTIRQ_OFFSET);
> +	lockdep_softirqs_on(_RET_IP_);
> +	__preempt_count_sub((int)SOFTIRQ_OFFSET - (int)HARDIRQ_OFFSET);
> +	WARN_ON_ONCE(!in_hardirq());

that is quite some WARN_ON_ONCE. We would like to see just
HARDIRQ_OFFSET at the end. Or SOFTIRQ_OFFSET before the end. One should
be enough or the math is wrong.

>  }
>  
>  static inline void ksoftirqd_run_begin(void)
> @@ -740,6 +761,8 @@ static inline void wake_timersd(void) { }
>  
>  #endif
>  
> +#define IRQ_EXIT_TIMERS	(NMI_MASK | HARDIRQ_MASK)
> +
>  static inline void __irq_exit_rcu(void)
>  {
>  #ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
> @@ -748,8 +771,7 @@ static inline void __irq_exit_rcu(void)
>  	lockdep_assert_irqs_disabled();
>  #endif
>  	account_hardirq_exit(current);
> -	preempt_count_sub(HARDIRQ_OFFSET);
> -	if (!in_interrupt() && local_softirq_pending()) {
> +	if (irq_count() == HARDIRQ_OFFSET && local_softirq_pending()) {

irq_enter_rcu() did preempt_count_add(HARDIRQ_OFFSET), did record
task_struct::preempt_disable_ip. Due to the split, it does not recording
the softirq handling as disabling preemption point but keeps the
original until the end. This looks like an improvement.

>  		/*
>  		 * If we left hrtimers unarmed, make sure to arm them now,
>  		 * before enabling interrupts to run softirq.
> @@ -759,9 +781,11 @@ static inline void __irq_exit_rcu(void)
>  	}
>  
>  	if (IS_ENABLED(CONFIG_IRQ_FORCED_THREADING) && force_irqthreads() &&
> -	    local_timers_pending_force_th() && !(in_nmi() | in_hardirq()))
> +	    local_timers_pending_force_th() &&
> +	    (preempt_count() & IRQ_EXIT_TIMERS) == HARDIRQ_OFFSET)

Why is this preempt_count() instead irq_count(). Why is there
IRQ_EXIT_TIMERS? It is almost as the first check except now we would
like to ignore the additional softirq_count().

>  		wake_timersd();
>  
> +	preempt_count_sub(HARDIRQ_OFFSET);
>  	tick_irq_exit();
>  }
>  
> 
> base-commit: 2af470916a208b576ac9975d221d9a378cf8ace9

Sebastian