linux-kernel - Re: [RFC][PATCH 2/3] locking,entry: #PF vs TRACE

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200810134938.GX2674@hirez.programming.kicks-ass.net>
Date:   Mon, 10 Aug 2020 15:49:38 +0200
From:   peterz@...radead.org
To:     tglx@...utronix.de, mingo@...nel.org, will@...nel.org
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, elver@...gle.com,
        paulmck@...nel.org, rostedt@...dmis.org, rjw@...ysocki.net
Subject: Re: [RFC][PATCH 2/3] locking,entry: #PF vs TRACE_IRQFLAGS

On Fri, Aug 07, 2020 at 09:23:38PM +0200, Peter Zijlstra wrote:
> Much of the complexity in irqenter_{enter,exit}() is due to #PF being
> the sole exception that can schedule from kernel context.
> 
> One additional wrinkle with #PF is that it is non-maskable, it can
> happen _anywhere_. Due to this, and the wonders of tracing, we can get
> the 'normal' NMI nesting vs TRACE_IRQFLAGS:
> 
> 	local_irq_disable()
> 	  raw_local_irq_disable();
> 	  trace_hardirqs_off();
> 
> 	local_irq_enable();
> 	  trace_hardirqs_on();
> 	  <#PF>
> 	    trace_hardirqs_off()
> 	    ...
> 	    if (!regs_irqs_disabled(regs)
> 	      trace_hardirqs_on();
> 	  </#PF>
> 	  // WHOOPS -- lockdep thinks IRQs are disabled again!
> 	  raw_local_irqs_enable();
> 
> Rework irqenter_{enter,exit}() to save/restore the software state.

So with #3 v1.1, we can maybe do away with this patch.

So the actual case that triggered the above was:


	raw_local_irq_disable();
	trace_lock_acquire()
	  ... tracing ...
	  <#PF/>

Now, as Marco spotted, DEBUG_LOCKDEP would trigger in this case, because
'... tracing ...' includes rcu_dereference(), and that in turn calls
lock_is_held()/check_flags() and goes *boom*, because we did
raw_local_irq_disable().

Now, the new patch, moves the tracepoint out from under the
raw_local_irq_disable() too, and given RCU-lockdep complains in this
situation, I'm thinking we're actually free of such cases... fingers
crossed.