| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEf4BzZd30RmiZaGvDju9X0jybkcdhgOk71fbcdySeJdPzmrAQ@mail.gmail.com>
Date: Wed, 15 Jul 2020 18:41:27 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
kernel-team <kernel-team@...udflare.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Marek Majkowski <marek@...udflare.com>
Subject: Re: [PATCH bpf-next v4 02/16] bpf: Introduce SK_LOOKUP program type
with a dedicated attach point
On Mon, Jul 13, 2020 at 10:47 AM Jakub Sitnicki <jakub@...udflare.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@...udflare.com>
> Signed-off-by: Jakub Sitnicki <jakub@...udflare.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@...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?
> + 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.
> + 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;
> +
[...]
Powered by blists - more mailing lists