On 06/11/2023 11:20, Kevin Brodsky wrote:
On 30/10/2023 13:48, Zachary Leaf wrote:
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index f1c8733f76b8..c870a29c9be8 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -5015,6 +5015,298 @@ static int bpf_prog_bind_map(union bpf_attr *attr) return ret; } +static void convert_compat_bpf_attr(union bpf_attr *dest,
const union compat_bpf_attr *cattr, int cmd)+{
- struct bpf_prog *prog;
- memset(dest, 0, sizeof(union bpf_attr));
I'm realising, isn't this already done in copy_bpf_attr_from_user()?
Ah yep. Removed this one.
[...]
- case BPF_LINK_CREATE:
copy_field(dest, cattr, link_create.prog_fd);copy_field(dest, cattr, link_create.target_fd);/* u32 target_ifindex is in a union with u32 target_fd */copy_field(dest, cattr, link_create.attach_type);copy_field(dest, cattr, link_create.flags);prog = bpf_prog_get(cattr->link_create.prog_fd);if (prog->type == BPF_PROG_TYPE_CGROUP_SKB ||prog->type == BPF_PROG_TYPE_CGROUP_SOCK ||prog->type == BPF_PROG_TYPE_CGROUP_SOCK_ADDR ||prog->type == BPF_PROG_TYPE_SOCK_OPS ||prog->type == BPF_PROG_TYPE_CGROUP_DEVICE ||prog->type == BPF_PROG_TYPE_CGROUP_SYSCTL ||prog->type == BPF_PROG_TYPE_CGROUP_SOCKOPT)break;if (prog->type == BPF_PROG_TYPE_EXT) {copy_field(dest, cattr, link_create.tracing.target_btf_id);copy_field(dest, cattr, link_create.tracing.cookie);break;}if (prog->type == BPF_PROG_TYPE_LSM ||prog->type == BPF_PROG_TYPE_TRACING) {if (prog->expected_attach_type == BPF_TRACE_ITER) {/* iter_info is a user pointer to union* bpf_iter_link_info however since this union* contains no pointers, the size/offsets are* the same for compat64/purecap; hence no* conversion needed*/copy_field(dest, cattr, link_create.iter_info);copy_field(dest, cattr, link_create.iter_info_len);break;} else if (prog->expected_attach_type == BPF_TRACE_RAW_TP|| prog->expected_attach_type == BPF_LSM_CGROUP) {/* only uses common fields above */break;} else {copy_field(dest, cattr, link_create.target_btf_id);copy_field(dest, cattr, link_create.tracing.cookie);break;}}if (prog->type == BPF_PROG_TYPE_FLOW_DISSECTOR ||prog->type == BPF_PROG_TYPE_SK_LOOKUP ||prog->type == BPF_PROG_TYPE_XDP)break;/* bpf_cookie is used in bpf_perf_link_attach() */if (prog->type == BPF_PROG_TYPE_PERF_EVENT ||prog->type == BPF_PROG_TYPE_TRACEPOINT ||(prog->type == BPF_PROG_TYPE_KPROBE &&cattr->link_create.attach_type == BPF_PERF_EVENT)) {copy_field(dest, cattr, link_create.perf_event.bpf_cookie);break;}/* kprobe_multi is used in bpf_kprobe_multi_link_attach() */if (prog->type == BPF_PROG_TYPE_KPROBE &&cattr->link_create.attach_type != BPF_PERF_EVENT) {copy_field(dest, cattr, link_create.kprobe_multi.flags);copy_field(dest, cattr, link_create.kprobe_multi.cnt);copy_field(dest, cattr, link_create.kprobe_multi.syms);copy_field(dest, cattr, link_create.kprobe_multi.addrs);copy_field(dest, cattr, link_create.kprobe_multi.cookies);break;}if (prog->type == BPF_PROG_TYPE_NETFILTER) {copy_field(dest, cattr, link_create.netfilter.pf);copy_field(dest, cattr, link_create.netfilter.hooknum);copy_field(dest, cattr, link_create.netfilter.priority);copy_field(dest, cattr, link_create.netfilter.flags);break;}I don't feel that this approach is really maintainable. In the meantime, a new special case has already been added [1].
Two cases involve pointers. It seems to me that we can identify them based on attach_type alone: BPF_TRACE_ITER for iter_info, and BPF_TRACE_KPROBE_MULTI for kprobe_multi. There are conditions for a call with such attach_type values to be valid, but that doesn't matter - what does is that such attach_type values unambiguously mean that these fields are active.
Okay, good point. That's a good observation. BPF_LINK_CREATE is definitely the most annoying in terms of a bunch of conditionals, trying to account for all the interactions there is hard. Even the thought of trying to integrate [1] into that was already filling me with dread.
In all other cases, I think we could just use some memcpy() like in convert_compat_link_info_out() in patch 8. This way, we only need to update this code if operations involving pointers are added, which should be pretty infrequent.
Agreed this will be way more maintainable. Huge improvement to what is easily the worst part of bpf_attr for compat64 handling.
Thanks, Zach
Kevin
[1] https://lore.kernel.org/all/20230323032405.3735486-4-kuifeng@meta.com/