Re: [PATCHv3 bpf-next 02/26] bpf: Add multi uprobe link

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Jul 06, 2023 at 03:34:10PM -0700, Andrii Nakryiko wrote:

SNIP

> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 60a9d59beeab..a236139f08ce 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1036,6 +1036,7 @@ enum bpf_attach_type {
> >         BPF_LSM_CGROUP,
> >         BPF_STRUCT_OPS,
> >         BPF_NETFILTER,
> > +       BPF_TRACE_UPROBE_MULTI,
> >         __MAX_BPF_ATTACH_TYPE
> >  };
> >
> > @@ -1053,6 +1054,7 @@ enum bpf_link_type {
> >         BPF_LINK_TYPE_KPROBE_MULTI = 8,
> >         BPF_LINK_TYPE_STRUCT_OPS = 9,
> >         BPF_LINK_TYPE_NETFILTER = 10,
> > +       BPF_LINK_TYPE_UPROBE_MULTI = 11,
> >
> >         MAX_BPF_LINK_TYPE,
> >  };
> > @@ -1170,6 +1172,11 @@ enum bpf_link_type {
> >   */
> >  #define BPF_F_KPROBE_MULTI_RETURN      (1U << 0)
> >
> > +/* link_create.uprobe_multi.flags used in LINK_CREATE command for
> > + * BPF_TRACE_UPROBE_MULTI attach type to create return probe.
> > + */
> > +#define BPF_F_UPROBE_MULTI_RETURN      (1U << 0)
> > +
> 
> any reason why we don't use anonymous ENUMs for all these UAPI
> constants? When we need to use these flags from BPF side (e.g., for
> BPF LSM), having them as #defines will be a PITA, as they won't be
> present in vmlinux.h

ugh right, we already did that before.. will change

> 
> 
> >  /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
> >   * the following extensions:
> >   *
> > @@ -1579,6 +1586,13 @@ union bpf_attr {
> >                                 __s32           priority;
> >                                 __u32           flags;
> >                         } netfilter;
> > +                       struct {
> > +                               __u32           flags;
> > +                               __u32           cnt;
> 
> total nit, but I'd move it after path/offsets/ref_ctr_offsets, and
> make the order cnt (as it applies to previous two
> offsets/ref_ctr_offsets) and then flags last. Seems like more logical
> order, but totally subjective

ok

> 
> > +                               __aligned_u64   path;
> > +                               __aligned_u64   offsets;
> > +                               __aligned_u64   ref_ctr_offsets;
> > +                       } uprobe_multi;
> >                 };
> >         } link_create;
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 9046ad0f9b4e..3b0582a64ce4 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2813,10 +2813,12 @@ static void bpf_link_free_id(int id)
> >
> >  /* Clean up bpf_link and corresponding anon_inode file and FD. After
> >   * anon_inode is created, bpf_link can't be just kfree()'d due to deferred
> > - * anon_inode's release() call. This helper marksbpf_link as
> > + * anon_inode's release() call. This helper marks bpf_link as
> >   * defunct, releases anon_inode file and puts reserved FD. bpf_prog's refcnt
> >   * is not decremented, it's the responsibility of a calling code that failed
> >   * to complete bpf_link initialization.
> > + * This helper eventually calls link's dealloc callback, but does not call
> > + * link's release callback.
> 
> Thanks for clarifying comments!
> 
> >   */
> >  void bpf_link_cleanup(struct bpf_link_primer *primer)
> >  {
> > @@ -3589,8 +3591,12 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
> >                 if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI &&
> >                     attach_type != BPF_TRACE_KPROBE_MULTI)
> >                         return -EINVAL;
> > +               if (prog->expected_attach_type == BPF_TRACE_UPROBE_MULTI &&
> > +                   attach_type != BPF_TRACE_UPROBE_MULTI)
> > +                       return -EINVAL;
> >                 if (attach_type != BPF_PERF_EVENT &&
> > -                   attach_type != BPF_TRACE_KPROBE_MULTI)
> > +                   attach_type != BPF_TRACE_KPROBE_MULTI &&
> > +                   attach_type != BPF_TRACE_UPROBE_MULTI)
> 
> if this keeps growing, we should think about having a switch in a
> switch to not repeat BPF_TRACE_UPROBE_MULTI and BPF_TRACE_KPROBE_MULTI
> twice

ok

SNIP

> > +       for (i = 0; i < cnt; i++) {
> > +               err = uprobe_register_refctr(d_real_inode(link->path.dentry),
> > +                                            uprobes[i].offset,
> > +                                            ref_ctr_offsets ? ref_ctr_offsets[i] : 0,
> > +                                            &uprobes[i].consumer);
> > +               if (err) {
> > +                       bpf_uprobe_unregister(&path, uprobes, i);
> > +                       bpf_link_cleanup(&link_primer);
> > +                       kvfree(ref_ctr_offsets);
> 
> are we missing path_put() in this error handling path? so maybe goto
> error_path_put here instead of return?

aaaah right path_put needs to go to dealloc callback :-\ will change, thanks

jirka