On Mon, Oct 24, 2022 at 1:16 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Fri, Oct 21, 2022 at 03:02:30PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > > + if (err) {
> > > + kprobe_multi_put_modules(args.mods, args.mods_cnt);
> > > + kfree(args.mods);
> > > + return err;
> > > + }
> > > +
> > > + /* or number of modules found if everything is ok. */
> > > + *mods = args.mods;
> > > + return args.mods_cnt;
> > > +}
> > > +
> > > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > {
> > > struct bpf_kprobe_multi_link *link = NULL;
> > > @@ -2773,10 +2850,25 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > > bpf_kprobe_multi_cookie_cmp,
> > > bpf_kprobe_multi_cookie_swap,
> > > link);
> > > + } else {
> > > + /*
> > > + * We need to sort addrs array even if there are no cookies
> > > + * provided, to allow bsearch in get_modules_for_addrs.
> > > + */
> > > + sort(addrs, cnt, sizeof(*addrs),
> > > + bpf_kprobe_multi_addrs_cmp, NULL);
> > > + }
> > > +
> > > + err = get_modules_for_addrs(&link->mods, addrs, cnt);
> > > + if (err < 0) {
> > > + bpf_link_cleanup(&link_primer);
> > > + return err;
> > > }
> > > + link->mods_cnt = err;
> > >
> > > err = register_fprobe_ips(&link->fp, addrs, cnt);
> > > if (err) {
> > > + kprobe_multi_put_modules(link->mods, link->mods_cnt);
> >
> > I don't think bpf_link_cleanup() will free link->mods, you have to do
> > it explicitly here
>
> hum, so bpf_link_cleanup sets link->prog to NULL so bpf_link_free
> won't call link->ops->release, but will call link->ops->dealloc,
> so it should be fine AFAICS
oh, I completely forgot that bpf_link_free() will be called eventually
due to fput(primer->file);
so yes, you are right!
Please add my ack for next version, this was the only (non-)issue I found.
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> jirka
>
> >
> > > bpf_link_cleanup(&link_primer);
> > > return err;
> > > }
> > > --
> > > 2.37.3
> > >
