Re: [PATCH v2 bpf-next 3/5] bpf: Introduce bpf_sk_{,ancestor_}cgroup_id helpers

[Yonghong Song <yhs@fb.com>]

On 5/13/20 2:38 PM, Andrey Ignatov wrote:
> With having ability to lookup sockets in cgroup skb programs it becomes
> useful to access cgroup id of retrieved sockets so that policies can be
> implemented based on origin cgroup of such socket.
> 
> For example, a container running in a cgroup can have cgroup skb ingress
> program that can lookup peer socket that is sending packets to a process
> inside the container and decide whether those packets should be allowed
> or denied based on cgroup id of the peer.
> 
> More specifically such ingress program can implement intra-host policy
> "allow incoming packets only from this same container and not from any
> other container on same host" w/o relying on source IP addresses since
> quite often it can be the case that containers share same IP address on
> the host.
> 
> Introduce two new helpers for this use-case: bpf_sk_cgroup_id() and
> bpf_sk_ancestor_cgroup_id().
> 
> These helpers are similar to existing bpf_skb_{,ancestor_}cgroup_id
> helpers with the only difference that sk is used to get cgroup id
> instead of skb, and share code with them.
> 
> See documentation in UAPI for more details.
> 
> Signed-off-by: Andrey Ignatov <rdna@fb.com>

Ack with one nit below.
Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   include/uapi/linux/bpf.h       | 35 +++++++++++++++++++-
>   net/core/filter.c              | 60 +++++++++++++++++++++++++++++-----
>   tools/include/uapi/linux/bpf.h | 35 +++++++++++++++++++-
>   3 files changed, 119 insertions(+), 11 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index bfb31c1be219..e3cbc2790cdf 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3121,6 +3121,37 @@ union bpf_attr {
>    * 		0 on success, or a negative error in case of failure:
>    *
>    *		**-EOVERFLOW** if an overflow happened: The same object will be tried again.
> + *
> + * u64 bpf_sk_cgroup_id(struct bpf_sock *sk)
> + *	Description
> + *		Return the cgroup v2 id of the socket *sk*.
> + *
> + *		*sk* must be a non-**NULL** pointer that was returned from
> + *		**bpf_sk_lookup_xxx**\ (). The format of returned id is same

It should also include bpf_skc_lookup_tcp(), right?

> + *		as in **bpf_skb_cgroup_id**\ ().
> + *
> + *		This helper is available only if the kernel was compiled with
> + *		the **CONFIG_SOCK_CGROUP_DATA** configuration option.
> + *	Return
> + *		The id is returned or 0 in case the id could not be retrieved.
> + *
> + * u64 bpf_sk_ancestor_cgroup_id(struct bpf_sock *sk, int ancestor_level)
> + *	Description
> + *		Return id of cgroup v2 that is ancestor of cgroup associated
> + *		with the *sk* at the *ancestor_level*.  The root cgroup is at
> + *		*ancestor_level* zero and each step down the hierarchy
> + *		increments the level. If *ancestor_level* == level of cgroup
> + *		associated with *sk*, then return value will be same as that
> + *		of **bpf_sk_cgroup_id**\ ().
> + *
> + *		The helper is useful to implement policies based on cgroups
> + *		that are upper in hierarchy than immediate cgroup associated
> + *		with *sk*.
> + *
> + *		The format of returned id and helper limitations are same as in
> + *		**bpf_sk_cgroup_id**\ ().
> + *	Return
> + *		The id is returned or 0 in case the id could not be retrieved.
>    */
>   #define __BPF_FUNC_MAPPER(FN)		\
>   	FN(unspec),			\
> @@ -3250,7 +3281,9 @@ union bpf_attr {
>   	FN(sk_assign),			\
>   	FN(ktime_get_boot_ns),		\
>   	FN(seq_printf),			\
> -	FN(seq_write),
> +	FN(seq_write),			\
> +	FN(sk_cgroup_id),		\
> +	FN(sk_ancestor_cgroup_id),
>   
[...]