mbox series

[bpf-next,v3,0/2] Fix missing synack in BPF cgroup_skb filters

Message ID 20230620171409.166001-1-kuifeng@meta.com
Headers show
Series Fix missing synack in BPF cgroup_skb filters | expand

Message

Thinker Li June 20, 2023, 5:14 p.m. UTC
TCP SYN/ACK packets of connections from processes/sockets outside a
cgroup on the same host are not received by the cgroup's installed
cgroup_skb filters.

There were two BPF cgroup_skb programs attached to a cgroup named
"my_cgroup".

    SEC("cgroup_skb/ingress")
    int ingress(struct __sk_buff *skb)
    {
        /* .... process skb ... */
        return 1;
    }

    SEC("cgroup_skb/egress")
    int egress(struct __sk_buff *skb)
    {
        /* .... process skb ... */
        return 1;
    
    }

We discovered that when running the command "nc -6 -l 8000" in
"my_group" and connecting to it from outside of "my_cgroup" with the
command "nc -6 localhost 8000", the egress filter did not detect the
SYN/ACK packet. However, we did observe the SYN/ACK packet at the
ingress when connecting from a socket in "my_cgroup" to a socket
outside of it.

We came across BPF_CGROUP_RUN_PROG_INET_EGRESS(). This macro is
responsible for calling BPF programs that are attached to the egress
hook of a cgroup and it skips programs if the sending socket is not the
owner of the skb. Specifically, in our situation, the SYN/ACK
skb is owned by a struct request_sock instance, but the sending
socket is the listener socket we use to receive incoming
connections. The request_sock is created to manage an incoming
connection.

It has been determined that checking the owner of a skb against
the sending socket is not required. Removing this check will allow the
filters to receive SYN/ACK packets.

To ensure that cgroup_skb filters can receive all signaling packets,
including SYN, SYN/ACK, ACK, FIN, and FIN/ACK. A new self-test has
been added as well.

Changes from v2:

 - Remove redundant blank lines.

Changes from v1:

 - Check the number of observed packets instead of just sleeping.

 - Use ASSERT_XXX() instead of CHECK()/

[v1] https://lore.kernel.org/all/20230612191641.441774-1-kuifeng@meta.com/
[v2] https://lore.kernel.org/all/20230617052756.640916-2-kuifeng@meta.com/

Kui-Feng Lee (2):
  net: bpf: Always call BPF cgroup filters for egress.
  selftests/bpf: Verify that the cgroup_skb filters receive expected
    packets.

 include/linux/bpf-cgroup.h                    |   2 +-
 tools/testing/selftests/bpf/cgroup_helpers.c  |  12 +
 tools/testing/selftests/bpf/cgroup_helpers.h  |   1 +
 tools/testing/selftests/bpf/cgroup_tcp_skb.h  |  35 ++
 .../selftests/bpf/prog_tests/cgroup_tcp_skb.c | 399 ++++++++++++++++++
 .../selftests/bpf/progs/cgroup_tcp_skb.c      | 382 +++++++++++++++++
 6 files changed, 830 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/cgroup_tcp_skb.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_tcp_skb.c
 create mode 100644 tools/testing/selftests/bpf/progs/cgroup_tcp_skb.c

Comments

Yonghong Song June 22, 2023, 3:37 a.m. UTC | #1
On 6/20/23 10:14 AM, Kui-Feng Lee wrote:
> Always call BPF filters if CGROUP BPF is enabled for EGRESS without
> checking skb->sk against sk.
> 
> The filters were called only if skb is owned by the sock that the
> skb is sent out through.  In another words, skb->sk should point to
> the sock that it is sending through its egress.  However, the filters would
> miss SYNACK skbs that they are owned by a request_sock but sent through
> the listening sock, that is the socket listening incoming connections.
> This is an unnecessary restrict.

The original patch which introduced 'sk == skb->sk' is
   3007098494be  cgroup: add support for eBPF programs
There are no mentioning in commit message why 'sk == skb->sk'
is needed. So it is possible that this is just restricted
for use cases at that moment. Now there are use cases
where 'sk != skb->sk' so removing this check can enable
the new use case. Maybe you can add this into your commit
message so people can understand the history of 'sk == skb->sk'.

