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
jirka
>
> >
> > I think I can add simple selftest to have this path covered
> >
> > thanks,
> > jirka
SNIP
