Re: [PATCH 1/3] X86/db: Change __this_cpu_read() to this_cpu_read() in hw_breakpoint_active()

Peter Zijlstra
2021-12-13T19:46:44Z

On Mon, Dec 13, 2021 at 12:22:13PM +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
>
> __this_cpu_read() can not be instrumented except its own debugging code
> when CONFIG_DEBUG_PREEMPT. The debugging code will call
> __this_cpu_preempt_check(). __this_cpu_preempt_check() itself is also
> noinstr, so __this_cpu_read() can be used in noinstr.
>
> But these is one exception when exc_debug_kernel() calls local_db_save()
> which calls hw_breakpoint_active() which calls __this_cpu_read(). If
> the data accessed by __this_cpu_preempt_check() is also watched by
> hw_breakpoints, it would cause recursive #DB.
>
> this_cpu_read() in X86 is also non instrumentable, and it doesn't access
> to any extra data except the percpu cpu_dr7, and cpu_dr7 is disallowed
> to be watched in arch_build_bp_info(). So this_cpu_read() is safe to
> be used when hw_breakpoints is still active, and __this_cpu_read() here
> should be changed to this_cpu_read().
>
> This problem can only happen when the system owner uses a kernel with
> CONFIG_DEBUG_PREEMPT enabled and deliberately use hw_breakpoints on
> the data that __this_cpu_preempt_check() accesses. Sot it is just a
> problem with no significance.
>
> One might suggest that, all the data accessed by noinstr functions
> should be marked in denylist for hw_breakpoints. That would complexify
> the noinstrment framework and add hurdles to anyone that who want to
> add a new noinstr function. All we need is to suppress #DB in the IST
> interrupt entry path until safe place where #DB is disabled in hardware
> or #DB handler can handle well even it hits data accessed by noinstr
> function. Changing __this_cpu_read() to this_cpu_read() is fit for it.
>
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
> arch/x86/include/asm/debugreg.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> index cfdf307ddc01..20189ce41578 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
> @@ -87,7 +87,7 @@ static inline void hw_breakpoint_disable(void)
>
> static __always_inline bool hw_breakpoint_active(void)
> {
> - return __this_cpu_read(cpu_dr7) & DR_GLOBAL_ENABLE_MASK;
> + return this_cpu_read(cpu_dr7) & DR_GLOBAL_ENABLE_MASK;

I don't really follow the argument for why this_cpu_read(); why not
raw_cpu_read() instead, which is what __this_cpu_read() is based on.
Also, this really needs a comment.

Alternatively, we should remove noinstr from check_preemption_disabled()
and fix up all the fallout, but that seems like far more work than it's
worth.

/*
* Must not hit a breakpoint in check_preempt_disabled()
*/
return raw_cpu_read(cpu_dr7) & DR_GLOBAL_ENABLE_MASK;

> }
>
> extern void hw_breakpoint_restore(void);
> --
> 2.19.1.6.gb485710b
>