mbox series

[RFC,bpf-next,0/5] bpf, x86: Add bpf_get_func_ip helper

Message ID 20210629192945.1071862-1-jolsa@kernel.org
Headers show
Series bpf, x86: Add bpf_get_func_ip helper | expand

Message

Jiri Olsa June 29, 2021, 7:29 p.m. UTC 
hi,
adding bpf_get_func_ip helper that returns IP address of the
caller function for trampoline and krobe programs.

There're 2 specific implementation of the bpf_get_func_ip
helper, one for trampoline progs and one for kprobe/kretprobe
progs.

The trampoline helper call is replaced/inlined by verifier
with simple move instruction. The kprobe/kretprobe is actual
helper call that returns prepared caller address.

The trampoline extra 3 instructions for storing IP address
is now optional, which I'm not completely sure is necessary,
so I plan to do some benchmarks, if it's noticeable, hence
the RFC. I'm also not completely sure about the kprobe/kretprobe
implementation.

Also available at:
  https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  bpf/get_func_ip

thanks,
jirka


---
Jiri Olsa (5):
      bpf, x86: Store caller's ip in trampoline stack
      bpf: Enable BPF_TRAMP_F_IP_ARG for trampolines with call_get_func_ip
      bpf: Add bpf_get_func_ip helper for tracing programs
      bpf: Add bpf_get_func_ip helper for kprobe programs
      selftests/bpf: Add test for bpf_get_func_ip helper

 arch/x86/net/bpf_jit_comp.c                               | 19 +++++++++++++++++++
 include/linux/bpf.h                                       |  5 +++++
 include/linux/filter.h                                    |  3 ++-
 include/uapi/linux/bpf.h                                  |  7 +++++++
 kernel/bpf/trampoline.c                                   | 12 +++++++++---
 kernel/bpf/verifier.c                                     | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/trace/bpf_trace.c                                  | 29 +++++++++++++++++++++++++++++
 kernel/trace/trace_kprobe.c                               | 20 ++++++++++++++++++--
 kernel/trace/trace_probe.h                                |  5 +++++
 tools/include/uapi/linux/bpf.h                            |  7 +++++++
 tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/get_func_ip_test.c      | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 12 files changed, 260 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
 create mode 100644 tools/testing/selftests/bpf/progs/get_func_ip_test.c

Comments

Yonghong Song June 30, 2021, 5:47 p.m. UTC | #1 
On 6/29/21 12:29 PM, Jiri Olsa wrote:
> Adding bpf_get_func_ip helper for BPF_PROG_TYPE_KPROBE programs,

> so it's now possible to call bpf_get_func_ip from both kprobe and

> kretprobe programs.

> 

> Taking the caller's address from 'struct kprobe::addr', which is

> defined for both kprobe and kretprobe.

> 

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

> ---

>   include/uapi/linux/bpf.h       |  2 +-

>   kernel/bpf/verifier.c          |  2 ++

>   kernel/trace/bpf_trace.c       | 14 ++++++++++++++

>   kernel/trace/trace_kprobe.c    | 20 ++++++++++++++++++--

>   kernel/trace/trace_probe.h     |  5 +++++

>   tools/include/uapi/linux/bpf.h |  2 +-

>   6 files changed, 41 insertions(+), 4 deletions(-)

> 

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h

> index 83e87ffdbb6e..4894f99a1993 100644

> --- a/include/uapi/linux/bpf.h

> +++ b/include/uapi/linux/bpf.h

> @@ -4783,7 +4783,7 @@ union bpf_attr {

>    *

>    * u64 bpf_get_func_ip(void *ctx)

>    * 	Description

> - * 		Get address of the traced function (for tracing programs).

> + * 		Get address of the traced function (for tracing and kprobe programs).

>    * 	Return

>    * 		Address of the traced function.

>    */

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c

> index 701ff7384fa7..b66e0a7104f8 100644

> --- a/kernel/bpf/verifier.c

> +++ b/kernel/bpf/verifier.c

> @@ -5979,6 +5979,8 @@ static bool has_get_func_ip(struct bpf_verifier_env *env)

>   			return -ENOTSUPP;

>   		}

>   		return 0;

> +	} else if (type == BPF_PROG_TYPE_KPROBE) {

> +		return 0;

>   	}

>   

>   	verbose(env, "func %s#%d not supported for program type %d\n",

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c

> index 9edd3b1a00ad..1a5bddce9abd 100644

> --- a/kernel/trace/bpf_trace.c

> +++ b/kernel/trace/bpf_trace.c

> @@ -961,6 +961,18 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_tracing = {

>   	.arg1_type	= ARG_PTR_TO_CTX,

>   };

>   

> +BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)

> +{

> +	return trace_current_kprobe_addr();

> +}

> +

> +static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {

> +	.func		= bpf_get_func_ip_kprobe,

> +	.gpl_only	= true,

> +	.ret_type	= RET_INTEGER,

> +	.arg1_type	= ARG_PTR_TO_CTX,

> +};

> +

>   const struct bpf_func_proto *

>   bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

