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

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

On Thu, Mar 12, 2020 at 4:41 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/13/20 12:27 AM, Andrii Nakryiko wrote:
> > 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.
>
> Meaning it's a logical part of your future series. When you added the bpf_link_*
> stuff only the symbols should have been in bpf.h that are actually used in the
> tree outside of syscall.c, and when you extend the series in future /then/ we
> can export more as needed, so everything is kept self-contained. This is common
> practice.

Alright, will remove in v2.

>
> Thanks,
> Daniel