Re: [PATCH bpf-next] bpf: add bpf_link_new_file that doesn't install FD

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Mar 11, 2020 at 6:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/10/20 12:10 AM, Andrii Nakryiko wrote:
> > Add bpf_link_new_file() API for cases when we need to ensure anon_inode is
> > successfully created before we proceed with expensive BPF program attachment
> > procedure, which will require equally (if not more so) expensive and
> > potentially failing compensation detachment procedure just because anon_inode
> > creation failed. This API allows to simplify code by ensuring first that
> > anon_inode is created and after BPF program is attached proceed with
> > fd_install() that can't fail.
> >
> > After anon_inode file is created, link can't be just kfree()'d anymore,
> > because its destruction will be performed by deferred file_operations->release
> > call. For this, bpf_link API required specifying two separate operations:
> > release() and dealloc(), former performing detachment only, while the latter
> > frees memory used by bpf_link itself. dealloc() needs to be specified, because
> > struct bpf_link is frequently embedded into link type-specific container
> > struct (e.g., struct bpf_raw_tp_link), so bpf_link itself doesn't know how to
> > properly free the memory. In case when anon_inode file was successfully
> > created, but subsequent BPF attachment failed, bpf_link needs to be marked as
> > "defunct", so that file's release() callback will perform only memory
> > deallocation, but no detachment.
> >
> > Convert raw tracepoint and tracing attachment to new API and eliminate
> > detachment from error handling path.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Applied, but ...
>
> [...]
> > @@ -2337,20 +2374,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
> >       }
> >       bpf_link_init(&link->link, &bpf_tracing_link_lops, prog);
> >
> > -     err = bpf_trampoline_link_prog(prog);
> > -     if (err)
> > -             goto out_free_link;
> > +     link_file = bpf_link_new_file(&link->link, &link_fd);
> > +     if (IS_ERR(link_file)) {
> > +             kfree(link);
> > +             err = PTR_ERR(link_file);
> > +             goto out_put_prog;
> > +     }
> >
> > -     link_fd = bpf_link_new_fd(&link->link);
> > -     if (link_fd < 0) {
> > -             WARN_ON_ONCE(bpf_trampoline_unlink_prog(prog));
> > -             err = link_fd;
> > -             goto out_free_link;
> > +     err = bpf_trampoline_link_prog(prog);
> > +     if (err) {
> > +             bpf_link_defunct(&link->link);
> > +             fput(link_file);
> > +             put_unused_fd(link_fd);
>
> Given the tear-down in error case requires 3 manual steps here, I think this begs
> for a small helper.

Sounds good, will follow up. Thanks for applying!

>
> > +             goto out_put_prog;
> >       }
> > +
> > +     fd_install(link_fd, link_file);
> >       return link_fd;
> >
> [...]
> > @@ -2431,28 +2481,32 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
> >               goto out_put_prog;
> >       }
> >
> [...]
> >
> > -     link_fd = bpf_link_new_fd(&raw_tp->link);
> > -     if (link_fd < 0) {
> > -             bpf_probe_unregister(raw_tp->btp, prog);
> > -             err = link_fd;
> > -             goto out_free_tp;
> > +     err = bpf_probe_register(link->btp, prog);
> > +     if (err) {
> > +             bpf_link_defunct(&link->link);
> > +             fput(link_file);
> > +             put_unused_fd(link_fd);
>
> Especially since you need it in multiple places; please follow-up.
>
> > +             goto out_put_btp;
> >       }
> > +
> > +     fd_install(link_fd, link_file);
> >       return link_fd;
> >
> > -out_free_tp:
> > -     kfree(raw_tp);
> >   out_put_btp:
> >       bpf_put_raw_tracepoint(btp);
> >   out_put_prog:
> >
>