bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Sitnicki <jakub@cloudflare.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Networking <netdev@vger.kernel.org>,
	kernel-team <kernel-team@cloudflare.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Marek Majkowski <marek@cloudflare.com>
Subject: Re: [PATCH bpf-next v4 02/16] bpf: Introduce SK_LOOKUP program type with a dedicated attach point
Date: Thu, 16 Jul 2020 14:17:35 +0200	[thread overview]
Message-ID: <877dv3y7q8.fsf@cloudflare.com> (raw)
In-Reply-To: <CAEf4BzZd30RmiZaGvDju9X0jybkcdhgOk71fbcdySeJdPzmrAQ@mail.gmail.com>

On Thu, Jul 16, 2020 at 03:41 AM CEST, Andrii Nakryiko wrote:
> On Mon, Jul 13, 2020 at 10:47 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> Add a new program type BPF_PROG_TYPE_SK_LOOKUP with a dedicated attach type
>> BPF_SK_LOOKUP. The new program kind is to be invoked by the transport layer
>> when looking up a listening socket for a new connection request for
>> connection oriented protocols, or when looking up an unconnected socket for
>> a packet for connection-less protocols.
>>
>> When called, SK_LOOKUP BPF program can select a socket that will receive
>> the packet. This serves as a mechanism to overcome the limits of what
>> bind() API allows to express. Two use-cases driving this work are:
>>
>>  (1) steer packets destined to an IP range, on fixed port to a socket
>>
>>      192.0.2.0/24, port 80 -> NGINX socket
>>
>>  (2) steer packets destined to an IP address, on any port to a socket
>>
>>      198.51.100.1, any port -> L7 proxy socket
>>
>> In its run-time context program receives information about the packet that
>> triggered the socket lookup. Namely IP version, L4 protocol identifier, and
>> address 4-tuple. Context can be further extended to include ingress
>> interface identifier.
>>
>> To select a socket BPF program fetches it from a map holding socket
>> references, like SOCKMAP or SOCKHASH, and calls bpf_sk_assign(ctx, sk, ...)
>> helper to record the selection. Transport layer then uses the selected
>> socket as a result of socket lookup.
>>
>> This patch only enables the user to attach an SK_LOOKUP program to a
>> network namespace. Subsequent patches hook it up to run on local delivery
>> path in ipv4 and ipv6 stacks.
>>
>> Suggested-by: Marek Majkowski <marek@cloudflare.com>
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>>
>> Notes:
>>     v4:
>>     - Reintroduce narrow load support for most BPF context fields. (Yonghong)
>>     - Fix null-ptr-deref in BPF context access when IPv6 address not set.
>>     - Unpack v4/v6 IP address union in bpf_sk_lookup context type.
>>     - Add verifier support for ARG_PTR_TO_SOCKET_OR_NULL.
>>     - Allow resetting socket selection with bpf_sk_assign(ctx, NULL).
>>     - Document that bpf_sk_assign accepts a NULL socket.
>>
>>     v3:
>>     - Allow bpf_sk_assign helper to replace previously selected socket only
>>       when BPF_SK_LOOKUP_F_REPLACE flag is set, as a precaution for multiple
>>       programs running in series to accidentally override each other's verdict.
>>     - Let BPF program decide that load-balancing within a reuseport socket group
>>       should be skipped for the socket selected with bpf_sk_assign() by passing
>>       BPF_SK_LOOKUP_F_NO_REUSEPORT flag. (Martin)
>>     - Extend struct bpf_sk_lookup program context with an 'sk' field containing
>>       the selected socket with an intention for multiple attached program
>>       running in series to see each other's choices. However, currently the
>>       verifier doesn't allow checking if pointer is set.
>>     - Use bpf-netns infra for link-based multi-program attachment. (Alexei)
>>     - Get rid of macros in convert_ctx_access to make it easier to read.
>>     - Disallow 1-,2-byte access to context fields containing IP addresses.
>>
>>     v2:
>>     - Make bpf_sk_assign reject sockets that don't use RCU freeing.
>>       Update bpf_sk_assign docs accordingly. (Martin)
>>     - Change bpf_sk_assign proto to take PTR_TO_SOCKET as argument. (Martin)
>>     - Fix broken build when CONFIG_INET is not selected. (Martin)
>>     - Rename bpf_sk_lookup{} src_/dst_* fields remote_/local_*. (Martin)
>>     - Enforce BPF_SK_LOOKUP attach point on load & attach. (Martin)
>>
>>  include/linux/bpf-netns.h  |   3 +
>>  include/linux/bpf.h        |   1 +
>>  include/linux/bpf_types.h  |   2 +
>>  include/linux/filter.h     |  17 ++++
>>  include/uapi/linux/bpf.h   |  77 ++++++++++++++++
>>  kernel/bpf/net_namespace.c |   5 ++
>>  kernel/bpf/syscall.c       |   9 ++
>>  kernel/bpf/verifier.c      |  10 ++-
>>  net/core/filter.c          | 179 +++++++++++++++++++++++++++++++++++++
>>  scripts/bpf_helpers_doc.py |   9 +-
>>  10 files changed, 308 insertions(+), 4 deletions(-)
>>
>
> Looks good, two suggestions below.
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> [...]
>
>> +
>> +static const struct bpf_func_proto *
>> +sk_lookup_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>> +{
>> +       switch (func_id) {
>> +       case BPF_FUNC_sk_assign:
>> +               return &bpf_sk_lookup_assign_proto;
>> +       case BPF_FUNC_sk_release:
>> +               return &bpf_sk_release_proto;
>> +       default:
>
> Wouldn't it be useful to have functions like
> get_current_comm/get_current_pid_tgid/perf_event_output as well?
> Similarly how they were added to a bunch of other socket-related BPF
> program types recently?

I can certainly see value in perf_event_output as a way to log/trace
prog decisions. Less so for helpers that provide access to current task,
as the prog usually will be called in softirq context.

bpf_get_socket_cookie and bpf_get_netns_cookie have been on my mind, but
first they need to be taught to accept ARG_PTR_TO_SOCKET.

That is to say, I expected the list of allowed helpers to grow.

>
>
>> +               return bpf_base_func_proto(func_id);
>> +       }
>> +}
>> +
>
> [...]
>
>> +       case offsetof(struct bpf_sk_lookup, local_ip4):
>> +               *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
>> +                                     bpf_target_off(struct bpf_sk_lookup_kern,
>> +                                                    v4.daddr, 4, target_size));
>> +               break;
>> +
>> +       case bpf_ctx_range_till(struct bpf_sk_lookup,
>> +                               remote_ip6[0], remote_ip6[3]):
>> +#if IS_ENABLED(CONFIG_IPV6)
>
> nit: if you added {} to this case block, you could have combined the
> above `int off` section with this one.