>   {

> @@ -1092,6 +1104,8 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

>   	case BPF_FUNC_override_return:

>   		return &bpf_override_return_proto;

>   #endif

> +	case BPF_FUNC_get_func_ip:

> +		return &bpf_get_func_ip_proto_kprobe;

>   	default:

>   		return bpf_tracing_func_proto(func_id, prog);

>   	}

> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c

> index ea6178cb5e33..b07d5888db14 100644

> --- a/kernel/trace/trace_kprobe.c

> +++ b/kernel/trace/trace_kprobe.c

> @@ -1570,6 +1570,18 @@ static int kretprobe_event_define_fields(struct trace_event_call *event_call)

>   }

>   

>   #ifdef CONFIG_PERF_EVENTS

> +/* Used by bpf get_func_ip helper */

> +DEFINE_PER_CPU(u64, current_kprobe_addr) = 0;


Didn't check other architectures. But this should work
for x86 where if nested kprobe happens, the second
kprobe will not call kprobe handlers.

This essentially is to provide an additional parameter to
bpf program. Andrii is developing a mechanism to
save arbitrary data in *current task_struct*, which
might be used here to save current_kprobe_addr, we can
save one per cpu variable.

> +

> +u64 trace_current_kprobe_addr(void)

> +{

> +	return *this_cpu_ptr(&current_kprobe_addr);

> +}

> +

> +static void trace_current_kprobe_set(struct trace_kprobe *tk)

> +{

> +	__this_cpu_write(current_kprobe_addr, (u64) tk->rp.kp.addr);

> +}

>   

>   /* Kprobe profile handler */

>   static int

> @@ -1585,6 +1597,7 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)

>   		unsigned long orig_ip = instruction_pointer(regs);

>   		int ret;

>   

> +		trace_current_kprobe_set(tk);

>   		ret = trace_call_bpf(call, regs);

>   

>   		/*

> @@ -1631,8 +1644,11 @@ kretprobe_perf_func(struct trace_kprobe *tk, struct kretprobe_instance *ri,

>   	int size, __size, dsize;

>   	int rctx;

>   

> -	if (bpf_prog_array_valid(call) && !trace_call_bpf(call, regs))

> -		return;

> +	if (bpf_prog_array_valid(call)) {

> +		trace_current_kprobe_set(tk);

> +		if (!trace_call_bpf(call, regs))

> +			return;

> +	}

>   

>   	head = this_cpu_ptr(call->perf_events);

>   	if (hlist_empty(head))

[...]
Masami Hiramatsu (Google) June 30, 2021, 11:58 p.m. UTC | #2 
On Wed, 30 Jun 2021 10:47:01 -0700
Yonghong Song <yhs@fb.com> wrote:

> 
> 
> On 6/29/21 12:29 PM, Jiri Olsa wrote:
> > Adding bpf_get_func_ip helper for BPF_PROG_TYPE_KPROBE programs,
> > so it's now possible to call bpf_get_func_ip from both kprobe and
> > kretprobe programs.
> > 
> > Taking the caller's address from 'struct kprobe::addr', which is
> > defined for both kprobe and kretprobe.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   include/uapi/linux/bpf.h       |  2 +-
> >   kernel/bpf/verifier.c          |  2 ++
> >   kernel/trace/bpf_trace.c       | 14 ++++++++++++++
> >   kernel/trace/trace_kprobe.c    | 20 ++++++++++++++++++--
> >   kernel/trace/trace_probe.h     |  5 +++++
> >   tools/include/uapi/linux/bpf.h |  2 +-
> >   6 files changed, 41 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 83e87ffdbb6e..4894f99a1993 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -4783,7 +4783,7 @@ union bpf_attr {
> >    *
> >    * u64 bpf_get_func_ip(void *ctx)
> >    * 	Description
> > - * 		Get address of the traced function (for tracing programs).
> > + * 		Get address of the traced function (for tracing and kprobe programs).
> >    * 	Return
> >    * 		Address of the traced function.
> >    */
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 701ff7384fa7..b66e0a7104f8 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5979,6 +5979,8 @@ static bool has_get_func_ip(struct bpf_verifier_env *env)
> >   			return -ENOTSUPP;
> >   		}
> >   		return 0;
> > +	} else if (type == BPF_PROG_TYPE_KPROBE) {
> > +		return 0;
> >   	}
> >   
> >   	verbose(env, "func %s#%d not supported for program type %d\n",
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 9edd3b1a00ad..1a5bddce9abd 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -961,6 +961,18 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_tracing = {
> >   	.arg1_type	= ARG_PTR_TO_CTX,
> >   };
> >   
> > +BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
> > +{
> > +	return trace_current_kprobe_addr();
> > +}
> > +
> > +static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {
> > +	.func		= bpf_get_func_ip_kprobe,
> > +	.gpl_only	= true,
> > +	.ret_type	= RET_INTEGER,
> > +	.arg1_type	= ARG_PTR_TO_CTX,
> > +};
> > +
> >   const struct bpf_func_proto *
> >   bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >   {
> > @@ -1092,6 +1104,8 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >   	case BPF_FUNC_override_return:
> >   		return &bpf_override_return_proto;
> >   #endif
> > +	case BPF_FUNC_get_func_ip:
> > +		return &bpf_get_func_ip_proto_kprobe;
> >   	default:
> >   		return bpf_tracing_func_proto(func_id, prog);
> >   	}
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index ea6178cb5e33..b07d5888db14 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -1570,6 +1570,18 @@ static int kretprobe_event_define_fields(struct trace_event_call *event_call)
> >   }
> >   
> >   #ifdef CONFIG_PERF_EVENTS
> > +/* Used by bpf get_func_ip helper */
> > +DEFINE_PER_CPU(u64, current_kprobe_addr) = 0;
> 
> Didn't check other architectures. But this should work
> for x86 where if nested kprobe happens, the second
> kprobe will not call kprobe handlers.

