Re: [PATCH bpf 05/11] bpf: sockmap, TCP data stall on recv before accept

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 21, 2023 at 2:52 PM John Fastabend <john.fastabend@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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 3a0f43f3afd8..b1ba58be0c5a 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -209,6 +209,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_lockless(&sk->sk_receive_queue))) {

socket is locked here, please use skb_queue_empty() ?

We shall reserve skb_queue_empty_lockless() for lockless contexts.

> +               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.
> +                */
> +               if (unlikely(!skb_queue_empty_lockless(&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
> --
> 2.33.0
>




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux