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

[Mark Rutland]

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@xxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx

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@xxxxxxx>

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;