Re: [PATCH bpf-next v2 5/9] bpf: sockmap: allow UDP sockets

From: Martin KaFai Lau <kafai@fb.com>
To: Lorenz Bauer <lmb@cloudflare.com>
Cc: <john.fastabend@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jakub Sitnicki <jakub@cloudflare.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	Alexei Starovoitov <ast@kernel.org>, <kernel-team@cloudflare.com>,
	<linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
	<bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next v2 5/9] bpf: sockmap: allow UDP sockets
Date: Tue, 3 Mar 2020 09:56:32 -0800	[thread overview]
Message-ID: <20200303175632.l6fjsbl4oa2csnco@kafai-mbp> (raw)
In-Reply-To: <20200228115344.17742-6-lmb@cloudflare.com>

On Fri, Feb 28, 2020 at 11:53:40AM +0000, Lorenz Bauer wrote:
> Add basic psock hooks for UDP sockets. This allows adding and
> removing sockets, as well as automatic removal on unhash and close.
> 
> sock_map_sk_state_allowed is called from the syscall path, and
> ensures that only established or listening sockets are added.
> No such check exists for the BPF path: we rely on sockets being
> in particular states when a BPF sock ops hook is executed.
> For the passive open hook this means that sockets are actually in
> TCP_SYN_RECV state (and unhashed) when they are added to the
> sock map.
> 
> UDP sockets are not saddled with this inconsistency, and so the
> checks for both syscall and BPF path should be identical. Rather
> than duplicating the logic into sock_map_sk_state_allowed merge
> it with sock_map_sk_is_suitable.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  MAINTAINERS         |  1 +
>  include/linux/udp.h |  4 ++++
>  net/core/sock_map.c | 52 ++++++++++++++++++++++++++------------------
>  net/ipv4/Makefile   |  1 +
>  net/ipv4/udp_bpf.c  | 53 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 90 insertions(+), 21 deletions(-)
>  create mode 100644 net/ipv4/udp_bpf.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2af5fa73155e..495ba52038ad 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9358,6 +9358,7 @@ F:	include/linux/skmsg.h
>  F:	net/core/skmsg.c
>  F:	net/core/sock_map.c
>  F:	net/ipv4/tcp_bpf.c
> +F:	net/ipv4/udp_bpf.c
>  
>  LANTIQ / INTEL Ethernet drivers
>  M:	Hauke Mehrtens <hauke@hauke-m.de>
> diff --git a/include/linux/udp.h b/include/linux/udp.h
> index aa84597bdc33..2485a35d113c 100644
> --- a/include/linux/udp.h
> +++ b/include/linux/udp.h
> @@ -143,4 +143,8 @@ static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff *skb)
>  
>  #define IS_UDPLITE(__sk) (__sk->sk_protocol == IPPROTO_UDPLITE)
>  
> +#ifdef CONFIG_BPF_STREAM_PARSER
> +int udp_bpf_init(struct sock *sk);
> +#endif
> +
>  #endif	/* _LINUX_UDP_H */
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index c84cc9fc7f6b..d742e1538ae9 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c

[ ... ]

> @@ -466,15 +479,20 @@ static bool sock_map_op_okay(const struct bpf_sock_ops_kern *ops)
>  	       ops->op == BPF_SOCK_OPS_TCP_LISTEN_CB;
>  }
>  
> -static bool sock_map_sk_is_suitable(const struct sock *sk)
> +static bool sock_map_sk_is_udp(const struct sock *sk)
>  {
> -	return sk->sk_type == SOCK_STREAM &&
> -	       sk->sk_protocol == IPPROTO_TCP;
> +	return sk->sk_type == SOCK_DGRAM && sk->sk_protocol == IPPROTO_UDP;
>  }
>  
> -static bool sock_map_sk_state_allowed(const struct sock *sk)
> +static bool sock_map_sk_is_suitable(const struct sock *sk, bool from_bpf)
"from_bpf" seems unnecessary.

>  {
> -	return (1 << sk->sk_state) & (TCPF_ESTABLISHED | TCPF_LISTEN);
> +	int tcp_flags = TCPF_ESTABLISHED | TCPF_LISTEN;
> +
> +	if (from_bpf)
> +		tcp_flags |= TCPF_SYN_RECV;
> +
> +	return (sock_map_sk_is_udp(sk) && sk_hashed(sk)) ||
> +	       (sock_map_sk_is_tcp(sk) && (1 << sk->sk_state) & tcp_flags);
>  }
>  
>  static int sock_map_update_elem(struct bpf_map *map, void *key,