No problem, other architecture also does not call nested kprobes handlers.
However, you don't need this because you can use kprobe_running()
in kprobe context.

kp = kprobe_running();
if (kp)
	return kp->addr;

BTW, I'm not sure why don't you use instruction_pointer(regs)?

Thank you,

> 
> This essentially is to provide an additional parameter to
> bpf program. Andrii is developing a mechanism to
> save arbitrary data in *current task_struct*, which
> might be used here to save current_kprobe_addr, we can
> save one per cpu variable.
> 
> > +
> > +u64 trace_current_kprobe_addr(void)
> > +{
> > +	return *this_cpu_ptr(&current_kprobe_addr);
> > +}
> > +
> > +static void trace_current_kprobe_set(struct trace_kprobe *tk)
> > +{
> > +	__this_cpu_write(current_kprobe_addr, (u64) tk->rp.kp.addr);
> > +}
> >   
> >   /* Kprobe profile handler */
> >   static int
> > @@ -1585,6 +1597,7 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)
> >   		unsigned long orig_ip = instruction_pointer(regs);
> >   		int ret;
> >   
> > +		trace_current_kprobe_set(tk);
> >   		ret = trace_call_bpf(call, regs);
> >   
> >   		/*
> > @@ -1631,8 +1644,11 @@ kretprobe_perf_func(struct trace_kprobe *tk, struct kretprobe_instance *ri,
> >   	int size, __size, dsize;
> >   	int rctx;
> >   
> > -	if (bpf_prog_array_valid(call) && !trace_call_bpf(call, regs))
> > -		return;
> > +	if (bpf_prog_array_valid(call)) {
> > +		trace_current_kprobe_set(tk);
> > +		if (!trace_call_bpf(call, regs))
> > +			return;
> > +	}
> >   
> >   	head = this_cpu_ptr(call->perf_events);
> >   	if (hlist_empty(head))
> [...]
Yonghong Song July 1, 2021, 1:45 a.m. UTC | #3 
On 6/30/21 4:58 PM, Masami Hiramatsu wrote:
> On Wed, 30 Jun 2021 10:47:01 -0700

> Yonghong Song <yhs@fb.com> wrote:

> 

>>

>>

>> On 6/29/21 12:29 PM, Jiri Olsa wrote:

>>> Adding bpf_get_func_ip helper for BPF_PROG_TYPE_KPROBE programs,

>>> so it's now possible to call bpf_get_func_ip from both kprobe and

>>> kretprobe programs.

>>>

>>> Taking the caller's address from 'struct kprobe::addr', which is

>>> defined for both kprobe and kretprobe.

>>>

>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

>>> ---

>>>    include/uapi/linux/bpf.h       |  2 +-

>>>    kernel/bpf/verifier.c          |  2 ++

>>>    kernel/trace/bpf_trace.c       | 14 ++++++++++++++

>>>    kernel/trace/trace_kprobe.c    | 20 ++++++++++++++++++--

>>>    kernel/trace/trace_probe.h     |  5 +++++

>>>    tools/include/uapi/linux/bpf.h |  2 +-

>>>    6 files changed, 41 insertions(+), 4 deletions(-)

>>>

>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h

>>> index 83e87ffdbb6e..4894f99a1993 100644

>>> --- a/include/uapi/linux/bpf.h

>>> +++ b/include/uapi/linux/bpf.h

>>> @@ -4783,7 +4783,7 @@ union bpf_attr {

>>>     *

>>>     * u64 bpf_get_func_ip(void *ctx)

>>>     * 	Description

>>> - * 		Get address of the traced function (for tracing programs).

>>> + * 		Get address of the traced function (for tracing and kprobe programs).

>>>     * 	Return

>>>     * 		Address of the traced function.

>>>     */

>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c

>>> index 701ff7384fa7..b66e0a7104f8 100644

>>> --- a/kernel/bpf/verifier.c

>>> +++ b/kernel/bpf/verifier.c

>>> @@ -5979,6 +5979,8 @@ static bool has_get_func_ip(struct bpf_verifier_env *env)

>>>    			return -ENOTSUPP;

>>>    		}

>>>    		return 0;

>>> +	} else if (type == BPF_PROG_TYPE_KPROBE) {

>>> +		return 0;

>>>    	}

>>>    

