From: Martin KaFai Lau <martin.lau@linux.dev>
To: Daan De Meyer <daan.j.demeyer@gmail.com>
Cc: kernel-team@meta.com, netdev@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v3 4/9] bpf: Implement cgroup sockaddr hooks for unix sockets
Date: Tue, 5 Sep 2023 12:02:52 -0700 [thread overview]
Message-ID: <52177bd8-65a5-ef4d-b00d-47509855c3e4@linux.dev> (raw)
In-Reply-To: <20230831153455.1867110-5-daan.j.demeyer@gmail.com>
On 8/31/23 8:34 AM, Daan De Meyer wrote:
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0680569f9bd0..d8f508c56055 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14540,14 +14540,19 @@ static int check_return_code(struct bpf_verifier_env *env)
> case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
> if (env->prog->expected_attach_type == BPF_CGROUP_UDP4_RECVMSG ||
> env->prog->expected_attach_type == BPF_CGROUP_UDP6_RECVMSG ||
> + env->prog->expected_attach_type == BPF_CGROUP_UNIX_RECVMSG ||
> env->prog->expected_attach_type == BPF_CGROUP_INET4_GETPEERNAME ||
> env->prog->expected_attach_type == BPF_CGROUP_INET6_GETPEERNAME ||
> + env->prog->expected_attach_type == BPF_CGROUP_UNIX_GETPEERNAME ||
> env->prog->expected_attach_type == BPF_CGROUP_INET4_GETSOCKNAME ||
> - env->prog->expected_attach_type == BPF_CGROUP_INET6_GETSOCKNAME)
> + env->prog->expected_attach_type == BPF_CGROUP_INET6_GETSOCKNAME ||
> + env->prog->expected_attach_type == BPF_CGROUP_UNIX_GETSOCKNAME)
> range = tnum_range(1, 1);
A note that getpeername, getsockname, and recvmsg cannot return err (err is
value 0 for cgroup-bpf). More on this later.
> if (env->prog->expected_attach_type == BPF_CGROUP_INET4_BIND ||
> env->prog->expected_attach_type == BPF_CGROUP_INET6_BIND)
> range = tnum_range(0, 3);
> + if (env->prog->expected_attach_type == BPF_CGROUP_UNIX_BIND)
> + range = tnum_range(0, 1);
A few words in the commit message is needed for the difference on the return
code between UNIX_BIND and INET[46]_BIND. (ie. the
BPF_RET_BIND_NO_CAP_NET_BIND_SERVICE).
Also, the default range should be (0, 1) already (the 'struct tnum range =
tnum_range(0, 1)' at the beginning of this function). The same goes for
UNIX_SENDMSG (and the existing INET[46]_SENDMSG) which should already have the
default (0, 1) range. Thus, no need to have a special test case here.
> break;
> case BPF_PROG_TYPE_CGROUP_SKB:
> if (env->prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) {
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3ed6cd33b268..be4e0e923aa6 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -81,6 +81,7 @@
> #include <net/xdp.h>
> #include <net/mptcp.h>
> #include <net/netfilter/nf_conntrack_bpf.h>
> +#include <linux/un.h>
Is this needed?
>
> static const struct bpf_func_proto *
> bpf_sk_base_func_proto(enum bpf_func_id func_id);
> @@ -7828,6 +7829,7 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> switch (prog->expected_attach_type) {
> case BPF_CGROUP_INET4_CONNECT:
> case BPF_CGROUP_INET6_CONNECT:
> + case BPF_CGROUP_UNIX_CONNECT:
> return &bpf_bind_proto;
> default:
> return NULL;
> @@ -7856,16 +7858,22 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> switch (prog->expected_attach_type) {
> case BPF_CGROUP_INET4_BIND:
> case BPF_CGROUP_INET6_BIND:
> + case BPF_CGROUP_UNIX_BIND:
> case BPF_CGROUP_INET4_CONNECT:
> case BPF_CGROUP_INET6_CONNECT:
> + case BPF_CGROUP_UNIX_CONNECT:
> case BPF_CGROUP_UDP4_RECVMSG:
> case BPF_CGROUP_UDP6_RECVMSG:
> + case BPF_CGROUP_UNIX_RECVMSG:
> case BPF_CGROUP_UDP4_SENDMSG:
> case BPF_CGROUP_UDP6_SENDMSG:
> + case BPF_CGROUP_UNIX_SENDMSG:
> case BPF_CGROUP_INET4_GETPEERNAME:
> case BPF_CGROUP_INET6_GETPEERNAME:
> + case BPF_CGROUP_UNIX_GETPEERNAME:
> case BPF_CGROUP_INET4_GETSOCKNAME:
> case BPF_CGROUP_INET6_GETSOCKNAME:
> + case BPF_CGROUP_UNIX_GETSOCKNAME:
> return &bpf_sock_addr_setsockopt_proto;
> default:
> return NULL;
> @@ -7874,16 +7882,22 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> switch (prog->expected_attach_type) {
> case BPF_CGROUP_INET4_BIND:
> case BPF_CGROUP_INET6_BIND:
> + case BPF_CGROUP_UNIX_BIND:
> case BPF_CGROUP_INET4_CONNECT:
> case BPF_CGROUP_INET6_CONNECT:
> + case BPF_CGROUP_UNIX_CONNECT:
> case BPF_CGROUP_UDP4_RECVMSG:
> case BPF_CGROUP_UDP6_RECVMSG:
> + case BPF_CGROUP_UNIX_RECVMSG:
> case BPF_CGROUP_UDP4_SENDMSG:
> case BPF_CGROUP_UDP6_SENDMSG:
> + case BPF_CGROUP_UNIX_SENDMSG:
> case BPF_CGROUP_INET4_GETPEERNAME:
> case BPF_CGROUP_INET6_GETPEERNAME:
> + case BPF_CGROUP_UNIX_GETPEERNAME:
> case BPF_CGROUP_INET4_GETSOCKNAME:
> case BPF_CGROUP_INET6_GETSOCKNAME:
> + case BPF_CGROUP_UNIX_GETSOCKNAME:
> return &bpf_sock_addr_getsockopt_proto;
> default:
> return NULL;
> @@ -8931,8 +8945,8 @@ static bool sock_addr_is_valid_access(int off, int size,
> if (off % size != 0)
> return false;
>
> - /* Disallow access to IPv6 fields from IPv4 contex and vise
> - * versa.
> + /* Disallow access to fields not belonging to the attach type's address
> + * family.
> */
> switch (off) {
> case bpf_ctx_range(struct bpf_sock_addr, user_ip4):
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 86930a8ed012..94fd6f2441d8 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -116,6 +116,7 @@
> #include <linux/freezer.h>
> #include <linux/file.h>
> #include <linux/btf_ids.h>
> +#include <linux/bpf-cgroup.h>
>
> #include "scm.h"
>
> @@ -1323,6 +1324,12 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> struct sock *sk = sock->sk;
> int err;
>
> + if (cgroup_bpf_enabled(CGROUP_UNIX_BIND)) {
It is a dup test. The same static_key test will be done in
BPF_CGROUP_RUN_SA_PROG*() also?
The same comment for other places before calling BPF_CGROUP_RUN_PROG_UNIX_* and
BPF_CGROUP_RUN_SA_PROG().
> + err = BPF_CGROUP_RUN_PROG_UNIX_BIND_LOCK(sk, uaddr, &addr_len);
> + if (err)
> + return err;
> + }
> +
> if (addr_len == offsetof(struct sockaddr_un, sun_path) &&
> sunaddr->sun_family == AF_UNIX)
> return unix_autobind(sk);
> @@ -1377,6 +1384,13 @@ static int unix_dgram_connect(struct socket *sock, struct sockaddr *addr,
> goto out;
>
> if (addr->sa_family != AF_UNSPEC) {
> + if (cgroup_bpf_enabled(CGROUP_UNIX_CONNECT)) {
> + err = BPF_CGROUP_RUN_PROG_UNIX_CONNECT_LOCK(sk, addr,
> + &alen);
> + if (err)
> + goto out;
> + }
> +
> err = unix_validate_addr(sunaddr, alen);
> if (err)
> goto out;
> @@ -1486,6 +1500,13 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> int err;
> int st;
>
> + if (cgroup_bpf_enabled(CGROUP_UNIX_CONNECT)) {
> + err = BPF_CGROUP_RUN_PROG_UNIX_CONNECT_LOCK(sk, uaddr,
> + &addr_len);
> + if (err)
> + goto out;
> + }
> +
> err = unix_validate_addr(sunaddr, addr_len);
> if (err)
> goto out;
> @@ -1749,7 +1770,7 @@ static int unix_getname(struct socket *sock, struct sockaddr *uaddr, int peer)
> struct sock *sk = sock->sk;
> struct unix_address *addr;
> DECLARE_SOCKADDR(struct sockaddr_un *, sunaddr, uaddr);
> - int err = 0;
> + int addr_len = 0, err = 0;
>
> if (peer) {
> sk = unix_peer_get(sk);
> @@ -1766,14 +1787,37 @@ static int unix_getname(struct socket *sock, struct sockaddr *uaddr, int peer)
> if (!addr) {
> sunaddr->sun_family = AF_UNIX;
> sunaddr->sun_path[0] = 0;
> - err = offsetof(struct sockaddr_un, sun_path);
> + addr_len = offsetof(struct sockaddr_un, sun_path);
> } else {
> - err = addr->len;
> + addr_len = addr->len;
> memcpy(sunaddr, addr->name, addr->len);
> }
> +
> + if (peer && cgroup_bpf_enabled(CGROUP_UNIX_GETPEERNAME)) {
> + err = BPF_CGROUP_RUN_SA_PROG(sk, uaddr, &addr_len,
> + CGROUP_UNIX_GETPEERNAME);
> + if (err)
UNIX_GETPEERNAME can only have return value 1 (OK), so no need to do err check here.
> + goto out;
> +
> + err = unix_validate_addr(sunaddr, addr_len);
Since the kfunc is specific to the unix address, how about doing the
unix_validate_addr check in the kfunc itself?
> + if (err)
> + goto out;
> + }
> +
> + if (!peer && cgroup_bpf_enabled(CGROUP_UNIX_GETSOCKNAME)) {
> + err = BPF_CGROUP_RUN_SA_PROG(sk, uaddr, &addr_len,
> + CGROUP_UNIX_GETSOCKNAME);
> + if (err)
Same here on the unnecessary err check.
> + goto out;
> +
> + err = unix_validate_addr(sunaddr, addr_len);
> + if (err)
> + goto out;
> + }
> +
> sock_put(sk);
> out:
> - return err;
> + return err ?: addr_len;
> }
>
> static void unix_peek_fds(struct scm_cookie *scm, struct sk_buff *skb)
> @@ -1919,6 +1963,15 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> goto out;
>
> if (msg->msg_namelen) {
> + if (cgroup_bpf_enabled(CGROUP_UNIX_SENDMSG)) {
> + err = BPF_CGROUP_RUN_PROG_UNIX_SENDMSG_LOCK(sk,
> + msg->msg_name,
> + &msg->msg_namelen,
> + NULL);
> + if (err)
> + goto out;
> + }
> +
> err = unix_validate_addr(sunaddr, msg->msg_namelen);
> if (err)
> goto out;
> @@ -2328,14 +2381,30 @@ static int unix_seqpacket_recvmsg(struct socket *sock, struct msghdr *msg,
> return unix_dgram_recvmsg(sock, msg, size, flags);
> }
>
> -static void unix_copy_addr(struct msghdr *msg, struct sock *sk)
> +static int unix_recvmsg_copy_addr(struct msghdr *msg, struct sock *sk)
> {
> struct unix_address *addr = smp_load_acquire(&unix_sk(sk)->addr);
> + int err;
>
> if (addr) {
> msg->msg_namelen = addr->len;
> memcpy(msg->msg_name, addr->name, addr->len);
> +
> + if (cgroup_bpf_enabled(CGROUP_UNIX_RECVMSG)) {
> + err = BPF_CGROUP_RUN_PROG_UNIX_RECVMSG_LOCK(sk,
> + msg->msg_name,
> + &msg->msg_namelen);
> + if (err)
Same here on the unnecessary err check.
> + return err;
> +
> + err = unix_validate_addr(msg->msg_name,
> + msg->msg_namelen);
If unix_validate_addr is done in the kfunc, the unix_recvmsg_copy_addr does not
need to return error and the changes in the unix_recvmsg_copy_addr's caller is
not needed also.
> + if (err)
> + return err;
> + }
> }
> +
> + return 0;
> }
>
> int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
> @@ -2390,8 +2459,11 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
> EPOLLOUT | EPOLLWRNORM |
> EPOLLWRBAND);
>
> - if (msg->msg_name)
> - unix_copy_addr(msg, skb->sk);
> + if (msg->msg_name) {
> + err = unix_recvmsg_copy_addr(msg, skb->sk);
> + if (err)
> + goto out_free;
> + }
>
> if (size > skb->len - skip)
> size = skb->len - skip;
> @@ -2743,7 +2815,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
> if (state->msg && state->msg->msg_name) {
> DECLARE_SOCKADDR(struct sockaddr_un *, sunaddr,
> state->msg->msg_name);
> - unix_copy_addr(state->msg, skb->sk);
> + err = unix_recvmsg_copy_addr(state->msg, skb->sk);
> + if (err)
> + break;
> sunaddr = NULL;
> }
next prev parent reply other threads:[~2023-09-05 19:05 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-31 15:34 [PATCH bpf-next v4 0/9] Add cgroup sockaddr hooks for unix sockets Daan De Meyer
2023-08-31 15:34 ` [PATCH bpf-next v3 1/9] selftests/bpf: Add missing section name tests for getpeername/getsockname Daan De Meyer
2023-08-31 15:34 ` [PATCH bpf-next v3 2/9] bpf: Propagate modified uaddrlen from cgroup sockaddr programs Daan De Meyer
2023-09-05 21:21 ` Martin KaFai Lau
2023-08-31 15:34 ` [PATCH bpf-next v3 3/9] bpf: Add bpf_sock_addr_set_unix_addr() to allow writing unix sockaddr from bpf Daan De Meyer
2023-09-04 20:58 ` Alexei Starovoitov
2023-09-05 21:37 ` Martin KaFai Lau
2023-08-31 15:34 ` [PATCH bpf-next v3 4/9] bpf: Implement cgroup sockaddr hooks for unix sockets Daan De Meyer
2023-09-01 19:34 ` Kuniyuki Iwashima
2023-09-05 19:02 ` Martin KaFai Lau [this message]
2023-09-05 21:38 ` Martin KaFai Lau
2023-08-31 15:34 ` [PATCH bpf-next v3 5/9] libbpf: Add support for cgroup unix socket address hooks Daan De Meyer
2023-08-31 15:34 ` [PATCH bpf-next v3 6/9] bpftool: " Daan De Meyer
2023-08-31 16:58 ` Quentin Monnet
2023-08-31 15:34 ` [PATCH bpf-next v3 7/9] documentation/bpf: Document " Daan De Meyer
2023-08-31 15:34 ` [PATCH bpf-next v3 8/9] selftests/bpf: Make sure mount directory exists Daan De Meyer
2023-08-31 15:34 ` [PATCH bpf-next v3 9/9] selftests/bpf: Add tests for cgroup unix socket address hooks Daan De Meyer
2023-09-05 20:07 ` Martin KaFai Lau
-- strict thread matches above, loose matches on Subject: below --
2023-08-29 10:18 [PATCH bpf-next v3 0/9] Add cgroup sockaddr hooks for unix sockets Daan De Meyer
2023-08-29 10:18 ` [PATCH bpf-next v3 4/9] bpf: Implement " Daan De Meyer
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=52177bd8-65a5-ef4d-b00d-47509855c3e4@linux.dev \
--to=martin.lau@linux.dev \
--cc=bpf@vger.kernel.org \
--cc=daan.j.demeyer@gmail.com \
--cc=kernel-team@meta.com \
--cc=netdev@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.