Re: [PATCHv2 bpf-next 01/24] bpf: Add multi uprobe link

Jiri Olsa
2023-06-26T19:23:27Z

On Mon, Jun 26, 2023 at 11:27:25AM -0700, Andrii Nakryiko wrote:

SNIP

> > > > > 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.

ok, will add the comment

jirka

>
> >
> > jirka
> >
> > >
> > > >
> > > > I think I can add simple selftest to have this path covered
> > > >
> > > > thanks,
> > > > jirka
> >
> > SNIP