Re: [for-next][PATCH 04/18] x86/ftrace: Do not jump to direct code in created trampolines

[Steven Rostedt]

On Fri, 3 Jul 2020 10:10:00 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Thu, Jul 02, 2020 at 05:58:16PM -0400, Steven Rostedt wrote:
> 
> > +	/* No need to test direct calls on created trampolines */
> > +	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > +		/* NOP the jnz 1f; but make sure it's a 2 byte jnz */
> > +		ip = trampoline + (jmp_offset - start_offset);
> > +		if (WARN_ON(*(char *)ip != 0x75))
> > +			goto fail;
> > +		ret = copy_from_kernel_nofault(ip, ideal_nops[2], 2);  
> 
> I really don't get this paranoia, what's wrong with memcpy() ?

Habit. As when ftrace was introduced, it was extremely careful about
touching memory like this. And even with all of that extra care, we
still broke NICs (actually, some of the reason those NICs broke was
because of the extra care we took :-p)

> 
> > +		if (ret < 0)
> > +			goto fail;
> > +	}  
> 
> How about something like this?
> 
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -359,17 +359,11 @@ create_trampoline(struct ftrace_ops *ops
>  	npages = DIV_ROUND_UP(*tramp_size, PAGE_SIZE);
>  
>  	/* Copy ftrace_caller onto the trampoline memory */
> -	ret = copy_from_kernel_nofault(trampoline, (void *)start_offset, size);
> -	if (WARN_ON(ret < 0))
> -		goto fail;
> -
> -	ip = trampoline + size;
> +	memcpy(trampoline, (void *)start_offset, size);
>  
>  	/* The trampoline ends with ret(q) */
> -	retq = (unsigned long)ftrace_stub;
> -	ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE);
> -	if (WARN_ON(ret < 0))
> -		goto fail;
> +	ip = trampoline + size;
> +	memcpy(ip, text_gen_insn(RET_INSN_OPCODE, NULL, NULL), RET_INSN_SIZE);
>  
>  	/* No need to test direct calls on created trampolines */
>  	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> @@ -377,9 +371,7 @@ create_trampoline(struct ftrace_ops *ops
>  		ip = trampoline + (jmp_offset - start_offset);
>  		if (WARN_ON(*(char *)ip != 0x75))
>  			goto fail;
> -		ret = copy_from_kernel_nofault(ip, ideal_nops[2], 2);
> -		if (ret < 0)
> -			goto fail;
> +		memcpy(ip, ideal_nops[2], 2);

If you want to add this change on top of this, then I'm fine with that.

If it breaks something, I can at least point the blame at you ;-)

-- Steve


>  	}
>  
>  	/*