linux-kernel - Re: [PATCH] lockdep: Fix inconsistency in irq tracking on NMIs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250621085706.GM1613200@noisy.programming.kicks-ass.net>
Date: Sat, 21 Jun 2025 10:57:06 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Gabriele Monaco <gmonaco@...hat.com>
Cc: linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Andy Lutomirski <luto@...nel.org>, Ingo Molnar <mingo@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] lockdep: Fix inconsistency in irq tracking on NMIs

On Fri, Jun 20, 2025 at 02:51:13PM +0200, Gabriele Monaco wrote:

>  local_irq_enable()
>    void trace_hardirqs_on(void)
>    {
>    	if (tracing_irq_cpu) {
>    		trace(irq_enable);
>    		tracing_irq_cpu = 0;
>    	}
> 
>                 /*
>                  * NMI here
>                  * tracing_irq_cpu == 0 (done tracing)
>                  * lockdep_hardirqs_enabled == 0 (IRQs still disabled)
>                  */
> 
>                                    irqentry_nmi_enter()
>                                        irq_state.lockdep = 0
>                                        trace(irq_disable);

				So you're saying this ^^^^^ is the
				actual problem?

> 
>                                    irqentry_nmi_exit()
>                                        // irq_state.lockdep == 0
>                                        // do not trace(irq_enable)

                                Because this ^^^^ might lead one to
				believe the lack of trace(irq_enable)
				is the problem.

>    	lockdep_hardirqs_on();
>    }


Because I'm thinking the trace(irq_disable) is actually correct. We are
entering an NMI handler, and that very much has IRQs disabled.

> Prevent this scenario by checking lockdep_hardirqs_enabled to trace also
> on nmi_entry.
> 
> Fixes: ba1f2b2eaa2a ("x86/entry: Fix NMI vs IRQ state tracking")
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Masami Hiramatsu <mhiramat@...nel.org>
> Cc: linux-trace-kernel@...r.kernel.org
> Signed-off-by: Gabriele Monaco <gmonaco@...hat.com>
> ---
>  kernel/entry/common.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index a8dd1f27417cf..7369132c00ba4 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -326,13 +326,15 @@ irqentry_state_t noinstr irqentry_nmi_enter(struct pt_regs *regs)
>  	irq_state.lockdep = lockdep_hardirqs_enabled();
>  
>  	__nmi_enter();
> -	lockdep_hardirqs_off(CALLER_ADDR0);
> +	if (irq_state.lockdep)
> +		lockdep_hardirqs_off(CALLER_ADDR0);

This isn't needed... it is perfectly fine calling lockdep_hardirq_off()
again here. You'll hit the redundant_hardirqs_off counter.

>  	lockdep_hardirq_enter();
>  	ct_nmi_enter();
>  
>  	instrumentation_begin();
>  	kmsan_unpoison_entry_regs(regs);
> -	trace_hardirqs_off_finish();
> +	if (irq_state.lockdep)
> +		trace_hardirqs_off_finish();

So I really think you're doing the wrong thing here. We traced IRQs are
enabled, but then take an NMI, meaning IRQs are very much disabled. So
we want this irqs_off to fire.

The much more fun case is:

	if (tracing_irq_cpu) {
		trace(irq_enable);
		<NMI>

Because then it will see tracing_irq_cpu set, but also have issued
irq_enable and not issue irq_disable, and then things are really messed
up.


So yes, you found a fun case, but your solution seemed aimed at pleasing
the model, rather than reality.