linux-kernel - Re: entry: Fix the incorrect ordering of lockdep and RCU check

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]

Date:   Wed, 4 Nov 2020 16:29:18 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Paul E. McKenney" <paulmck@...nel.org>, x86@...nel.org
Subject: Re: entry: Fix the incorrect ordering of lockdep and RCU check

On Wed, Nov 04, 2020 at 02:06:23PM +0100, Thomas Gleixner wrote:
> When an exception/interrupt hits kernel space and the kernel is not
> currently in the idle task then RCU must be watching.
> 
> irqentry_enter() validates this via rcu_irq_enter_check_tick(), which in
> turn invokes lockdep when taking a lock. But at that point lockdep does not
> yet know about the fact that interrupts have been disabled by the CPU,
> which triggers a lockdep splat complaining about inconsistent state.
> 
> Invoking trace_hardirqs_off() before rcu_irq_enter_check_tick() defeats the
> point of rcu_irq_enter_check_tick() because trace_hardirqs_off() uses RCU.
> 
> So use the same sequence as for the idle case and tell lockdep about the
> irq state change first, invoke the RCU check and then do the lockdep and
> tracer update.
> 
> Fixes: a5497bab5f72 ("entry: Provide generic interrupt entry/exit code")
> Reported-by: Mark Rutland <mark.rutland@....com>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: stable@...r.kernel.org

I just gave this a spin on x86_64 defconfig + PROVE_LOCKING +
DEBUG_LOCKDEP + NO_HZ_FULL + CONTEXT_TRACKING_FORCE, and it gets rid of
the boot-time splat. So FWIW:

Tested-by: Mark Rutland <mark.rutland@....com>

Mark.

> ---
>  kernel/entry/common.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -337,10 +337,10 @@ noinstr irqentry_state_t irqentry_enter(
>  	 * already contains a warning when RCU is not watching, so no point
>  	 * in having another one here.
>  	 */
> +	lockdep_hardirqs_off(CALLER_ADDR0);
>  	instrumentation_begin();
>  	rcu_irq_enter_check_tick();
> -	/* Use the combo lockdep/tracing function */
> -	trace_hardirqs_off();
> +	trace_hardirqs_off_finish();
>  	instrumentation_end();
>  
>  	return ret;