Re: [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>, Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next] bpf: abstract away entire bpf_link clean up procedure
Date: Thu, 12 Mar 2020 16:27:06 -0700	[thread overview]
Message-ID: <CAEf4BzbcSC3LXckg3ksRhTN27g4sAXp_9-GgJFog21ZWAJU-DQ@mail.gmail.com> (raw)
In-Reply-To: <2d6ae192-fe22-0239-54c7-142ec21b7794@iogearbox.net>

On Thu, Mar 12, 2020 at 4:23 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/12/20 9:39 PM, Andrii Nakryiko wrote:
> > Instead of requiring users to do three steps for cleaning up bpf_link, its
> > anon_inode file, and unused fd, abstract that away into bpf_link_cleanup()
> > helper. bpf_link_defunct() is removed, as it shouldn't be needed as an
> > individual operation anymore.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >   include/linux/bpf.h  |  3 ++-
> >   kernel/bpf/syscall.c | 18 +++++++++++-------
> >   2 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 4fd91b7c95ea..358f3eb07c01 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1075,7 +1075,8 @@ struct bpf_link_ops {
> >
> >   void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
> >                  struct bpf_prog *prog);
> > -void bpf_link_defunct(struct bpf_link *link);
> > +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
> > +                   int link_fd);
> >   void bpf_link_inc(struct bpf_link *link);
> >   void bpf_link_put(struct bpf_link *link);
> >   int bpf_link_new_fd(struct bpf_link *link);
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index b2f73ecacced..d2f49ae225b0 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2188,9 +2188,17 @@ void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops,
> >       link->prog = prog;
> >   }
> >
> > -void bpf_link_defunct(struct bpf_link *link)
> > +/* Clean up bpf_link and corresponding anon_inode file and FD. After
> > + * anon_inode is created, bpf_link can't be just kfree()'d due to deferred
> > + * anon_inode's release() call. This helper manages marking bpf_link as
> > + * defunct, releases anon_inode file and puts reserved FD.
> > + */
> > +void bpf_link_cleanup(struct bpf_link *link, struct file *link_file,
> > +                   int link_fd)
>
> Looks good, but given it is only used here this should be static instead.
>

This is part of bpf_link internal API. I have patches locally for
cgroup bpf_link that use this for clean up as well already, other
bpf_link types will also use this.

> >   {
> >       link->prog = NULL;
> > +     fput(link_file);
> > +     put_unused_fd(link_fd);
> >   }
> >
> >   void bpf_link_inc(struct bpf_link *link)
> > @@ -2383,9 +2391,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
> >
> >       err = bpf_trampoline_link_prog(prog);
> >       if (err) {
> > -             bpf_link_defunct(&link->link);
> > -             fput(link_file);
> > -             put_unused_fd(link_fd);
> > +             bpf_link_cleanup(&link->link, link_file, link_fd);
> >               goto out_put_prog;
> >       }
> >
> > @@ -2498,9 +2504,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
> >
> >       err = bpf_probe_register(link->btp, prog);
> >       if (err) {
> > -             bpf_link_defunct(&link->link);
> > -             fput(link_file);
> > -             put_unused_fd(link_fd);
> > +             bpf_link_cleanup(&link->link, link_file, link_fd);
> >               goto out_put_btp;
> >       }
> >
> >
>