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

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Aug 01, 2023 at 01:43:53PM -0700, Alexei Starovoitov wrote:
> On Tue, Aug 1, 2023 at 1:18 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> >
> >
> >
> > On 8/1/23 12:44 PM, Yonghong Song wrote:
> > >
> > >
> > > On 8/1/23 4:53 AM, Yafang Shao wrote:
> > >> On Tue, Aug 1, 2023 at 3:30 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >>>
> > >>> Adding support for bpf_get_func_ip helper for uprobe program to return
> > >>> probed address for both uprobe and return uprobe.
> > >>>
> > >>> We discussed this in [1] and agreed that uprobe can have special use
> > >>> of bpf_get_func_ip helper that differs from kprobe.
> > >>>
> > >>> The kprobe bpf_get_func_ip returns:
> > >>>    - address of the function if probe is attach on function entry
> > >>>      for both kprobe and return kprobe
> > >>>    - 0 if the probe is not attach on function entry
> > >>>
> > >>> The uprobe bpf_get_func_ip returns:
> > >>>    - address of the probe for both uprobe and return uprobe
> > >>>
> > >>> The reason for this semantic change is that kernel can't really tell
> > >>> if the probe user space address is function entry.
> > >>>
> > >>> The uprobe program is actually kprobe type program attached as uprobe.
> > >>> One of the consequences of this design is that uprobes do not have its
> > >>> own set of helpers, but share them with kprobes.
> > >>>
> > >>> As we need different functionality for bpf_get_func_ip helper for
> > >>> uprobe,
> > >>> I'm adding the bool value to the bpf_trace_run_ctx, so the helper can
> > >>> detect that it's executed in uprobe context and call specific code.
> > >>>
> > >>> The is_uprobe bool is set as true in bpf_prog_run_array_sleepable which
> > >>> is currently used only for executing bpf programs in uprobe.
> > >>
> > >> That is error-prone.  If we don't intend to rename
> > >> bpf_prog_run_array_sleepable() to bpf_prog_run_array_uprobe(), I think
> > >> we'd better introduce a new parameter 'bool is_uprobe' into it.
> > >
> > > Agree that renaming bpf_prog_run_array_sleepable() to
> > > bpf_prog_run_array_uprobe() probably better. This way, it is
> > > self-explainable for `run_ctx.is_uprobe = true`.
> > >
> > > If unlikely case in the future, another sleepable run prog array
> > > is needed. They can have their own bpf_prog_run_array_<..>
> > > and underlying bpf_prog_run_array_sleepable() can be factored out.
> >
> > Or if want to avoid unnecessary code churn, at least add
> > a comment in bpf_prog_run_array_sleepable() to explain
> > that why it is safe to do `run_ctx.is_uprobe = true;`.
> 
> I think renaming to _uprobe() is a good idea.
> I would prefer if we can remove the bool is_uprobe run-time check,
> but don't see a way to do it cleanly.

ok, I'll rename it

thanks,
jirka