* [PATCH RFC bpf-next 0/3] Revamp bpf_attr and make it easier to evolve
@ 2023-05-24 21:02 Andrii Nakryiko
2023-05-24 21:02 ` [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct Andrii Nakryiko
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2023-05-24 21:02 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team
RFC patch set revamping anonymous substructs of union bpf_attr, which would
allow nicer and more coherent evolution of bpf() syscall arguments, especially
for commands like BPF_MAP_CREATE and BPF_PROG_LOAD. See patch #1 for
justification and more details. Patch #2 demonstrates how straightforward it
is to switch to new-style substricts in kernel code (and keep in mind that
this is optional until we need some new field for a given command, so we can
do it completely asynchronously from landing bpf_attr changes themselves).
Patch #3 shows also similar libbpf changes, except for libbpf single patches
switches over entire libbpf code base to new-style substructs (except
skel_internal.h, due to concerns that users might be reliant on outdated
system-wide linux/bpf.h UAPI header).
Andrii Nakryiko (3):
bpf: revamp bpf_attr and name each command's field and substruct
bpf: use new named bpf_attr substructs for few commands
libbpf: use new bpf_xxx_attr structs for bpf() commands
include/uapi/linux/bpf.h | 235 +++++++++++++++++----
kernel/bpf/syscall.c | 77 ++++---
tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++----
tools/lib/bpf/bpf.c | 355 ++++++++++++++++----------------
tools/lib/bpf/gen_loader.c | 78 +++----
tools/lib/bpf/libbpf.c | 4 +-
tools/lib/bpf/libbpf_internal.h | 2 +-
7 files changed, 641 insertions(+), 345 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct
2023-05-24 21:02 [PATCH RFC bpf-next 0/3] Revamp bpf_attr and make it easier to evolve Andrii Nakryiko
@ 2023-05-24 21:02 ` Andrii Nakryiko
2023-05-25 3:18 ` Alexei Starovoitov
2023-05-24 21:02 ` [PATCH RFC bpf-next 2/3] bpf: use new named bpf_attr substructs for few commands Andrii Nakryiko
2023-05-24 21:02 ` [PATCH RFC bpf-next 3/3] libbpf: use new bpf_xxx_attr structs for bpf() commands Andrii Nakryiko
2 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2023-05-24 21:02 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team
union bpf_attr, used to provide input argument to bpf() syscall, has
per-command (or sometimes a per-group of commands) substruct section
with relevant arguments.
Unfortunately, as it evolved, we ended up with a situation where some
commands use anonymous struct field and some other named fields. Member
of anonymous field structs are basically in a "global namespace" and
would potentially conflict with fields in other such anonymous field
substruct. This means that commands like BPF_PROG_LOAD, BPF_BTF_LOAD,
BPF_MAP_CREATE, etc have to avoid naming fields the same, even if it
makes total sense. E.g., just because of this, BPF_BTF_LOAD couldn't
have uniformly and logically named `log_level`, `log_size`, and
`log_buf` fields, and we had to "namespace" them as `btf_log_level`,
`btf_log_size`, and `btf_log_buf`. If some other command relying on
anonymous field struct would like to add logging support, we'd need to
use yet another naming prefix to avoid conflicts with BPF_PROG_LOAD.
This is quite suboptimal to say the least, espcially when needing to add
some conceptually the same field across multiple commands. One such
example might be "token_fd" that would represent a BPF token object
(patches not posted yet, just an example and motivating case for these
changes). It would be great to just add `__u32 token_fd;` across
multiple commands with uniform name, but with current state of `union
bpf_attr` this is impossible, and we'll need to do `prog_load_token_fd`
vs `map_create_token_fd` vs `btf_load_token_fd`, etc.
This patch is attempting to rectify this situation and set bpf_attr and
bpf() syscall for cleaner evolution story. To that end, each command or
group of similar commands always use named fields. E.g., BPF_PROG_CREATE
will have `prog_create` field with a struct that contains all relevant
attributes. Of course, we can't break backwards compatibility, so to
make all the old code both compile with no changes and keep working with
no changes, we keep all the currently anonymous field structs, just move
them to the end into "LEGACY" section of bpf_attr. In their stead,
though, we add named field with exactly the same memory layout and
(mostly) the same field names. This makes sure that layout is exactly
the same, but gives each command its own naming namespace that will
allow to make naming decisions independent from any other command.
Another aspect is that these per-command structs are not anonymous
anymore, and they are actually named now. This significantly minimizes
amount of user-space code changes for applications and libraries that
would like to use new named field substructs (e.g., like libbpf in the
next patch). But this is not even the main benefit. Having each
substruct named allows to reduce accidental use of wrong fields that
don't belong to a given command. Follow up kernel-side patch
demonstrates how we can modify kernel code to ensure that we only work
with per-command part of bpf_attr, instead of hoping to never mix up
field names.
Given we keep source code and ABI backwards compat, we can do gradual
migration of both kernel and user-space code to new named substructs,
where necessary and/or benefitial.
While at it, I've used an opportunity to split some commands that just
happened to be "colocated" in terms of which substruct was used, but
that are conceptually completely separate. E.g., BPF_MAP_FREEZE gets its
own tiny struct. GET_FD_BY_ID group is split from OBJ_GET_NEXT_ID
command. I've also split PROG_ATTACH from PROG_DETACH, OBJ_PIN from
OBJ_GET, etc.
Also some fields were renamed to make their use clearer (e.g., for
PROG_DETACH `attach_bpf_fd` made no sense). For GET_FD_BY_ID union of
"link_id", "prog_id", "map_id", and "btf_id" were unified into "obj_id",
which matches struct's `bpf_obj_fd_by_id_attr` name nicely.
For a few commands we had a mix of "flags", "file_flags", and
"open_flags". What's worse, "file_flags" for OBJ_PIN/OBJ_GET is already
a misnomer as it has more than just file permissions flags. So for few
of those commands where there was single flags field, I unified the
naming to be just "flags".
For OBJ_GET command "bpf_fd" was meaningless, so I renamed it to
"__reserved" and we could reuse it later, if necessary. For
MAP_{LOOKUP,DELETE,etc}_ELEM, `next_key` field was meaningless and
dropped. It stayed in split out bpf_map_next_key_attr struct, which had
`value` dropped.
And there were a bunch of other similar changes. Please take a thorough
look and suggest more changes or which changes to drop. I'm not married
to any of them, it just felt like a good improvement.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
kernel/bpf/syscall.c | 40 +++---
tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
3 files changed, 405 insertions(+), 105 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 9273c654743c..83066cc0f24b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1320,7 +1320,8 @@ struct bpf_stack_build_id {
#define BPF_OBJ_NAME_LEN 16U
union bpf_attr {
- struct { /* anonymous struct used by BPF_MAP_CREATE command */
+ /* BPF_MAP_CREATE command */
+ struct bpf_map_create_attr {
__u32 map_type; /* one of enum bpf_map_type */
__u32 key_size; /* size of key in bytes */
__u32 value_size; /* size of value in bytes */
@@ -1348,19 +1349,30 @@ union bpf_attr {
* to using 5 hash functions).
*/
__u64 map_extra;
- };
+ } map_create;
- struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
+ /* BPF_MAP_{LOOKUP,UPDATE,DELETE,LOOKUP_AND_DELETE}_ELEM commands */
+ struct bpf_map_elem_attr {
__u32 map_fd;
__aligned_u64 key;
- union {
- __aligned_u64 value;
- __aligned_u64 next_key;
- };
+ __aligned_u64 value;
__u64 flags;
- };
+ } map_elem;
- struct { /* struct used by BPF_MAP_*_BATCH commands */
+ /* BPF_MAP_GET_NEXT_KEY command */
+ struct bpf_map_next_key_attr {
+ __u32 map_fd;
+ __aligned_u64 key;
+ __aligned_u64 next_key;
+ } map_next_key;
+
+ /* BPF_MAP_FREEZE command */
+ struct bpf_map_freeze_attr {
+ __u32 map_fd;
+ } map_freeze;
+
+ /* BPF_MAP_{LOOKUP,UPDATE,DELETE,LOOKUP_AND_DELETE}_BATCH commands */
+ struct bpf_map_batch_attr {
__aligned_u64 in_batch; /* start batch,
* NULL to start from beginning
*/
@@ -1377,7 +1389,8 @@ union bpf_attr {
__u64 flags;
} batch;
- struct { /* anonymous struct used by BPF_PROG_LOAD command */
+ /* BPF_PROG_LOAD command */
+ struct bpf_prog_load_attr {
__u32 prog_type; /* one of enum bpf_prog_type */
__u32 insn_cnt;
__aligned_u64 insns;
@@ -1417,12 +1430,13 @@ union bpf_attr {
* truncated), or smaller (if log buffer wasn't filled completely).
*/
__u32 log_true_size;
- };
+ } prog_load;
- struct { /* anonymous struct used by BPF_OBJ_* commands */
+ /* BPF_OBJ_PIN command */
+ struct bpf_obj_pin_attr {
__aligned_u64 pathname;
__u32 bpf_fd;
- __u32 file_flags;
+ __u32 flags;
/* Same as dirfd in openat() syscall; see openat(2)
* manpage for details of path FD and pathname semantics;
* path_fd should accompanied by BPF_F_PATH_FD flag set in
@@ -1430,9 +1444,24 @@ union bpf_attr {
* if BPF_F_PATH_FD flag is not set, AT_FDCWD is assumed.
*/
__s32 path_fd;
- };
+ } obj_pin;
- struct { /* anonymous struct used by BPF_PROG_ATTACH/DETACH commands */
+ /* BPF_OBJ_GET command */
+ struct bpf_obj_get_attr {
+ __aligned_u64 pathname;
+ __u32 __reserved;
+ __u32 flags;
+ /* Same as dirfd in openat() syscall; see openat(2)
+ * manpage for details of path FD and pathname semantics;
+ * path_fd should accompanied by BPF_F_PATH_FD flag set in
+ * file_flags field, otherwise it should be set to zero;
+ * if BPF_F_PATH_FD flag is not set, AT_FDCWD is assumed.
+ */
+ __s32 path_fd;
+ } obj_get;
+
+ /* BPF_PROG_ATTACH command */
+ struct bpf_prog_attach_attr {
__u32 target_fd; /* container object to attach to */
__u32 attach_bpf_fd; /* eBPF program to attach */
__u32 attach_type;
@@ -1441,9 +1470,16 @@ union bpf_attr {
* program to replace if
* BPF_F_REPLACE is used
*/
- };
+ } prog_attach;
- struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
+ /* BPF_PROG_DETACH command */
+ struct bpf_prog_detach_attr {
+ __u32 target_fd; /* container object to attach to */
+ __u32 prog_fd; /* eBPF program to detach */
+ __u32 attach_type;
+ } prog_detach;
+
+ struct bpf_prog_run_attr { /* anonymous struct used by BPF_PROG_TEST_RUN command */
__u32 prog_fd;
__u32 retval;
__u32 data_size_in; /* input: len of data_in */
@@ -1467,25 +1503,26 @@ union bpf_attr {
__u32 batch_size;
} test;
- struct { /* anonymous struct used by BPF_*_GET_*_ID */
- union {
- __u32 start_id;
- __u32 prog_id;
- __u32 map_id;
- __u32 btf_id;
- __u32 link_id;
- };
+ /* BPF_{MAP,PROG,BTF,LINK}_GET_FD_BY_ID commands */
+ struct bpf_obj_fd_by_id_attr {
+ __u32 obj_id;
+ __u32 __reserved;
+ __u32 flags;
+ } obj_fd_by_id;
+
+ /* BPF_OBJ_GET_NEXT_ID command */
+ struct bpf_obj_next_id_attr {
+ __u32 start_id;
__u32 next_id;
- __u32 open_flags;
- };
+ } obj_next_id;
- struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
+ struct bpf_obj_info_by_fd_attr { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
__u32 bpf_fd;
__u32 info_len;
__aligned_u64 info;
} info;
- struct { /* anonymous struct used by BPF_PROG_QUERY command */
+ struct bpf_prog_query_attr { /* anonymous struct used by BPF_PROG_QUERY command */
__u32 target_fd; /* container object to query */
__u32 attach_type;
__u32 query_flags;
@@ -1498,25 +1535,26 @@ union bpf_attr {
__aligned_u64 prog_attach_flags;
} query;
- struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
+ struct bpf_raw_tp_open_attr { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
__u64 name;
__u32 prog_fd;
} raw_tracepoint;
- struct { /* anonymous struct for BPF_BTF_LOAD */
+ /* BPF_BTF_LOAD command */
+ struct bpf_btf_load_attr {
__aligned_u64 btf;
- __aligned_u64 btf_log_buf;
+ __aligned_u64 log_buf;
__u32 btf_size;
- __u32 btf_log_size;
- __u32 btf_log_level;
+ __u32 log_size;
+ __u32 log_level;
/* output: actual total log contents size (including termintaing zero).
* It could be both larger than original log_size (if log was
* truncated), or smaller (if log buffer wasn't filled completely).
*/
- __u32 btf_log_true_size;
- };
+ __u32 log_true_size;
+ } btf_load;
- struct {
+ struct bpf_task_fd_query_attr {
__u32 pid; /* input: pid */
__u32 fd; /* input: fd */
__u32 flags; /* input: flags */
@@ -1532,7 +1570,7 @@ union bpf_attr {
__u64 probe_addr; /* output: probe_addr */
} task_fd_query;
- struct { /* struct used by BPF_LINK_CREATE command */
+ struct bpf_link_create_attr { /* struct used by BPF_LINK_CREATE command */
union {
__u32 prog_fd; /* eBPF program to attach */
__u32 map_fd; /* struct_ops to attach */
@@ -1581,7 +1619,7 @@ union bpf_attr {
};
} link_create;
- struct { /* struct used by BPF_LINK_UPDATE command */
+ struct bpf_link_update_attr { /* struct used by BPF_LINK_UPDATE command */
__u32 link_fd; /* link fd */
union {
/* new program fd to update link with */
@@ -1602,25 +1640,136 @@ union bpf_attr {
};
} link_update;
- struct {
+ struct bpf_link_detach_attr {
__u32 link_fd;
} link_detach;
- struct { /* struct used by BPF_ENABLE_STATS command */
+ struct bpf_enable_stats_attr { /* struct used by BPF_ENABLE_STATS command */
__u32 type;
} enable_stats;
- struct { /* struct used by BPF_ITER_CREATE command */
+ struct bpf_iter_create_attr { /* struct used by BPF_ITER_CREATE command */
__u32 link_fd;
__u32 flags;
} iter_create;
- struct { /* struct used by BPF_PROG_BIND_MAP command */
+ struct bpf_prog_bind_map_attr { /* struct used by BPF_PROG_BIND_MAP command */
__u32 prog_fd;
__u32 map_fd;
__u32 flags; /* extra flags */
} prog_bind_map;
+ /*
+ * LEGACY anonymous substructs, for backwards compatibility.
+ * Each of the below anonymous substructs are ABI compatible with one
+ * of the above named substructs. Please use named substructs.
+ */
+
+ struct { /* legacy BPF_MAP_CREATE attrs, use .map_create instead */
+ __u32 map_type;
+ __u32 key_size;
+ __u32 value_size;
+ __u32 max_entries;
+ __u32 map_flags;
+ __u32 inner_map_fd;
+ __u32 numa_node;
+ char map_name[BPF_OBJ_NAME_LEN];
+ __u32 map_ifindex;
+ __u32 btf_fd;
+ __u32 btf_key_type_id;
+ __u32 btf_value_type_id;
+ __u32 btf_vmlinux_value_type_id;
+ __u64 map_extra;
+ };
+ /*
+ * legacy BPF_MAP_*_ELEM and BPF_MAP_GET_NEXT_KEY attrs,
+ * use .map_elem or .get_next_key, respectively, instead
+ */
+ struct {
+ __u32 map_fd;
+ __aligned_u64 key;
+ union {
+ __aligned_u64 value;
+ __aligned_u64 next_key;
+ };
+ __u64 flags;
+ };
+ struct { /* legacy BPF_PROG_LOAD attrs, use .prog_load instead */
+ __u32 prog_type;
+ __u32 insn_cnt;
+ __aligned_u64 insns;
+ __aligned_u64 license;
+ __u32 log_level;
+ __u32 log_size;
+ __aligned_u64 log_buf;
+ __u32 kern_version;
+ __u32 prog_flags;
+ char prog_name[BPF_OBJ_NAME_LEN];
+ __u32 prog_ifindex;
+ __u32 expected_attach_type;
+ __u32 prog_btf_fd;
+ __u32 func_info_rec_size;
+ __aligned_u64 func_info;
+ __u32 func_info_cnt;
+ __u32 line_info_rec_size;
+ __aligned_u64 line_info;
+ __u32 line_info_cnt;
+ __u32 attach_btf_id;
+ union {
+ __u32 attach_prog_fd;
+ __u32 attach_btf_obj_fd;
+ };
+ __u32 core_relo_cnt;
+ __aligned_u64 fd_array;
+ __aligned_u64 core_relos;
+ __u32 core_relo_rec_size;
+ __u32 log_true_size;
+ };
+ /* legacy BPF_OBJ_{PIN, GET} attrs, use .obj_pin or .obj_get instead */
+ struct {
+ __aligned_u64 pathname;
+ __u32 bpf_fd;
+ __u32 file_flags;
+ __s32 path_fd;
+ };
+ /*
+ * legacy BPF_PROG_{ATTACH,DETACH} attrs,
+ * use .prog_attach or .prog_detach instead
+ */
+ struct {
+ __u32 target_fd; /* container object to attach to */
+ __u32 attach_bpf_fd; /* eBPF program to attach */
+ __u32 attach_type;
+ __u32 attach_flags;
+ __u32 replace_bpf_fd; /* previously attached eBPF
+ * program to replace if
+ * BPF_F_REPLACE is used
+ */
+ };
+ /*
+ * legacy BPF_*_GET_FD_BY_ID and BPF_OBJ_GET_NEXT_ID attrs,
+ * use .obj_fd_by_id or .obj_next_id, respectively, instead
+ */
+ struct {
+ union {
+ __u32 start_id;
+ __u32 prog_id;
+ __u32 map_id;
+ __u32 btf_id;
+ __u32 link_id;
+ };
+ __u32 next_id;
+ __u32 open_flags;
+ };
+ /* legacy BPF_BTF_LOAD attrs, use .btf_load instead */
+ struct {
+ __aligned_u64 btf;
+ __aligned_u64 btf_log_buf;
+ __u32 btf_size;
+ __u32 btf_log_size;
+ __u32 btf_log_level;
+ __u32 btf_log_true_size;
+ };
} __attribute__((aligned(8)));
/* The description below is an attempt at providing documentation to eBPF
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c7f6807215e6..babf1d56c2d9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1123,7 +1123,7 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
return ret;
}
-#define BPF_MAP_CREATE_LAST_FIELD map_extra
+#define BPF_MAP_CREATE_LAST_FIELD map_create.map_extra
/* called via syscall */
static int map_create(union bpf_attr *attr)
{
@@ -1352,7 +1352,7 @@ static void *___bpf_copy_key(bpfptr_t ukey, u64 key_size)
}
/* last field in 'union bpf_attr' used by this command */
-#define BPF_MAP_LOOKUP_ELEM_LAST_FIELD flags
+#define BPF_MAP_LOOKUP_ELEM_LAST_FIELD map_elem.flags
static int map_lookup_elem(union bpf_attr *attr)
{
@@ -1427,7 +1427,7 @@ static int map_lookup_elem(union bpf_attr *attr)
}
-#define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags
+#define BPF_MAP_UPDATE_ELEM_LAST_FIELD map_elem.flags
static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
{
@@ -1483,7 +1483,7 @@ static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
return err;
}
-#define BPF_MAP_DELETE_ELEM_LAST_FIELD key
+#define BPF_MAP_DELETE_ELEM_LAST_FIELD map_elem.key
static int map_delete_elem(union bpf_attr *attr, bpfptr_t uattr)
{
@@ -1538,7 +1538,7 @@ static int map_delete_elem(union bpf_attr *attr, bpfptr_t uattr)
}
/* last field in 'union bpf_attr' used by this command */
-#define BPF_MAP_GET_NEXT_KEY_LAST_FIELD next_key
+#define BPF_MAP_GET_NEXT_KEY_LAST_FIELD map_next_key.next_key
static int map_get_next_key(union bpf_attr *attr)
{
@@ -1817,7 +1817,7 @@ int generic_map_lookup_batch(struct bpf_map *map,
return err;
}
-#define BPF_MAP_LOOKUP_AND_DELETE_ELEM_LAST_FIELD flags
+#define BPF_MAP_LOOKUP_AND_DELETE_ELEM_LAST_FIELD map_elem.flags
static int map_lookup_and_delete_elem(union bpf_attr *attr)
{
@@ -1910,7 +1910,7 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
return err;
}
-#define BPF_MAP_FREEZE_LAST_FIELD map_fd
+#define BPF_MAP_FREEZE_LAST_FIELD map_freeze.map_fd
static int map_freeze(const union bpf_attr *attr)
{
@@ -2493,7 +2493,7 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
}
/* last field in 'union bpf_attr' used by this command */
-#define BPF_PROG_LOAD_LAST_FIELD log_true_size
+#define BPF_PROG_LOAD_LAST_FIELD prog_load.log_true_size
static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
{
@@ -2697,13 +2697,13 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
return err;
}
-#define BPF_OBJ_LAST_FIELD path_fd
+#define BPF_OBJ_PIN_LAST_FIELD obj_pin.path_fd
static int bpf_obj_pin(const union bpf_attr *attr)
{
int path_fd;
- if (CHECK_ATTR(BPF_OBJ) || attr->file_flags & ~BPF_F_PATH_FD)
+ if (CHECK_ATTR(BPF_OBJ_PIN) || attr->file_flags & ~BPF_F_PATH_FD)
return -EINVAL;
/* path_fd has to be accompanied by BPF_F_PATH_FD flag */
@@ -2715,11 +2715,13 @@ static int bpf_obj_pin(const union bpf_attr *attr)
u64_to_user_ptr(attr->pathname));
}
+#define BPF_OBJ_GET_LAST_FIELD obj_get.path_fd
+
static int bpf_obj_get(const union bpf_attr *attr)
{
int path_fd;
- if (CHECK_ATTR(BPF_OBJ) || attr->bpf_fd != 0 ||
+ if (CHECK_ATTR(BPF_OBJ_GET) || attr->bpf_fd != 0 ||
attr->file_flags & ~(BPF_OBJ_FLAG_MASK | BPF_F_PATH_FD))
return -EINVAL;
@@ -3526,7 +3528,7 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
}
}
-#define BPF_PROG_ATTACH_LAST_FIELD replace_bpf_fd
+#define BPF_PROG_ATTACH_LAST_FIELD prog_attach.replace_bpf_fd
#define BPF_F_ATTACH_MASK \
(BPF_F_ALLOW_OVERRIDE | BPF_F_ALLOW_MULTI | BPF_F_REPLACE)
@@ -3590,7 +3592,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
return ret;
}
-#define BPF_PROG_DETACH_LAST_FIELD attach_type
+#define BPF_PROG_DETACH_LAST_FIELD prog_detach.attach_type
static int bpf_prog_detach(const union bpf_attr *attr)
{
@@ -3706,7 +3708,7 @@ static int bpf_prog_test_run(const union bpf_attr *attr,
return ret;
}
-#define BPF_OBJ_GET_NEXT_ID_LAST_FIELD next_id
+#define BPF_OBJ_GET_NEXT_ID_LAST_FIELD obj_next_id.next_id
static int bpf_obj_get_next_id(const union bpf_attr *attr,
union bpf_attr __user *uattr,
@@ -3772,7 +3774,7 @@ struct bpf_prog *bpf_prog_get_curr_or_next(u32 *id)
return prog;
}
-#define BPF_PROG_GET_FD_BY_ID_LAST_FIELD prog_id
+#define BPF_PROG_GET_FD_BY_ID_LAST_FIELD obj_fd_by_id.obj_id
struct bpf_prog *bpf_prog_by_id(u32 id)
{
@@ -3814,7 +3816,7 @@ static int bpf_prog_get_fd_by_id(const union bpf_attr *attr)
return fd;
}
-#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD open_flags
+#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD obj_fd_by_id.flags
static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
{
@@ -4385,7 +4387,7 @@ static int bpf_obj_get_info_by_fd(const union bpf_attr *attr,
return err;
}
-#define BPF_BTF_LOAD_LAST_FIELD btf_log_true_size
+#define BPF_BTF_LOAD_LAST_FIELD btf_load.log_true_size
static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size)
{
@@ -4398,7 +4400,7 @@ static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_
return btf_new_fd(attr, uattr, uattr_size);
}
-#define BPF_BTF_GET_FD_BY_ID_LAST_FIELD btf_id
+#define BPF_BTF_GET_FD_BY_ID_LAST_FIELD obj_fd_by_id.obj_id
static int bpf_btf_get_fd_by_id(const union bpf_attr *attr)
{
@@ -4859,7 +4861,7 @@ struct bpf_link *bpf_link_get_curr_or_next(u32 *id)
return link;
}
-#define BPF_LINK_GET_FD_BY_ID_LAST_FIELD link_id
+#define BPF_LINK_GET_FD_BY_ID_LAST_FIELD obj_fd_by_id.obj_id
static int bpf_link_get_fd_by_id(const union bpf_attr *attr)
{
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 9273c654743c..83066cc0f24b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1320,7 +1320,8 @@ struct bpf_stack_build_id {
#define BPF_OBJ_NAME_LEN 16U
union bpf_attr {
- struct { /* anonymous struct used by BPF_MAP_CREATE command */
+ /* BPF_MAP_CREATE command */
+ struct bpf_map_create_attr {
__u32 map_type; /* one of enum bpf_map_type */
__u32 key_size; /* size of key in bytes */
__u32 value_size; /* size of value in bytes */
@@ -1348,19 +1349,30 @@ union bpf_attr {
* to using 5 hash functions).
*/
__u64 map_extra;
- };
+ } map_create;
- struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
+ /* BPF_MAP_{LOOKUP,UPDATE,DELETE,LOOKUP_AND_DELETE}_ELEM commands */
+ struct bpf_map_elem_attr {
__u32 map_fd;
__aligned_u64 key;
- union {
- __aligned_u64 value;
- __aligned_u64 next_key;
- };
+ __aligned_u64 value;
__u64 flags;
- };
+ } map_elem;
- struct { /* struct used by BPF_MAP_*_BATCH commands */
+ /* BPF_MAP_GET_NEXT_KEY command */
+ struct bpf_map_next_key_attr {
+ __u32 map_fd;
+ __aligned_u64 key;
+ __aligned_u64 next_key;
+ } map_next_key;
+
+ /* BPF_MAP_FREEZE command */
+ struct bpf_map_freeze_attr {
+ __u32 map_fd;
+ } map_freeze;
+
+ /* BPF_MAP_{LOOKUP,UPDATE,DELETE,LOOKUP_AND_DELETE}_BATCH commands */
+ struct bpf_map_batch_attr {
__aligned_u64 in_batch; /* start batch,
* NULL to start from beginning
*/
@@ -1377,7 +1389,8 @@ union bpf_attr {
__u64 flags;
} batch;
- struct { /* anonymous struct used by BPF_PROG_LOAD command */
+ /* BPF_PROG_LOAD command */
+ struct bpf_prog_load_attr {
__u32 prog_type; /* one of enum bpf_prog_type */
__u32 insn_cnt;
__aligned_u64 insns;
@@ -1417,12 +1430,13 @@ union bpf_attr {
* truncated), or smaller (if log buffer wasn't filled completely).
*/
__u32 log_true_size;
- };
+ } prog_load;
- struct { /* anonymous struct used by BPF_OBJ_* commands */
+ /* BPF_OBJ_PIN command */
+ struct bpf_obj_pin_attr {
__aligned_u64 pathname;
__u32 bpf_fd;
- __u32 file_flags;
+ __u32 flags;
/* Same as dirfd in openat() syscall; see openat(2)
* manpage for details of path FD and pathname semantics;
* path_fd should accompanied by BPF_F_PATH_FD flag set in
@@ -1430,9 +1444,24 @@ union bpf_attr {
* if BPF_F_PATH_FD flag is not set, AT_FDCWD is assumed.
*/
__s32 path_fd;
- };
+ } obj_pin;
- struct { /* anonymous struct used by BPF_PROG_ATTACH/DETACH commands */
+ /* BPF_OBJ_GET command */
+ struct bpf_obj_get_attr {
+ __aligned_u64 pathname;
+ __u32 __reserved;
+ __u32 flags;
+ /* Same as dirfd in openat() syscall; see openat(2)
+ * manpage for details of path FD and pathname semantics;
+ * path_fd should accompanied by BPF_F_PATH_FD flag set in
+ * file_flags field, otherwise it should be set to zero;
+ * if BPF_F_PATH_FD flag is not set, AT_FDCWD is assumed.
+ */
+ __s32 path_fd;
+ } obj_get;
+
+ /* BPF_PROG_ATTACH command */
+ struct bpf_prog_attach_attr {
__u32 target_fd; /* container object to attach to */
__u32 attach_bpf_fd; /* eBPF program to attach */
__u32 attach_type;
@@ -1441,9 +1470,16 @@ union bpf_attr {
* program to replace if
* BPF_F_REPLACE is used
*/
- };
+ } prog_attach;
- struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
+ /* BPF_PROG_DETACH command */
+ struct bpf_prog_detach_attr {
+ __u32 target_fd; /* container object to attach to */
+ __u32 prog_fd; /* eBPF program to detach */
+ __u32 attach_type;
+ } prog_detach;
+
+ struct bpf_prog_run_attr { /* anonymous struct used by BPF_PROG_TEST_RUN command */
__u32 prog_fd;
__u32 retval;
__u32 data_size_in; /* input: len of data_in */
@@ -1467,25 +1503,26 @@ union bpf_attr {
__u32 batch_size;
} test;
- struct { /* anonymous struct used by BPF_*_GET_*_ID */
- union {
- __u32 start_id;
- __u32 prog_id;
- __u32 map_id;
- __u32 btf_id;
- __u32 link_id;
- };
+ /* BPF_{MAP,PROG,BTF,LINK}_GET_FD_BY_ID commands */
+ struct bpf_obj_fd_by_id_attr {
+ __u32 obj_id;
+ __u32 __reserved;
+ __u32 flags;
+ } obj_fd_by_id;
+
+ /* BPF_OBJ_GET_NEXT_ID command */
+ struct bpf_obj_next_id_attr {
+ __u32 start_id;
__u32 next_id;
- __u32 open_flags;
- };
+ } obj_next_id;
- struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
+ struct bpf_obj_info_by_fd_attr { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
__u32 bpf_fd;
__u32 info_len;
__aligned_u64 info;
} info;
- struct { /* anonymous struct used by BPF_PROG_QUERY command */
+ struct bpf_prog_query_attr { /* anonymous struct used by BPF_PROG_QUERY command */
__u32 target_fd; /* container object to query */
__u32 attach_type;
__u32 query_flags;
@@ -1498,25 +1535,26 @@ union bpf_attr {
__aligned_u64 prog_attach_flags;
} query;
- struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
+ struct bpf_raw_tp_open_attr { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
__u64 name;
__u32 prog_fd;
} raw_tracepoint;
- struct { /* anonymous struct for BPF_BTF_LOAD */
+ /* BPF_BTF_LOAD command */
+ struct bpf_btf_load_attr {
__aligned_u64 btf;
- __aligned_u64 btf_log_buf;
+ __aligned_u64 log_buf;
__u32 btf_size;
- __u32 btf_log_size;
- __u32 btf_log_level;
+ __u32 log_size;
+ __u32 log_level;
/* output: actual total log contents size (including termintaing zero).
* It could be both larger than original log_size (if log was
* truncated), or smaller (if log buffer wasn't filled completely).
*/
- __u32 btf_log_true_size;
- };
+ __u32 log_true_size;
+ } btf_load;
- struct {
+ struct bpf_task_fd_query_attr {
__u32 pid; /* input: pid */
__u32 fd; /* input: fd */
__u32 flags; /* input: flags */
@@ -1532,7 +1570,7 @@ union bpf_attr {
__u64 probe_addr; /* output: probe_addr */
} task_fd_query;
- struct { /* struct used by BPF_LINK_CREATE command */
+ struct bpf_link_create_attr { /* struct used by BPF_LINK_CREATE command */
union {
__u32 prog_fd; /* eBPF program to attach */
__u32 map_fd; /* struct_ops to attach */
@@ -1581,7 +1619,7 @@ union bpf_attr {
};
} link_create;
- struct { /* struct used by BPF_LINK_UPDATE command */
+ struct bpf_link_update_attr { /* struct used by BPF_LINK_UPDATE command */
__u32 link_fd; /* link fd */
union {
/* new program fd to update link with */
@@ -1602,25 +1640,136 @@ union bpf_attr {
};
} link_update;
- struct {
+ struct bpf_link_detach_attr {
__u32 link_fd;
} link_detach;
- struct { /* struct used by BPF_ENABLE_STATS command */
+ struct bpf_enable_stats_attr { /* struct used by BPF_ENABLE_STATS command */
__u32 type;
} enable_stats;
- struct { /* struct used by BPF_ITER_CREATE command */
+ struct bpf_iter_create_attr { /* struct used by BPF_ITER_CREATE command */
__u32 link_fd;
__u32 flags;
} iter_create;
- struct { /* struct used by BPF_PROG_BIND_MAP command */
+ struct bpf_prog_bind_map_attr { /* struct used by BPF_PROG_BIND_MAP command */
__u32 prog_fd;
__u32 map_fd;
__u32 flags; /* extra flags */
} prog_bind_map;
+ /*
+ * LEGACY anonymous substructs, for backwards compatibility.
+ * Each of the below anonymous substructs are ABI compatible with one
+ * of the above named substructs. Please use named substructs.
+ */
+
+ struct { /* legacy BPF_MAP_CREATE attrs, use .map_create instead */
+ __u32 map_type;
+ __u32 key_size;
+ __u32 value_size;
+ __u32 max_entries;
+ __u32 map_flags;
+ __u32 inner_map_fd;
+ __u32 numa_node;
+ char map_name[BPF_OBJ_NAME_LEN];
+ __u32 map_ifindex;
+ __u32 btf_fd;
+ __u32 btf_key_type_id;
+ __u32 btf_value_type_id;
+ __u32 btf_vmlinux_value_type_id;
+ __u64 map_extra;
+ };
+ /*
+ * legacy BPF_MAP_*_ELEM and BPF_MAP_GET_NEXT_KEY attrs,
+ * use .map_elem or .get_next_key, respectively, instead
+ */
+ struct {
+ __u32 map_fd;
+ __aligned_u64 key;
+ union {
+ __aligned_u64 value;
+ __aligned_u64 next_key;
+ };
+ __u64 flags;
+ };
+ struct { /* legacy BPF_PROG_LOAD attrs, use .prog_load instead */
+ __u32 prog_type;
+ __u32 insn_cnt;
+ __aligned_u64 insns;
+ __aligned_u64 license;
+ __u32 log_level;
+ __u32 log_size;
+ __aligned_u64 log_buf;
+ __u32 kern_version;
+ __u32 prog_flags;
+ char prog_name[BPF_OBJ_NAME_LEN];
+ __u32 prog_ifindex;
+ __u32 expected_attach_type;
+ __u32 prog_btf_fd;
+ __u32 func_info_rec_size;
+ __aligned_u64 func_info;
+ __u32 func_info_cnt;
+ __u32 line_info_rec_size;
+ __aligned_u64 line_info;
+ __u32 line_info_cnt;
+ __u32 attach_btf_id;
+ union {
+ __u32 attach_prog_fd;
+ __u32 attach_btf_obj_fd;
+ };
+ __u32 core_relo_cnt;
+ __aligned_u64 fd_array;
+ __aligned_u64 core_relos;
+ __u32 core_relo_rec_size;
+ __u32 log_true_size;
+ };
+ /* legacy BPF_OBJ_{PIN, GET} attrs, use .obj_pin or .obj_get instead */
+ struct {
+ __aligned_u64 pathname;
+ __u32 bpf_fd;
+ __u32 file_flags;
+ __s32 path_fd;
+ };
+ /*
+ * legacy BPF_PROG_{ATTACH,DETACH} attrs,
+ * use .prog_attach or .prog_detach instead
+ */
+ struct {
+ __u32 target_fd; /* container object to attach to */
+ __u32 attach_bpf_fd; /* eBPF program to attach */
+ __u32 attach_type;
+ __u32 attach_flags;
+ __u32 replace_bpf_fd; /* previously attached eBPF
+ * program to replace if
+ * BPF_F_REPLACE is used
+ */
+ };
+ /*
+ * legacy BPF_*_GET_FD_BY_ID and BPF_OBJ_GET_NEXT_ID attrs,
+ * use .obj_fd_by_id or .obj_next_id, respectively, instead
+ */
+ struct {
+ union {
+ __u32 start_id;
+ __u32 prog_id;
+ __u32 map_id;
+ __u32 btf_id;
+ __u32 link_id;
+ };
+ __u32 next_id;
+ __u32 open_flags;
+ };
+ /* legacy BPF_BTF_LOAD attrs, use .btf_load instead */
+ struct {
+ __aligned_u64 btf;
+ __aligned_u64 btf_log_buf;
+ __u32 btf_size;
+ __u32 btf_log_size;
+ __u32 btf_log_level;
+ __u32 btf_log_true_size;
+ };
} __attribute__((aligned(8)));
/* The description below is an attempt at providing documentation to eBPF
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH RFC bpf-next 2/3] bpf: use new named bpf_attr substructs for few commands
2023-05-24 21:02 [PATCH RFC bpf-next 0/3] Revamp bpf_attr and make it easier to evolve Andrii Nakryiko
2023-05-24 21:02 ` [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct Andrii Nakryiko
@ 2023-05-24 21:02 ` Andrii Nakryiko
2023-05-24 21:02 ` [PATCH RFC bpf-next 3/3] libbpf: use new bpf_xxx_attr structs for bpf() commands Andrii Nakryiko
2 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2023-05-24 21:02 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team
Switch to new named bpf_xxx_attr substructs for a few commands to
demonstrate how minimal are the changes, especially for simple commands.
For more "sprawling" commands like BPF_MAP_CREATE and BPF_PROG_LOAD
changes will be of a similar nature (replacing union bpf_attr with
command-specific struct bpf_xxx_attr), but will touch more of helper
functions. Given the RFC status I wanted to avoid unnecessary mundane
work before getting a green light for the entire approach.
Note the changes to CHECK_ATTR() implementation. It's equivalent in
functionality, but, IMO, more straightforward and actually support both
old-style `union bpf_attr` and new-style `struct bpf_xxx_attr` usage
transparently.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/syscall.c | 37 +++++++++++++++++--------------------
1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index babf1d56c2d9..fbc5c86c7bca 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -974,11 +974,8 @@ int bpf_get_file_flag(int flags)
/* helper macro to check that unused fields 'union bpf_attr' are zero */
#define CHECK_ATTR(CMD) \
- memchr_inv((void *) &attr->CMD##_LAST_FIELD + \
- sizeof(attr->CMD##_LAST_FIELD), 0, \
- sizeof(*attr) - \
- offsetof(union bpf_attr, CMD##_LAST_FIELD) - \
- sizeof(attr->CMD##_LAST_FIELD)) != NULL
+ memchr_inv((void *)attr + offsetofend(union bpf_attr, CMD##_LAST_FIELD), 0, \
+ sizeof(union bpf_attr) - offsetofend(union bpf_attr, CMD##_LAST_FIELD)) != NULL
/* dst and src must have at least "size" number of bytes.
* Return strlen on success and < 0 on error.
@@ -1354,7 +1351,7 @@ static void *___bpf_copy_key(bpfptr_t ukey, u64 key_size)
/* last field in 'union bpf_attr' used by this command */
#define BPF_MAP_LOOKUP_ELEM_LAST_FIELD map_elem.flags
-static int map_lookup_elem(union bpf_attr *attr)
+static int map_lookup_elem(struct bpf_map_elem_attr *attr)
{
void __user *ukey = u64_to_user_ptr(attr->key);
void __user *uvalue = u64_to_user_ptr(attr->value);
@@ -1429,7 +1426,7 @@ static int map_lookup_elem(union bpf_attr *attr)
#define BPF_MAP_UPDATE_ELEM_LAST_FIELD map_elem.flags
-static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
+static int map_update_elem(struct bpf_map_elem_attr *attr, bpfptr_t uattr)
{
bpfptr_t ukey = make_bpfptr(attr->key, uattr.is_kernel);
bpfptr_t uvalue = make_bpfptr(attr->value, uattr.is_kernel);
@@ -1485,7 +1482,7 @@ static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
#define BPF_MAP_DELETE_ELEM_LAST_FIELD map_elem.key
-static int map_delete_elem(union bpf_attr *attr, bpfptr_t uattr)
+static int map_delete_elem(struct bpf_map_elem_attr *attr, bpfptr_t uattr)
{
bpfptr_t ukey = make_bpfptr(attr->key, uattr.is_kernel);
int ufd = attr->map_fd;
@@ -1540,7 +1537,7 @@ static int map_delete_elem(union bpf_attr *attr, bpfptr_t uattr)
/* last field in 'union bpf_attr' used by this command */
#define BPF_MAP_GET_NEXT_KEY_LAST_FIELD map_next_key.next_key
-static int map_get_next_key(union bpf_attr *attr)
+static int map_get_next_key(struct bpf_map_next_key_attr *attr)
{
void __user *ukey = u64_to_user_ptr(attr->key);
void __user *unext_key = u64_to_user_ptr(attr->next_key);
@@ -1912,7 +1909,7 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
#define BPF_MAP_FREEZE_LAST_FIELD map_freeze.map_fd
-static int map_freeze(const union bpf_attr *attr)
+static int map_freeze(const struct bpf_map_freeze_attr *attr)
{
int err = 0, ufd = attr->map_fd;
struct bpf_map *map;
@@ -3710,7 +3707,7 @@ static int bpf_prog_test_run(const union bpf_attr *attr,
#define BPF_OBJ_GET_NEXT_ID_LAST_FIELD obj_next_id.next_id
-static int bpf_obj_get_next_id(const union bpf_attr *attr,
+static int bpf_obj_get_next_id(const struct bpf_obj_next_id_attr *attr,
union bpf_attr __user *uattr,
struct idr *idr,
spinlock_t *lock)
@@ -5064,19 +5061,19 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
err = map_create(&attr);
break;
case BPF_MAP_LOOKUP_ELEM:
- err = map_lookup_elem(&attr);
+ err = map_lookup_elem(&attr.map_elem);
break;
case BPF_MAP_UPDATE_ELEM:
- err = map_update_elem(&attr, uattr);
+ err = map_update_elem(&attr.map_elem, uattr);
break;
case BPF_MAP_DELETE_ELEM:
- err = map_delete_elem(&attr, uattr);
+ err = map_delete_elem(&attr.map_elem, uattr);
break;
case BPF_MAP_GET_NEXT_KEY:
- err = map_get_next_key(&attr);
+ err = map_get_next_key(&attr.map_next_key);
break;
case BPF_MAP_FREEZE:
- err = map_freeze(&attr);
+ err = map_freeze(&attr.map_freeze);
break;
case BPF_PROG_LOAD:
err = bpf_prog_load(&attr, uattr, size);
@@ -5100,15 +5097,15 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
err = bpf_prog_test_run(&attr, uattr.user);
break;
case BPF_PROG_GET_NEXT_ID:
- err = bpf_obj_get_next_id(&attr, uattr.user,
+ err = bpf_obj_get_next_id(&attr.obj_next_id, uattr.user,
&prog_idr, &prog_idr_lock);
break;
case BPF_MAP_GET_NEXT_ID:
- err = bpf_obj_get_next_id(&attr, uattr.user,
+ err = bpf_obj_get_next_id(&attr.obj_next_id, uattr.user,
&map_idr, &map_idr_lock);
break;
case BPF_BTF_GET_NEXT_ID:
- err = bpf_obj_get_next_id(&attr, uattr.user,
+ err = bpf_obj_get_next_id(&attr.obj_next_id, uattr.user,
&btf_idr, &btf_idr_lock);
break;
case BPF_PROG_GET_FD_BY_ID:
@@ -5158,7 +5155,7 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
err = bpf_link_get_fd_by_id(&attr);
break;
case BPF_LINK_GET_NEXT_ID:
- err = bpf_obj_get_next_id(&attr, uattr.user,
+ err = bpf_obj_get_next_id(&attr.obj_next_id, uattr.user,
&link_idr, &link_idr_lock);
break;
case BPF_ENABLE_STATS:
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH RFC bpf-next 3/3] libbpf: use new bpf_xxx_attr structs for bpf() commands
2023-05-24 21:02 [PATCH RFC bpf-next 0/3] Revamp bpf_attr and make it easier to evolve Andrii Nakryiko
2023-05-24 21:02 ` [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct Andrii Nakryiko
2023-05-24 21:02 ` [PATCH RFC bpf-next 2/3] bpf: use new named bpf_attr substructs for few commands Andrii Nakryiko
@ 2023-05-24 21:02 ` Andrii Nakryiko
2 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2023-05-24 21:02 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team
Adapt libbpf code to new named bpf_attr substructs. The only exception
is skel_internal.h which is left as is to not harm users that might have
old system-wide bpf.h UAPI headers installed.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/bpf.c | 355 ++++++++++++++++----------------
tools/lib/bpf/gen_loader.c | 78 +++----
tools/lib/bpf/libbpf.c | 4 +-
tools/lib/bpf/libbpf_internal.h | 2 +-
4 files changed, 219 insertions(+), 220 deletions(-)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index ed86b37d8024..ab69fb2a4b24 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -69,13 +69,13 @@ static inline __u64 ptr_to_u64(const void *ptr)
return (__u64) (unsigned long) ptr;
}
-static inline int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr,
+static inline int sys_bpf(enum bpf_cmd cmd, void *attr,
unsigned int size)
{
return syscall(__NR_bpf, cmd, attr, size);
}
-static inline int sys_bpf_fd(enum bpf_cmd cmd, union bpf_attr *attr,
+static inline int sys_bpf_fd(enum bpf_cmd cmd, void *attr,
unsigned int size)
{
int fd;
@@ -84,7 +84,7 @@ static inline int sys_bpf_fd(enum bpf_cmd cmd, union bpf_attr *attr,
return ensure_good_fd(fd);
}
-int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int attempts)
+int sys_bpf_prog_load(struct bpf_prog_load_attr *attr, unsigned int size, int attempts)
{
int fd;
@@ -105,13 +105,13 @@ int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int attempts)
*/
int probe_memcg_account(void)
{
- const size_t attr_sz = offsetofend(union bpf_attr, attach_btf_obj_fd);
+ const size_t attr_sz = offsetofend(struct bpf_prog_load_attr, attach_btf_obj_fd);
struct bpf_insn insns[] = {
BPF_EMIT_CALL(BPF_FUNC_ktime_get_coarse_ns),
BPF_EXIT_INSN(),
};
size_t insn_cnt = ARRAY_SIZE(insns);
- union bpf_attr attr;
+ struct bpf_prog_load_attr attr;
int prog_fd;
/* attempt loading freplace trying to use custom BTF */
@@ -169,17 +169,16 @@ int bpf_map_create(enum bpf_map_type map_type,
__u32 max_entries,
const struct bpf_map_create_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, map_extra);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_create_attr, map_extra);
+ struct bpf_map_create_attr attr;
int fd;
- bump_rlimit_memlock();
-
- memset(&attr, 0, attr_sz);
-
if (!OPTS_VALID(opts, bpf_map_create_opts))
return libbpf_err(-EINVAL);
+ bump_rlimit_memlock();
+
+ memset(&attr, 0, attr_sz);
attr.map_type = map_type;
if (map_name && kernel_supports(NULL, FEAT_PROG_NAME))
libbpf_strlcpy(attr.map_name, map_name, sizeof(attr.map_name));
@@ -232,20 +231,20 @@ int bpf_prog_load(enum bpf_prog_type prog_type,
const struct bpf_insn *insns, size_t insn_cnt,
struct bpf_prog_load_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, log_true_size);
+ const size_t attr_sz = offsetofend(struct bpf_prog_load_attr, log_true_size);
void *finfo = NULL, *linfo = NULL;
const char *func_info, *line_info;
__u32 log_size, log_level, attach_prog_fd, attach_btf_obj_fd;
__u32 func_info_rec_size, line_info_rec_size;
int fd, attempts;
- union bpf_attr attr;
+ struct bpf_prog_load_attr attr;
char *log_buf;
- bump_rlimit_memlock();
-
if (!OPTS_VALID(opts, bpf_prog_load_opts))
return libbpf_err(-EINVAL);
+ bump_rlimit_memlock();
+
attempts = OPTS_GET(opts, attempts, 0);
if (attempts < 0)
return libbpf_err(-EINVAL);
@@ -380,8 +379,8 @@ int bpf_prog_load(enum bpf_prog_type prog_type,
int bpf_map_update_elem(int fd, const void *key, const void *value,
__u64 flags)
{
- const size_t attr_sz = offsetofend(union bpf_attr, flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_elem_attr, flags);
+ struct bpf_map_elem_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -396,8 +395,8 @@ int bpf_map_update_elem(int fd, const void *key, const void *value,
int bpf_map_lookup_elem(int fd, const void *key, void *value)
{
- const size_t attr_sz = offsetofend(union bpf_attr, flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_elem_attr, flags);
+ struct bpf_map_elem_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -411,8 +410,8 @@ int bpf_map_lookup_elem(int fd, const void *key, void *value)
int bpf_map_lookup_elem_flags(int fd, const void *key, void *value, __u64 flags)
{
- const size_t attr_sz = offsetofend(union bpf_attr, flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_elem_attr, flags);
+ struct bpf_map_elem_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -427,8 +426,8 @@ int bpf_map_lookup_elem_flags(int fd, const void *key, void *value, __u64 flags)
int bpf_map_lookup_and_delete_elem(int fd, const void *key, void *value)
{
- const size_t attr_sz = offsetofend(union bpf_attr, flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_elem_attr, flags);
+ struct bpf_map_elem_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -442,8 +441,8 @@ int bpf_map_lookup_and_delete_elem(int fd, const void *key, void *value)
int bpf_map_lookup_and_delete_elem_flags(int fd, const void *key, void *value, __u64 flags)
{
- const size_t attr_sz = offsetofend(union bpf_attr, flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_elem_attr, flags);
+ struct bpf_map_elem_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -458,8 +457,8 @@ int bpf_map_lookup_and_delete_elem_flags(int fd, const void *key, void *value, _
int bpf_map_delete_elem(int fd, const void *key)
{
- const size_t attr_sz = offsetofend(union bpf_attr, flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_elem_attr, flags);
+ struct bpf_map_elem_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -472,8 +471,8 @@ int bpf_map_delete_elem(int fd, const void *key)
int bpf_map_delete_elem_flags(int fd, const void *key, __u64 flags)
{
- const size_t attr_sz = offsetofend(union bpf_attr, flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_elem_attr, flags);
+ struct bpf_map_elem_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -487,8 +486,8 @@ int bpf_map_delete_elem_flags(int fd, const void *key, __u64 flags)
int bpf_map_get_next_key(int fd, const void *key, void *next_key)
{
- const size_t attr_sz = offsetofend(union bpf_attr, next_key);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_next_key_attr, next_key);
+ struct bpf_map_next_key_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -502,8 +501,8 @@ int bpf_map_get_next_key(int fd, const void *key, void *next_key)
int bpf_map_freeze(int fd)
{
- const size_t attr_sz = offsetofend(union bpf_attr, map_fd);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_freeze_attr, map_fd);
+ struct bpf_map_freeze_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -518,25 +517,25 @@ static int bpf_map_batch_common(int cmd, int fd, void *in_batch,
__u32 *count,
const struct bpf_map_batch_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, batch);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_map_batch_attr, flags);
+ struct bpf_map_batch_attr attr;
int ret;
if (!OPTS_VALID(opts, bpf_map_batch_opts))
return libbpf_err(-EINVAL);
memset(&attr, 0, attr_sz);
- attr.batch.map_fd = fd;
- attr.batch.in_batch = ptr_to_u64(in_batch);
- attr.batch.out_batch = ptr_to_u64(out_batch);
- attr.batch.keys = ptr_to_u64(keys);
- attr.batch.values = ptr_to_u64(values);
- attr.batch.count = *count;
- attr.batch.elem_flags = OPTS_GET(opts, elem_flags, 0);
- attr.batch.flags = OPTS_GET(opts, flags, 0);
+ attr.map_fd = fd;
+ attr.in_batch = ptr_to_u64(in_batch);
+ attr.out_batch = ptr_to_u64(out_batch);
+ attr.keys = ptr_to_u64(keys);
+ attr.values = ptr_to_u64(values);
+ attr.count = *count;
+ attr.elem_flags = OPTS_GET(opts, elem_flags, 0);
+ attr.flags = OPTS_GET(opts, flags, 0);
ret = sys_bpf(cmd, &attr, attr_sz);
- *count = attr.batch.count;
+ *count = attr.count;
return libbpf_err_errno(ret);
}
@@ -574,8 +573,8 @@ int bpf_map_update_batch(int fd, const void *keys, const void *values, __u32 *co
int bpf_obj_pin_opts(int fd, const char *pathname, const struct bpf_obj_pin_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, path_fd);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_obj_pin_attr, path_fd);
+ struct bpf_obj_pin_attr attr;
int ret;
if (!OPTS_VALID(opts, bpf_obj_pin_opts))
@@ -584,7 +583,7 @@ int bpf_obj_pin_opts(int fd, const char *pathname, const struct bpf_obj_pin_opts
memset(&attr, 0, attr_sz);
attr.path_fd = OPTS_GET(opts, path_fd, 0);
attr.pathname = ptr_to_u64((void *)pathname);
- attr.file_flags = OPTS_GET(opts, file_flags, 0);
+ attr.flags = OPTS_GET(opts, file_flags, 0);
attr.bpf_fd = fd;
ret = sys_bpf(BPF_OBJ_PIN, &attr, attr_sz);
@@ -603,8 +602,8 @@ int bpf_obj_get(const char *pathname)
int bpf_obj_get_opts(const char *pathname, const struct bpf_obj_get_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, path_fd);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_obj_get_attr, path_fd);
+ struct bpf_obj_get_attr attr;
int fd;
if (!OPTS_VALID(opts, bpf_obj_get_opts))
@@ -613,7 +612,7 @@ int bpf_obj_get_opts(const char *pathname, const struct bpf_obj_get_opts *opts)
memset(&attr, 0, attr_sz);
attr.path_fd = OPTS_GET(opts, path_fd, 0);
attr.pathname = ptr_to_u64((void *)pathname);
- attr.file_flags = OPTS_GET(opts, file_flags, 0);
+ attr.flags = OPTS_GET(opts, file_flags, 0);
fd = sys_bpf_fd(BPF_OBJ_GET, &attr, attr_sz);
return libbpf_err_errno(fd);
@@ -633,8 +632,8 @@ int bpf_prog_attach_opts(int prog_fd, int target_fd,
enum bpf_attach_type type,
const struct bpf_prog_attach_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_prog_attach_attr, replace_bpf_fd);
+ struct bpf_prog_attach_attr attr;
int ret;
if (!OPTS_VALID(opts, bpf_prog_attach_opts))
@@ -653,8 +652,8 @@ int bpf_prog_attach_opts(int prog_fd, int target_fd,
int bpf_prog_detach(int target_fd, enum bpf_attach_type type)
{
- const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_prog_detach_attr, attach_type);
+ struct bpf_prog_detach_attr attr;
int ret;
memset(&attr, 0, attr_sz);
@@ -667,13 +666,13 @@ int bpf_prog_detach(int target_fd, enum bpf_attach_type type)
int bpf_prog_detach2(int prog_fd, int target_fd, enum bpf_attach_type type)
{
- const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_prog_detach_attr, attach_type);
+ struct bpf_prog_detach_attr attr;
int ret;
memset(&attr, 0, attr_sz);
- attr.target_fd = target_fd;
- attr.attach_bpf_fd = prog_fd;
+ attr.target_fd = target_fd;
+ attr.prog_fd = prog_fd;
attr.attach_type = type;
ret = sys_bpf(BPF_PROG_DETACH, &attr, attr_sz);
@@ -684,9 +683,9 @@ int bpf_link_create(int prog_fd, int target_fd,
enum bpf_attach_type attach_type,
const struct bpf_link_create_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, link_create);
+ const size_t attr_sz = sizeof(struct bpf_link_create_attr);
__u32 target_btf_id, iter_info_len;
- union bpf_attr attr;
+ struct bpf_link_create_attr attr;
int fd, err;
if (!OPTS_VALID(opts, bpf_link_create_opts))
@@ -704,32 +703,32 @@ int bpf_link_create(int prog_fd, int target_fd,
}
memset(&attr, 0, attr_sz);
- attr.link_create.prog_fd = prog_fd;
- attr.link_create.target_fd = target_fd;
- attr.link_create.attach_type = attach_type;
- attr.link_create.flags = OPTS_GET(opts, flags, 0);
+ attr.prog_fd = prog_fd;
+ attr.target_fd = target_fd;
+ attr.attach_type = attach_type;
+ attr.flags = OPTS_GET(opts, flags, 0);
if (target_btf_id) {
- attr.link_create.target_btf_id = target_btf_id;
+ attr.target_btf_id = target_btf_id;
goto proceed;
}
switch (attach_type) {
case BPF_TRACE_ITER:
- attr.link_create.iter_info = ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
- attr.link_create.iter_info_len = iter_info_len;
+ attr.iter_info = ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
+ attr.iter_info_len = iter_info_len;
break;
case BPF_PERF_EVENT:
- attr.link_create.perf_event.bpf_cookie = OPTS_GET(opts, perf_event.bpf_cookie, 0);
+ attr.perf_event.bpf_cookie = OPTS_GET(opts, perf_event.bpf_cookie, 0);
if (!OPTS_ZEROED(opts, perf_event))
return libbpf_err(-EINVAL);
break;
case BPF_TRACE_KPROBE_MULTI:
- attr.link_create.kprobe_multi.flags = OPTS_GET(opts, kprobe_multi.flags, 0);
- attr.link_create.kprobe_multi.cnt = OPTS_GET(opts, kprobe_multi.cnt, 0);
- attr.link_create.kprobe_multi.syms = ptr_to_u64(OPTS_GET(opts, kprobe_multi.syms, 0));
- attr.link_create.kprobe_multi.addrs = ptr_to_u64(OPTS_GET(opts, kprobe_multi.addrs, 0));
- attr.link_create.kprobe_multi.cookies = ptr_to_u64(OPTS_GET(opts, kprobe_multi.cookies, 0));
+ attr.kprobe_multi.flags = OPTS_GET(opts, kprobe_multi.flags, 0);
+ attr.kprobe_multi.cnt = OPTS_GET(opts, kprobe_multi.cnt, 0);
+ attr.kprobe_multi.syms = ptr_to_u64(OPTS_GET(opts, kprobe_multi.syms, 0));
+ attr.kprobe_multi.addrs = ptr_to_u64(OPTS_GET(opts, kprobe_multi.addrs, 0));
+ attr.kprobe_multi.cookies = ptr_to_u64(OPTS_GET(opts, kprobe_multi.cookies, 0));
if (!OPTS_ZEROED(opts, kprobe_multi))
return libbpf_err(-EINVAL);
break;
@@ -737,7 +736,7 @@ int bpf_link_create(int prog_fd, int target_fd,
case BPF_TRACE_FEXIT:
case BPF_MODIFY_RETURN:
case BPF_LSM_MAC:
- attr.link_create.tracing.cookie = OPTS_GET(opts, tracing.cookie, 0);
+ attr.tracing.cookie = OPTS_GET(opts, tracing.cookie, 0);
if (!OPTS_ZEROED(opts, tracing))
return libbpf_err(-EINVAL);
break;
@@ -760,7 +759,7 @@ int bpf_link_create(int prog_fd, int target_fd,
/* if user used features not supported by
* BPF_RAW_TRACEPOINT_OPEN command, then just give up immediately
*/
- if (attr.link_create.target_fd || attr.link_create.target_btf_id)
+ if (attr.target_fd || attr.target_btf_id)
return libbpf_err(err);
if (!OPTS_ZEROED(opts, sz))
return libbpf_err(err);
@@ -783,12 +782,12 @@ int bpf_link_create(int prog_fd, int target_fd,
int bpf_link_detach(int link_fd)
{
- const size_t attr_sz = offsetofend(union bpf_attr, link_detach);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_link_detach_attr, link_fd);
+ struct bpf_link_detach_attr attr;
int ret;
memset(&attr, 0, attr_sz);
- attr.link_detach.link_fd = link_fd;
+ attr.link_fd = link_fd;
ret = sys_bpf(BPF_LINK_DETACH, &attr, attr_sz);
return libbpf_err_errno(ret);
@@ -797,8 +796,8 @@ int bpf_link_detach(int link_fd)
int bpf_link_update(int link_fd, int new_prog_fd,
const struct bpf_link_update_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, link_update);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_link_update_attr, old_prog_fd);
+ struct bpf_link_update_attr attr;
int ret;
if (!OPTS_VALID(opts, bpf_link_update_opts))
@@ -808,13 +807,13 @@ int bpf_link_update(int link_fd, int new_prog_fd,
return libbpf_err(-EINVAL);
memset(&attr, 0, attr_sz);
- attr.link_update.link_fd = link_fd;
- attr.link_update.new_prog_fd = new_prog_fd;
- attr.link_update.flags = OPTS_GET(opts, flags, 0);
+ attr.link_fd = link_fd;
+ attr.new_prog_fd = new_prog_fd;
+ attr.flags = OPTS_GET(opts, flags, 0);
if (OPTS_GET(opts, old_prog_fd, 0))
- attr.link_update.old_prog_fd = OPTS_GET(opts, old_prog_fd, 0);
+ attr.old_prog_fd = OPTS_GET(opts, old_prog_fd, 0);
else if (OPTS_GET(opts, old_map_fd, 0))
- attr.link_update.old_map_fd = OPTS_GET(opts, old_map_fd, 0);
+ attr.old_map_fd = OPTS_GET(opts, old_map_fd, 0);
ret = sys_bpf(BPF_LINK_UPDATE, &attr, attr_sz);
return libbpf_err_errno(ret);
@@ -822,12 +821,12 @@ int bpf_link_update(int link_fd, int new_prog_fd,
int bpf_iter_create(int link_fd)
{
- const size_t attr_sz = offsetofend(union bpf_attr, iter_create);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_iter_create_attr, flags);
+ struct bpf_iter_create_attr attr;
int fd;
memset(&attr, 0, attr_sz);
- attr.iter_create.link_fd = link_fd;
+ attr.link_fd = link_fd;
fd = sys_bpf_fd(BPF_ITER_CREATE, &attr, attr_sz);
return libbpf_err_errno(fd);
@@ -837,8 +836,8 @@ int bpf_prog_query_opts(int target_fd,
enum bpf_attach_type type,
struct bpf_prog_query_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, query);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_prog_query_attr, prog_attach_flags);
+ struct bpf_prog_query_attr attr;
int ret;
if (!OPTS_VALID(opts, bpf_prog_query_opts))
@@ -846,17 +845,17 @@ int bpf_prog_query_opts(int target_fd,
memset(&attr, 0, attr_sz);
- attr.query.target_fd = target_fd;
- attr.query.attach_type = type;
- attr.query.query_flags = OPTS_GET(opts, query_flags, 0);
- attr.query.prog_cnt = OPTS_GET(opts, prog_cnt, 0);
- attr.query.prog_ids = ptr_to_u64(OPTS_GET(opts, prog_ids, NULL));
- attr.query.prog_attach_flags = ptr_to_u64(OPTS_GET(opts, prog_attach_flags, NULL));
+ attr.target_fd = target_fd;
+ attr.attach_type = type;
+ attr.query_flags = OPTS_GET(opts, query_flags, 0);
+ attr.prog_cnt = OPTS_GET(opts, prog_cnt, 0);
+ attr.prog_ids = ptr_to_u64(OPTS_GET(opts, prog_ids, NULL));
+ attr.prog_attach_flags = ptr_to_u64(OPTS_GET(opts, prog_attach_flags, NULL));
ret = sys_bpf(BPF_PROG_QUERY, &attr, attr_sz);
- OPTS_SET(opts, attach_flags, attr.query.attach_flags);
- OPTS_SET(opts, prog_cnt, attr.query.prog_cnt);
+ OPTS_SET(opts, attach_flags, attr.attach_flags);
+ OPTS_SET(opts, prog_cnt, attr.prog_cnt);
return libbpf_err_errno(ret);
}
@@ -882,43 +881,43 @@ int bpf_prog_query(int target_fd, enum bpf_attach_type type, __u32 query_flags,
int bpf_prog_test_run_opts(int prog_fd, struct bpf_test_run_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, test);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_prog_run_attr, batch_size);
+ struct bpf_prog_run_attr attr;
int ret;
if (!OPTS_VALID(opts, bpf_test_run_opts))
return libbpf_err(-EINVAL);
memset(&attr, 0, attr_sz);
- attr.test.prog_fd = prog_fd;
- attr.test.batch_size = OPTS_GET(opts, batch_size, 0);
- attr.test.cpu = OPTS_GET(opts, cpu, 0);
- attr.test.flags = OPTS_GET(opts, flags, 0);
- attr.test.repeat = OPTS_GET(opts, repeat, 0);
- attr.test.duration = OPTS_GET(opts, duration, 0);
- attr.test.ctx_size_in = OPTS_GET(opts, ctx_size_in, 0);
- attr.test.ctx_size_out = OPTS_GET(opts, ctx_size_out, 0);
- attr.test.data_size_in = OPTS_GET(opts, data_size_in, 0);
- attr.test.data_size_out = OPTS_GET(opts, data_size_out, 0);
- attr.test.ctx_in = ptr_to_u64(OPTS_GET(opts, ctx_in, NULL));
- attr.test.ctx_out = ptr_to_u64(OPTS_GET(opts, ctx_out, NULL));
- attr.test.data_in = ptr_to_u64(OPTS_GET(opts, data_in, NULL));
- attr.test.data_out = ptr_to_u64(OPTS_GET(opts, data_out, NULL));
+ attr.prog_fd = prog_fd;
+ attr.batch_size = OPTS_GET(opts, batch_size, 0);
+ attr.cpu = OPTS_GET(opts, cpu, 0);
+ attr.flags = OPTS_GET(opts, flags, 0);
+ attr.repeat = OPTS_GET(opts, repeat, 0);
+ attr.duration = OPTS_GET(opts, duration, 0);
+ attr.ctx_size_in = OPTS_GET(opts, ctx_size_in, 0);
+ attr.ctx_size_out = OPTS_GET(opts, ctx_size_out, 0);
+ attr.data_size_in = OPTS_GET(opts, data_size_in, 0);
+ attr.data_size_out = OPTS_GET(opts, data_size_out, 0);
+ attr.ctx_in = ptr_to_u64(OPTS_GET(opts, ctx_in, NULL));
+ attr.ctx_out = ptr_to_u64(OPTS_GET(opts, ctx_out, NULL));
+ attr.data_in = ptr_to_u64(OPTS_GET(opts, data_in, NULL));
+ attr.data_out = ptr_to_u64(OPTS_GET(opts, data_out, NULL));
ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, attr_sz);
- OPTS_SET(opts, data_size_out, attr.test.data_size_out);
- OPTS_SET(opts, ctx_size_out, attr.test.ctx_size_out);
- OPTS_SET(opts, duration, attr.test.duration);
- OPTS_SET(opts, retval, attr.test.retval);
+ OPTS_SET(opts, data_size_out, attr.data_size_out);
+ OPTS_SET(opts, ctx_size_out, attr.ctx_size_out);
+ OPTS_SET(opts, duration, attr.duration);
+ OPTS_SET(opts, retval, attr.retval);
return libbpf_err_errno(ret);
}
static int bpf_obj_get_next_id(__u32 start_id, __u32 *next_id, int cmd)
{
- const size_t attr_sz = offsetofend(union bpf_attr, open_flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_obj_next_id_attr, next_id);
+ struct bpf_obj_next_id_attr attr;
int err;
memset(&attr, 0, attr_sz);
@@ -954,16 +953,16 @@ int bpf_link_get_next_id(__u32 start_id, __u32 *next_id)
int bpf_prog_get_fd_by_id_opts(__u32 id,
const struct bpf_get_fd_by_id_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, open_flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_obj_fd_by_id_attr, flags);
+ struct bpf_obj_fd_by_id_attr attr;
int fd;
if (!OPTS_VALID(opts, bpf_get_fd_by_id_opts))
return libbpf_err(-EINVAL);
memset(&attr, 0, attr_sz);
- attr.prog_id = id;
- attr.open_flags = OPTS_GET(opts, open_flags, 0);
+ attr.obj_id = id;
+ attr.flags = OPTS_GET(opts, open_flags, 0);
fd = sys_bpf_fd(BPF_PROG_GET_FD_BY_ID, &attr, attr_sz);
return libbpf_err_errno(fd);
@@ -977,16 +976,16 @@ int bpf_prog_get_fd_by_id(__u32 id)
int bpf_map_get_fd_by_id_opts(__u32 id,
const struct bpf_get_fd_by_id_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, open_flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_obj_fd_by_id_attr, flags);
+ struct bpf_obj_fd_by_id_attr attr;
int fd;
if (!OPTS_VALID(opts, bpf_get_fd_by_id_opts))
return libbpf_err(-EINVAL);
memset(&attr, 0, attr_sz);
- attr.map_id = id;
- attr.open_flags = OPTS_GET(opts, open_flags, 0);
+ attr.obj_id = id;
+ attr.flags = OPTS_GET(opts, open_flags, 0);
fd = sys_bpf_fd(BPF_MAP_GET_FD_BY_ID, &attr, attr_sz);
return libbpf_err_errno(fd);
@@ -1000,16 +999,16 @@ int bpf_map_get_fd_by_id(__u32 id)
int bpf_btf_get_fd_by_id_opts(__u32 id,
const struct bpf_get_fd_by_id_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, open_flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_obj_fd_by_id_attr, flags);
+ struct bpf_obj_fd_by_id_attr attr;
int fd;
if (!OPTS_VALID(opts, bpf_get_fd_by_id_opts))
return libbpf_err(-EINVAL);
memset(&attr, 0, attr_sz);
- attr.btf_id = id;
- attr.open_flags = OPTS_GET(opts, open_flags, 0);
+ attr.obj_id = id;
+ attr.flags = OPTS_GET(opts, open_flags, 0);
fd = sys_bpf_fd(BPF_BTF_GET_FD_BY_ID, &attr, attr_sz);
return libbpf_err_errno(fd);
@@ -1023,16 +1022,16 @@ int bpf_btf_get_fd_by_id(__u32 id)
int bpf_link_get_fd_by_id_opts(__u32 id,
const struct bpf_get_fd_by_id_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, open_flags);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_obj_fd_by_id_attr, flags);
+ struct bpf_obj_fd_by_id_attr attr;
int fd;
if (!OPTS_VALID(opts, bpf_get_fd_by_id_opts))
return libbpf_err(-EINVAL);
memset(&attr, 0, attr_sz);
- attr.link_id = id;
- attr.open_flags = OPTS_GET(opts, open_flags, 0);
+ attr.obj_id = id;
+ attr.flags = OPTS_GET(opts, open_flags, 0);
fd = sys_bpf_fd(BPF_LINK_GET_FD_BY_ID, &attr, attr_sz);
return libbpf_err_errno(fd);
@@ -1045,18 +1044,18 @@ int bpf_link_get_fd_by_id(__u32 id)
int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len)
{
- const size_t attr_sz = offsetofend(union bpf_attr, info);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_obj_info_by_fd_attr, info);
+ struct bpf_obj_info_by_fd_attr attr;
int err;
memset(&attr, 0, attr_sz);
- attr.info.bpf_fd = bpf_fd;
- attr.info.info_len = *info_len;
- attr.info.info = ptr_to_u64(info);
+ attr.bpf_fd = bpf_fd;
+ attr.info_len = *info_len;
+ attr.info = ptr_to_u64(info);
err = sys_bpf(BPF_OBJ_GET_INFO_BY_FD, &attr, attr_sz);
if (!err)
- *info_len = attr.info.info_len;
+ *info_len = attr.info_len;
return libbpf_err_errno(err);
}
@@ -1082,13 +1081,13 @@ int bpf_link_get_info_by_fd(int link_fd, struct bpf_link_info *info, __u32 *info
int bpf_raw_tracepoint_open(const char *name, int prog_fd)
{
- const size_t attr_sz = offsetofend(union bpf_attr, raw_tracepoint);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_raw_tp_open_attr, prog_fd);
+ struct bpf_raw_tp_open_attr attr;
int fd;
memset(&attr, 0, attr_sz);
- attr.raw_tracepoint.name = ptr_to_u64(name);
- attr.raw_tracepoint.prog_fd = prog_fd;
+ attr.name = ptr_to_u64(name);
+ attr.prog_fd = prog_fd;
fd = sys_bpf_fd(BPF_RAW_TRACEPOINT_OPEN, &attr, attr_sz);
return libbpf_err_errno(fd);
@@ -1096,20 +1095,20 @@ int bpf_raw_tracepoint_open(const char *name, int prog_fd)
int bpf_btf_load(const void *btf_data, size_t btf_size, struct bpf_btf_load_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, btf_log_true_size);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_btf_load_attr, log_true_size);
+ struct bpf_btf_load_attr attr;
char *log_buf;
size_t log_size;
__u32 log_level;
int fd;
+ if (!OPTS_VALID(opts, bpf_btf_load_opts))
+ return libbpf_err(-EINVAL);
+
bump_rlimit_memlock();
memset(&attr, 0, attr_sz);
- if (!OPTS_VALID(opts, bpf_btf_load_opts))
- return libbpf_err(-EINVAL);
-
log_buf = OPTS_GET(opts, log_buf, NULL);
log_size = OPTS_GET(opts, log_size, 0);
log_level = OPTS_GET(opts, log_level, 0);
@@ -1127,20 +1126,20 @@ int bpf_btf_load(const void *btf_data, size_t btf_size, struct bpf_btf_load_opts
* APIs within libbpf and provides a sensible behavior in practice
*/
if (log_level) {
- attr.btf_log_buf = ptr_to_u64(log_buf);
- attr.btf_log_size = (__u32)log_size;
- attr.btf_log_level = log_level;
+ attr.log_buf = ptr_to_u64(log_buf);
+ attr.log_size = (__u32)log_size;
+ attr.log_level = log_level;
}
fd = sys_bpf_fd(BPF_BTF_LOAD, &attr, attr_sz);
if (fd < 0 && log_buf && log_level == 0) {
- attr.btf_log_buf = ptr_to_u64(log_buf);
- attr.btf_log_size = (__u32)log_size;
- attr.btf_log_level = 1;
+ attr.log_buf = ptr_to_u64(log_buf);
+ attr.log_size = (__u32)log_size;
+ attr.log_level = 1;
fd = sys_bpf_fd(BPF_BTF_LOAD, &attr, attr_sz);
}
- OPTS_SET(opts, log_true_size, attr.btf_log_true_size);
+ OPTS_SET(opts, log_true_size, attr.log_true_size);
return libbpf_err_errno(fd);
}
@@ -1148,36 +1147,36 @@ int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf, __u32 *buf_len,
__u32 *prog_id, __u32 *fd_type, __u64 *probe_offset,
__u64 *probe_addr)
{
- const size_t attr_sz = offsetofend(union bpf_attr, task_fd_query);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_task_fd_query_attr, probe_addr);
+ struct bpf_task_fd_query_attr attr;
int err;
memset(&attr, 0, attr_sz);
- attr.task_fd_query.pid = pid;
- attr.task_fd_query.fd = fd;
- attr.task_fd_query.flags = flags;
- attr.task_fd_query.buf = ptr_to_u64(buf);
- attr.task_fd_query.buf_len = *buf_len;
+ attr.pid = pid;
+ attr.fd = fd;
+ attr.flags = flags;
+ attr.buf = ptr_to_u64(buf);
+ attr.buf_len = *buf_len;
err = sys_bpf(BPF_TASK_FD_QUERY, &attr, attr_sz);
- *buf_len = attr.task_fd_query.buf_len;
- *prog_id = attr.task_fd_query.prog_id;
- *fd_type = attr.task_fd_query.fd_type;
- *probe_offset = attr.task_fd_query.probe_offset;
- *probe_addr = attr.task_fd_query.probe_addr;
+ *buf_len = attr.buf_len;
+ *prog_id = attr.prog_id;
+ *fd_type = attr.fd_type;
+ *probe_offset = attr.probe_offset;
+ *probe_addr = attr.probe_addr;
return libbpf_err_errno(err);
}
int bpf_enable_stats(enum bpf_stats_type type)
{
- const size_t attr_sz = offsetofend(union bpf_attr, enable_stats);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_enable_stats_attr, type);
+ struct bpf_enable_stats_attr attr;
int fd;
memset(&attr, 0, attr_sz);
- attr.enable_stats.type = type;
+ attr.type = type;
fd = sys_bpf_fd(BPF_ENABLE_STATS, &attr, attr_sz);
return libbpf_err_errno(fd);
@@ -1186,17 +1185,17 @@ int bpf_enable_stats(enum bpf_stats_type type)
int bpf_prog_bind_map(int prog_fd, int map_fd,
const struct bpf_prog_bind_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, prog_bind_map);
- union bpf_attr attr;
+ const size_t attr_sz = offsetofend(struct bpf_prog_bind_map_attr, flags);
+ struct bpf_prog_bind_map_attr attr;
int ret;
if (!OPTS_VALID(opts, bpf_prog_bind_opts))
return libbpf_err(-EINVAL);
memset(&attr, 0, attr_sz);
- attr.prog_bind_map.prog_fd = prog_fd;
- attr.prog_bind_map.map_fd = map_fd;
- attr.prog_bind_map.flags = OPTS_GET(opts, flags, 0);
+ attr.prog_fd = prog_fd;
+ attr.map_fd = map_fd;
+ attr.flags = OPTS_GET(opts, flags, 0);
ret = sys_bpf(BPF_PROG_BIND_MAP, &attr, attr_sz);
return libbpf_err_errno(ret);
diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index cf3323fd47b8..f6c540fb6250 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c
@@ -417,9 +417,9 @@ void bpf_gen__free(struct bpf_gen *gen)
void bpf_gen__load_btf(struct bpf_gen *gen, const void *btf_raw_data,
__u32 btf_raw_size)
{
- int attr_size = offsetofend(union bpf_attr, btf_log_level);
+ int attr_size = offsetofend(struct bpf_btf_load_attr, log_level);
int btf_data, btf_load_attr;
- union bpf_attr attr;
+ struct bpf_btf_load_attr attr;
memset(&attr, 0, attr_size);
pr_debug("gen: load_btf: size %d\n", btf_raw_size);
@@ -429,14 +429,14 @@ void bpf_gen__load_btf(struct bpf_gen *gen, const void *btf_raw_data,
btf_load_attr = add_data(gen, &attr, attr_size);
/* populate union bpf_attr with user provided log details */
- move_ctx2blob(gen, attr_field(btf_load_attr, btf_log_level), 4,
+ move_ctx2blob(gen, attr_field(btf_load_attr, btf_load.log_level), 4,
offsetof(struct bpf_loader_ctx, log_level), false);
- move_ctx2blob(gen, attr_field(btf_load_attr, btf_log_size), 4,
+ move_ctx2blob(gen, attr_field(btf_load_attr, btf_load.log_size), 4,
offsetof(struct bpf_loader_ctx, log_size), false);
- move_ctx2blob(gen, attr_field(btf_load_attr, btf_log_buf), 8,
+ move_ctx2blob(gen, attr_field(btf_load_attr, btf_load.log_buf), 8,
offsetof(struct bpf_loader_ctx, log_buf), false);
/* populate union bpf_attr with a pointer to the BTF data */
- emit_rel_store(gen, attr_field(btf_load_attr, btf), btf_data);
+ emit_rel_store(gen, attr_field(btf_load_attr, btf_load.btf), btf_data);
/* emit BTF_LOAD command */
emit_sys_bpf(gen, BPF_BTF_LOAD, btf_load_attr, attr_size);
debug_ret(gen, "btf_load size %d", btf_raw_size);
@@ -451,10 +451,10 @@ void bpf_gen__map_create(struct bpf_gen *gen,
__u32 key_size, __u32 value_size, __u32 max_entries,
struct bpf_map_create_opts *map_attr, int map_idx)
{
- int attr_size = offsetofend(union bpf_attr, map_extra);
+ int attr_size = offsetofend(struct bpf_map_create_attr, map_extra);
bool close_inner_map_fd = false;
int map_create_attr, idx;
- union bpf_attr attr;
+ struct bpf_map_create_attr attr;
memset(&attr, 0, attr_size);
attr.map_type = map_type;
@@ -476,12 +476,12 @@ void bpf_gen__map_create(struct bpf_gen *gen,
map_create_attr = add_data(gen, &attr, attr_size);
if (attr.btf_value_type_id)
/* populate union bpf_attr with btf_fd saved in the stack earlier */
- move_stack2blob(gen, attr_field(map_create_attr, btf_fd), 4,
+ move_stack2blob(gen, attr_field(map_create_attr, map_create.btf_fd), 4,
stack_off(btf_fd));
switch (attr.map_type) {
case BPF_MAP_TYPE_ARRAY_OF_MAPS:
case BPF_MAP_TYPE_HASH_OF_MAPS:
- move_stack2blob(gen, attr_field(map_create_attr, inner_map_fd), 4,
+ move_stack2blob(gen, attr_field(map_create_attr, map_create.inner_map_fd), 4,
stack_off(inner_map_fd));
close_inner_map_fd = true;
break;
@@ -490,7 +490,7 @@ void bpf_gen__map_create(struct bpf_gen *gen,
}
/* conditionally update max_entries */
if (map_idx >= 0)
- move_ctx2blob(gen, attr_field(map_create_attr, max_entries), 4,
+ move_ctx2blob(gen, attr_field(map_create_attr, map_create.max_entries), 4,
sizeof(struct bpf_loader_ctx) +
sizeof(struct bpf_map_desc) * map_idx +
offsetof(struct bpf_map_desc, max_entries),
@@ -937,8 +937,8 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
struct bpf_prog_load_opts *load_attr, int prog_idx)
{
int prog_load_attr, license_off, insns_off, func_info, line_info, core_relos;
- int attr_size = offsetofend(union bpf_attr, core_relo_rec_size);
- union bpf_attr attr;
+ int attr_size = offsetofend(struct bpf_prog_load_attr, core_relo_rec_size);
+ struct bpf_prog_load_attr attr;
memset(&attr, 0, attr_size);
pr_debug("gen: prog_load: type %d insns_cnt %zd progi_idx %d\n",
@@ -975,32 +975,32 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
prog_load_attr = add_data(gen, &attr, attr_size);
/* populate union bpf_attr with a pointer to license */
- emit_rel_store(gen, attr_field(prog_load_attr, license), license_off);
+ emit_rel_store(gen, attr_field(prog_load_attr, prog_load.license), license_off);
/* populate union bpf_attr with a pointer to instructions */
- emit_rel_store(gen, attr_field(prog_load_attr, insns), insns_off);
+ emit_rel_store(gen, attr_field(prog_load_attr, prog_load.insns), insns_off);
/* populate union bpf_attr with a pointer to func_info */
- emit_rel_store(gen, attr_field(prog_load_attr, func_info), func_info);
+ emit_rel_store(gen, attr_field(prog_load_attr, prog_load.func_info), func_info);
/* populate union bpf_attr with a pointer to line_info */
- emit_rel_store(gen, attr_field(prog_load_attr, line_info), line_info);
+ emit_rel_store(gen, attr_field(prog_load_attr, prog_load.line_info), line_info);
/* populate union bpf_attr with a pointer to core_relos */
- emit_rel_store(gen, attr_field(prog_load_attr, core_relos), core_relos);
+ emit_rel_store(gen, attr_field(prog_load_attr, prog_load.core_relos), core_relos);
/* populate union bpf_attr fd_array with a pointer to data where map_fds are saved */
- emit_rel_store(gen, attr_field(prog_load_attr, fd_array), gen->fd_array);
+ emit_rel_store(gen, attr_field(prog_load_attr, prog_load.fd_array), gen->fd_array);
/* populate union bpf_attr with user provided log details */
- move_ctx2blob(gen, attr_field(prog_load_attr, log_level), 4,
+ move_ctx2blob(gen, attr_field(prog_load_attr, prog_load.log_level), 4,
offsetof(struct bpf_loader_ctx, log_level), false);
- move_ctx2blob(gen, attr_field(prog_load_attr, log_size), 4,
+ move_ctx2blob(gen, attr_field(prog_load_attr, prog_load.log_size), 4,
offsetof(struct bpf_loader_ctx, log_size), false);
- move_ctx2blob(gen, attr_field(prog_load_attr, log_buf), 8,
+ move_ctx2blob(gen, attr_field(prog_load_attr, prog_load.log_buf), 8,
offsetof(struct bpf_loader_ctx, log_buf), false);
/* populate union bpf_attr with btf_fd saved in the stack earlier */
- move_stack2blob(gen, attr_field(prog_load_attr, prog_btf_fd), 4,
+ move_stack2blob(gen, attr_field(prog_load_attr, prog_load.prog_btf_fd), 4,
stack_off(btf_fd));
if (gen->attach_kind) {
emit_find_attach_target(gen);
@@ -1008,10 +1008,10 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_0, BPF_PSEUDO_MAP_IDX_VALUE,
0, 0, 0, prog_load_attr));
emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_0, BPF_REG_7,
- offsetof(union bpf_attr, attach_btf_id)));
+ offsetof(union bpf_attr, prog_load.attach_btf_id)));
emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_7, 32));
emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_0, BPF_REG_7,
- offsetof(union bpf_attr, attach_btf_obj_fd)));
+ offsetof(union bpf_attr, prog_load.attach_btf_obj_fd)));
}
emit_relos(gen, insns_off);
/* emit PROG_LOAD command */
@@ -1021,7 +1021,7 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
cleanup_relos(gen, insns_off);
if (gen->attach_kind) {
emit_sys_close_blob(gen,
- attr_field(prog_load_attr, attach_btf_obj_fd));
+ attr_field(prog_load_attr, prog_load.attach_btf_obj_fd));
gen->attach_kind = 0;
}
emit_check_err(gen);
@@ -1034,9 +1034,9 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
void bpf_gen__map_update_elem(struct bpf_gen *gen, int map_idx, void *pvalue,
__u32 value_size)
{
- int attr_size = offsetofend(union bpf_attr, flags);
+ int attr_size = offsetofend(struct bpf_map_elem_attr, flags);
int map_update_attr, value, key;
- union bpf_attr attr;
+ struct bpf_map_elem_attr attr;
int zero = 0;
memset(&attr, 0, attr_size);
@@ -1068,10 +1068,10 @@ void bpf_gen__map_update_elem(struct bpf_gen *gen, int map_idx, void *pvalue,
emit(gen, BPF_EMIT_CALL(BPF_FUNC_probe_read_kernel));
map_update_attr = add_data(gen, &attr, attr_size);
- move_blob2blob(gen, attr_field(map_update_attr, map_fd), 4,
+ move_blob2blob(gen, attr_field(map_update_attr, map_elem.map_fd), 4,
blob_fd_array_off(gen, map_idx));
- emit_rel_store(gen, attr_field(map_update_attr, key), key);
- emit_rel_store(gen, attr_field(map_update_attr, value), value);
+ emit_rel_store(gen, attr_field(map_update_attr, map_elem.key), key);
+ emit_rel_store(gen, attr_field(map_update_attr, map_elem.value), value);
/* emit MAP_UPDATE_ELEM command */
emit_sys_bpf(gen, BPF_MAP_UPDATE_ELEM, map_update_attr, attr_size);
debug_ret(gen, "update_elem idx %d value_size %d", map_idx, value_size);
@@ -1081,9 +1081,9 @@ void bpf_gen__map_update_elem(struct bpf_gen *gen, int map_idx, void *pvalue,
void bpf_gen__populate_outer_map(struct bpf_gen *gen, int outer_map_idx, int slot,
int inner_map_idx)
{
- int attr_size = offsetofend(union bpf_attr, flags);
+ int attr_size = offsetofend(struct bpf_map_elem_attr, flags);
int map_update_attr, key;
- union bpf_attr attr;
+ struct bpf_map_elem_attr attr;
memset(&attr, 0, attr_size);
pr_debug("gen: populate_outer_map: outer %d key %d inner %d\n",
@@ -1092,10 +1092,10 @@ void bpf_gen__populate_outer_map(struct bpf_gen *gen, int outer_map_idx, int slo
key = add_data(gen, &slot, sizeof(slot));
map_update_attr = add_data(gen, &attr, attr_size);
- move_blob2blob(gen, attr_field(map_update_attr, map_fd), 4,
+ move_blob2blob(gen, attr_field(map_update_attr, map_elem.map_fd), 4,
blob_fd_array_off(gen, outer_map_idx));
- emit_rel_store(gen, attr_field(map_update_attr, key), key);
- emit_rel_store(gen, attr_field(map_update_attr, value),
+ emit_rel_store(gen, attr_field(map_update_attr, map_elem.key), key);
+ emit_rel_store(gen, attr_field(map_update_attr, map_elem.value),
blob_fd_array_off(gen, inner_map_idx));
/* emit MAP_UPDATE_ELEM command */
@@ -1107,14 +1107,14 @@ void bpf_gen__populate_outer_map(struct bpf_gen *gen, int outer_map_idx, int slo
void bpf_gen__map_freeze(struct bpf_gen *gen, int map_idx)
{
- int attr_size = offsetofend(union bpf_attr, map_fd);
+ int attr_size = offsetofend(struct bpf_map_freeze_attr, map_fd);
int map_freeze_attr;
- union bpf_attr attr;
+ struct bpf_map_freeze_attr attr;
memset(&attr, 0, attr_size);
pr_debug("gen: map_freeze: idx %d\n", map_idx);
map_freeze_attr = add_data(gen, &attr, attr_size);
- move_blob2blob(gen, attr_field(map_freeze_attr, map_fd), 4,
+ move_blob2blob(gen, attr_field(map_freeze_attr, map_freeze.map_fd), 4,
blob_fd_array_off(gen, map_idx));
/* emit MAP_FREEZE command */
emit_sys_bpf(gen, BPF_MAP_FREEZE, map_freeze_attr, attr_size);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 5cca00979aae..113dbe9cb240 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4525,12 +4525,12 @@ static int probe_fd(int fd)
static int probe_kern_prog_name(void)
{
- const size_t attr_sz = offsetofend(union bpf_attr, prog_name);
+ const size_t attr_sz = offsetofend(struct bpf_prog_load_attr, prog_name);
struct bpf_insn insns[] = {
BPF_MOV64_IMM(BPF_REG_0, 0),
BPF_EXIT_INSN(),
};
- union bpf_attr attr;
+ struct bpf_prog_load_attr attr;
int ret;
memset(&attr, 0, attr_sz);
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index e4d05662a96c..9a809c610f36 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -575,6 +575,6 @@ static inline bool is_pow_of_2(size_t x)
}
#define PROG_LOAD_ATTEMPTS 5
-int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int attempts);
+int sys_bpf_prog_load(struct bpf_prog_load_attr *attr, unsigned int size, int attempts);
#endif /* __LIBBPF_LIBBPF_INTERNAL_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct
2023-05-24 21:02 ` [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct Andrii Nakryiko
@ 2023-05-25 3:18 ` Alexei Starovoitov
2023-05-25 17:19 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2023-05-25 3:18 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, daniel, martin.lau, kernel-team
On Wed, May 24, 2023 at 02:02:41PM -0700, Andrii Nakryiko wrote:
>
> And there were a bunch of other similar changes. Please take a thorough
> look and suggest more changes or which changes to drop. I'm not married
> to any of them, it just felt like a good improvement.
Agree that current layout sucks, but ...
> include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> kernel/bpf/syscall.c | 40 +++---
> tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> 3 files changed, 405 insertions(+), 105 deletions(-)
... the diff makes it worse. The diffstat for "nop" change is a red flag.
> + /*
> + * LEGACY anonymous substructs, for backwards compatibility.
> + * Each of the below anonymous substructs are ABI compatible with one
> + * of the above named substructs. Please use named substructs.
> + */
> +
All of them cannot be removed. This bagage will be a forever eyesore.
Currently it's not pretty. The diffs make uapi file just ugly.
Especially considering how 'named' and 'legacy' will start diverging.
New commands are thankfully named. We've learned the lesson,
but prior mistake is unfixable. We have to live with it.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct
2023-05-25 3:18 ` Alexei Starovoitov
@ 2023-05-25 17:19 ` Andrii Nakryiko
2023-05-25 21:51 ` Daniel Borkmann
0 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2023-05-25 17:19 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, bpf, ast, daniel, martin.lau, kernel-team
On Wed, May 24, 2023 at 8:18 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, May 24, 2023 at 02:02:41PM -0700, Andrii Nakryiko wrote:
> >
> > And there were a bunch of other similar changes. Please take a thorough
> > look and suggest more changes or which changes to drop. I'm not married
> > to any of them, it just felt like a good improvement.
>
> Agree that current layout sucks, but ...
>
> > include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> > kernel/bpf/syscall.c | 40 +++---
> > tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> > 3 files changed, 405 insertions(+), 105 deletions(-)
>
> ... the diff makes it worse. The diffstat for "nop" change is a red flag.
Only 100 lines are a real "nop" change to copy/paste existing fields
that are in unnamed fields. The rest is a value add.
I don't think the deal is in stats, though, right?
>
> > + /*
> > + * LEGACY anonymous substructs, for backwards compatibility.
> > + * Each of the below anonymous substructs are ABI compatible with one
> > + * of the above named substructs. Please use named substructs.
> > + */
> > +
>
> All of them cannot be removed. This bagage will be a forever eyesore.
> Currently it's not pretty. The diffs make uapi file just ugly.
> Especially considering how 'named' and 'legacy' will start diverging.
We have to allow "divergence" (only in the sense that new fields only
go into named substructs, but the existing fields stay fixed, of
course), to avoid more naming conflicts. If that wasn't the case,
using struct_group() macro could have been used to avoid a copy/paste
of those anonymous field/struct copies.
So I'm not happy about those 100 lines copy paste of fixed fields
either, but at least that would get us out of the current global
naming namespace for PROG_LOAD, MAP_CREATE, BTF_LOAD, etc.
> New commands are thankfully named. We've learned the lesson,
Unfortunately, the problem is that unnamed commands are the ones that
are most likely to keep evolving.
> but prior mistake is unfixable. We have to live with it.
Ok, too bad, but it's fine. It was worth a try.
I tried to come up with something like struct_group() approach to
minimize code changes in UAPI header, but we have a more complicated
situation where part of struct has to be both anonymous and named,
while another part (newly added fields) should go only to named parts.
And that doesn't seem to be possible to support with a macro,
unfortunately.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct
2023-05-25 17:19 ` Andrii Nakryiko
@ 2023-05-25 21:51 ` Daniel Borkmann
2023-05-25 23:39 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Borkmann @ 2023-05-25 21:51 UTC (permalink / raw)
To: Andrii Nakryiko, Alexei Starovoitov
Cc: Andrii Nakryiko, bpf, ast, martin.lau, kernel-team
On 5/25/23 7:19 PM, Andrii Nakryiko wrote:
> On Wed, May 24, 2023 at 8:18 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Wed, May 24, 2023 at 02:02:41PM -0700, Andrii Nakryiko wrote:
>>>
>>> And there were a bunch of other similar changes. Please take a thorough
>>> look and suggest more changes or which changes to drop. I'm not married
>>> to any of them, it just felt like a good improvement.
>>
>> Agree that current layout sucks, but ...
>>
>>> include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
>>> kernel/bpf/syscall.c | 40 +++---
>>> tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
>>> 3 files changed, 405 insertions(+), 105 deletions(-)
>>
>> ... the diff makes it worse. The diffstat for "nop" change is a red flag.
>
> Only 100 lines are a real "nop" change to copy/paste existing fields
> that are in unnamed fields. The rest is a value add.
>
> I don't think the deal is in stats, though, right?
>
>>> + /*
>>> + * LEGACY anonymous substructs, for backwards compatibility.
>>> + * Each of the below anonymous substructs are ABI compatible with one
>>> + * of the above named substructs. Please use named substructs.
>>> + */
>>
>> All of them cannot be removed. This bagage will be a forever eyesore.
>> Currently it's not pretty. The diffs make uapi file just ugly.
>> Especially considering how 'named' and 'legacy' will start diverging.
>
> We have to allow "divergence" (only in the sense that new fields only
> go into named substructs, but the existing fields stay fixed, of
> course), to avoid more naming conflicts. If that wasn't the case,
> using struct_group() macro could have been used to avoid a copy/paste
> of those anonymous field/struct copies.
>
> So I'm not happy about those 100 lines copy paste of fixed fields
> either, but at least that would get us out of the current global
> naming namespace for PROG_LOAD, MAP_CREATE, BTF_LOAD, etc.
>
>> New commands are thankfully named. We've learned the lesson,
>
> Unfortunately, the problem is that unnamed commands are the ones that
> are most likely to keep evolving.
>
>> but prior mistake is unfixable. We have to live with it.
>
> Ok, too bad, but it's fine. It was worth a try.
>
> I tried to come up with something like struct_group() approach to
> minimize code changes in UAPI header, but we have a more complicated
> situation where part of struct has to be both anonymous and named,
> while another part (newly added fields) should go only to named parts.
> And that doesn't seem to be possible to support with a macro,
> unfortunately.
Nice idea on the struct_group()-like approach, but agree that this is
going to be tough given we need to divert anonymous and named parts as
you mention. One other wild thought ... we remove the bpf_attr entirely
from the uapi header, and have a kernel/bpf/bpf.cmd description and
generate the bpf_attr into a uapi header via script which the main header
can include. Kind of similar to the suggestion, but more flexible than
macro magic. We also have things like syscall table header generated via
script.. so it wouldn't be first. Still doesn't remove the eyesore, just
packages it differently. ;/
Thanks,
Daniel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct
2023-05-25 21:51 ` Daniel Borkmann
@ 2023-05-25 23:39 ` Andrii Nakryiko
2023-05-30 17:41 ` Alexei Starovoitov
0 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2023-05-25 23:39 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Andrii Nakryiko, bpf, ast, martin.lau, kernel-team
On Thu, May 25, 2023 at 2:51 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 5/25/23 7:19 PM, Andrii Nakryiko wrote:
> > On Wed, May 24, 2023 at 8:18 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >> On Wed, May 24, 2023 at 02:02:41PM -0700, Andrii Nakryiko wrote:
> >>>
> >>> And there were a bunch of other similar changes. Please take a thorough
> >>> look and suggest more changes or which changes to drop. I'm not married
> >>> to any of them, it just felt like a good improvement.
> >>
> >> Agree that current layout sucks, but ...
> >>
> >>> include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> >>> kernel/bpf/syscall.c | 40 +++---
> >>> tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> >>> 3 files changed, 405 insertions(+), 105 deletions(-)
> >>
> >> ... the diff makes it worse. The diffstat for "nop" change is a red flag.
> >
> > Only 100 lines are a real "nop" change to copy/paste existing fields
> > that are in unnamed fields. The rest is a value add.
> >
> > I don't think the deal is in stats, though, right?
> >
> >>> + /*
> >>> + * LEGACY anonymous substructs, for backwards compatibility.
> >>> + * Each of the below anonymous substructs are ABI compatible with one
> >>> + * of the above named substructs. Please use named substructs.
> >>> + */
> >>
> >> All of them cannot be removed. This bagage will be a forever eyesore.
> >> Currently it's not pretty. The diffs make uapi file just ugly.
> >> Especially considering how 'named' and 'legacy' will start diverging.
> >
> > We have to allow "divergence" (only in the sense that new fields only
> > go into named substructs, but the existing fields stay fixed, of
> > course), to avoid more naming conflicts. If that wasn't the case,
> > using struct_group() macro could have been used to avoid a copy/paste
> > of those anonymous field/struct copies.
> >
> > So I'm not happy about those 100 lines copy paste of fixed fields
> > either, but at least that would get us out of the current global
> > naming namespace for PROG_LOAD, MAP_CREATE, BTF_LOAD, etc.
> >
> >> New commands are thankfully named. We've learned the lesson,
> >
> > Unfortunately, the problem is that unnamed commands are the ones that
> > are most likely to keep evolving.
> >
> >> but prior mistake is unfixable. We have to live with it.
> >
> > Ok, too bad, but it's fine. It was worth a try.
> >
> > I tried to come up with something like struct_group() approach to
> > minimize code changes in UAPI header, but we have a more complicated
> > situation where part of struct has to be both anonymous and named,
> > while another part (newly added fields) should go only to named parts.
> > And that doesn't seem to be possible to support with a macro,
> > unfortunately.
>
> Nice idea on the struct_group()-like approach, but agree that this is
> going to be tough given we need to divert anonymous and named parts as
> you mention. One other wild thought ... we remove the bpf_attr entirely
> from the uapi header, and have a kernel/bpf/bpf.cmd description and
> generate the bpf_attr into a uapi header via script which the main header
> can include. Kind of similar to the suggestion, but more flexible than
> macro magic. We also have things like syscall table header generated via
> script.. so it wouldn't be first. Still doesn't remove the eyesore, just
> packages it differently. ;/
There are two more ways, neither is that pretty. But I'll just outline
them here for completeness.
First, we can define about 6 variants (one for each command with anon
field) of macro with different numbers of arguments, one for each
existing field. Replace all semicolons with commas and do something
like this (we can prettify the below some more, I didn't want to waste
too much time on this demo):
#define __bpf_cmd4(type, f1, f2, f3, f4, new_fields...) \
struct { \
f1; f2; f3; f4; \
}; \
struct type { \
f1; f2; f3; f4; \
new_fields \
}
/* BPF_OBJ_PIN command */
__bpf_cmd4(bpf_obj_pin_attr,
__aligned_u64 pathname,
__u32 bpf_fd,
__u32 file_flags,
/* Same as dirfd in openat() syscall; see openat(2)
* manpage for details of path FD and pathname semantics;
* path_fd should accompanied by BPF_F_PATH_FD flag set in
* file_flags field, otherwise it should be set to zero;
* if BPF_F_PATH_FD flag is not set, AT_FDCWD is assumed.
*/
__s32 path_fd,
__u32 token_fd;
) obj_pin;
Note that I also added `__u32 token_fd;` as a demonstration how we can
new fields, and that new fields will have proper semicolons at the
end. The largest command (BPF_PROG_LOAD) will need 28 arg variant, but
that can be fit in few lines pretty cleanly, if the overall approach
would be deemed acceptable.
This approach also has a slight downside that we can rename fields
(e.g. for BPF_BTF_LOAD command). We still can split out dedicated new
named structs. So too big of a deal.
Second approach. If it's mostly about aesthetics, then we can add
include/uapi/linux/bpf_legacy.h, where we put all these unnamed fields
and structs in one stashed away place, and then in original
include/uapi/linux/bpf.h header we just
union bpf_attr {
... named structs and fields go here ...
/* include backwards compat legacy anon fields/structs */
#include "bpf_legacy.h"
};
This way this eyesore will be somewhat hidden away (but still lookup-able).
Curious if any of the above is more palatable?
>
> Thanks,
> Daniel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct
2023-05-25 23:39 ` Andrii Nakryiko
@ 2023-05-30 17:41 ` Alexei Starovoitov
2023-05-30 18:26 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2023-05-30 17:41 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Daniel Borkmann, Andrii Nakryiko, bpf, Alexei Starovoitov,
Martin KaFai Lau, Kernel Team
On Thu, May 25, 2023 at 4:40 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, May 25, 2023 at 2:51 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 5/25/23 7:19 PM, Andrii Nakryiko wrote:
> > > On Wed, May 24, 2023 at 8:18 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > >> On Wed, May 24, 2023 at 02:02:41PM -0700, Andrii Nakryiko wrote:
> > >>>
> > >>> And there were a bunch of other similar changes. Please take a thorough
> > >>> look and suggest more changes or which changes to drop. I'm not married
> > >>> to any of them, it just felt like a good improvement.
> > >>
> > >> Agree that current layout sucks, but ...
> > >>
> > >>> include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> > >>> kernel/bpf/syscall.c | 40 +++---
> > >>> tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> > >>> 3 files changed, 405 insertions(+), 105 deletions(-)
> > >>
> > >> ... the diff makes it worse. The diffstat for "nop" change is a red flag.
> > >
> > > Only 100 lines are a real "nop" change to copy/paste existing fields
> > > that are in unnamed fields. The rest is a value add.
> > >
> > > I don't think the deal is in stats, though, right?
> > >
> > >>> + /*
> > >>> + * LEGACY anonymous substructs, for backwards compatibility.
> > >>> + * Each of the below anonymous substructs are ABI compatible with one
> > >>> + * of the above named substructs. Please use named substructs.
> > >>> + */
> > >>
> > >> All of them cannot be removed. This bagage will be a forever eyesore.
> > >> Currently it's not pretty. The diffs make uapi file just ugly.
> > >> Especially considering how 'named' and 'legacy' will start diverging.
> > >
> > > We have to allow "divergence" (only in the sense that new fields only
> > > go into named substructs, but the existing fields stay fixed, of
> > > course), to avoid more naming conflicts. If that wasn't the case,
> > > using struct_group() macro could have been used to avoid a copy/paste
> > > of those anonymous field/struct copies.
> > >
> > > So I'm not happy about those 100 lines copy paste of fixed fields
> > > either, but at least that would get us out of the current global
> > > naming namespace for PROG_LOAD, MAP_CREATE, BTF_LOAD, etc.
> > >
> > >> New commands are thankfully named. We've learned the lesson,
> > >
> > > Unfortunately, the problem is that unnamed commands are the ones that
> > > are most likely to keep evolving.
> > >
> > >> but prior mistake is unfixable. We have to live with it.
> > >
> > > Ok, too bad, but it's fine. It was worth a try.
> > >
> > > I tried to come up with something like struct_group() approach to
> > > minimize code changes in UAPI header, but we have a more complicated
> > > situation where part of struct has to be both anonymous and named,
> > > while another part (newly added fields) should go only to named parts.
> > > And that doesn't seem to be possible to support with a macro,
> > > unfortunately.
> >
> > Nice idea on the struct_group()-like approach, but agree that this is
> > going to be tough given we need to divert anonymous and named parts as
> > you mention. One other wild thought ... we remove the bpf_attr entirely
> > from the uapi header, and have a kernel/bpf/bpf.cmd description and
> > generate the bpf_attr into a uapi header via script which the main header
> > can include. Kind of similar to the suggestion, but more flexible than
> > macro magic. We also have things like syscall table header generated via
> > script.. so it wouldn't be first. Still doesn't remove the eyesore, just
> > packages it differently. ;/
>
> There are two more ways, neither is that pretty. But I'll just outline
> them here for completeness.
>
> First, we can define about 6 variants (one for each command with anon
> field) of macro with different numbers of arguments, one for each
> existing field. Replace all semicolons with commas and do something
> like this (we can prettify the below some more, I didn't want to waste
> too much time on this demo):
>
> #define __bpf_cmd4(type, f1, f2, f3, f4, new_fields...) \
> struct { \
> f1; f2; f3; f4; \
> }; \
> struct type { \
> f1; f2; f3; f4; \
> new_fields \
> }
>
> /* BPF_OBJ_PIN command */
> __bpf_cmd4(bpf_obj_pin_attr,
> __aligned_u64 pathname,
> __u32 bpf_fd,
> __u32 file_flags,
> /* Same as dirfd in openat() syscall; see openat(2)
> * manpage for details of path FD and pathname semantics;
> * path_fd should accompanied by BPF_F_PATH_FD flag set in
> * file_flags field, otherwise it should be set to zero;
> * if BPF_F_PATH_FD flag is not set, AT_FDCWD is assumed.
> */
> __s32 path_fd,
> __u32 token_fd;
> ) obj_pin;
>
> Note that I also added `__u32 token_fd;` as a demonstration how we can
> new fields, and that new fields will have proper semicolons at the
> end. The largest command (BPF_PROG_LOAD) will need 28 arg variant, but
> that can be fit in few lines pretty cleanly, if the overall approach
> would be deemed acceptable.
>
> This approach also has a slight downside that we can rename fields
> (e.g. for BPF_BTF_LOAD command). We still can split out dedicated new
> named structs. So too big of a deal.
>
>
> Second approach. If it's mostly about aesthetics, then we can add
> include/uapi/linux/bpf_legacy.h, where we put all these unnamed fields
> and structs in one stashed away place, and then in original
> include/uapi/linux/bpf.h header we just
>
> union bpf_attr {
> ... named structs and fields go here ...
>
> /* include backwards compat legacy anon fields/structs */
> #include "bpf_legacy.h"
> };
>
> This way this eyesore will be somewhat hidden away (but still lookup-able).
>
>
> Curious if any of the above is more palatable?
Frankly I don't like either Daniel's .cmd idea or these two "aesthetics".
We just need new *_token_fd fields in several structures.
imo adding several such fields with different prefixes are cleaner
than revamping the whole thing.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct
2023-05-30 17:41 ` Alexei Starovoitov
@ 2023-05-30 18:26 ` Andrii Nakryiko
0 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2023-05-30 18:26 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Andrii Nakryiko, bpf, Alexei Starovoitov,
Martin KaFai Lau, Kernel Team
On Tue, May 30, 2023 at 10:41 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, May 25, 2023 at 4:40 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, May 25, 2023 at 2:51 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > > On 5/25/23 7:19 PM, Andrii Nakryiko wrote:
> > > > On Wed, May 24, 2023 at 8:18 PM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > >> On Wed, May 24, 2023 at 02:02:41PM -0700, Andrii Nakryiko wrote:
> > > >>>
> > > >>> And there were a bunch of other similar changes. Please take a thorough
> > > >>> look and suggest more changes or which changes to drop. I'm not married
> > > >>> to any of them, it just felt like a good improvement.
> > > >>
> > > >> Agree that current layout sucks, but ...
> > > >>
> > > >>> include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> > > >>> kernel/bpf/syscall.c | 40 +++---
> > > >>> tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> > > >>> 3 files changed, 405 insertions(+), 105 deletions(-)
> > > >>
> > > >> ... the diff makes it worse. The diffstat for "nop" change is a red flag.
> > > >
> > > > Only 100 lines are a real "nop" change to copy/paste existing fields
> > > > that are in unnamed fields. The rest is a value add.
> > > >
> > > > I don't think the deal is in stats, though, right?
> > > >
> > > >>> + /*
> > > >>> + * LEGACY anonymous substructs, for backwards compatibility.
> > > >>> + * Each of the below anonymous substructs are ABI compatible with one
> > > >>> + * of the above named substructs. Please use named substructs.
> > > >>> + */
> > > >>
> > > >> All of them cannot be removed. This bagage will be a forever eyesore.
> > > >> Currently it's not pretty. The diffs make uapi file just ugly.
> > > >> Especially considering how 'named' and 'legacy' will start diverging.
> > > >
> > > > We have to allow "divergence" (only in the sense that new fields only
> > > > go into named substructs, but the existing fields stay fixed, of
> > > > course), to avoid more naming conflicts. If that wasn't the case,
> > > > using struct_group() macro could have been used to avoid a copy/paste
> > > > of those anonymous field/struct copies.
> > > >
> > > > So I'm not happy about those 100 lines copy paste of fixed fields
> > > > either, but at least that would get us out of the current global
> > > > naming namespace for PROG_LOAD, MAP_CREATE, BTF_LOAD, etc.
> > > >
> > > >> New commands are thankfully named. We've learned the lesson,
> > > >
> > > > Unfortunately, the problem is that unnamed commands are the ones that
> > > > are most likely to keep evolving.
> > > >
> > > >> but prior mistake is unfixable. We have to live with it.
> > > >
> > > > Ok, too bad, but it's fine. It was worth a try.
> > > >
> > > > I tried to come up with something like struct_group() approach to
> > > > minimize code changes in UAPI header, but we have a more complicated
> > > > situation where part of struct has to be both anonymous and named,
> > > > while another part (newly added fields) should go only to named parts.
> > > > And that doesn't seem to be possible to support with a macro,
> > > > unfortunately.
> > >
> > > Nice idea on the struct_group()-like approach, but agree that this is
> > > going to be tough given we need to divert anonymous and named parts as
> > > you mention. One other wild thought ... we remove the bpf_attr entirely
> > > from the uapi header, and have a kernel/bpf/bpf.cmd description and
> > > generate the bpf_attr into a uapi header via script which the main header
> > > can include. Kind of similar to the suggestion, but more flexible than
> > > macro magic. We also have things like syscall table header generated via
> > > script.. so it wouldn't be first. Still doesn't remove the eyesore, just
> > > packages it differently. ;/
> >
> > There are two more ways, neither is that pretty. But I'll just outline
> > them here for completeness.
> >
> > First, we can define about 6 variants (one for each command with anon
> > field) of macro with different numbers of arguments, one for each
> > existing field. Replace all semicolons with commas and do something
> > like this (we can prettify the below some more, I didn't want to waste
> > too much time on this demo):
> >
> > #define __bpf_cmd4(type, f1, f2, f3, f4, new_fields...) \
> > struct { \
> > f1; f2; f3; f4; \
> > }; \
> > struct type { \
> > f1; f2; f3; f4; \
> > new_fields \
> > }
> >
> > /* BPF_OBJ_PIN command */
> > __bpf_cmd4(bpf_obj_pin_attr,
> > __aligned_u64 pathname,
> > __u32 bpf_fd,
> > __u32 file_flags,
> > /* Same as dirfd in openat() syscall; see openat(2)
> > * manpage for details of path FD and pathname semantics;
> > * path_fd should accompanied by BPF_F_PATH_FD flag set in
> > * file_flags field, otherwise it should be set to zero;
> > * if BPF_F_PATH_FD flag is not set, AT_FDCWD is assumed.
> > */
> > __s32 path_fd,
> > __u32 token_fd;
> > ) obj_pin;
> >
> > Note that I also added `__u32 token_fd;` as a demonstration how we can
> > new fields, and that new fields will have proper semicolons at the
> > end. The largest command (BPF_PROG_LOAD) will need 28 arg variant, but
> > that can be fit in few lines pretty cleanly, if the overall approach
> > would be deemed acceptable.
> >
> > This approach also has a slight downside that we can rename fields
> > (e.g. for BPF_BTF_LOAD command). We still can split out dedicated new
> > named structs. So too big of a deal.
> >
> >
> > Second approach. If it's mostly about aesthetics, then we can add
> > include/uapi/linux/bpf_legacy.h, where we put all these unnamed fields
> > and structs in one stashed away place, and then in original
> > include/uapi/linux/bpf.h header we just
> >
> > union bpf_attr {
> > ... named structs and fields go here ...
> >
> > /* include backwards compat legacy anon fields/structs */
> > #include "bpf_legacy.h"
> > };
> >
> > This way this eyesore will be somewhat hidden away (but still lookup-able).
> >
> >
> > Curious if any of the above is more palatable?
>
> Frankly I don't like either Daniel's .cmd idea or these two "aesthetics".
> We just need new *_token_fd fields in several structures.
> imo adding several such fields with different prefixes are cleaner
> than revamping the whole thing.
Ok, sgtm.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-05-30 18:26 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-24 21:02 [PATCH RFC bpf-next 0/3] Revamp bpf_attr and make it easier to evolve Andrii Nakryiko
2023-05-24 21:02 ` [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct Andrii Nakryiko
2023-05-25 3:18 ` Alexei Starovoitov
2023-05-25 17:19 ` Andrii Nakryiko
2023-05-25 21:51 ` Daniel Borkmann
2023-05-25 23:39 ` Andrii Nakryiko
2023-05-30 17:41 ` Alexei Starovoitov
2023-05-30 18:26 ` Andrii Nakryiko
2023-05-24 21:02 ` [PATCH RFC bpf-next 2/3] bpf: use new named bpf_attr substructs for few commands Andrii Nakryiko
2023-05-24 21:02 ` [PATCH RFC bpf-next 3/3] libbpf: use new bpf_xxx_attr structs for bpf() commands Andrii Nakryiko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.