On Sun, Aug 11, 2024 at 5:35 AM Oleg Nesterov <o...@redhat.com> wrote:
>
> On 08/11, Oleg Nesterov wrote:
> >
> > Hmm, bpf_uprobe_multi_link_attach() looks obviously wrong.
> >
> > bpf_link_prime() is called after the
> >
> > for (i = 0; i < cnt; i++) {
> > uprobe_register(...);
> > ...
> > }
> >
> > loop. If bpf_link_prime() fails, bpf_uprobe_multi_link_attach() just do
> > kvfree(uprobes) without _unregister(). In particular, this leaks the freed
> > bpf_uprobe->consumer in the uprobe->consumers list.
> >
> > After that another _unregister() on the same uprobe can hit the problem.
> >
> > I guess we need a simple patch for -stable...
>
> Something like below on top of perf/core. But I don't like the usage of
> "i" in the +error_unregister path...
>
Wouldn't the below be cleaner?
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cd098846e251..3ca65454f888 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3491,8 +3491,10 @@ int bpf_uprobe_multi_link_attach(const union
bpf_attr *attr, struct bpf_prog *pr
}
err = bpf_link_prime(&link->link, &link_primer);
- if (err)
+ if (err) {
+ bpf_uprobe_unregister(&path, uprobes, cnt);
goto error_free;
+ }
return bpf_link_settle(&link_primer);
We should probably route this through the bpf tree, I don't think it
will conflict with your changes, right?
> Oleg.
>
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3486,17 +3486,19 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr
> *attr, struct bpf_prog *pr
> &uprobes[i].consumer);
> if (IS_ERR(uprobes[i].uprobe)) {
> err = PTR_ERR(uprobes[i].uprobe);
> - bpf_uprobe_unregister(uprobes, i);
> - goto error_free;
> + goto error_unregister;
> }
> }
>
> err = bpf_link_prime(&link->link, &link_primer);
> if (err)
> - goto error_free;
> + goto error_unregister;
>
> return bpf_link_settle(&link_primer);
>
> +error_unregister:
> + bpf_uprobe_unregister(uprobes, i);
> +
> error_free:
> kvfree(uprobes);
> kfree(link);
>
