| Message ID | 20200327042556.11560-1-joe@wand.net.nz |
|---|---|
| Headers | show |
| Series | Add bpf_sk_assign eBPF helper | expand |
On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote: > Introduce a new helper that allows assigning a previously-found socket > to the skb as the packet is received towards the stack, to cause the > stack to guide the packet towards that socket subject to local routing > configuration. The intention is to support TProxy use cases more > directly from eBPF programs attached at TC ingress, to simplify and > streamline Linux stack configuration in scale environments with Cilium. Thanks for the quick respin. It builds. And tests are passing for me. The lack of acks and reviewed-by is a bit concerning for such important feature. Folks, please be more generous with acks :) so we can apply it with more confidence.
On 3/26/20 10:02 PM, Alexei Starovoitov wrote: > On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote: >> Introduce a new helper that allows assigning a previously-found socket >> to the skb as the packet is received towards the stack, to cause the >> stack to guide the packet towards that socket subject to local routing >> configuration. The intention is to support TProxy use cases more >> directly from eBPF programs attached at TC ingress, to simplify and >> streamline Linux stack configuration in scale environments with Cilium. > > Thanks for the quick respin. > It builds. And tests are passing for me. > The lack of acks and reviewed-by is a bit concerning for such important feature. > > Folks, please be more generous with acks :) > so we can apply it with more confidence. > I can review this tomorrow morning, thanks.
On 2020-03-27 12:25 a.m., Joe Stringer wrote: > Introduce a new helper that allows assigning a previously-found socket > to the skb as the packet is received towards the stack, to cause the > stack to guide the packet towards that socket subject to local routing > configuration. The intention is to support TProxy use cases more > directly from eBPF programs attached at TC ingress, to simplify and > streamline Linux stack configuration in scale environments with Cilium. > > Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any > existing socket reference associated with the skb. Existing tproxy > implementations in netfilter get around this restriction by running the > tproxy logic after ip_rcv_core() in the PREROUTING table. However, this > is not an option for TC-based logic (including eBPF programs attached at > TC ingress). > > This series introduces the BPF helper bpf_sk_assign() to associate the > socket with the skb on the ingress path as the packet is passed up the > stack. The initial patch in the series simply takes a reference on the > socket to ensure safety, but later patches relax this for listen > sockets. > > To ensure delivery to the relevant socket, we still consult the routing > table, for full examples of how to configure see the tests in patch #5; > the simplest form of the route would look like this: > > $ ip route add local default dev lo > Trying to understand so if we can port our tc action (and upstream), we would need to replicate: bpf_sk_assign() - invoked everytime we succeed finding the sk bpf_sk_release() - invoked everytime we are done processing the sk Anything else i missed? cheers, jamal
On Fri, Mar 27, 2020 at 7:14 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > On 2020-03-27 12:25 a.m., Joe Stringer wrote: > > Introduce a new helper that allows assigning a previously-found socket > > to the skb as the packet is received towards the stack, to cause the > > stack to guide the packet towards that socket subject to local routing > > configuration. The intention is to support TProxy use cases more > > directly from eBPF programs attached at TC ingress, to simplify and > > streamline Linux stack configuration in scale environments with Cilium. > > > > Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any > > existing socket reference associated with the skb. Existing tproxy > > implementations in netfilter get around this restriction by running the > > tproxy logic after ip_rcv_core() in the PREROUTING table. However, this > > is not an option for TC-based logic (including eBPF programs attached at > > TC ingress). > > > > This series introduces the BPF helper bpf_sk_assign() to associate the > > socket with the skb on the ingress path as the packet is passed up the > > stack. The initial patch in the series simply takes a reference on the > > socket to ensure safety, but later patches relax this for listen > > sockets. > > > > To ensure delivery to the relevant socket, we still consult the routing > > table, for full examples of how to configure see the tests in patch #5; > > the simplest form of the route would look like this: > > > > $ ip route add local default dev lo > > > > Trying to understand so if we can port our tc action (and upstream), > we would need to replicate: > > bpf_sk_assign() - invoked everytime we succeed finding the sk > bpf_sk_release() - invoked everytime we are done processing the sk The skb->destructor = sock_pfree() is the balanced other half of bpf_sk_assign(), so you shouldn't need to explicitly call bpf_sk_release() to handle the refcounting of the assigned socket. The `bpf_sk_release()` pairs with BPF socket lookup, so if you already have other socket lookup code handling the core tproxy logic (looking up established, then looking up listen sockets with different tuple) then you're presumably already handling that to avoid leaking references. I think that looking at the test_sk_assign.c BPF program in patch 4/5 should give you a good sense for what you'd need in the TC action logic.
On 2020-03-27 1:43 p.m., Joe Stringer wrote: > On Fri, Mar 27, 2020 at 7:14 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> [..] >> >> Trying to understand so if we can port our tc action (and upstream), >> we would need to replicate: >> >> bpf_sk_assign() - invoked everytime we succeed finding the sk >> bpf_sk_release() - invoked everytime we are done processing the sk > > The skb->destructor = sock_pfree() is the balanced other half of > bpf_sk_assign(), so you shouldn't need to explicitly call > bpf_sk_release() to handle the refcounting of the assigned socket. > per other thread, I think once you factor out what those two functions call into the kernel proper we will just call those same things.. > The `bpf_sk_release()` pairs with BPF socket lookup, so if you already > have other socket lookup code handling the core tproxy logic (looking > up established, then looking up listen sockets with different tuple) > then you're presumably already handling that to avoid leaking > references. > Yes, we have all that code already. > I think that looking at the test_sk_assign.c BPF program in patch 4/5 > should give you a good sense for what you'd need in the TC action > logic. Seems like we are on track. Thanks again for working on this. cheers, jamal
On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote: > Introduce a new helper that allows assigning a previously-found socket > to the skb as the packet is received towards the stack, to cause the > stack to guide the packet towards that socket subject to local routing > configuration. The intention is to support TProxy use cases more > directly from eBPF programs attached at TC ingress, to simplify and > streamline Linux stack configuration in scale environments with Cilium. > > Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any > existing socket reference associated with the skb. Existing tproxy > implementations in netfilter get around this restriction by running the > tproxy logic after ip_rcv_core() in the PREROUTING table. However, this > is not an option for TC-based logic (including eBPF programs attached at > TC ingress). > > This series introduces the BPF helper bpf_sk_assign() to associate the > socket with the skb on the ingress path as the packet is passed up the > stack. The initial patch in the series simply takes a reference on the > socket to ensure safety, but later patches relax this for listen > sockets. > > To ensure delivery to the relevant socket, we still consult the routing > table, for full examples of how to configure see the tests in patch #5; > the simplest form of the route would look like this: > > $ ip route add local default dev lo > > This series is laid out as follows: > * Patch 1 extends the eBPF API to add sk_assign() and defines a new > socket free function to allow the later paths to understand when the > socket associated with the skb should be kept through receive. > * Patches 2-3 optimize the receive path to avoid taking a reference on > listener sockets during receive. > * Patches 4-5 extends the selftests with examples of the new > functionality and validation of correct behaviour. > > Changes since v2: > * Add selftests for UDP socket redirection > * Drop the early demux optimization patch (defer for more testing) > * Fix check for orphaning after TC act return > * Tidy up the tests to clean up properly and be less noisy. > > Changes since v1: > * Replace the metadata_dst approach with using the skb->destructor to > determine whether the socket has been prefetched. This is much > simpler. > * Avoid taking a reference on listener sockets during receive > * Restrict assigning sockets across namespaces > * Restrict assigning SO_REUSEPORT sockets > * Fix cookie usage for socket dst check > * Rebase the tests against test_progs infrastructure > * Tidy up commit messages lgtm. Acked-by: Martin KaFai Lau <kafai@fb.com>
On Fri, Mar 27, 2020 at 11:46 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote: > > Introduce a new helper that allows assigning a previously-found socket > > to the skb as the packet is received towards the stack, to cause the > > stack to guide the packet towards that socket subject to local routing > > configuration. The intention is to support TProxy use cases more > > directly from eBPF programs attached at TC ingress, to simplify and > > streamline Linux stack configuration in scale environments with Cilium. > > > > Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any > > existing socket reference associated with the skb. Existing tproxy > > implementations in netfilter get around this restriction by running the > > tproxy logic after ip_rcv_core() in the PREROUTING table. However, this > > is not an option for TC-based logic (including eBPF programs attached at > > TC ingress). > > > > This series introduces the BPF helper bpf_sk_assign() to associate the > > socket with the skb on the ingress path as the packet is passed up the > > stack. The initial patch in the series simply takes a reference on the > > socket to ensure safety, but later patches relax this for listen > > sockets. > > > > To ensure delivery to the relevant socket, we still consult the routing > > table, for full examples of how to configure see the tests in patch #5; > > the simplest form of the route would look like this: > > > > $ ip route add local default dev lo > > > > This series is laid out as follows: > > * Patch 1 extends the eBPF API to add sk_assign() and defines a new > > socket free function to allow the later paths to understand when the > > socket associated with the skb should be kept through receive. > > * Patches 2-3 optimize the receive path to avoid taking a reference on > > listener sockets during receive. > > * Patches 4-5 extends the selftests with examples of the new > > functionality and validation of correct behaviour. > > > > Changes since v2: > > * Add selftests for UDP socket redirection > > * Drop the early demux optimization patch (defer for more testing) > > * Fix check for orphaning after TC act return > > * Tidy up the tests to clean up properly and be less noisy. > > > > Changes since v1: > > * Replace the metadata_dst approach with using the skb->destructor to > > determine whether the socket has been prefetched. This is much > > simpler. > > * Avoid taking a reference on listener sockets during receive > > * Restrict assigning sockets across namespaces > > * Restrict assigning SO_REUSEPORT sockets > > * Fix cookie usage for socket dst check > > * Rebase the tests against test_progs infrastructure > > * Tidy up commit messages > lgtm. > > Acked-by: Martin KaFai Lau <kafai@fb.com> Thanks for the reviews! I've rolled in the current nits + acks into the branch below, pending any further feedback. Alexei, happy to respin this on the mailinglist at some point if that's easier for you. https://github.com/joestringer/linux/tree/submit/bpf-sk-assign-v3+
On 3/27/20 10:05 PM, Joe Stringer wrote: > On Fri, Mar 27, 2020 at 11:46 AM Martin KaFai Lau <kafai@fb.com> wrote: >> >> On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote: >>> Introduce a new helper that allows assigning a previously-found socket >>> to the skb as the packet is received towards the stack, to cause the [...] >>> Changes since v1: >>> * Replace the metadata_dst approach with using the skb->destructor to >>> determine whether the socket has been prefetched. This is much >>> simpler. >>> * Avoid taking a reference on listener sockets during receive >>> * Restrict assigning sockets across namespaces >>> * Restrict assigning SO_REUSEPORT sockets >>> * Fix cookie usage for socket dst check >>> * Rebase the tests against test_progs infrastructure >>> * Tidy up commit messages >> lgtm. >> >> Acked-by: Martin KaFai Lau <kafai@fb.com> > > Thanks for the reviews! > > I've rolled in the current nits + acks into the branch below, pending > any further feedback. Alexei, happy to respin this on the mailinglist > at some point if that's easier for you. > > https://github.com/joestringer/linux/tree/submit/bpf-sk-assign-v3+ Please send the updated series to the list with Martin's ACK retained, so that we can process the series through our patchwork scripts wrt formatting, tags etc (please also make sure it's rebased). Thanks, Daniel
On Sat, Mar 28, 2020 at 10:26 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 3/27/20 10:05 PM, Joe Stringer wrote: > > On Fri, Mar 27, 2020 at 11:46 AM Martin KaFai Lau <kafai@fb.com> wrote: > >> > >> On Thu, Mar 26, 2020 at 09:25:51PM -0700, Joe Stringer wrote: > >>> Introduce a new helper that allows assigning a previously-found socket > >>> to the skb as the packet is received towards the stack, to cause the > [...] > >>> Changes since v1: > >>> * Replace the metadata_dst approach with using the skb->destructor to > >>> determine whether the socket has been prefetched. This is much > >>> simpler. > >>> * Avoid taking a reference on listener sockets during receive > >>> * Restrict assigning sockets across namespaces > >>> * Restrict assigning SO_REUSEPORT sockets > >>> * Fix cookie usage for socket dst check > >>> * Rebase the tests against test_progs infrastructure > >>> * Tidy up commit messages > >> lgtm. > >> > >> Acked-by: Martin KaFai Lau <kafai@fb.com> > > > > Thanks for the reviews! > > > > I've rolled in the current nits + acks into the branch below, pending > > any further feedback. Alexei, happy to respin this on the mailinglist > > at some point if that's easier for you. > > > > https://github.com/joestringer/linux/tree/submit/bpf-sk-assign-v3+ > > Please send the updated series to the list with Martin's ACK retained, so > that we can process the series through our patchwork scripts wrt formatting, > tags etc (please also make sure it's rebased). Sure thing, will send it out soon.
Introduce a new helper that allows assigning a previously-found socket to the skb as the packet is received towards the stack, to cause the stack to guide the packet towards that socket subject to local routing configuration. The intention is to support TProxy use cases more directly from eBPF programs attached at TC ingress, to simplify and streamline Linux stack configuration in scale environments with Cilium. Normally in ip{,6}_rcv_core(), the skb will be orphaned, dropping any existing socket reference associated with the skb. Existing tproxy implementations in netfilter get around this restriction by running the tproxy logic after ip_rcv_core() in the PREROUTING table. However, this is not an option for TC-based logic (including eBPF programs attached at TC ingress). This series introduces the BPF helper bpf_sk_assign() to associate the socket with the skb on the ingress path as the packet is passed up the stack. The initial patch in the series simply takes a reference on the socket to ensure safety, but later patches relax this for listen sockets. To ensure delivery to the relevant socket, we still consult the routing table, for full examples of how to configure see the tests in patch #5; the simplest form of the route would look like this: $ ip route add local default dev lo This series is laid out as follows: * Patch 1 extends the eBPF API to add sk_assign() and defines a new socket free function to allow the later paths to understand when the socket associated with the skb should be kept through receive. * Patches 2-3 optimize the receive path to avoid taking a reference on listener sockets during receive. * Patches 4-5 extends the selftests with examples of the new functionality and validation of correct behaviour. Changes since v2: * Add selftests for UDP socket redirection * Drop the early demux optimization patch (defer for more testing) * Fix check for orphaning after TC act return * Tidy up the tests to clean up properly and be less noisy. Changes since v1: * Replace the metadata_dst approach with using the skb->destructor to determine whether the socket has been prefetched. This is much simpler. * Avoid taking a reference on listener sockets during receive * Restrict assigning sockets across namespaces * Restrict assigning SO_REUSEPORT sockets * Fix cookie usage for socket dst check * Rebase the tests against test_progs infrastructure * Tidy up commit messages Joe Stringer (4): bpf: Add socket assign support net: Track socket refcounts in skb_steal_sock() bpf: Don't refcount LISTEN sockets in sk_assign() selftests: bpf: Extend sk_assign tests for UDP Lorenz Bauer (1): selftests: bpf: add test for sk_assign include/net/inet6_hashtables.h | 3 +- include/net/inet_hashtables.h | 3 +- include/net/sock.h | 42 ++- include/uapi/linux/bpf.h | 25 +- net/core/filter.c | 35 +- net/core/sock.c | 10 + net/ipv4/ip_input.c | 3 +- net/ipv4/udp.c | 6 +- net/ipv6/ip6_input.c | 3 +- net/ipv6/udp.c | 9 +- net/sched/act_bpf.c | 3 + tools/include/uapi/linux/bpf.h | 25 +- .../selftests/bpf/prog_tests/sk_assign.c | 309 ++++++++++++++++++ .../selftests/bpf/progs/test_sk_assign.c | 204 ++++++++++++ 14 files changed, 656 insertions(+), 24 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/sk_assign.c create mode 100644 tools/testing/selftests/bpf/progs/test_sk_assign.c