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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <a49096e0-6cc7-7741-a283-27c8629da80f@iogearbox.net>
Date:   Thu, 12 Nov 2020 01:22:29 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     John Fastabend <john.fastabend@...il.com>, ast@...nel.org,
        jakub@...udflare.com
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [bpf PATCH 3/5] bpf, sockmap: Avoid returning unneeded EAGAIN
 when redirecting to self

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@...il.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;
> +}
> +