Re: [syzbot] [perf?] KASAN: slab-use-after-free Read in __uprobe_unregister

[Oleg Nesterov <oleg@redhat.com>]

On 08/12, Andrii Nakryiko wrote:
>
> adding bpf ML, given it's bpf's code base

Thanks,

> On Mon, Aug 12, 2024 at 3:00 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > --- 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,

Well, YES and NO ;) please see below.

> so be it (it's too trivial code
> to argue about).

Agreed. On a closer look both the code and the problem look very trivial.

But note that nobody noticed this trivial problem before. Including me who
had to change this trivial code to adapt to the recent API changes.

May be this means that we should keep the error handling in this function
more consistent ;)

> We do have quite a lot of "hybrid" error handling

And YES, I don't like this kind of error handling.

But, at the same time: NO, I never-never argue with the maintainers when it
comes to "cosmetic" issues.

My main point was (and you seem to agree) that this simpler patch above won't
simplify the routing. I too thought about the change above initially.

-------------------------------------------------------------------------------
> 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@kernel.org>

Thanks! I'll write the changelog and send this patch with your ack included
tomorrow.

Oleg.