On Wed, Mar 11, 2020 at 6:22 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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@xxxxxx>
>
> 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:
> >
>
