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()?
[...]
- 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.
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.
Kevin
[1] https://lore.kernel.org/all/20230323032405.3735486-4-kuifeng@meta.com/