Re: [PATCHv2] lockdep: report broken irq restoration

[Mark Rutland]

Hi all,

Any thoughts on this? I'd like to get this in soon if we could as it'll
help to flush out any remaining issues that are liable to get in the way
of planned rework for arm64 and x86.

Thomas, are you happy to pick this?

Thanks,
Mark.

On Mon, Jan 11, 2021 at 03:37:07PM +0000, Mark Rutland wrote:
> We generally expect local_irq_save() and local_irq_restore() to be
> paired and sanely nested, and so local_irq_restore() expects to be
> called with irqs disabled. Thus, within local_irq_restore() we only
> trace irq flag changes when unmasking irqs.
> 
> This means that a sequence such as:
> 
> | local_irq_disable();
> | local_irq_save(flags);
> | local_irq_enable();
> | local_irq_restore(flags);
> 
> ... is liable to break things, as the local_irq_restore() would mask
> irqs without tracing this change. Similar problems may exist for
> architectures whose arch_irq_restore() function depends on being called
> with irqs disabled.
> 
> We don't consider such sequences to be a good idea, so let's define
> those as forbidden, and add tooling to detect such broken cases.
> 
> This patch adds debug code to WARN() when raw_local_irq_restore() is
> called with irqs enabled. As raw_local_irq_restore() is expected to pair
> with raw_local_irq_save(), it should never be called with irqs enabled.
> 
> To avoid the possibility of circular header dependencies between
> irqflags.h and bug.h, the warning is handled in a separate C file.
> 
> The new code is all conditional on a new CONFIG_DEBUG_IRQFLAGS symbol
> which is independent of CONFIG_TRACE_IRQFLAGS. As noted above such cases
> will confuse lockdep, so CONFIG_DEBUG_LOCKDEP now selects
> CONFIG_DEBUG_IRQFLAGS.
> 
> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  include/linux/irqflags.h       | 12 ++++++++++++
>  kernel/locking/Makefile        |  1 +
>  kernel/locking/irqflag-debug.c | 11 +++++++++++
>  lib/Kconfig.debug              |  8 ++++++++
>  4 files changed, 32 insertions(+)
>  create mode 100644 kernel/locking/irqflag-debug.c
> 
> Since v1 [1]:
> * Remove per-instance bool in favour of a single WARN_ONCE()
> * Move check into raw_local_irq_restore()
> * Update Kconfig text and cover
> 
> [1] https://lore.kernel.org/r/20201209183337.1912-1-mark.rutland@xxxxxxx
> 
> Mark.
> 
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index 8de0e1373de7..600c10da321a 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -149,6 +149,17 @@ do {						\
>  # define start_critical_timings() do { } while (0)
>  #endif
>  
> +#ifdef CONFIG_DEBUG_IRQFLAGS
> +extern void warn_bogus_irq_restore(void);
> +#define raw_check_bogus_irq_restore()			\
> +	do {						\
> +		if (unlikely(!arch_irqs_disabled()))	\
> +			warn_bogus_irq_restore();	\
> +	} while (0)
> +#else
> +#define raw_check_bogus_irq_restore() do { } while (0)
> +#endif
> +
>  /*
>   * Wrap the arch provided IRQ routines to provide appropriate checks.
>   */
> @@ -162,6 +173,7 @@ do {						\
>  #define raw_local_irq_restore(flags)			\
>  	do {						\
>  		typecheck(unsigned long, flags);	\
> +		raw_check_bogus_irq_restore();		\
>  		arch_local_irq_restore(flags);		\
>  	} while (0)
>  #define raw_local_save_flags(flags)			\
> diff --git a/kernel/locking/Makefile b/kernel/locking/Makefile
> index 6d11cfb9b41f..8838f1d7c4a2 100644
> --- a/kernel/locking/Makefile
> +++ b/kernel/locking/Makefile
> @@ -15,6 +15,7 @@ CFLAGS_REMOVE_mutex-debug.o = $(CC_FLAGS_FTRACE)
>  CFLAGS_REMOVE_rtmutex-debug.o = $(CC_FLAGS_FTRACE)
>  endif
>  
> +obj-$(CONFIG_DEBUG_IRQFLAGS) += irqflag-debug.o
>  obj-$(CONFIG_DEBUG_MUTEXES) += mutex-debug.o
>  obj-$(CONFIG_LOCKDEP) += lockdep.o
>  ifeq ($(CONFIG_PROC_FS),y)
> diff --git a/kernel/locking/irqflag-debug.c b/kernel/locking/irqflag-debug.c
> new file mode 100644
> index 000000000000..9603d207a4ca
> --- /dev/null
> +++ b/kernel/locking/irqflag-debug.c
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/bug.h>
> +#include <linux/export.h>
> +#include <linux/irqflags.h>
> +
> +void warn_bogus_irq_restore(void)
> +{
> +	WARN_ONCE(1, "raw_local_irq_restore() called with IRQs enabled\n");
> +}
> +EXPORT_SYMBOL(warn_bogus_irq_restore);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 7937265ef879..5ea0c1773b0a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1335,6 +1335,7 @@ config LOCKDEP_SMALL
>  config DEBUG_LOCKDEP
>  	bool "Lock dependency engine debugging"
>  	depends on DEBUG_KERNEL && LOCKDEP
> +	select DEBUG_IRQFLAGS
>  	help
>  	  If you say Y here, the lock dependency engine will do
>  	  additional runtime checks to debug itself, at the price
> @@ -1423,6 +1424,13 @@ config TRACE_IRQFLAGS_NMI
>  	depends on TRACE_IRQFLAGS
>  	depends on TRACE_IRQFLAGS_NMI_SUPPORT
>  
> +config DEBUG_IRQFLAGS
> +	bool "Debug IRQ flag manipulation"
> +	help
> +	  Enables checks for potentially unsafe enabling or disabling of
> +	  interrupts, such as calling raw_local_irq_restore() when interrupts
> +	  are enabled.
> +
>  config STACKTRACE
>  	bool "Stack backtrace support"
>  	depends on STACKTRACE_SUPPORT
> -- 
> 2.11.0
>