Re: [PATCH 10/19] bpf: Allow to store caller's ip as argument

From: Yonghong Song <yhs@fb.com>
To: Jiri Olsa <jolsa@kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andriin@fb.com>,
	"Steven Rostedt (VMware)" <rostedt@goodmis.org>
Cc: <netdev@vger.kernel.org>, <bpf@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>, Daniel Xu <dxu@dxuuu.xyz>,
	Viktor Malik <vmalik@redhat.com>
Subject: Re: [PATCH 10/19] bpf: Allow to store caller's ip as argument
Date: Sun, 6 Jun 2021 20:21:51 -0700	[thread overview]
Message-ID: <03777e58-a2b0-83b5-959f-3ae4afadb191@fb.com> (raw)
In-Reply-To: <20210605111034.1810858-11-jolsa@kernel.org>

On 6/5/21 4:10 AM, Jiri Olsa wrote:
> When we will have multiple functions attached to trampoline
> we need to propagate the function's address to the bpf program.
> 
> Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline
> function that will store origin caller's address before function's
> arguments.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++----
>   include/linux/bpf.h         |  5 +++++
>   2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index b77e6bd78354..d2425c18272a 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1951,7 +1951,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>   				void *orig_call)
>   {
>   	int ret, i, cnt = 0, nr_args = m->nr_args;
> -	int stack_size = nr_args * 8;
> +	int stack_size = nr_args * 8, ip_arg = 0;
>   	struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY];
>   	struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT];
>   	struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];
> @@ -1975,6 +1975,9 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>   		 */
>   		orig_call += X86_PATCH_SIZE;
>   
> +	if (flags & BPF_TRAMP_F_IP_ARG)
> +		stack_size += 8;
> +
>   	prog = image;
>   
>   	EMIT1(0x55);		 /* push rbp */
> @@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>   	EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */
>   	EMIT1(0x53);		 /* push rbx */
>   
> -	save_regs(m, &prog, nr_args, stack_size);
> +	if (flags & BPF_TRAMP_F_IP_ARG) {
> +		emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
> +		EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE); /* sub $X86_PATCH_SIZE,%rax*/

Could you explain what the above EMIT4 is for? I am not quite familiar 
with this piece of code and hence the question. Some comments here
should help too.

> +		emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_size);
> +		ip_arg = 8;
> +	}
> +
> +	save_regs(m, &prog, nr_args, stack_size - ip_arg);
>   
>   	if (flags & BPF_TRAMP_F_CALL_ORIG) {
>   		/* arg1: mov rdi, im */
> @@ -2011,7 +2021,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>   	}
>   
>   	if (flags & BPF_TRAMP_F_CALL_ORIG) {
> -		restore_regs(m, &prog, nr_args, stack_size);
> +		restore_regs(m, &prog, nr_args, stack_size - ip_arg);
>   
>   		if (flags & BPF_TRAMP_F_ORIG_STACK) {
>   			emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
> @@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>   		}
>   
>   	if (flags & BPF_TRAMP_F_RESTORE_REGS)
> -		restore_regs(m, &prog, nr_args, stack_size);
> +		restore_regs(m, &prog, nr_args, stack_size - ip_arg);
>   
>   	/* This needs to be done regardless. If there were fmod_ret programs,
>   	 * the return value is only updated on the stack and still needs to be
[...]