Re: [PATCH v5 2/4] x86/traps: Print address on #GP

[Sean Christopherson]

On Thu, Nov 28, 2019 at 12:49:14AM +0100, Jann Horn wrote:

With a few nits below,

Reviewed-and-tested-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>

> +#define GPFSTR "general protection fault"
> +
>  dotraplinkage void
>  do_general_protection(struct pt_regs *regs, long error_code)
>  {
> -	const char *desc = "general protection fault";
>  	struct task_struct *tsk;
> +	char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;

Nit, x86 maintainers prefer inverse fir tree for variable declarations.

>  
>  	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
>  	cond_local_irq_enable(regs);
> @@ -540,6 +587,9 @@ do_general_protection(struct pt_regs *regs, long error_code)
>  
>  	tsk = current;
>  	if (!user_mode(regs)) {
> +		enum kernel_gp_hint hint = GP_NO_HINT;
> +		unsigned long gp_addr;
> +
>  		if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
>  			return;
>  
> @@ -556,8 +606,22 @@ do_general_protection(struct pt_regs *regs, long error_code)
>  			return;
>  
>  		if (notify_die(DIE_GPF, desc, regs, error_code,
> -			       X86_TRAP_GP, SIGSEGV) != NOTIFY_STOP)
> -			die(desc, regs, error_code);
> +			       X86_TRAP_GP, SIGSEGV) == NOTIFY_STOP)
> +			return;
> +
> +		if (error_code)
> +			snprintf(desc, sizeof(desc), "segment-related " GPFSTR);
> +		else
> +			hint = get_kernel_gp_address(regs, &gp_addr);
> +
> +		if (hint != GP_NO_HINT)
> +			snprintf(desc, sizeof(desc), GPFSTR " %s 0x%lx",

Nit, probably should have a comma before the hint, i.e. GPFSTR ", %s...".

    general protection fault maybe for address 0xffffc9000017cf58: 0000 [#1] SMP
    general protection fault probably for non-canonical address 0xdead000000000000: 0000 [#1] SMP

  vs. 

    general protection fault, maybe for address 0xffffc9000017cf58: 0000 [#1] SMP
    general protection fault, probably for non-canonical address 0xdead000000000000: 0000 [#1] SMP

> +				 (hint == GP_NON_CANONICAL) ?
> +				 "probably for non-canonical address" :
> +				 "maybe for address",
> +				 gp_addr);
> +
> +		die(desc, regs, error_code);
>  		return;
>  	}
>  
> -- 
> 2.24.0.432.g9d3f5f5b63-goog
>