adding bpf ML, given it's bpf's code base
On Mon, Aug 12, 2024 at 3:00 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> On 08/11, Andrii Nakryiko wrote:
> >
> > On Sun, Aug 11, 2024 at 5:35 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> > >
> > > 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);
>
> I disagree. This code already uses the "goto error_xxx" pattern, why
Well, if you have strong preferences, so be it (it's too trivial code
to argue about). We do have quite a lot of "hybrid" error handling
code that combines undoing the last step (especially if it's a simple
function call) and then doing goto for the rest of common error
handling, so I didn't (and still don't) see any problem with that.
> duplicate bpf_uprobe_unregister() ? What if another "can fail" code
> comes between register and bpf_link_prime() ?
>
> See the patch below, on top of perf/core.
>
> > We should probably route this through the bpf tree, I don't think it
> > will conflict with your changes, right?
>
> It will conflict, and in this sense it is even worse than the "#syz test"
> patch I sent in https://lore.kernel.org/all/20240811125816.GC30068@xxxxxxxxxx/
>
> Because with your version above the necessary change
>
> - bpf_uprobe_unregister(&path, uprobes, cnt);
> + bpf_uprobe_unregister(uprobes, cnt);
>
> won't be noticed during the merge, I guess.
>
Yeah, my bad, I forgot that the signature of bpf_uprobe_unregister()
also changed with your patches.
> So can we route this fix through the perf/core ? I'll add "cc: stable",
> in the next merge window the Greg's scripts will report the "FAILED"
> status of the -stable patch, I'll send the trivial backport in reply.
Yep, absolutely, given the bpf_uprobe_unregister() change, I don't see
any problem for it to go together with your refactorings.
For the fix:
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> No?
>
> Oleg.
> ---
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 4e391daafa64..90cd30e9723e 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3484,17 +3484,20 @@ 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;
> + link->cnt = i;
> + 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, link->cnt);
> +
> error_free:
> kvfree(uprobes);
> kfree(link);
>
