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 2/9] bpf: Propagate modified uaddrlen from cgroup sockaddr programs
Date: Tue, 5 Sep 2023 14:21:00 -0700 [thread overview]
Message-ID: <c73a6a41-9e05-c473-0e46-56d0fcfb9ac8@linux.dev> (raw)
In-Reply-To: <20230831153455.1867110-3-daan.j.demeyer@gmail.com>
On 8/31/23 8:34 AM, Daan De Meyer wrote:
> As prep for adding unix socket support to the cgroup sockaddr hooks,
> let's propagate the sockaddr length back to the caller after running
> a bpf cgroup sockaddr hook program. While not important for AF_INET or
> AF_INET6, the sockaddr length is important when working with AF_UNIX
> sockaddrs as the size of the sockaddr cannot be determined just from the
> address family or the sockaddr's contents.
>
> __cgroup_bpf_run_filter_sock_addr() is modified to take the uaddrlen as
> an input/output argument. After running the program, the modified sockaddr
> length is stored in the uaddrlen pointer. If no uaddrlen pointer is
> provided, we determine the uaddrlen based on the socket family. For the
> existing AF_INET and AF_INET6 use cases, we don't pass in the address
> length explicitly and just determine it based on the passed in socket
> family.
The description on the inet address length needs an update, at least the
AF_INET6 one.
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 8506690dbb9c..31561e789715 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -120,6 +120,7 @@ int __cgroup_bpf_run_filter_sk(struct sock *sk,
>
> int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> struct sockaddr *uaddr,
> + int *uaddrlen,
> enum cgroup_bpf_attach_type atype,
> void *t_ctx,
> u32 *flags);
> @@ -230,22 +231,22 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
> #define BPF_CGROUP_RUN_PROG_INET6_POST_BIND(sk) \
> BPF_CGROUP_RUN_SK_PROG(sk, CGROUP_INET6_POST_BIND)
>
> -#define BPF_CGROUP_RUN_SA_PROG(sk, uaddr, atype) \
> +#define BPF_CGROUP_RUN_SA_PROG(sk, uaddr, uaddrlen, atype) \
> ({ \
> int __ret = 0; \
> if (cgroup_bpf_enabled(atype)) \
> - __ret = __cgroup_bpf_run_filter_sock_addr(sk, uaddr, atype, \
> - NULL, NULL); \
> + __ret = __cgroup_bpf_run_filter_sock_addr(sk, uaddr, uaddrlen, \
> + atype, NULL, NULL); \
> __ret; \
> })
>
> -#define BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, atype, t_ctx) \
> +#define BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, uaddrlen, atype, t_ctx) \
> ({ \
> int __ret = 0; \
> if (cgroup_bpf_enabled(atype)) { \
> lock_sock(sk); \
> - __ret = __cgroup_bpf_run_filter_sock_addr(sk, uaddr, atype, \
> - t_ctx, NULL); \
> + __ret = __cgroup_bpf_run_filter_sock_addr(sk, uaddr, uaddrlen, \
> + atype, t_ctx, NULL); \
> release_sock(sk); \
> } \
> __ret; \
> @@ -256,14 +257,14 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
> * (at bit position 0) is to indicate CAP_NET_BIND_SERVICE capability check
> * should be bypassed (BPF_RET_BIND_NO_CAP_NET_BIND_SERVICE).
> */
> -#define BPF_CGROUP_RUN_PROG_INET_BIND_LOCK(sk, uaddr, atype, bind_flags) \
> +#define BPF_CGROUP_RUN_PROG_INET_BIND_LOCK(sk, uaddr, uaddrlen, atype, bind_flags) \
> ({ \
> u32 __flags = 0; \
> int __ret = 0; \
> if (cgroup_bpf_enabled(atype)) { \
> lock_sock(sk); \
> - __ret = __cgroup_bpf_run_filter_sock_addr(sk, uaddr, atype, \
> - NULL, &__flags); \
> + __ret = __cgroup_bpf_run_filter_sock_addr(sk, uaddr, uaddrlen, \
> + atype, NULL, &__flags); \
> release_sock(sk); \
> if (__flags & BPF_RET_BIND_NO_CAP_NET_BIND_SERVICE) \
> *bind_flags |= BIND_NO_CAP_NET_BIND_SERVICE; \
> @@ -276,29 +277,29 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
> cgroup_bpf_enabled(CGROUP_INET6_CONNECT)) && \
> (sk)->sk_prot->pre_connect)
>
> -#define BPF_CGROUP_RUN_PROG_INET4_CONNECT(sk, uaddr) \
> - BPF_CGROUP_RUN_SA_PROG(sk, uaddr, CGROUP_INET4_CONNECT)
> +#define BPF_CGROUP_RUN_PROG_INET4_CONNECT(sk, uaddr, uaddrlen) \
> + BPF_CGROUP_RUN_SA_PROG(sk, uaddr, uaddrlen, CGROUP_INET4_CONNECT)
>
> -#define BPF_CGROUP_RUN_PROG_INET6_CONNECT(sk, uaddr) \
> - BPF_CGROUP_RUN_SA_PROG(sk, uaddr, CGROUP_INET6_CONNECT)
> +#define BPF_CGROUP_RUN_PROG_INET6_CONNECT(sk, uaddr, uaddrlen) \
> + BPF_CGROUP_RUN_SA_PROG(sk, uaddr, uaddrlen, CGROUP_INET6_CONNECT)
>
> -#define BPF_CGROUP_RUN_PROG_INET4_CONNECT_LOCK(sk, uaddr) \
> - BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, CGROUP_INET4_CONNECT, NULL)
> +#define BPF_CGROUP_RUN_PROG_INET4_CONNECT_LOCK(sk, uaddr, uaddrlen) \
> + BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, uaddrlen, CGROUP_INET4_CONNECT, NULL)
>
> -#define BPF_CGROUP_RUN_PROG_INET6_CONNECT_LOCK(sk, uaddr) \
> - BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, CGROUP_INET6_CONNECT, NULL)
> +#define BPF_CGROUP_RUN_PROG_INET6_CONNECT_LOCK(sk, uaddr, uaddrlen) \
> + BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, uaddrlen, CGROUP_INET6_CONNECT, NULL)
>
> -#define BPF_CGROUP_RUN_PROG_UDP4_SENDMSG_LOCK(sk, uaddr, t_ctx) \
> - BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, CGROUP_UDP4_SENDMSG, t_ctx)
> +#define BPF_CGROUP_RUN_PROG_UDP4_SENDMSG_LOCK(sk, uaddr, uaddrlen, t_ctx) \
> + BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, uaddrlen, CGROUP_UDP4_SENDMSG, t_ctx)
>
> -#define BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK(sk, uaddr, t_ctx) \
> - BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, CGROUP_UDP6_SENDMSG, t_ctx)
> +#define BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK(sk, uaddr, uaddrlen, t_ctx) \
> + BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, uaddrlen, CGROUP_UDP6_SENDMSG, t_ctx)
>
> -#define BPF_CGROUP_RUN_PROG_UDP4_RECVMSG_LOCK(sk, uaddr) \
> - BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, CGROUP_UDP4_RECVMSG, NULL)
> +#define BPF_CGROUP_RUN_PROG_UDP4_RECVMSG_LOCK(sk, uaddr, uaddrlen) \
> + BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, uaddrlen, CGROUP_UDP4_RECVMSG, NULL)
>
> -#define BPF_CGROUP_RUN_PROG_UDP6_RECVMSG_LOCK(sk, uaddr) \
> - BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, CGROUP_UDP6_RECVMSG, NULL)
> +#define BPF_CGROUP_RUN_PROG_UDP6_RECVMSG_LOCK(sk, uaddr, uaddrlen) \
> + BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, uaddrlen, CGROUP_UDP6_RECVMSG, NULL)
>
> /* The SOCK_OPS"_SK" macro should be used when sock_ops->sk is not a
> * fullsock and its parent fullsock cannot be traced by
> @@ -477,24 +478,24 @@ static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
> }
>
> #define cgroup_bpf_enabled(atype) (0)
> -#define BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, atype, t_ctx) ({ 0; })
> -#define BPF_CGROUP_RUN_SA_PROG(sk, uaddr, atype) ({ 0; })
> +#define BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, uaddrlen, atype, t_ctx) ({ 0; })
> +#define BPF_CGROUP_RUN_SA_PROG(sk, uaddr, uaddrlen, atype) ({ 0; })
> #define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
> #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk,skb) ({ 0; })
> #define BPF_CGROUP_RUN_PROG_INET_EGRESS(sk,skb) ({ 0; })
> #define BPF_CGROUP_RUN_PROG_INET_SOCK(sk) ({ 0; })
> #define BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk) ({ 0; })
> -#define BPF_CGROUP_RUN_PROG_INET_BIND_LOCK(sk, uaddr, atype, flags) ({ 0; })
> +#define BPF_CGROUP_RUN_PROG_INET_BIND_LOCK(sk, uaddr, uaddrlen, atype, flags) ({ 0; })
> #define BPF_CGROUP_RUN_PROG_INET4_POST_BIND(sk) ({ 0; })
> #define BPF_CGROUP_RUN_PROG_INET6_POST_BIND(sk) ({ 0; })
> -#define BPF_CGROUP_RUN_PROG_INET4_CONNECT(sk, uaddr) ({ 0; })
> -#define BPF_CGROUP_RUN_PROG_INET4_CONNECT_LOCK(sk, uaddr) ({ 0; })
> -#define BPF_CGROUP_RUN_PROG_INET6_CONNECT(sk, uaddr) ({ 0; })
> -#define BPF_CGROUP_RUN_PROG_INET6_CONNECT_LOCK(sk, uaddr) ({ 0; })
> -#define BPF_CGROUP_RUN_PROG_UDP4_SENDMSG_LOCK(sk, uaddr, t_ctx) ({ 0; })
> -#define BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK(sk, uaddr, t_ctx) ({ 0; })
> -#define BPF_CGROUP_RUN_PROG_UDP4_RECVMSG_LOCK(sk, uaddr) ({ 0; })
> -#define BPF_CGROUP_RUN_PROG_UDP6_RECVMSG_LOCK(sk, uaddr) ({ 0; })
> +#define BPF_CGROUP_RUN_PROG_INET4_CONNECT(sk, uaddr, uaddrlen) ({ 0; })
> +#define BPF_CGROUP_RUN_PROG_INET4_CONNECT_LOCK(sk, uaddr, uaddrlen) ({ 0; })
> +#define BPF_CGROUP_RUN_PROG_INET6_CONNECT(sk, uaddr, uaddrlen) ({ 0; })
> +#define BPF_CGROUP_RUN_PROG_INET6_CONNECT_LOCK(sk, uaddr, uaddrlen) ({ 0; })
> +#define BPF_CGROUP_RUN_PROG_UDP4_SENDMSG_LOCK(sk, uaddr, uaddrlen, t_ctx) ({ 0; })
> +#define BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK(sk, uaddr, uaddrlen, t_ctx) ({ 0; })
> +#define BPF_CGROUP_RUN_PROG_UDP4_RECVMSG_LOCK(sk, uaddr, uaddrlen) ({ 0; })
> +#define BPF_CGROUP_RUN_PROG_UDP6_RECVMSG_LOCK(sk, uaddr, uaddrlen) ({ 0; })
> #define BPF_CGROUP_RUN_PROG_SOCK_OPS(sock_ops) ({ 0; })
> #define BPF_CGROUP_RUN_PROG_DEVICE_CGROUP(atype, major, minor, access) ({ 0; })
> #define BPF_CGROUP_RUN_PROG_SYSCTL(head,table,write,buf,count,pos) ({ 0; })
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 761af6b3cf2b..77db4263d68d 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1285,6 +1285,7 @@ struct bpf_sock_addr_kern {
> */
> u64 tmp_reg;
> void *t_ctx; /* Attach type specific context. */
> + u32 uaddrlen;
> };
>
> struct bpf_sock_ops_kern {
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 5b2741aa0d9b..534b6c7f5659 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -1449,6 +1449,7 @@ EXPORT_SYMBOL(__cgroup_bpf_run_filter_sk);
> * provided by user sockaddr
> * @sk: sock struct that will use sockaddr
> * @uaddr: sockaddr struct provided by user
> + * @uaddrlen: Pointer to the size of the sockaddr struct provided by user
> * @type: The type of program to be executed
> * @t_ctx: Pointer to attach type specific context
> * @flags: Pointer to u32 which contains higher bits of BPF program
> @@ -1461,6 +1462,7 @@ EXPORT_SYMBOL(__cgroup_bpf_run_filter_sk);
> */
> int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> struct sockaddr *uaddr,
> + int *uaddrlen,
> enum cgroup_bpf_attach_type atype,
> void *t_ctx,
> u32 *flags)
> @@ -1472,6 +1474,7 @@ int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> };
> struct sockaddr_storage unspec;
> struct cgroup *cgrp;
> + int ret;
>
> /* Check socket family since not all sockets represent network
> * endpoint (e.g. AF_UNIX).
> @@ -1482,11 +1485,22 @@ int __cgroup_bpf_run_filter_sock_addr(struct sock *sk,
> if (!ctx.uaddr) {
> memset(&unspec, 0, sizeof(unspec));
> ctx.uaddr = (struct sockaddr *)&unspec;
> - }
> + ctx.uaddrlen = 0;
> + } else if (uaddrlen)
> + ctx.uaddrlen = *uaddrlen;
> + else if (sk->sk_family == AF_INET)
> + ctx.uaddrlen = sizeof(struct sockaddr_in);
> + else if (sk->sk_family == AF_INET6)
> + ctx.uaddrlen = sizeof(struct sockaddr_in6);
I was thinking to pass addrlen whenever possible for AF_INET and AF_INET6.
If I read correctly, all AF_INET6 cases have addrlen available in this patch.
May be just pass addrlen for AF_INET also, then the new sk->sk_family test here
can be avoided?
next prev parent reply other threads:[~2023-09-05 21:21 UTC|newest]
Thread overview: 18+ 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 [this message]
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
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
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=c73a6a41-9e05-c473-0e46-56d0fcfb9ac8@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 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).