Message-ID: <6b4a9ce3-249a-03c1-df4f-d637d3cc9b7c@iogearbox.net>
Date: Fri, 30 Jun 2023 16:12:40 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Kui-Feng Lee <thinker.li@...il.com>, bpf@...r.kernel.org, ast@...nel.org,
 martin.lau@...ux.dev, song@...nel.org, kernel-team@...a.com,
 andrii@...nel.org, yhs@...com, kpsingh@...nel.org, shuah@...nel.org,
 john.fastabend@...il.com, sdf@...gle.com, mykolal@...com,
 linux-kselftest@...r.kernel.org, jolsa@...nel.org, haoluo@...gle.com,
 netdev@...r.kernel.org
Cc: Kui-Feng Lee <kuifeng@...a.com>
Subject: Re: [PATCH bpf-next v4 1/2] net: bpf: Check SKB ownership against
 full socket.

On 6/24/23 3:45 AM, Kui-Feng Lee wrote:
> Check SKB ownership of an SKB against full sockets instead of request_sock.
> 
> The filters were called only if an 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 SYN/ACK
> SKBs that they are owned by a request_sock but sent through the listener
> sock, that is the socket listening incoming connections.
> 
> However, the listener socket is also the full socket of the request socket.
> We should use the full socket as the owner socket of an SKB instead.
> 
> What is the ownership check for?
> ====================================
> 
> BPF_CGROUP_RUN_PROG_INET_EGRESS() checked sk == skb->sk to ensure the
> ownership of an SKB. Alexei referred to a mailing list conversation that
> took place a few years ago. [1] In that conversation, Daniel Borkmann
> stated that:
> 
>      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?
> 
> According to what Daniel said, the ownership check mentioned earlier
> presumably prevents multiple calls of egress filters caused by an skb.
> 
> A test that reproduce this scenario shows that the BPF cgroup egress
> programs can be called multiple times for one SKB if this ownership
> check is not there.  So, we can not just remove this check.
> 
> Test Stacked Devices
> =======================
> 
> We use L2TP to build an environment of stacked devices. L2TP (Layer 2
> Tunneling Protocol) is a tunneling protocol used to support virtual private
> networks (VPNs). It relays encapsulated packets; for example in UDP, to its
> peer by using a socket.
> 
> Using L2TP, packets are first sent through the IP stack and should then
> arrive at an L2TP device. The device will expand its SKB header to
> encapsulate the packet. The SKB will be sent back to the IP stack using the
> socket that was made for the L2TP session. After that, the routing process
> will occur once more, but this time for a new destination.
> 
> We changed tools/testing/selftests/net/l2tp.sh to set up a test environment
> using L2TP.  The run_ping() function in l2tp.sh is where the main change
> occurred.
> 
>      run_ping()
>      {
>          local desc="$1"
> 
>          sleep 10
>          run_cmd host-1 ${ping6} -s 227 -c 4 -i 10 -I fc00:101::1
>          fc00:101::2
>          log_test $? 0 "IPv6 route through L2TP tunnel ${desc}"
>          sleep 10
> 
>      }
> 
> The test will use L2TP devices to send PING messages. These messages will
> have a message size of 227 bytes as a special label to distinguish them.
> This is not an ideal solution, but works.
> 
> During the execution of the test script, bpftrace was attached to
> ip6_finish_output() and l2tp_xmit_skb(). BPF
> 
>      bpftrace -e '
>        kfunc:ip6_finish_output {
>          time("%H:%M:%S: ");
>          printf("ip6_finish_output skb=%p skb->len=%d cgroup=%p sk=%p
>                  skb->sk=%p\n", args->skb, args->skb->len,
>                 args->sk->sk_cgrp_data.cgroup, args->sk, args->skb->sk); }
>        kfunc:l2tp_xmit_skb {
>          time("%H:%M:%S: ");
>          printf("l2tp_xmit_skb skb=%p sk=%p\n", args->skb,
> 	       args->session->tunnel->sock); }'
> 
> The following is part of the output messages printed by bpftrace.
> 
>      16:35:20: ip6_finish_output skb=0xffff888103d8e600 skb->len=275
>                cgroup=0xffff88810741f800 sk=0xffff888105f3b900
>                skb->sk=0xffff888105f3b900
> 
>      16:35:20: l2tp_xmit_skb skb=0xffff888103d8e600 sk=0xffff888103dd6300
> 
>      16:35:20: ip6_finish_output skb=0xffff888103d8e600 skb->len=337
>                cgroup=0xffff88810741f800 sk=0xffff888103dd6300
>                skb->sk=0xffff888105f3b900
> 
>      16:35:20: ip6_finish_output skb=0xffff888103d8e600 skb->len=337
>                cgroup=(nil) sk=(nil) skb->sk=(nil)
> 
>      16:35:20: ip6_finish_output skb=0xffff888103d8e000 skb->len=275
>                cgroup=0xffffffff837741d0 sk=0xffff888101fe0000
>                skb->sk=0xffff888101fe0000
> 
>      16:35:20: l2tp_xmit_skb skb=0xffff888103d8e000 sk=0xffff888103483180
> 
>      16:35:20: ip6_finish_output skb=0xffff888103d8e000 skb->len=337
>                cgroup=0xffff88810741f800 sk=0xffff888103483180
>                skb->sk=0xffff888101fe0000
> 
>      16:35:20: ip6_finish_output skb=0xffff888103d8e000 skb->len=337
>                cgroup=(nil) sk=(nil) skb->sk=(nil)
> 
> The first four entries describe a PING message that was sent using the ping
> command, whereas the following four entries describe the response received.
> Multiple sockets are used to send one skb, including the socket used by the
> L2TP session. This can be observed.
> 
> Based on this information, it seems that the ownership check is designed to
> avoid multiple calls of egress filters caused by a single skb.

Thanks for this investigation and adding these details to the commit log,
this is very useful to keep for the archive. I did some minor formatting
and pushed it out to bpf-next.

Powered by blists - more mailing lists