>>>    	verbose(env, "func %s#%d not supported for program type %d\n",

>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c

>>> index 9edd3b1a00ad..1a5bddce9abd 100644

>>> --- a/kernel/trace/bpf_trace.c

>>> +++ b/kernel/trace/bpf_trace.c

>>> @@ -961,6 +961,18 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_tracing = {

>>>    	.arg1_type	= ARG_PTR_TO_CTX,

>>>    };

>>>    

>>> +BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)

>>> +{

>>> +	return trace_current_kprobe_addr();

>>> +}

>>> +

>>> +static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {

>>> +	.func		= bpf_get_func_ip_kprobe,

>>> +	.gpl_only	= true,

>>> +	.ret_type	= RET_INTEGER,

>>> +	.arg1_type	= ARG_PTR_TO_CTX,

>>> +};

>>> +

>>>    const struct bpf_func_proto *

>>>    bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

>>>    {

>>> @@ -1092,6 +1104,8 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

>>>    	case BPF_FUNC_override_return:

>>>    		return &bpf_override_return_proto;

>>>    #endif

>>> +	case BPF_FUNC_get_func_ip:

>>> +		return &bpf_get_func_ip_proto_kprobe;

>>>    	default:

>>>    		return bpf_tracing_func_proto(func_id, prog);

>>>    	}

>>> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c

>>> index ea6178cb5e33..b07d5888db14 100644

>>> --- a/kernel/trace/trace_kprobe.c

>>> +++ b/kernel/trace/trace_kprobe.c

>>> @@ -1570,6 +1570,18 @@ static int kretprobe_event_define_fields(struct trace_event_call *event_call)

>>>    }

>>>    

>>>    #ifdef CONFIG_PERF_EVENTS

>>> +/* Used by bpf get_func_ip helper */

>>> +DEFINE_PER_CPU(u64, current_kprobe_addr) = 0;

>>

>> Didn't check other architectures. But this should work

>> for x86 where if nested kprobe happens, the second

>> kprobe will not call kprobe handlers.

> 

> No problem, other architecture also does not call nested kprobes handlers.

> However, you don't need this because you can use kprobe_running()

> in kprobe context.

> 

> kp = kprobe_running();

> if (kp)

> 	return kp->addr;

> 

> BTW, I'm not sure why don't you use instruction_pointer(regs)?


How about kretprobe? I guess kp->addr should still point to
function address but instruction_pointer(regs) does not?

> 

> Thank you,

> 

>>

>> This essentially is to provide an additional parameter to

>> bpf program. Andrii is developing a mechanism to

>> save arbitrary data in *current task_struct*, which

>> might be used here to save current_kprobe_addr, we can

>> save one per cpu variable.

>>

>>> +

>>> +u64 trace_current_kprobe_addr(void)

>>> +{

>>> +	return *this_cpu_ptr(&current_kprobe_addr);

>>> +}

>>> +

>>> +static void trace_current_kprobe_set(struct trace_kprobe *tk)

>>> +{

>>> +	__this_cpu_write(current_kprobe_addr, (u64) tk->rp.kp.addr);

>>> +}

>>>    

>>>    /* Kprobe profile handler */

>>>    static int

>>> @@ -1585,6 +1597,7 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)

>>>    		unsigned long orig_ip = instruction_pointer(regs);

>>>    		int ret;

>>>    

>>> +		trace_current_kprobe_set(tk);

>>>    		ret = trace_call_bpf(call, regs);

>>>    

>>>    		/*

>>> @@ -1631,8 +1644,11 @@ kretprobe_perf_func(struct trace_kprobe *tk, struct kretprobe_instance *ri,

>>>    	int size, __size, dsize;

>>>    	int rctx;

>>>    

>>> -	if (bpf_prog_array_valid(call) && !trace_call_bpf(call, regs))

>>> -		return;

>>> +	if (bpf_prog_array_valid(call)) {

>>> +		trace_current_kprobe_set(tk);

>>> +		if (!trace_call_bpf(call, regs))

>>> +			return;

>>> +	}

>>>    

>>>    	head = this_cpu_ptr(call->perf_events);

>>>    	if (hlist_empty(head))

>> [...]

> 

>
Masami Hiramatsu (Google) July 1, 2021, 2:01 a.m. UTC | #4 
On Wed, 30 Jun 2021 18:45:46 -0700
Yonghong Song <yhs@fb.com> wrote:

> 

> 

> On 6/30/21 4:58 PM, Masami Hiramatsu wrote:

> > On Wed, 30 Jun 2021 10:47:01 -0700

> > Yonghong Song <yhs@fb.com> wrote:

> > 

> >>

> >>

> >> On 6/29/21 12:29 PM, Jiri Olsa wrote:

> >>> Adding bpf_get_func_ip helper for BPF_PROG_TYPE_KPROBE programs,

> >>> so it's now possible to call bpf_get_func_ip from both kprobe and

> >>> kretprobe programs.

> >>>

> >>> Taking the caller's address from 'struct kprobe::addr', which is

> >>> defined for both kprobe and kretprobe.

> >>>

> >>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

> >>> ---

> >>>    include/uapi/linux/bpf.h       |  2 +-

> >>>    kernel/bpf/verifier.c          |  2 ++

> >>>    kernel/trace/bpf_trace.c       | 14 ++++++++++++++

> >>>    kernel/trace/trace_kprobe.c    | 20 ++++++++++++++++++--

> >>>    kernel/trace/trace_probe.h     |  5 +++++

> >>>    tools/include/uapi/linux/bpf.h |  2 +-

> >>>    6 files changed, 41 insertions(+), 4 deletions(-)

> >>>

> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h

> >>> index 83e87ffdbb6e..4894f99a1993 100644

> >>> --- a/include/uapi/linux/bpf.h

> >>> +++ b/include/uapi/linux/bpf.h

> >>> @@ -4783,7 +4783,7 @@ union bpf_attr {

> >>>     *

> >>>     * u64 bpf_get_func_ip(void *ctx)

> >>>     * 	Description

> >>> - * 		Get address of the traced function (for tracing programs).

> >>> + * 		Get address of the traced function (for tracing and kprobe programs).

> >>>     * 	Return

> >>>     * 		Address of the traced function.

> >>>     */

> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c

> >>> index 701ff7384fa7..b66e0a7104f8 100644

> >>> --- a/kernel/bpf/verifier.c

> >>> +++ b/kernel/bpf/verifier.c

> >>> @@ -5979,6 +5979,8 @@ static bool has_get_func_ip(struct bpf_verifier_env *env)

> >>>    			return -ENOTSUPP;

> >>>    		}

> >>>    		return 0;

> >>> +	} else if (type == BPF_PROG_TYPE_KPROBE) {

> >>> +		return 0;

> >>>    	}

> >>>    

> >>>    	verbose(env, "func %s#%d not supported for program type %d\n",

> >>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c

> >>> index 9edd3b1a00ad..1a5bddce9abd 100644

> >>> --- a/kernel/trace/bpf_trace.c

> >>> +++ b/kernel/trace/bpf_trace.c

> >>> @@ -961,6 +961,18 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_tracing = {

> >>>    	.arg1_type	= ARG_PTR_TO_CTX,

> >>>    };

> >>>    

> >>> +BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)

> >>> +{

> >>> +	return trace_current_kprobe_addr();

> >>> +}

> >>> +

> >>> +static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {

> >>> +	.func		= bpf_get_func_ip_kprobe,

> >>> +	.gpl_only	= true,

> >>> +	.ret_type	= RET_INTEGER,

> >>> +	.arg1_type	= ARG_PTR_TO_CTX,

> >>> +};

> >>> +

> >>>    const struct bpf_func_proto *

> >>>    bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

> >>>    {

> >>> @@ -1092,6 +1104,8 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

> >>>    	case BPF_FUNC_override_return:

> >>>    		return &bpf_override_return_proto;

> >>>    #endif

> >>> +	case BPF_FUNC_get_func_ip:

> >>> +		return &bpf_get_func_ip_proto_kprobe;

> >>>    	default:

> >>>    		return bpf_tracing_func_proto(func_id, prog);

> >>>    	}

> >>> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c

> >>> index ea6178cb5e33..b07d5888db14 100644

> >>> --- a/kernel/trace/trace_kprobe.c

> >>> +++ b/kernel/trace/trace_kprobe.c

> >>> @@ -1570,6 +1570,18 @@ static int kretprobe_event_define_fields(struct trace_event_call *event_call)

> >>>    }

> >>>    

> >>>    #ifdef CONFIG_PERF_EVENTS

> >>> +/* Used by bpf get_func_ip helper */

> >>> +DEFINE_PER_CPU(u64, current_kprobe_addr) = 0;

> >>

> >> Didn't check other architectures. But this should work

> >> for x86 where if nested kprobe happens, the second

> >> kprobe will not call kprobe handlers.

> > 

> > No problem, other architecture also does not call nested kprobes handlers.

> > However, you don't need this because you can use kprobe_running()

> > in kprobe context.

> > 

> > kp = kprobe_running();

> > if (kp)

> > 	return kp->addr;

> > 

> > BTW, I'm not sure why don't you use instruction_pointer(regs)?

> 

> How about kretprobe? I guess kp->addr should still point to

> function address but instruction_pointer(regs) does not?


It is now under review (waiting for merge), please check this series.

https://lore.kernel.org/bpf/162400000592.506599.4695807810528866713.stgit@devnote2/

Then you can use instruction_pointer(regs) in kretprobes too.

Thank you,

> 

> > 

> > Thank you,

> > 

> >>

> >> This essentially is to provide an additional parameter to

> >> bpf program. Andrii is developing a mechanism to

> >> save arbitrary data in *current task_struct*, which

> >> might be used here to save current_kprobe_addr, we can

> >> save one per cpu variable.

> >>

> >>> +

> >>> +u64 trace_current_kprobe_addr(void)

> >>> +{

> >>> +	return *this_cpu_ptr(&current_kprobe_addr);

> >>> +}

> >>> +

> >>> +static void trace_current_kprobe_set(struct trace_kprobe *tk)

> >>> +{

> >>> +	__this_cpu_write(current_kprobe_addr, (u64) tk->rp.kp.addr);

> >>> +}

> >>>    

> >>>    /* Kprobe profile handler */

> >>>    static int

> >>> @@ -1585,6 +1597,7 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)

