Re: [PATCHv2 bpf-next 1/3] bpf: Add support for bpf_get_func_ip helper for uprobe program

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Aug 03, 2023 at 08:50:59AM -0700, Yonghong Song wrote:

SNIP

> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 83bde2475ae5..d35f9750065a 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1046,9 +1046,28 @@ static unsigned long get_entry_ip(unsigned long fentry_ip)
> >   #define get_entry_ip(fentry_ip) fentry_ip
> >   #endif
> > +#ifdef CONFIG_UPROBES
> > +static unsigned long bpf_get_func_ip_uprobe(struct pt_regs *regs)
> > +{
> > +	struct uprobe_dispatch_data *udd;
> > +
> > +	udd = (struct uprobe_dispatch_data *) current->utask->vaddr;
> > +	return udd->bp_addr;
> > +}
> > +#else
> > +#define bpf_get_func_ip_uprobe(regs) (u64) -EOPNOTSUPP
> > +#endif
> 
> If I understand correctly, if below run_ctx->is_uprobe is true,
> then bpf_get_func_ip_uprobe() func in the above will be called.
> If run_ctx->is_uprobe is false, the above bpf_get_func_ip_uprobe
> macro will be not be called. The that macro definition with
> -EOPNOTSUPP really does not matter.
> 
> To avoid the above confusion, maybe we should put the CONFIG_UPROBES inside
> bpf_get_func_ip_kprobe like below.
> 
> > +
> >   BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
> >   {
> > -	struct kprobe *kp = kprobe_running();
> > +	struct bpf_trace_run_ctx *run_ctx;
> > +	struct kprobe *kp;
> > +
> > +	run_ctx = container_of(current->bpf_ctx, struct bpf_trace_run_ctx, run_ctx);
> > +	if (run_ctx->is_uprobe)
> > +		return bpf_get_func_ip_uprobe(regs);
> > +
> > +	kp = kprobe_running();
> 
> ...
> struct bpf_trace_run_ctx *run_ctx __maybe_unused;
> ...
> 
> #ifdef CONFIG_UPROBES
> 	run_ctx = container_of(current->bpf_ctx, struct bpf_trace_run_ctx,
> run_ctx);
> 	if (run_ctx->is_uprobe)
> 		return ((struct uprobe_dispatch_data *)current->utask->vaddr)->bp_addr;
> #endif
> 
> What do you think?

I thought having that in function is nicer, but yes, that will save
some cycles if CONFIG_UPROBES is disabled... on the other hand I'd
think it's enabled everywhere ... then it's true the function is just
multiple deref.. so yea, sure ;-)

thanks,
jirka

> 	
> 
> >   	if (!kp || !(kp->flags & KPROBE_FLAG_ON_FUNC_ENTRY))
> >   		return 0;
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index 01ea148723de..7dde806be91e 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -519,3 +519,8 @@ void __trace_probe_log_err(int offset, int err);
> >   #define trace_probe_log_err(offs, err)	\
> >   	__trace_probe_log_err(offs, TP_ERR_##err)
> > +
> > +struct uprobe_dispatch_data {
> > +	struct trace_uprobe	*tu;
> > +	unsigned long		bp_addr;
> > +};
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index 555c223c3232..576b3bcb8ebd 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -88,11 +88,6 @@ static struct trace_uprobe *to_trace_uprobe(struct dyn_event *ev)
> >   static int register_uprobe_event(struct trace_uprobe *tu);
> >   static int unregister_uprobe_event(struct trace_uprobe *tu);
> > -struct uprobe_dispatch_data {
> > -	struct trace_uprobe	*tu;
> > -	unsigned long		bp_addr;
> > -};
> > -
> >   static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs);
> >   static int uretprobe_dispatcher(struct uprobe_consumer *con,
> >   				unsigned long func, struct pt_regs *regs);
> > @@ -1352,7 +1347,7 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
> >   	if (bpf_prog_array_valid(call)) {
> >   		u32 ret;
> > -		ret = bpf_prog_run_array_sleepable(call->prog_array, regs, bpf_prog_run);
> > +		ret = bpf_prog_run_array_uprobe(call->prog_array, regs, bpf_prog_run);
> >   		if (!ret)
> >   			return;
> >   	}
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 70da85200695..d21deb46f49f 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -5086,9 +5086,14 @@ union bpf_attr {
> >    * u64 bpf_get_func_ip(void *ctx)
> >    * 	Description
> >    * 		Get address of the traced function (for tracing and kprobe programs).
> > + *
> > + * 		When called for kprobe program attached as uprobe it returns
> > + * 		probe address for both entry and return uprobe.
> > + *
> >    * 	Return
> > - * 		Address of the traced function.
> > + * 		Address of the traced function for kprobe.
> >    * 		0 for kprobes placed within the function (not at the entry).
> > + * 		Address of the probe for uprobe and return uprobe.
> >    *
> >    * u64 bpf_get_attach_cookie(void *ctx)
> >    * 	Description