> 
> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
> ---
>   include/linux/bpf-cgroup.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 57e9e109257e..e656da531f9f 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -199,7 +199,7 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
>   #define BPF_CGROUP_RUN_PROG_INET_EGRESS(sk, skb)			       \
>   ({									       \
>   	int __ret = 0;							       \
> -	if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk && sk == skb->sk) { \
> +	if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) {		       \
>   		typeof(sk) __sk = sk_to_full_sk(sk);			       \
>   		if (sk_fullsock(__sk) &&				       \
>   		    cgroup_bpf_sock_enabled(__sk, CGROUP_INET_EGRESS))	       \
Kui-Feng Lee June 22, 2023, 3:34 p.m. UTC | #2
On 6/21/23 20:37, Yonghong Song wrote:
> 
> 
> On 6/20/23 10:14 AM, Kui-Feng Lee wrote:
>> Always call BPF filters if CGROUP BPF is enabled for EGRESS without
>> checking skb->sk against sk.
>>
>> The filters were called only if skb is owned by the sock that the
>> skb is sent out through.  In another words, skb->sk should point to
>> the sock that it is sending through its egress.  However, the filters 
>> would
>> miss SYNACK skbs that they are owned by a request_sock but sent through
>> the listening sock, that is the socket listening incoming connections.
>> This is an unnecessary restrict.
> 
> The original patch which introduced 'sk == skb->sk' is
>    3007098494be  cgroup: add support for eBPF programs
> There are no mentioning in commit message why 'sk == skb->sk'
> is needed. So it is possible that this is just restricted
> for use cases at that moment. Now there are use cases
> where 'sk != skb->sk' so removing this check can enable
> the new use case. Maybe you can add this into your commit
> message so people can understand the history of 'sk == skb->sk'.

I will put it down on the next version.

> 
>>
>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>> ---
>>   include/linux/bpf-cgroup.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
>> index 57e9e109257e..e656da531f9f 100644
>> --- a/include/linux/bpf-cgroup.h
>> +++ b/include/linux/bpf-cgroup.h
>> @@ -199,7 +199,7 @@ static inline bool cgroup_bpf_sock_enabled(struct 
>> sock *sk,
>>   #define BPF_CGROUP_RUN_PROG_INET_EGRESS(sk, skb)                   \
>>   ({                                           \
>>       int __ret = 0;                                   \
>> -    if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk && sk == 
>> skb->sk) { \
>> +    if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) {               \
>>           typeof(sk) __sk = sk_to_full_sk(sk);                   \
>>           if (sk_fullsock(__sk) &&                       \
>>               cgroup_bpf_sock_enabled(__sk, 
>> CGROUP_INET_EGRESS))           \
>
Kui-Feng Lee June 22, 2023, 5:15 p.m. UTC | #3
On 6/21/23 20:37, Yonghong Song wrote:
> 
> 
> On 6/20/23 10:14 AM, Kui-Feng Lee wrote:
>> Always call BPF filters if CGROUP BPF is enabled for EGRESS without
>> checking skb->sk against sk.
>>
>> The filters were called only if skb is owned by the sock that the
>> skb is sent out through.  In another words, skb->sk should point to
>> the sock that it is sending through its egress.  However, the filters 
>> would
>> miss SYNACK skbs that they are owned by a request_sock but sent through
>> the listening sock, that is the socket listening incoming connections.
>> This is an unnecessary restrict.
> 
> The original patch which introduced 'sk == skb->sk' is
>    3007098494be  cgroup: add support for eBPF programs
> There are no mentioning in commit message why 'sk == skb->sk'
> is needed. So it is possible that this is just restricted
> for use cases at that moment. Now there are use cases
> where 'sk != skb->sk' so removing this check can enable
> the new use case. Maybe you can add this into your commit
> message so people can understand the history of 'sk == skb->sk'.

After checking the code and the Alexei's comment[1] again, this check
may be different from what I thought. In another post[2],
Daniel Borkmann mentioned

     Wouldn't that mean however, when you go through stacked devices that
     you'd run the same eBPF cgroup program for skb->sk multiple times?

I read this paragraph several times.
This check ensures the filters are only called for the device on
the top of a stack.  So, I probably should change the check to

     sk == skb_to_full_sk(skb)

instead of removing it.  If we remove the check, egress filters
could be called multiple times for a skb, just like what Daniel said.

Does that make sense?

[1] 
https://lore.kernel.org/all/CAADnVQKi0c=Mf3b=z43=b6n2xBVhwPw4QoV_au5+pFE29iLkaQ@mail.gmail.com/
[2] https://lore.kernel.org/all/58193E9D.7040201@iogearbox.net/

> 
>>
>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>> ---
>>   include/linux/bpf-cgroup.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
>> index 57e9e109257e..e656da531f9f 100644
>> --- a/include/linux/bpf-cgroup.h
>> +++ b/include/linux/bpf-cgroup.h
>> @@ -199,7 +199,7 @@ static inline bool cgroup_bpf_sock_enabled(struct 
>> sock *sk,
>>   #define BPF_CGROUP_RUN_PROG_INET_EGRESS(sk, skb)                   \
>>   ({                                           \
>>       int __ret = 0;                                   \
>> -    if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk && sk == 
>> skb->sk) { \
>> +    if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) {               \
>>           typeof(sk) __sk = sk_to_full_sk(sk);                   \
>>           if (sk_fullsock(__sk) &&                       \
>>               cgroup_bpf_sock_enabled(__sk, 
>> CGROUP_INET_EGRESS))           \
>
Yonghong Song June 22, 2023, 6:28 p.m. UTC | #4
On 6/22/23 10:15 AM, Kui-Feng Lee wrote:
> 
> 
> On 6/21/23 20:37, Yonghong Song wrote:
>>
>>
>> On 6/20/23 10:14 AM, Kui-Feng Lee wrote:
>>> Always call BPF filters if CGROUP BPF is enabled for EGRESS without
>>> checking skb->sk against sk.
>>>
>>> The filters were called only if skb is owned by the sock that the
>>> skb is sent out through.  In another words, skb->sk should point to
>>> the sock that it is sending through its egress.  However, the filters 
>>> would
>>> miss SYNACK skbs that they are owned by a request_sock but sent through
>>> the listening sock, that is the socket listening incoming connections.
>>> This is an unnecessary restrict.
>>
>> The original patch which introduced 'sk == skb->sk' is
>>    3007098494be  cgroup: add support for eBPF programs
>> There are no mentioning in commit message why 'sk == skb->sk'
>> is needed. So it is possible that this is just restricted
>> for use cases at that moment. Now there are use cases
>> where 'sk != skb->sk' so removing this check can enable
>> the new use case. Maybe you can add this into your commit
>> message so people can understand the history of 'sk == skb->sk'.
> 
> After checking the code and the Alexei's comment[1] again, this check
> may be different from what I thought. In another post[2],
> Daniel Borkmann mentioned
> 
>      Wouldn't that mean however, when you go through stacked devices that
>      you'd run the same eBPF cgroup program for skb->sk multiple times?
> 
> I read this paragraph several times.
> This check ensures the filters are only called for the device on
> the top of a stack.  So, I probably should change the check to
> 
>      sk == skb_to_full_sk(skb)

I think this should work. It exactly covers your use case:
   they are owned by a request_sock but sent through
   the listening sock, that is the socket listening incoming connections
and sk == skb->sk for non request_sock/listening_sock case.

I originally though whether you could do
   sk == skb->sk || skb->sk->sk_state == TCP_NEW_SYN_RECV
but obviously your approach is better.

> 
> instead of removing it.  If we remove the check, egress filters
> could be called multiple times for a skb, just like what Daniel said.
> 
> Does that make sense?
> 
> [1] 
> https://lore.kernel.org/all/CAADnVQKi0c=Mf3b=z43=b6n2xBVhwPw4QoV_au5+pFE29iLkaQ@mail.gmail.com/
> [2] https://lore.kernel.org/all/58193E9D.7040201@iogearbox.net/
> 
>>
>>>
>>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>>> ---
>>>   include/linux/bpf-cgroup.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
>>> index 57e9e109257e..e656da531f9f 100644
>>> --- a/include/linux/bpf-cgroup.h
>>> +++ b/include/linux/bpf-cgroup.h
>>> @@ -199,7 +199,7 @@ static inline bool cgroup_bpf_sock_enabled(struct 
>>> sock *sk,
>>>   #define BPF_CGROUP_RUN_PROG_INET_EGRESS(sk, skb)                   \
>>>   ({                                           \
>>>       int __ret = 0;                                   \
>>> -    if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk && sk == 
>>> skb->sk) { \
>>> +    if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) {               \
>>>           typeof(sk) __sk = sk_to_full_sk(sk);                   \
>>>           if (sk_fullsock(__sk) &&                       \
>>>               cgroup_bpf_sock_enabled(__sk, 
>>> CGROUP_INET_EGRESS))           \
>>
Daniel Borkmann June 22, 2023, 8:06 p.m. UTC | #5
On 6/22/23 8:28 PM, Yonghong Song wrote:
> On 6/22/23 10:15 AM, Kui-Feng Lee wrote:
>> On 6/21/23 20:37, Yonghong Song wrote:
>>> On 6/20/23 10:14 AM, Kui-Feng Lee wrote:
>>>> Always call BPF filters if CGROUP BPF is enabled for EGRESS without
>>>> checking skb->sk against sk.
>>>>
>>>> The filters were called only if skb is owned by the sock that the
>>>> skb is sent out through.  In another words, skb->sk should point to
>>>> the sock that it is sending through its egress.  However, the filters would
>>>> miss SYNACK skbs that they are owned by a request_sock but sent through
>>>> the listening sock, that is the socket listening incoming connections.
>>>> This is an unnecessary restrict.
>>>
>>> The original patch which introduced 'sk == skb->sk' is
>>>    3007098494be  cgroup: add support for eBPF programs
>>> There are no mentioning in commit message why 'sk == skb->sk'
>>> is needed. So it is possible that this is just restricted
>>> for use cases at that moment. Now there are use cases
>>> where 'sk != skb->sk' so removing this check can enable
>>> the new use case. Maybe you can add this into your commit
>>> message so people can understand the history of 'sk == skb->sk'.
>>
>> After checking the code and the Alexei's comment[1] again, this check
>> may be different from what I thought. In another post[2],
>> Daniel Borkmann mentioned
>>
>>      Wouldn't that mean however, when you go through stacked devices that
>>      you'd run the same eBPF cgroup program for skb->sk multiple times?
>>
>> I read this paragraph several times.
>> This check ensures the filters are only called for the device on
>> the top of a stack.  So, I probably should change the check to
>>
>>      sk == skb_to_full_sk(skb)
> 
> I think this should work. It exactly covers your use case:
>    they are owned by a request_sock but sent through
>    the listening sock, that is the socket listening incoming connections
> and sk == skb->sk for non request_sock/listening_sock case.

Just a thought, should the test look like the below?

         int __ret = 0;                                                         \
         if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) {                    \
                 typeof(sk) __sk = sk_to_full_sk(sk);                           \
                 if (sk_fullsock(__sk) && __sk == skb_to_full_sk(skb) &&        \
                     cgroup_bpf_sock_enabled(__sk, CGROUP_INET_EGRESS))         \
                         __ret = __cgroup_bpf_run_filter_skb(__sk, skb,         \
                                                       CGROUP_INET_EGRESS); \
         }                                                                      \

Iow, we do already convert __sk to full sk, so we should then also use that
for the test with skb_to_full_sk(skb).

Thanks,
Daniel
Kui-Feng Lee June 22, 2023, 11:55 p.m. UTC | #6
On 6/22/23 13:06, Daniel Borkmann wrote:
> On 6/22/23 8:28 PM, Yonghong Song wrote:
>> On 6/22/23 10:15 AM, Kui-Feng Lee wrote:
>>> On 6/21/23 20:37, Yonghong Song wrote:
>>>> On 6/20/23 10:14 AM, Kui-Feng Lee wrote:
>>>>> Always call BPF filters if CGROUP BPF is enabled for EGRESS without
>>>>> checking skb->sk against sk.
>>>>>
>>>>> The filters were called only if skb is owned by the sock that the
>>>>> skb is sent out through.  In another words, skb->sk should point to
>>>>> the sock that it is sending through its egress.  However, the 
>>>>> filters would
>>>>> miss SYNACK skbs that they are owned by a request_sock but sent 
>>>>> through
>>>>> the listening sock, that is the socket listening incoming connections.
>>>>> This is an unnecessary restrict.
>>>>
>>>> The original patch which introduced 'sk == skb->sk' is
>>>>    3007098494be  cgroup: add support for eBPF programs
>>>> There are no mentioning in commit message why 'sk == skb->sk'
>>>> is needed. So it is possible that this is just restricted
>>>> for use cases at that moment. Now there are use cases
>>>> where 'sk != skb->sk' so removing this check can enable
>>>> the new use case. Maybe you can add this into your commit
>>>> message so people can understand the history of 'sk == skb->sk'.
>>>
>>> After checking the code and the Alexei's comment[1] again, this check
>>> may be different from what I thought. In another post[2],
>>> Daniel Borkmann mentioned
>>>
>>>      Wouldn't that mean however, when you go through stacked devices 
>>> that
>>>      you'd run the same eBPF cgroup program for skb->sk multiple times?
>>>
>>> I read this paragraph several times.
>>> This check ensures the filters are only called for the device on
>>> the top of a stack.  So, I probably should change the check to
>>>
>>>      sk == skb_to_full_sk(skb)
>>
>> I think this should work. It exactly covers your use case:
>>    they are owned by a request_sock but sent through
>>    the listening sock, that is the socket listening incoming connections
>> and sk == skb->sk for non request_sock/listening_sock case.
> 
> Just a thought, should the test look like the below?
> 
>          int __ret = 
> 0;                                                         \
>          if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) 
> {                    \
>                  typeof(sk) __sk = 
> sk_to_full_sk(sk);                           \
>                  if (sk_fullsock(__sk) && __sk == skb_to_full_sk(skb) 
> &&        \
>                      cgroup_bpf_sock_enabled(__sk, 
> CGROUP_INET_EGRESS))         \
>                          __ret = __cgroup_bpf_run_filter_skb(__sk, 
> skb,         \
>                                                        
> CGROUP_INET_EGRESS); \
>          
> }                                                                      \
> 
> Iow, we do already convert __sk to full sk, so we should then also use that
> for the test with skb_to_full_sk(skb).

Agree!

> 
> Thanks,
> Daniel
Daniel Borkmann June 23, 2023, 8:50 a.m. UTC | #7
On 6/23/23 1:55 AM, Kui-Feng Lee wrote:
> On 6/22/23 13:06, Daniel Borkmann wrote:
>> On 6/22/23 8:28 PM, Yonghong Song wrote:
>>> On 6/22/23 10:15 AM, Kui-Feng Lee wrote:
>>>> On 6/21/23 20:37, Yonghong Song wrote:
>>>>> On 6/20/23 10:14 AM, Kui-Feng Lee wrote:
>>>>>> Always call BPF filters if CGROUP BPF is enabled for EGRESS without
>>>>>> checking skb->sk against sk.
>>>>>>
>>>>>> The filters were called only if skb is owned by the sock that the
>>>>>> skb is sent out through.  In another words, skb->sk should point to
>>>>>> the sock that it is sending through its egress.  However, the filters would
>>>>>> miss SYNACK skbs that they are owned by a request_sock but sent through
>>>>>> the listening sock, that is the socket listening incoming connections.
>>>>>> This is an unnecessary restrict.
>>>>>
>>>>> The original patch which introduced 'sk == skb->sk' is
>>>>>    3007098494be  cgroup: add support for eBPF programs
>>>>> There are no mentioning in commit message why 'sk == skb->sk'
>>>>> is needed. So it is possible that this is just restricted
>>>>> for use cases at that moment. Now there are use cases
>>>>> where 'sk != skb->sk' so removing this check can enable
>>>>> the new use case. Maybe you can add this into your commit
>>>>> message so people can understand the history of 'sk == skb->sk'.
>>>>
>>>> After checking the code and the Alexei's comment[1] again, this check
>>>> may be different from what I thought. In another post[2],
>>>> Daniel Borkmann mentioned
>>>>
>>>>      Wouldn't that mean however, when you go through stacked devices that
>>>>      you'd run the same eBPF cgroup program for skb->sk multiple times?
>>>>
>>>> I read this paragraph several times.
>>>> This check ensures the filters are only called for the device on
>>>> the top of a stack.  So, I probably should change the check to
>>>>
>>>>      sk == skb_to_full_sk(skb)
>>>
>>> I think this should work. It exactly covers your use case:
>>>    they are owned by a request_sock but sent through
>>>    the listening sock, that is the socket listening incoming connections
>>> and sk == skb->sk for non request_sock/listening_sock case.
>>
>> Just a thought, should the test look like the below?
>>
>>          int __ret = 0;                                                         \
>>          if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) {                    \
>>                  typeof(sk) __sk = sk_to_full_sk(sk);                           \
>>                  if (sk_fullsock(__sk) && __sk == skb_to_full_sk(skb) &&        \
>>                      cgroup_bpf_sock_enabled(__sk, CGROUP_INET_EGRESS))         \
>>                          __ret = __cgroup_bpf_run_filter_skb(__sk, skb,         \
>> CGROUP_INET_EGRESS); \
>> }                                                                      \
>>
>> Iow, we do already convert __sk to full sk, so we should then also use that
>> for the test with skb_to_full_sk(skb).
> 
> Agree!

It would also be useful to do an in-depth analysis for the commit msg in which
cases the sk == skb->sk matches and sk was not a full sock (but __sk is) given
the __sk = sk_to_full_sk(sk) exists in the code to document which situation this
is covering in the existing code (... perhaps it used to work back then for
synack just that later changes altered it without anyone noticing until now).

Thanks,
Daniel
Kui-Feng Lee June 23, 2023, 4:30 p.m. UTC | #8
On 6/23/23 01:50, Daniel Borkmann wrote:
> On 6/23/23 1:55 AM, Kui-Feng Lee wrote:
>> On 6/22/23 13:06, Daniel Borkmann wrote:
>>> On 6/22/23 8:28 PM, Yonghong Song wrote:
>>>> On 6/22/23 10:15 AM, Kui-Feng Lee wrote:
>>>>> On 6/21/23 20:37, Yonghong Song wrote:
>>>>>> On 6/20/23 10:14 AM, Kui-Feng Lee wrote:
>>>>>>> Always call BPF filters if CGROUP BPF is enabled for EGRESS without
>>>>>>> checking skb->sk against sk.
>>>>>>>
>>>>>>> The filters were called only if skb is owned by the sock that the
>>>>>>> skb is sent out through.  In another words, skb->sk should point to
>>>>>>> the sock that it is sending through its egress.  However, the 
>>>>>>> filters would
>>>>>>> miss SYNACK skbs that they are owned by a request_sock but sent 
>>>>>>> through
>>>>>>> the listening sock, that is the socket listening incoming 
>>>>>>> connections.
>>>>>>> This is an unnecessary restrict.
>>>>>>
>>>>>> The original patch which introduced 'sk == skb->sk' is
>>>>>>    3007098494be  cgroup: add support for eBPF programs
>>>>>> There are no mentioning in commit message why 'sk == skb->sk'
>>>>>> is needed. So it is possible that this is just restricted
>>>>>> for use cases at that moment. Now there are use cases
>>>>>> where 'sk != skb->sk' so removing this check can enable
>>>>>> the new use case. Maybe you can add this into your commit
>>>>>> message so people can understand the history of 'sk == skb->sk'.
>>>>>
>>>>> After checking the code and the Alexei's comment[1] again, this check
>>>>> may be different from what I thought. In another post[2],
>>>>> Daniel Borkmann mentioned
>>>>>
>>>>>      Wouldn't that mean however, when you go through stacked 
>>>>> devices that
>>>>>      you'd run the same eBPF cgroup program for skb->sk multiple 
>>>>> times?
>>>>>
>>>>> I read this paragraph several times.
>>>>> This check ensures the filters are only called for the device on
>>>>> the top of a stack.  So, I probably should change the check to
>>>>>
>>>>>      sk == skb_to_full_sk(skb)
>>>>
>>>> I think this should work. It exactly covers your use case:
>>>>    they are owned by a request_sock but sent through
>>>>    the listening sock, that is the socket listening incoming 
>>>> connections
>>>> and sk == skb->sk for non request_sock/listening_sock case.
>>>
>>> Just a thought, should the test look like the below?
>>>
>>>          int __ret = 
>>> 0;                                                         \
>>>          if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) 
>>> {                    \
>>>                  typeof(sk) __sk = 
>>> sk_to_full_sk(sk);                           \
>>>                  if (sk_fullsock(__sk) && __sk == skb_to_full_sk(skb) 
>>> &&        \
>>>                      cgroup_bpf_sock_enabled(__sk, 
>>> CGROUP_INET_EGRESS))         \
>>>                          __ret = __cgroup_bpf_run_filter_skb(__sk, 
>>> skb,         \
>>> CGROUP_INET_EGRESS); \
>>> }                                                                      \
>>>
>>> Iow, we do already convert __sk to full sk, so we should then also 
>>> use that
>>> for the test with skb_to_full_sk(skb).
>>
>> Agree!
> 
> It would also be useful to do an in-depth analysis for the commit msg in 
> which
> cases the sk == skb->sk matches and sk was not a full sock (but __sk is) 
> given
> the __sk = sk_to_full_sk(sk) exists in the code to document which 
> situation this
> is covering in the existing code (... perhaps it used to work back then for
> synack just that later changes altered it without anyone noticing until 
> now).

I did a test that trace how a packet going through L2TP
devices. I am going to include the analysis of the test and other
related links of discussions in the commit log.