netdev - Re: [PATCH bpf-next] bpf: abstract away entire bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Mar 2020 00:41:56 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next] bpf: abstract away entire bpf_link clean up
 procedure

On 3/13/20 12:27 AM, Andrii Nakryiko wrote:
> On Thu, Mar 12, 2020 at 4:23 PM Daniel Borkmann <daniel@...earbox.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@...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.

Thanks,
Daniel