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

[Yonghong Song <yhs@fb.com>]

On 5/14/20 9:55 AM, Andrey Ignatov wrote:
> Yonghong Song <yhs@fb.com> [Thu, 2020-05-14 08:16 -0700]:
>> On 5/13/20 2:38 PM, Andrey Ignatov wrote:
> 
>>> 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?
> 
>  From what I see it should not.
> 
> cgroup id is available from sk->sk_cgrp_data that is a field of `struct
> sock', i.e. `struct sock_common' doesn't have this field.
> 
> bpf_skc_lookup_tcp() returns RET_PTR_TO_SOCK_COMMON_OR_NULL and it can
> be for example `struct request_sock` that has only `struct sock_common`
> member, i.e. it doesn't have cgroup id.

So you can do bpf_skc_lookup_tcp() and then do tcp_sock() to get a full
socket which will have cgroup_id. I think maybe this is the reason you
added bpf_skc_lookup_tcp() in patch #1, right?

If this is the case, maybe rewording a little bit for the description
to include bpf_skc_lookup_tcp() + bpf_tcp_sock() as another input
to bpf_sk_cgroup_id()?