netdev - Re: [PATCH bpf v6 05/12] bpf: sockmap, TCP data stall on recv before accept

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87h6tlgzt3.fsf@cloudflare.com>
Date:   Wed, 12 Apr 2023 12:26:04 +0200
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     daniel@...earbox.net, lmb@...valent.com, edumazet@...gle.com,
        bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...nel.org,
        andrii@...nel.org, will@...valent.com
Subject: Re: [PATCH bpf v6 05/12] bpf: sockmap, TCP data stall on recv
 before accept

On Fri, Apr 07, 2023 at 10:16 AM -07, John Fastabend wrote:
> A common mechanism to put a TCP socket into the sockmap is to hook the
> BPF_SOCK_OPS_{ACTIVE_PASSIVE}_ESTABLISHED_CB event with a BPF program
> that can map the socket info to the correct BPF verdict parser. When
> the user adds the socket to the map the psock is created and the new
> ops are assigned to ensure the verdict program will 'see' the sk_buffs
> as they arrive.
>
> Part of this process hooks the sk_data_ready op with a BPF specific
> handler to wake up the BPF verdict program when data is ready to read.
> The logic is simple enough (posted here for easy reading)
>
>  static void sk_psock_verdict_data_ready(struct sock *sk)
>  {
> 	struct socket *sock = sk->sk_socket;
>
> 	if (unlikely(!sock || !sock->ops || !sock->ops->read_skb))
> 		return;
> 	sock->ops->read_skb(sk, sk_psock_verdict_recv);
>  }
>
> The oversight here is sk->sk_socket is not assigned until the application
> accepts() the new socket. However, its entirely ok for the peer application
> to do a connect() followed immediately by sends. The socket on the receiver
> is sitting on the backlog queue of the listening socket until its accepted
> and the data is queued up. If the peer never accepts the socket or is slow
> it will eventually hit data limits and rate limit the session. But,
> important for BPF sockmap hooks when this data is received TCP stack does
> the sk_data_ready() call but the read_skb() for this data is never called
> because sk_socket is missing. The data sits on the sk_receive_queue.
>
> Then once the socket is accepted if we never receive more data from the
> peer there will be no further sk_data_ready calls and all the data
> is still on the sk_receive_queue(). Then user calls recvmsg after accept()
> and for TCP sockets in sockmap we use the tcp_bpf_recvmsg_parser() handler.
> The handler checks for data in the sk_msg ingress queue expecting that
> the BPF program has already run from the sk_data_ready hook and enqueued
> the data as needed. So we are stuck.
>
> To fix do an unlikely check in recvmsg handler for data on the
> sk_receive_queue and if it exists wake up data_ready. We have the sock
> locked in both read_skb and recvmsg so should avoid having multiple
> runners.
>
> Fixes: 04919bed948dc ("tcp: Introduce tcp_read_skb()")
> Signed-off-by: John Fastabend <john.fastabend@...il.com>
> ---
>  net/ipv4/tcp_bpf.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 804bd0c247d0..ae6c7130551c 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -212,6 +212,26 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  		return tcp_recvmsg(sk, msg, len, flags, addr_len);
>  
>  	lock_sock(sk);
> +
> +	/* We may have received data on the sk_receive_queue pre-accept and
> +	 * then we can not use read_skb in this context because we haven't
> +	 * assigned a sk_socket yet so have no link to the ops. The work-around
> +	 * is to check the sk_receive_queue and in these cases read skbs off
> +	 * queue again. The read_skb hook is not running at this point because
> +	 * of lock_sock so we avoid having multiple runners in read_skb.
> +	 */
> +	if (unlikely(!skb_queue_empty(&sk->sk_receive_queue))) {
> +		tcp_data_ready(sk);
> +		/* This handles the ENOMEM errors if we both receive data
> +		 * pre accept and are already under memory pressure. At least
> +		 * let user no to retry.

Nit: s/no/know/

> +		 */
> +		if (unlikely(!skb_queue_empty(&sk->sk_receive_queue))) {
> +			copied = -EAGAIN;
> +			goto out;
> +		}
> +	}
> +
>  msg_bytes_ready:
>  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
>  	/* The typical case for EFAULT is the socket was gracefully

Similar to patch 04/12, we will need this corner case fix in
tcp_bpf_recvmsg as well.

Reviewed-by: Jakub Sitnicki <jakub@...udflare.com>