linux-kernel - Re: [PATCH v2 3/4] x86/split_lock: Handle #AC exception for split lock

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f528ef21-0e6f-3c76-5fac-bd88279facc0@intel.com>
Date:   Fri, 29 Jun 2018 09:29:03 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, H Peter Anvin <hpa@...or.com>
Cc:     Ashok Raj <ashok.raj@...el.com>, Alan Cox <alan@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Rafael Wysocki <rafael.j.wysocki@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v2 3/4] x86/split_lock: Handle #AC exception for split
 lock

> +/*
> + * #AC handler for split lock is called by generic #AC handler.
> + *
> + * On split lock in kernel, warn and disable #AC for split lock on current CPU.
> + *
> + * On split lock in user process, send SIGBUS in the generic #AC handler.
> + */

Don't comment the function, comment the code, please.  The thing that
needs to be here that is missing is what the return values mean.

> +bool do_ac_split_lock(struct pt_regs *regs)
> +{
> +	/* Generic #AC handler will handle split lock in user. */
> +	if (user_mode(regs))
> +		return false;
> +
> +	/* Clear the split lock bit to disable the feature on local CPU. */
> +	msr_clear_bit(MSR_TEST_CTL, MSR_TEST_CTL_ENABLE_AC_SPLIT_LOCK_SHIFT);
> +
> +	WARN_ONCE(1, "A split lock issue is detected. Please FIX it\n");

I think folks understand that warnings need to get fixed.  They don't
need to be urged to FIX IT IN CAPS, or asked nicely.  This can simply be:

	"lock split across cacheline boundary"

But, warning here is also not super useful.  Shouldn't we be dumping out
the info in 'regs' instead of the current context?  We don't care about
the state in the #AC handler, we care about 'regs'.


> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 18de4e35a4e5..ca4ef8325dfe 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -954,6 +954,8 @@ void __init setup_arch(char **cmdline_p)
>  	parse_early_param();
>  
>  	detect_ac_split_lock();

This ^ needs:

	/* Do detection only on the boot cpu. */

> +	/* Set up #AC for split lock at the earliest phase. */
> +	setup_ac_split_lock();
>  
>  	if (efi_enabled(EFI_BOOT))
>  		efi_memblock_x86_reserve_range();
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index c2f7d1d2a5c3..d6b224e6284f 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -225,6 +225,9 @@ static void notrace start_secondary(void *unused)
>  #endif
>  	load_current_idt();
>  	cpu_init();
> +
> +	setup_ac_split_lock();

and:

	/* Feature detection was done on the boot cpu, only do setup */

> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index e6db475164ed..dd309a7b46bd 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -61,6 +61,7 @@
>  #include <asm/mpx.h>
>  #include <asm/vm86.h>
>  #include <asm/umip.h>
> +#include <asm/cpu.h>
>  
>  #ifdef CONFIG_X86_64
>  #include <asm/x86_init.h>
> @@ -318,7 +319,36 @@ DO_ERROR(X86_TRAP_OLD_MF, SIGFPE,  "coprocessor segment overrun",coprocessor_seg
>  DO_ERROR(X86_TRAP_TS,     SIGSEGV, "invalid TSS",		invalid_TSS)
>  DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present",	segment_not_present)
>  DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",		stack_segment)
> -DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",		alignment_check)
> +
> +dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code)
> +{
> +	unsigned int trapnr = X86_TRAP_AC;
> +	char str[] = "alignment check";
> +	int signr = SIGBUS;
> +	siginfo_t info;
> +	int ret;
> +
> +	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> +
> +	if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
> +			NOTIFY_STOP) {

There does not seem to be a _lot_ of consistency for whether
notify_die() gets called before or after kernel exceptions are handled.
Why did you choose to do it this way?

Also, please unindent this block and just return if notify_die() returns
false.

> +		/* #AC exception could be handled by split lock handler. */
> +		ret = do_ac_split_lock(regs);
> +		if (ret) {
> +			cond_local_irq_enable(regs);
> +
> +			return;
> +		}
> +
> +		cond_local_irq_enable(regs);

FWIW, you can consolidate the cond_local_irq_enable() calls:

	ret = do_ac_split_lock(regs);
	cond_local_irq_enable(regs);
	if (ret)
		return;

	...