On Sat, Jun 24, 2023 at 6:19 PM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Fri, Jun 23, 2023 at 09:24:22AM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > > > > +
> > > > > + if (!uprobes || !ref_ctr_offsets || !link)
> > > > > + goto error_free;
> > > > > +
> > > > > + for (i = 0; i < cnt; i++) {
> > > > > + if (uref_ctr_offsets && __get_user(ref_ctr_offset, uref_ctr_offsets + i)) {
> > > > > + err = -EFAULT;
> > > > > + goto error_free;
> > > > > + }
> > > > > + if (__get_user(offset, uoffsets + i)) {
> > > > > + err = -EFAULT;
> > > > > + goto error_free;
> > > > > + }
> > > > > +
> > > > > + uprobes[i].offset = offset;
> > > > > + uprobes[i].link = link;
> > > > > +
> > > > > + if (flags & BPF_F_UPROBE_MULTI_RETURN)
> > > > > + uprobes[i].consumer.ret_handler = uprobe_multi_link_ret_handler;
> > > > > + else
> > > > > + uprobes[i].consumer.handler = uprobe_multi_link_handler;
> > > > > +
> > > > > + ref_ctr_offsets[i] = ref_ctr_offset;
> > > > > + }
> > > > > +
> > > > > + link->cnt = cnt;
> > > > > + link->uprobes = uprobes;
> > > > > + link->path = path;
> > > > > +
> > > > > + bpf_link_init(&link->link, BPF_LINK_TYPE_UPROBE_MULTI,
> > > > > + &bpf_uprobe_multi_link_lops, prog);
> > > > > +
> > > > > + err = bpf_link_prime(&link->link, &link_primer);
> > > > > + if (err)
> > > > > + goto error_free;
> > > > > +
> > > > > + for (i = 0; i < cnt; i++) {
> > > > > + err = uprobe_register_refctr(d_real_inode(link->path.dentry),
> > > > > + uprobes[i].offset, ref_ctr_offsets[i],
> > > > > + &uprobes[i].consumer);
> > > > > + if (err) {
> > > > > + bpf_uprobe_unregister(&path, uprobes, i);
> > > >
> > > > bpf_link_cleanup() will do this through
> > > > bpf_uprobe_multi_link_release(), no? So you are double unregistering?
> > > > Either drop cnt to zero, or just don't do this here? Latter is better,
> > > > IMO.
> > >
> > > bpf_link_cleanup path won't call release callback so we have to do that
> >
> > bpf_link_cleanup() does fput(primer->file); which eventually calls
> > release callback, no? I'd add printk and simulate failure just to be
> > sure
>
> I recall we had similar discussion for kprobe_multi link ;-)
>
> I'll double check that but I think bpf_link_cleanup calls just
> dealloc callback not release
Let's document this in comments for bpf_link_cleanup() so we don't
have to discuss this again :)
I think you are right, btw. I see that bpf_link_cleanup() sets
link->prog to NULL, and bpf_link_free() won't call
link->ops->release() if link->prog is NULL.
Tricky, I keep forgetting this. Let's explicitly explain this in a comment.
>
> jirka
>
> >
> > >
> > > I think I can add simple selftest to have this path covered
> > >
> > > thanks,
> > > jirka
>
> SNIP
