Re: [PATCH bpf] x86/bpf: handle bpf-program-triggered exceptions properly

[Masami Hiramatsu <mhiramat@kernel.org>]

On Fri, 29 Jan 2021 11:08:07 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Jan 29, 2021 at 10:44:48AM +0100, Peter Zijlstra wrote:
> > There is one case where it hijacks the fault entirely, and I'm tempted
> > to rip that out, that's just gross. Also, it seems entirely unused in-kernel.
> 
> Masami, please explain why the below isn't appropriate.
> 
> ---
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index df776cdca327..86cd8f15a978 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -949,9 +949,13 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
>  		 * if handler tries to access user space by
>  		 * copy_from_user(), get_user() etc. Let the
>  		 * user-specified handler try to fix it first.
> -		 */
> +		 *
> +		 * Which is a bloody stupid thing to do from non-preemptible code
> +		 * so why should we support idiocy like that.
> +		 *
>  		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
>  			return 1;
> +		 */
>  	}

Hmm, good point. This is a fail-safe code which, as far as I know, systemtap
uses this hook to count faults and notify user an error (e.g. guru-mode).

I just maintained it to preserve the use case. Actually, in the kernel there
is no fault handler user. e.g. kprobe tracer uses non-fault (safe) kernel
memory access functions.

>  
>  	return 0;
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 106b22d1d189..817a93da794e 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1186,7 +1186,7 @@ do_kern_addr_fault(struct pt_regs *regs, unsigned long hw_error_code,
>  		return;
>  
>  	/* kprobes don't want to hook the spurious faults: */
> -	if (kprobe_page_fault(regs, X86_TRAP_PF))
> +	if (WARN_ON_ONCE(kprobe_page_fault(regs, X86_TRAP_PF)))
>  		return;
>  
>  	/*
> @@ -1217,7 +1217,7 @@ void do_user_addr_fault(struct pt_regs *regs,
>  	mm = tsk->mm;
>  
>  	/* kprobes don't want to hook the spurious faults: */
> -	if (unlikely(kprobe_page_fault(regs, X86_TRAP_PF)))
> +	if (WARN_ON_ONCE(kprobe_page_fault(regs, X86_TRAP_PF)))
>  		return;
>  
>  	/*


-- 
Masami Hiramatsu <mhiramat@kernel.org>