Re: [PATCH bpf-next 1/1] bpf: migrate cgroup_bpf to internal cgroup_bpf_attach_type enum

[Daniel Borkmann <daniel@iogearbox.net>]

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 <davemarchevsky@fb.com>

Overall looks good to me, great to reduce memory bloat. Some really minor bits
below ..

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

> +	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])
>   
[...]
> @@ -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)

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