Yonghong Song <yhs@xxxxxx> [Thu, 2020-05-14 10:24 -0700]:
>
>
> On 5/14/20 9:55 AM, Andrey Ignatov wrote:
> > Yonghong Song <yhs@xxxxxx> [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()?
Yeah, this bpf_skc_lookup_tcp() + bpf_tcp_sock() combination should also
return a full socket that can be used with the helper.
bpf_sk_fullsock() is one more way to get it.
I'm not sure it's worth listing all possible ways to get full socket
since it's 1) easy to miss something; 2) easy to forget to update this
list if a new way to get full socket is being added.
What about rephrasing to highlight that it has to be full socket and
**bpf_sk_lookup_xxx**\ () is an example of getting it?
For example:
*sk* must be a non-**NULL** pointer to full socket, e.g. one
returned from **bpf_sk_lookup_xxx**\ () or
**bpf_sk_fullsock**\ ().
Will it be better?
--
Andrey Ignatov
