Re: [PATCH bpf-next 1/1] bpf: migrate cgroup_bpf to internal cgroup_bpf_attach_type enum
flat view
From: Daniel Borkmann <daniel@iogearbox.net>
Date: 2021-08-10 11:48:26
Also in:
bpf
On 8/1/21 1:30 AM, Dave Marchevsky wrote:
Add an enum (cgroup_bpf_attach_type) containing only valid cgroup_bpf attach types and a function to map bpf_attach_type values to the new enum. Inspired by netns_bpf_attach_type. Then, migrate cgroup_bpf to use cgroup_bpf_attach_type wherever possible. Functionality is unchanged as attach_type_to_prog_type switches in bpf/syscall.c were preventing non-cgroup programs from making use of the invalid cgroup_bpf array slots. As a result struct cgroup_bpf uses 504 fewer bytes relative to when its arrays were sized using MAX_BPF_ATTACH_TYPE.
Nice!
bpf_cgroup_storage is notably not migrated as struct bpf_cgroup_storage_key is part of uapi and contains a bpf_attach_type member which is not meant to be opaque. Similarly, bpf_cgroup_link continues to report its bpf_attach_type member to userspace via fdinfo and bpf_link_info. To ease disambiguation, bpf_attach_type variables are renamed from 'type' to 'atype' when changed to cgroup_bpf_attach_type. Regarding testing: biggest concerns here are 1) attach/detach/run for programs which shouldn't map to a cgroup_bpf_attach_type should continue to not involve cgroup_bpf codepaths; and 2) attach types that should be mapped to a cgroup_bpf_attach_type do so correctly and run as expected. Existing selftests cover both scenarios well. The udp_limit selftest specifically validates the 2nd case - BPF_CGROUP_INET_SOCK_RELEASE is larger than MAX_CGROUP_BPF_ATTACH_TYPE so if it were not correctly mapped to CG_BPF_CGROUP_INET_SOCK_RELEASE the test would fail. Signed-off-by: Dave Marchevsky <redacted>
Overall looks good to me, great to reduce memory bloat. Some really minor bits below ..
quoted hunk
--- include/linux/bpf-cgroup.h | 200 +++++++++++++++++++++++---------- include/uapi/linux/bpf.h | 2 +- kernel/bpf/cgroup.c | 154 +++++++++++++++---------- net/ipv4/af_inet.c | 6 +- net/ipv4/udp.c | 2 +- net/ipv6/af_inet6.c | 6 +- net/ipv6/udp.c | 2 +- tools/include/uapi/linux/bpf.h | 2 +- 8 files changed, 243 insertions(+), 131 deletions(-)diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h index a74cd1c3bd87..0fdd8931ec5a 100644 --- a/include/linux/bpf-cgroup.h +++ b/include/linux/bpf-cgroup.h@@ -23,9 +23,91 @@ struct ctl_table_header; struct task_struct; #ifdef CONFIG_CGROUP_BPF +enum cgroup_bpf_attach_type { + CG_BPF_INVALID = -1, + CG_BPF_CGROUP_INET_INGRESS = 0, + CG_BPF_CGROUP_INET_EGRESS, + CG_BPF_CGROUP_INET_SOCK_CREATE, + CG_BPF_CGROUP_SOCK_OPS, + CG_BPF_CGROUP_DEVICE, + CG_BPF_CGROUP_INET4_BIND, + CG_BPF_CGROUP_INET6_BIND, + CG_BPF_CGROUP_INET4_CONNECT, + CG_BPF_CGROUP_INET6_CONNECT, + CG_BPF_CGROUP_INET4_POST_BIND, + CG_BPF_CGROUP_INET6_POST_BIND, + CG_BPF_CGROUP_UDP4_SENDMSG, + CG_BPF_CGROUP_UDP6_SENDMSG, + CG_BPF_CGROUP_SYSCTL, + CG_BPF_CGROUP_UDP4_RECVMSG, + CG_BPF_CGROUP_UDP6_RECVMSG, + CG_BPF_CGROUP_GETSOCKOPT, + CG_BPF_CGROUP_SETSOCKOPT, + CG_BPF_CGROUP_INET4_GETPEERNAME, + CG_BPF_CGROUP_INET6_GETPEERNAME, + CG_BPF_CGROUP_INET4_GETSOCKNAME, + CG_BPF_CGROUP_INET6_GETSOCKNAME, + CG_BPF_CGROUP_INET_SOCK_RELEASE,
small nit: I'd drop the CG_BPF_ prefix altogether, the CG is redundant with latter CGROUP in name.
quoted hunk
+ MAX_CGROUP_BPF_ATTACH_TYPE +}; + +static inline enum cgroup_bpf_attach_type +to_cgroup_bpf_attach_type(enum bpf_attach_type attach_type) +{ + switch (attach_type) { + case BPF_CGROUP_INET_INGRESS: + return CG_BPF_CGROUP_INET_INGRESS; + case BPF_CGROUP_INET_EGRESS: + return CG_BPF_CGROUP_INET_EGRESS; + case BPF_CGROUP_INET_SOCK_CREATE: + return CG_BPF_CGROUP_INET_SOCK_CREATE; + case BPF_CGROUP_SOCK_OPS: + return CG_BPF_CGROUP_SOCK_OPS; + case BPF_CGROUP_DEVICE: + return CG_BPF_CGROUP_DEVICE; + case BPF_CGROUP_INET4_BIND: + return CG_BPF_CGROUP_INET4_BIND; + case BPF_CGROUP_INET6_BIND: + return CG_BPF_CGROUP_INET6_BIND; + case BPF_CGROUP_INET4_CONNECT: + return CG_BPF_CGROUP_INET4_CONNECT; + case BPF_CGROUP_INET6_CONNECT: + return CG_BPF_CGROUP_INET6_CONNECT; + case BPF_CGROUP_INET4_POST_BIND: + return CG_BPF_CGROUP_INET4_POST_BIND; + case BPF_CGROUP_INET6_POST_BIND: + return CG_BPF_CGROUP_INET6_POST_BIND; + case BPF_CGROUP_UDP4_SENDMSG: + return CG_BPF_CGROUP_UDP4_SENDMSG; + case BPF_CGROUP_UDP6_SENDMSG: + return CG_BPF_CGROUP_UDP6_SENDMSG; + case BPF_CGROUP_SYSCTL: + return CG_BPF_CGROUP_SYSCTL; + case BPF_CGROUP_UDP4_RECVMSG: + return CG_BPF_CGROUP_UDP4_RECVMSG; + case BPF_CGROUP_UDP6_RECVMSG: + return CG_BPF_CGROUP_UDP6_RECVMSG; + case BPF_CGROUP_GETSOCKOPT: + return CG_BPF_CGROUP_GETSOCKOPT; + case BPF_CGROUP_SETSOCKOPT: + return CG_BPF_CGROUP_SETSOCKOPT; + case BPF_CGROUP_INET4_GETPEERNAME: + return CG_BPF_CGROUP_INET4_GETPEERNAME; + case BPF_CGROUP_INET6_GETPEERNAME: + return CG_BPF_CGROUP_INET6_GETPEERNAME; + case BPF_CGROUP_INET4_GETSOCKNAME: + return CG_BPF_CGROUP_INET4_GETSOCKNAME; + case BPF_CGROUP_INET6_GETSOCKNAME: + return CG_BPF_CGROUP_INET6_GETSOCKNAME; + case BPF_CGROUP_INET_SOCK_RELEASE: + return CG_BPF_CGROUP_INET_SOCK_RELEASE;
Maybe this could use a small macro helper to map: case BPF_<x>: return <x>;
quoted hunk
+ default: + return CG_BPF_INVALID; + } +} -extern struct static_key_false cgroup_bpf_enabled_key[MAX_BPF_ATTACH_TYPE]; -#define cgroup_bpf_enabled(type) static_branch_unlikely(&cgroup_bpf_enabled_key[type]) +extern struct static_key_false cgroup_bpf_enabled_key[MAX_CGROUP_BPF_ATTACH_TYPE]; +#define cgroup_bpf_enabled(atype) static_branch_unlikely(&cgroup_bpf_enabled_key[atype])
[...]
quoted hunk
@@ -667,12 +681,20 @@ static struct bpf_prog_list *find_detach_entry(struct list_head *progs, int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog, struct bpf_cgroup_link *link, enum bpf_attach_type type) { - struct list_head *progs = &cgrp->bpf.progs[type]; - u32 flags = cgrp->bpf.flags[type]; + struct list_head *progs; + u32 flags; struct bpf_prog_list *pl; struct bpf_prog *old_prog; + enum cgroup_bpf_attach_type atype; int err;
(small nit here and elsewhere in your patch: pls try to retain reverse xmas tree ordering if possible)
quoted hunk
+ atype = to_cgroup_bpf_attach_type(type); + if (atype < 0) + return -EINVAL; + + progs = &cgrp->bpf.progs[atype]; + flags = cgrp->bpf.flags[atype]; + if (prog && link) /* only one of prog or link can be specified */ return -EINVAL;@@ -686,7 +708,7 @@ int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog, pl->prog = NULL; pl->link = NULL; - err = update_effective_progs(cgrp, type); + err = update_effective_progs(cgrp, atype); if (err) goto cleanup;@@ -695,10 +717,10 @@ int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog, kfree(pl); if (list_empty(progs)) /* last program was detached, reset flags to zero */ - cgrp->bpf.flags[type] = 0; + cgrp->bpf.flags[atype] = 0; if (old_prog) bpf_prog_put(old_prog); - static_branch_dec(&cgroup_bpf_enabled_key[type]); + static_branch_dec(&cgroup_bpf_enabled_key[atype]); return 0; cleanup: