Re: [PATCH bpf-next v3 2/9] bpf: Propagate modified uaddrlen from cgroup sockaddr programs

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

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?