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 ..

small nit: I'd drop the CG_BPF_ prefix altogether, the CG is redundant
with latter CGROUP in name.

+	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>;

+	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])
  

[...]

(small nit here and elsewhere in your patch: pls try to retain reverse xmas
tree ordering if possible)