On Mon, Aug 12, 2024 at 12:25 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> 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.
Yep, error handling problems tend to go unnoticed frequently. The good
thing is that this is quite unlikely in practice for bpf_link_prime()
to fail, which is why it was a syzbot that found this.
>
> 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.
>
Agreed. Just stick to your code, it's fine. Thanks!
> -------------------------------------------------------------------------------
> > 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>
>
> Thanks! I'll write the changelog and send this patch with your ack included
> tomorrow.
>
> Oleg.
>
