Re: [bpf PATCH 3/5] bpf, sockmap: Avoid returning unneeded EAGAIN when redirecting to self

[Daniel Borkmann <daniel@iogearbox.net>]

On 11/7/20 8:38 PM, John Fastabend wrote:
> If a socket redirects to itself and it is under memory pressure it is
> possible to get a socket stuck so that recv() returns EAGAIN and the
> socket can not advance for some time. This happens because when
> redirecting a skb to the same socket we received the skb on we first
> check if it is OK to enqueue the skb on the receiving socket by checking
> memory limits. But, if the skb is itself the object holding the memory
> needed to enqueue the skb we will keep retrying from kernel side
> and always fail with EAGAIN. Then userspace will get a recv() EAGAIN
> error if there are no skbs in the psock ingress queue. This will continue
> until either some skbs get kfree'd causing the memory pressure to
> reduce far enough that we can enqueue the pending packet or the
> socket is destroyed. In some cases its possible to get a socket
> stuck for a noticable amount of time if the socket is only receiving
> skbs from sk_skb verdict programs. To reproduce I make the socket
> memory limits ridiculously low so sockets are always under memory
> pressure. More often though if under memory pressure it looks like
> a spurious EAGAIN error on user space side causing userspace to retry
> and typically enough has moved on the memory side that it works.
> 
> To fix skip memory checks and skb_orphan if receiving on the same
> sock as already assigned.
> 
> For SK_PASS cases this is easy, its always the same socket so we
> can just omit the orphan/set_owner pair.
> 
> For backlog cases we need to check skb->sk and decide if the orphan
> and set_owner pair are needed.
> 
> Fixes: 51199405f9672 ("bpf: skb_verdict, support SK_PASS on RX BPF path")
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
>   net/core/skmsg.c |   72 ++++++++++++++++++++++++++++++++++++++++--------------
>   1 file changed, 53 insertions(+), 19 deletions(-)
> 
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index fe44280c033e..580252e532da 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -399,38 +399,38 @@ int sk_msg_memcopy_from_iter(struct sock *sk, struct iov_iter *from,
>   }
>   EXPORT_SYMBOL_GPL(sk_msg_memcopy_from_iter);
>   
> -static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb)
> +static struct sk_msg *sk_psock_create_ingress_msg(struct sock *sk,
> +						  struct sk_buff *skb)
>   {
> -	struct sock *sk = psock->sk;
> -	int copied = 0, num_sge;
>   	struct sk_msg *msg;
>   
>   	if (atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf)
> -		return -EAGAIN;
> +		return NULL;
> +
> +	if (!sk_rmem_schedule(sk, skb, skb->len))

Isn't accounting always truesize based, thus we should fix & convert all skb->len
to skb->truesize ?

> +		return NULL;
>   
>   	msg = kzalloc(sizeof(*msg), __GFP_NOWARN | GFP_ATOMIC);
>   	if (unlikely(!msg))
> -		return -EAGAIN;
> -	if (!sk_rmem_schedule(sk, skb, skb->len)) {
> -		kfree(msg);
> -		return -EAGAIN;
> -	}
> +		return NULL;
>   
>   	sk_msg_init(msg);
> -	num_sge = skb_to_sgvec(skb, msg->sg.data, 0, skb->len);
> +	return msg;
> +}
> +