> >>>    		unsigned long orig_ip = instruction_pointer(regs);

> >>>    		int ret;

> >>>    

> >>> +		trace_current_kprobe_set(tk);

> >>>    		ret = trace_call_bpf(call, regs);

> >>>    

> >>>    		/*

> >>> @@ -1631,8 +1644,11 @@ kretprobe_perf_func(struct trace_kprobe *tk, struct kretprobe_instance *ri,

> >>>    	int size, __size, dsize;

> >>>    	int rctx;

> >>>    

> >>> -	if (bpf_prog_array_valid(call) && !trace_call_bpf(call, regs))

> >>> -		return;

> >>> +	if (bpf_prog_array_valid(call)) {

> >>> +		trace_current_kprobe_set(tk);

> >>> +		if (!trace_call_bpf(call, regs))

> >>> +			return;

> >>> +	}

> >>>    

> >>>    	head = this_cpu_ptr(call->perf_events);

> >>>    	if (hlist_empty(head))

> >> [...]

> > 

> > 



-- 
Masami Hiramatsu <mhiramat@kernel.org>
Jiri Olsa July 1, 2021, 8:34 a.m. UTC | #5 
On Wed, Jun 30, 2021 at 10:47:01AM -0700, Yonghong Song wrote:
> 

> 

> On 6/29/21 12:29 PM, Jiri Olsa wrote:

> > Adding bpf_get_func_ip helper for BPF_PROG_TYPE_KPROBE programs,

> > so it's now possible to call bpf_get_func_ip from both kprobe and

> > kretprobe programs.

> > 

> > Taking the caller's address from 'struct kprobe::addr', which is

> > defined for both kprobe and kretprobe.

> > 

> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>

> > ---

> >   include/uapi/linux/bpf.h       |  2 +-

> >   kernel/bpf/verifier.c          |  2 ++

> >   kernel/trace/bpf_trace.c       | 14 ++++++++++++++

> >   kernel/trace/trace_kprobe.c    | 20 ++++++++++++++++++--

> >   kernel/trace/trace_probe.h     |  5 +++++

> >   tools/include/uapi/linux/bpf.h |  2 +-

> >   6 files changed, 41 insertions(+), 4 deletions(-)

> > 

> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h

> > index 83e87ffdbb6e..4894f99a1993 100644

> > --- a/include/uapi/linux/bpf.h

> > +++ b/include/uapi/linux/bpf.h

> > @@ -4783,7 +4783,7 @@ union bpf_attr {

> >    *

> >    * u64 bpf_get_func_ip(void *ctx)

> >    * 	Description

> > - * 		Get address of the traced function (for tracing programs).

> > + * 		Get address of the traced function (for tracing and kprobe programs).

> >    * 	Return

> >    * 		Address of the traced function.

> >    */

> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c

> > index 701ff7384fa7..b66e0a7104f8 100644

> > --- a/kernel/bpf/verifier.c

> > +++ b/kernel/bpf/verifier.c

> > @@ -5979,6 +5979,8 @@ static bool has_get_func_ip(struct bpf_verifier_env *env)

> >   			return -ENOTSUPP;

> >   		}

> >   		return 0;

> > +	} else if (type == BPF_PROG_TYPE_KPROBE) {

> > +		return 0;

> >   	}

> >   	verbose(env, "func %s#%d not supported for program type %d\n",

> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c

> > index 9edd3b1a00ad..1a5bddce9abd 100644

> > --- a/kernel/trace/bpf_trace.c

> > +++ b/kernel/trace/bpf_trace.c

> > @@ -961,6 +961,18 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_tracing = {

> >   	.arg1_type	= ARG_PTR_TO_CTX,

> >   };

> > +BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)

> > +{

> > +	return trace_current_kprobe_addr();

> > +}

> > +

> > +static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {

> > +	.func		= bpf_get_func_ip_kprobe,

> > +	.gpl_only	= true,

> > +	.ret_type	= RET_INTEGER,

> > +	.arg1_type	= ARG_PTR_TO_CTX,

> > +};

> > +

> >   const struct bpf_func_proto *

> >   bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

> >   {

> > @@ -1092,6 +1104,8 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

> >   	case BPF_FUNC_override_return:

> >   		return &bpf_override_return_proto;

> >   #endif

> > +	case BPF_FUNC_get_func_ip:

> > +		return &bpf_get_func_ip_proto_kprobe;

> >   	default:

> >   		return bpf_tracing_func_proto(func_id, prog);

> >   	}

> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c

> > index ea6178cb5e33..b07d5888db14 100644

> > --- a/kernel/trace/trace_kprobe.c

> > +++ b/kernel/trace/trace_kprobe.c

> > @@ -1570,6 +1570,18 @@ static int kretprobe_event_define_fields(struct trace_event_call *event_call)

> >   }

> >   #ifdef CONFIG_PERF_EVENTS

> > +/* Used by bpf get_func_ip helper */

> > +DEFINE_PER_CPU(u64, current_kprobe_addr) = 0;

> 

> Didn't check other architectures. But this should work

> for x86 where if nested kprobe happens, the second

> kprobe will not call kprobe handlers.

> 

> This essentially is to provide an additional parameter to

