Re: [PATCH 3/3] uprobes: make uprobe_register() return struct uprobe *

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Jul 10, 2024 at 9:49 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Jul 10, 2024 at 06:31:33PM +0200, Oleg Nesterov wrote:
>
> SNIP
>
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 467f358c8ce7..7571811127a2 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -3157,6 +3157,7 @@ struct bpf_uprobe {
> >       loff_t offset;
> >       unsigned long ref_ctr_offset;
> >       u64 cookie;
> > +     struct uprobe *uprobe;
> >       struct uprobe_consumer consumer;
> >  };
> >
> > @@ -3180,10 +3181,8 @@ static void bpf_uprobe_unregister(struct path *path, struct bpf_uprobe *uprobes,
> >  {
> >       u32 i;
> >
> > -     for (i = 0; i < cnt; i++) {
> > -             uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset,
> > -                               &uprobes[i].consumer);
> > -     }
>
> nice, we could also drop path argument now

see my comments to Oleg, I think we can/should get rid of link->path
altogether if uprobe itself keeps inode alive.

BTW, Jiri, do we have any test for multi-uprobe that simulates partial
attachment success/failure (whichever way you want to look at it). It
would be super useful to have to check at least some error handling
code in the uprobe code base. If we don't, do you mind adding
something simple to BPF selftests?

>
> jirka
>
> > +     for (i = 0; i < cnt; i++)
> > +             uprobe_unregister(uprobes[i].uprobe, &uprobes[i].consumer);
> >  }
> >
> >  static void bpf_uprobe_multi_link_release(struct bpf_link *link)
> > @@ -3477,11 +3476,12 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> >                     &bpf_uprobe_multi_link_lops, prog);
> >
> >       for (i = 0; i < cnt; i++) {
> > -             err = uprobe_register(d_real_inode(link->path.dentry),
> > +             uprobes[i].uprobe = uprobe_register(d_real_inode(link->path.dentry),
> >                                            uprobes[i].offset,
> >                                            uprobes[i].ref_ctr_offset,
> >                                            &uprobes[i].consumer);
> > -             if (err) {
> > +             if (IS_ERR(uprobes[i].uprobe)) {
> > +                     err = PTR_ERR(uprobes[i].uprobe);
> >                       bpf_uprobe_unregister(&path, uprobes, i);
> >                       goto error_free;
> >               }