Nifty. Thanks.

>
>> +               off = si->off;
>> +               off -= offsetof(struct bpf_sk_lookup, remote_ip6[0]);
>> +               off += bpf_target_off(struct in6_addr, s6_addr32[0], 4, target_size);
>> +               *insn++ = BPF_LDX_MEM(BPF_SIZEOF(void *), si->dst_reg, si->src_reg,
>> +                                     offsetof(struct bpf_sk_lookup_kern, v6.saddr));
>> +               *insn++ = BPF_JMP_IMM(BPF_JEQ, si->dst_reg, 0, 1);
>> +               *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg, off);
>> +#else
>> +               *insn++ = BPF_MOV32_IMM(si->dst_reg, 0);
>> +#endif
>> +               break;
>> +
>
> [...]

  reply	other threads:[~2020-07-16 12:17 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-13 17:46 [PATCH bpf-next v4 00/16] Run a BPF program on socket lookup Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 01/16] bpf, netns: Handle multiple link attachments Jakub Sitnicki
2020-07-15 21:30   ` Andrii Nakryiko
2020-07-13 17:46 ` [PATCH bpf-next v4 02/16] bpf: Introduce SK_LOOKUP program type with a dedicated attach point Jakub Sitnicki
2020-07-16  1:41   ` Andrii Nakryiko
2020-07-16 12:17     ` Jakub Sitnicki [this message]
2020-07-13 17:46 ` [PATCH bpf-next v4 03/16] inet: Extract helper for selecting socket from reuseport group Jakub Sitnicki
2020-07-16  1:44   ` Andrii Nakryiko
2020-07-13 17:46 ` [PATCH bpf-next v4 04/16] inet: Run SK_LOOKUP BPF program on socket lookup Jakub Sitnicki
2020-07-16  2:23   ` Andrii Nakryiko
2020-07-16 12:32     ` Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 05/16] inet6: Extract helper for selecting socket from reuseport group Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 06/16] inet6: Run SK_LOOKUP BPF program on socket lookup Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 07/16] udp: Extract helper for selecting socket from reuseport group Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 08/16] udp: Run SK_LOOKUP BPF program on socket lookup Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 09/16] udp6: Extract helper for selecting socket from reuseport group Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 10/16] udp6: Run SK_LOOKUP BPF program on socket lookup Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 11/16] bpf: Sync linux/bpf.h to tools/ Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 12/16] libbpf: Add support for SK_LOOKUP program type Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 13/16] tools/bpftool: Add name mappings for SK_LOOKUP prog and attach type Jakub Sitnicki
2020-07-16  2:10   ` Andrii Nakryiko
2020-07-13 17:46 ` [PATCH bpf-next v4 14/16] selftests/bpf: Add verifier tests for bpf_sk_lookup context access Jakub Sitnicki
2020-07-16  2:13   ` Andrii Nakryiko
2020-07-13 17:46 ` [PATCH bpf-next v4 15/16] selftests/bpf: Rename test_sk_lookup_kern.c to test_ref_track_kern.c Jakub Sitnicki
2020-07-13 17:46 ` [PATCH bpf-next v4 16/16] selftests/bpf: Tests for BPF_SK_LOOKUP attach point Jakub Sitnicki
2020-07-16  2:19   ` Andrii Nakryiko
2020-07-16  2:25 ` [PATCH bpf-next v4 00/16] Run a BPF program on socket lookup Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877dv3y7q8.fsf@cloudflare.com \
    --to=jakub@cloudflare.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=kernel-team@cloudflare.com \
    --cc=kuba@kernel.org \
    --cc=marek@cloudflare.com \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH bpf-next v4 02/16] bpf: Introduce SK_LOOKUP program type with a dedicated attach point' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).