> bpf program. Andrii is developing a mechanism to

> save arbitrary data in *current task_struct*, which

> might be used here to save current_kprobe_addr, we can

> save one per cpu variable.


ok, will check.. was there a post already?

thanks,
jirka

> 

> > +

> > +u64 trace_current_kprobe_addr(void)

> > +{

> > +	return *this_cpu_ptr(&current_kprobe_addr);

> > +}

> > +

> > +static void trace_current_kprobe_set(struct trace_kprobe *tk)

> > +{

> > +	__this_cpu_write(current_kprobe_addr, (u64) tk->rp.kp.addr);

> > +}

> >   /* Kprobe profile handler */

> >   static int

> > @@ -1585,6 +1597,7 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)

> >   		unsigned long orig_ip = instruction_pointer(regs);

> >   		int ret;

> > +		trace_current_kprobe_set(tk);

> >   		ret = trace_call_bpf(call, regs);

> >   		/*

> > @@ -1631,8 +1644,11 @@ kretprobe_perf_func(struct trace_kprobe *tk, struct kretprobe_instance *ri,

> >   	int size, __size, dsize;

> >   	int rctx;

> > -	if (bpf_prog_array_valid(call) && !trace_call_bpf(call, regs))

> > -		return;

> > +	if (bpf_prog_array_valid(call)) {

> > +		trace_current_kprobe_set(tk);

> > +		if (!trace_call_bpf(call, regs))

> > +			return;

> > +	}

> >   	head = this_cpu_ptr(call->perf_events);

> >   	if (hlist_empty(head))

> [...]

>
Jiri Olsa July 1, 2021, 8:38 a.m. UTC | #6 
On Thu, Jul 01, 2021 at 08:58:54AM +0900, Masami Hiramatsu wrote:

SNIP

> > >   		return &bpf_override_return_proto;

> > >   #endif

> > > +	case BPF_FUNC_get_func_ip:

> > > +		return &bpf_get_func_ip_proto_kprobe;

> > >   	default:

> > >   		return bpf_tracing_func_proto(func_id, prog);

> > >   	}

> > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c

> > > index ea6178cb5e33..b07d5888db14 100644

> > > --- a/kernel/trace/trace_kprobe.c

> > > +++ b/kernel/trace/trace_kprobe.c

> > > @@ -1570,6 +1570,18 @@ static int kretprobe_event_define_fields(struct trace_event_call *event_call)

> > >   }

> > >   

> > >   #ifdef CONFIG_PERF_EVENTS

> > > +/* Used by bpf get_func_ip helper */

> > > +DEFINE_PER_CPU(u64, current_kprobe_addr) = 0;

> > 

> > Didn't check other architectures. But this should work

> > for x86 where if nested kprobe happens, the second

> > kprobe will not call kprobe handlers.

> 

> No problem, other architecture also does not call nested kprobes handlers.

> However, you don't need this because you can use kprobe_running()

> in kprobe context.

> 

> kp = kprobe_running();

> if (kp)

> 	return kp->addr;


great, that's easier

> 

> BTW, I'm not sure why don't you use instruction_pointer(regs)?


I tried that but it returns function address + 1,
and I thought that could be different on each arch
and we'd need arch specific code to deal with that

thanks,
jirka
Masami Hiramatsu (Google) July 1, 2021, 1:10 p.m. UTC | #7 
On Thu, 1 Jul 2021 10:38:14 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> On Thu, Jul 01, 2021 at 08:58:54AM +0900, Masami Hiramatsu wrote:

> 

> SNIP

> 

> > > >   		return &bpf_override_return_proto;

> > > >   #endif

> > > > +	case BPF_FUNC_get_func_ip:

> > > > +		return &bpf_get_func_ip_proto_kprobe;

> > > >   	default:

> > > >   		return bpf_tracing_func_proto(func_id, prog);

> > > >   	}

> > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c

> > > > index ea6178cb5e33..b07d5888db14 100644

> > > > --- a/kernel/trace/trace_kprobe.c

> > > > +++ b/kernel/trace/trace_kprobe.c

> > > > @@ -1570,6 +1570,18 @@ static int kretprobe_event_define_fields(struct trace_event_call *event_call)

> > > >   }

> > > >   

> > > >   #ifdef CONFIG_PERF_EVENTS

> > > > +/* Used by bpf get_func_ip helper */

> > > > +DEFINE_PER_CPU(u64, current_kprobe_addr) = 0;

> > > 

> > > Didn't check other architectures. But this should work

> > > for x86 where if nested kprobe happens, the second

> > > kprobe will not call kprobe handlers.

> > 

> > No problem, other architecture also does not call nested kprobes handlers.

> > However, you don't need this because you can use kprobe_running()

> > in kprobe context.

> > 

> > kp = kprobe_running();

> > if (kp)

> > 	return kp->addr;

> 

> great, that's easier

> 

> > 

> > BTW, I'm not sure why don't you use instruction_pointer(regs)?

> 

> I tried that but it returns function address + 1,

> and I thought that could be different on each arch

> and we'd need arch specific code to deal with that


Oh, I got it. Yes, since it emulates the int3 interruption, the
regs->ip must be kp->addr + 1 on x86. And indeed, it depends
on each arch.

