On 12/07/2023 12:20, Kevin Brodsky wrote:
On 08/06/2023 10:42, Zachary Leaf wrote:
[...]
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 588e4d5d4077..cdba1e563688 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -4430,24 +4430,161 @@ static int bpf_btf_get_info_by_fd(struct file *file, return btf_get_info_by_fd(btf, attr, uattr); } +#ifdef CONFIG_COMPAT64 +void convert_compat_link_info_in(struct bpf_link_info *dest,
struct compat_bpf_link_info *cinfo,enum bpf_link_type type)+{
- /*
* Only a few fields in bpf_link_info are used as input, the* rest do not need conversion in*/- if (type == BPF_LINK_TYPE_RAW_TRACEPOINT) {
dest->raw_tracepoint.tp_name =cinfo->raw_tracepoint.tp_name;dest->raw_tracepoint.tp_name_len =cinfo->raw_tracepoint.tp_name_len;return;- }
- if (type == BPF_LINK_TYPE_ITER) {
dest->iter.target_name = cinfo->iter.target_name;dest->iter.target_name_len = cinfo->iter.target_name_len;return;- }
Nit: could make it a switch.
Done
+}
+/* is_iter_XYZ_target copied from tools/bpf/bpftool/link.c */ +static bool is_iter_map_target(const char *target_name) +{
- return strcmp(target_name, "bpf_map_elem") == 0 ||
strcmp(target_name, "bpf_sk_storage_map") == 0;+}
+static bool is_iter_cgroup_target(const char *target_name) +{
- return strcmp(target_name, "cgroup") == 0;
+}
+static bool is_iter_task_target(const char *target_name) +{
- return strcmp(target_name, "task") == 0 ||
strcmp(target_name, "task_file") == 0 ||strcmp(target_name, "task_vma") == 0;+}
+void convert_compat_link_info_out(struct compat_bpf_link_info *dest,
struct bpf_link_info *info,enum bpf_link_type type)+{
- const char *iter_target;
- dest->type = info->type;
- dest->id = info->id;
- dest->prog_id = info->prog_id;
- switch (type) {
- case BPF_LINK_TYPE_RAW_TRACEPOINT:
dest->raw_tracepoint.tp_name =info->raw_tracepoint.tp_name;dest->raw_tracepoint.tp_name_len =info->raw_tracepoint.tp_name_len;return;- case BPF_LINK_TYPE_TRACING:
dest->tracing.attach_type =info->tracing.attach_type;dest->tracing.target_obj_id =info->tracing.target_obj_id;dest->tracing.target_btf_id =info->tracing.target_btf_id;return;- case BPF_LINK_TYPE_CGROUP:
dest->cgroup.cgroup_id = info->cgroup.cgroup_id;dest->cgroup.attach_type = info->cgroup.attach_type;return;- case BPF_LINK_TYPE_ITER:
dest->iter.target_name = info->iter.target_name;dest->iter.target_name_len = info->iter.target_name_len;iter_target =(const char *)(uintptr_t)info->iter.target_name;if (is_iter_map_target(iter_target))dest->iter.map.map_id = info->iter.map.map_id;else if (is_iter_cgroup_target(iter_target)) {dest->iter.cgroup.cgroup_id =info->iter.cgroup.cgroup_id;dest->iter.cgroup.order =info->iter.cgroup.order;} else if (is_iter_task_target(iter_target)) {dest->iter.task.tid = info->iter.task.tid;dest->iter.task.pid = info->iter.task.pid;}This looks like the semantically correct way to do the conversion, but the complexity of matching the string doesn't feel worth it, considering that nothing here is a pointer and therefore we don't strictly need to know which union member is in use. Everything is a fixed-size integer, so I think it is safe to assume that the layout is the same in native and compat64. This means we could directly memcpy() the remaining members of the iter struct, from offsetof(map) to the end of the struct.
This should do the trick:
memcpy((u8 *)dest+offsetof(struct compat_bpf_link_info, iter.map), (u8 *)info+offsetof(struct bpf_link_info, iter.map), offsetofend(struct bpf_link_info, iter.cgroup.order) - offsetof(struct bpf_link_info, iter.map.map_id));
4 lines instead of ~30 is definitely an improvement there.
Thanks, Zach
Kevin