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