Thank you,

> 

> thanks,

> jirka

> 



-- 
Masami Hiramatsu <mhiramat@kernel.org>
Alan Maguire July 1, 2021, 5:22 p.m. UTC | #8 
On Tue, 29 Jun 2021, Jiri Olsa wrote:

> hi,

> adding bpf_get_func_ip helper that returns IP address of the

> caller function for trampoline and krobe programs.

> 

> There're 2 specific implementation of the bpf_get_func_ip

> helper, one for trampoline progs and one for kprobe/kretprobe

> progs.

> 

> The trampoline helper call is replaced/inlined by verifier

> with simple move instruction. The kprobe/kretprobe is actual

> helper call that returns prepared caller address.

> 

> The trampoline extra 3 instructions for storing IP address

> is now optional, which I'm not completely sure is necessary,

> so I plan to do some benchmarks, if it's noticeable, hence

> the RFC. I'm also not completely sure about the kprobe/kretprobe

> implementation.

> 

> Also available at:

>   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git

>   bpf/get_func_ip

> 

> thanks,

> jirka

> 

>


This is great Jiri! Feel free to add for the series:

Tested-by: Alan Maguire <alan.maguire@oracle.com>


BTW I also verified that if we extend bpf_program__attach_kprobe() to
support the function+offset format in the func_name argument for kprobes, 
the following test will pass too:

__u64 test5_result = 0;
SEC("kprobe/bpf_fentry_test5+0x6")
int test5(struct pt_regs *ctx)
{
        __u64 addr = bpf_get_func_ip(ctx);

        test5_result = (const void *) addr == (&bpf_fentry_test5 + 0x6);
        return 0;
}

Thanks!

Alan
 
> ---

> Jiri Olsa (5):

>       bpf, x86: Store caller's ip in trampoline stack

>       bpf: Enable BPF_TRAMP_F_IP_ARG for trampolines with call_get_func_ip

>       bpf: Add bpf_get_func_ip helper for tracing programs

>       bpf: Add bpf_get_func_ip helper for kprobe programs

>       selftests/bpf: Add test for bpf_get_func_ip helper

> 

>  arch/x86/net/bpf_jit_comp.c                               | 19 +++++++++++++++++++

>  include/linux/bpf.h                                       |  5 +++++

>  include/linux/filter.h                                    |  3 ++-

>  include/uapi/linux/bpf.h                                  |  7 +++++++

>  kernel/bpf/trampoline.c                                   | 12 +++++++++---

>  kernel/bpf/verifier.c                                     | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++

>  kernel/trace/bpf_trace.c                                  | 29 +++++++++++++++++++++++++++++

>  kernel/trace/trace_kprobe.c                               | 20 ++++++++++++++++++--

>  kernel/trace/trace_probe.h                                |  5 +++++

>  tools/include/uapi/linux/bpf.h                            |  7 +++++++

>  tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c | 42 ++++++++++++++++++++++++++++++++++++++++++

>  tools/testing/selftests/bpf/progs/get_func_ip_test.c      | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

>  12 files changed, 260 insertions(+), 6 deletions(-)

>  create mode 100644 tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c

>  create mode 100644 tools/testing/selftests/bpf/progs/get_func_ip_test.c

> 

>
Jiri Olsa July 2, 2021, 8:16 a.m. UTC | #9 
On Thu, Jul 01, 2021 at 06:22:45PM +0100, Alan Maguire wrote:
> On Tue, 29 Jun 2021, Jiri Olsa wrote:

> 

> > hi,

> > adding bpf_get_func_ip helper that returns IP address of the

> > caller function for trampoline and krobe programs.

> > 

> > There're 2 specific implementation of the bpf_get_func_ip

> > helper, one for trampoline progs and one for kprobe/kretprobe

> > progs.

> > 

> > The trampoline helper call is replaced/inlined by verifier

> > with simple move instruction. The kprobe/kretprobe is actual

> > helper call that returns prepared caller address.

> > 

> > The trampoline extra 3 instructions for storing IP address

> > is now optional, which I'm not completely sure is necessary,

> > so I plan to do some benchmarks, if it's noticeable, hence

> > the RFC. I'm also not completely sure about the kprobe/kretprobe

> > implementation.

> > 

> > Also available at:

> >   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git

> >   bpf/get_func_ip

> > 

> > thanks,

> > jirka

> > 

> >

> 

> This is great Jiri! Feel free to add for the series:

> 

> Tested-by: Alan Maguire <alan.maguire@oracle.com>


great, thanks for testing

> 

> BTW I also verified that if we extend bpf_program__attach_kprobe() to

> support the function+offset format in the func_name argument for kprobes, 

> the following test will pass too:

> 

> __u64 test5_result = 0;

> SEC("kprobe/bpf_fentry_test5+0x6")

> int test5(struct pt_regs *ctx)

> {

>         __u64 addr = bpf_get_func_ip(ctx);

> 

>         test5_result = (const void *) addr == (&bpf_fentry_test5 + 0x6);

>         return 0;

> }


right, I did not think of this test, I'll add it

thanks,
jirka