On Mon, Jul 10, 2023 at 1:12 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
Extend libbpf attach opts and add a new detach opts API so this can be used
to add/remove fd-based tcx BPF programs. The old-style bpf_prog_detach() and
bpf_prog_detach2() APIs are refactored to reuse the new bpf_prog_detach_opts()
internally.
The bpf_prog_query_opts() API got extended to be able to handle the new
link_ids, link_attach_flags and revision fields.
For concrete usage examples, see the extensive selftests that have been
developed as part of this series.
Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
---
tools/lib/bpf/bpf.c | 105 +++++++++++++++++++++++++--------------
tools/lib/bpf/bpf.h | 92 ++++++++++++++++++++++++++++------
tools/lib/bpf/libbpf.c | 12 +++--
tools/lib/bpf/libbpf.map | 1 +
4 files changed, 157 insertions(+), 53 deletions(-)
Thanks for doc comments! Looks good, left a few nits with suggestions
for simplifying code, but it's minor.
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 3b0da19715e1..3dfc43b477c3 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -629,55 +629,87 @@ int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
return bpf_prog_attach_opts(prog_fd, target_fd, type, &opts);
}
-int bpf_prog_attach_opts(int prog_fd, int target_fd,
- enum bpf_attach_type type,
- const struct bpf_prog_attach_opts *opts)
+int bpf_prog_attach_opts(int prog_fd, int target,
+ enum bpf_attach_type type,
+ const struct bpf_prog_attach_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
+ const size_t attr_sz = offsetofend(union bpf_attr, expected_revision);
+ __u32 relative_id, flags;
union bpf_attr attr;
- int ret;
+ int ret, relative;
if (!OPTS_VALID(opts, bpf_prog_attach_opts))
return libbpf_err(-EINVAL);
+ relative_id = OPTS_GET(opts, relative_id, 0);
+ relative = OPTS_GET(opts, relative_fd, 0);
+ flags = OPTS_GET(opts, flags, 0);
+
+ /* validate we don't have unexpected combinations of non-zero fields */
+ if (relative > 0 && relative_id)
+ return libbpf_err(-EINVAL);
I left a comment in the next patch about this, I think it should be
simple `if (relative_fd && relative_id) { /* bad */ }`. But see the
next patch for why.
+ if (relative_id) {
+ relative = relative_id;
+ flags |= BPF_F_ID;
+ }
it's a bit hard to follow as written (to me at least). How about a
slight variation that has less in-place state update
int relative_fd, relative_id;
relative_fd = OPTS_GET(opts, relative_fd, 0);
relative_id = OPTS_GET(opts, relative_id, 0);
/* only one of fd or id can be specified */
if (relative_fd && relative_id > 0)
return libbpf_err(-EINVAL);
... then see further below
+
memset(&attr, 0, attr_sz);
- attr.target_fd = target_fd;
- attr.attach_bpf_fd = prog_fd;
- attr.attach_type = type;
- attr.attach_flags = OPTS_GET(opts, flags, 0);
- attr.replace_bpf_fd = OPTS_GET(opts, replace_prog_fd, 0);
+ attr.target_fd = target;
+ attr.attach_bpf_fd = prog_fd;
+ attr.attach_type = type;
+ attr.attach_flags = flags;
+ attr.relative_fd = relative;
instead of two lines above, have simple if/else
if (relative_if) {
attr.relative_id = relative_id;
attr.attach_flags = flags | BPF_F_ID;
} else {
attr.relative_fd = relative_fd;
attr.attach_flags = flags;
}
This combined with the piece above seems very straightforward in terms
of what is checked and what's passed into attr. WDYT?
+ attr.replace_bpf_fd = OPTS_GET(opts, replace_fd, 0);
+ attr.expected_revision = OPTS_GET(opts, expected_revision, 0);
ret = sys_bpf(BPF_PROG_ATTACH, &attr, attr_sz);
return libbpf_err_errno(ret);
}
-int bpf_prog_detach(int target_fd, enum bpf_attach_type type)
+int bpf_prog_detach_opts(int prog_fd, int target,
+ enum bpf_attach_type type,
+ const struct bpf_prog_detach_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
+ const size_t attr_sz = offsetofend(union bpf_attr, expected_revision);
+ __u32 relative_id, flags;
union bpf_attr attr;
- int ret;
+ int ret, relative;
+
+ if (!OPTS_VALID(opts, bpf_prog_detach_opts))
+ return libbpf_err(-EINVAL);
+
+ relative_id = OPTS_GET(opts, relative_id, 0);
+ relative = OPTS_GET(opts, relative_fd, 0);
+ flags = OPTS_GET(opts, flags, 0);
+
+ /* validate we don't have unexpected combinations of non-zero fields */
+ if (relative > 0 && relative_id)
+ return libbpf_err(-EINVAL);
+ if (relative_id) {
+ relative = relative_id;
+ flags |= BPF_F_ID;
+ }
see above, I think the same data flow simplification can be done
memset(&attr, 0, attr_sz);
- attr.target_fd = target_fd;
- attr.attach_type = type;
+ attr.target_fd = target;
+ attr.attach_bpf_fd = prog_fd;
+ attr.attach_type = type;
+ attr.attach_flags = flags;
+ attr.relative_fd = relative;
+ attr.expected_revision = OPTS_GET(opts, expected_revision, 0);
ret = sys_bpf(BPF_PROG_DETACH, &attr, attr_sz);
return libbpf_err_errno(ret);
}
[...]
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index d9ec4407befa..a95d39bbef90 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -396,4 +396,5 @@ LIBBPF_1.3.0 {
global:
bpf_obj_pin_opts;
bpf_program__attach_netfilter;
+ bpf_prog_detach_opts;
I think it sorts before bpf_program__attach_netfilter?
