Re: [PATCH 03/15] x86/split_lock: Handle #AC exception for split lock in kernel mode

[Dave Hansen <dave.hansen@intel.com>]

On 05/14/2018 11:52 AM, Fenghua Yu wrote:
> +#define	delay_ms	1

That seems like a dangerously-generic name that should not be a #define
anyway.

> +static void delayed_reenable_split_lock(struct work_struct *w)
> +{
> +	if (split_lock_ac == ENABLE_SPLIT_LOCK_AC)
> +		_setup_split_lock(ENABLE_SPLIT_LOCK_AC);
> +}

This seems like it might get confusing.  We have the split lock ac
*mode* (what the kernel is doing overall) and also the *status* (what
mode the CPU is in at the moment).

The naming here doesn't really split up those two concepts very well.

> +/* Will the faulting instruction be re-executed? */
> +static bool re_execute(struct pt_regs *regs)
> +{
> +	/*
> +	 * The only reason for generating #AC from kernel is because of
> +	 * split lock. The kernel faulting instruction will be re-executed.
> +	 */
> +	if (!user_mode(regs))
> +		return true;
> +
> +	return false;
> +}

This helper with a single user is a bit unnecessary.  Just open-code
this and move the comments into the caller.

> +/*
> + * #AC handler for kernel split lock is called by generic #AC handler.
> + *
> + * Disable #AC for split lock on this CPU so that the faulting instruction
> + * gets executed. The #AC for split lock is re-enabled later.
> + */
> +bool do_split_lock_exception(struct pt_regs *regs, unsigned long error_code)
> +{
> +	unsigned long delay = msecs_to_jiffies(delay_ms);
> +	unsigned long address = read_cr2(); /* Get the faulting address */
> +	int this_cpu = smp_processor_id();

How does this end up working?  This seems to depend on this handler not
getting preempted.

> +	if (!re_execute(regs))
> +		return false;
> +
> +	pr_info_ratelimited("Alignment check for split lock at %lx\n", address);

This is a potential KASLR bypass, I believe.  We shouldn't be printing
raw kernel addresses.

We have some nice printk's for page faults that give you kernel symbols.
 Could you copy one of those?

> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 03f3d7695dac..c07b817bbbe9 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>
> @@ -286,10 +287,21 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
>  			  unsigned long trapnr, int signr)
>  {
>  	siginfo_t info;
> +	int ret;
>  
>  	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
>  
>  	/*
> +	 * #AC exception could be handled by split lock handler.
> +	 * If the handler can't handle the exception, go to generic #AC handler.
> +	 */
> +	if (trapnr == X86_TRAP_AC) {
> +		ret = do_split_lock_exception(regs, error_code);
> +		if (ret)
> +			return;
> +	}

Why are you hooking into do_error_trap()?  Shouldn't you just be
installing do_split_lock_exception() as *the* #AC handler and put it in
the IDT?