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

[John Fastabend <john.fastabend@gmail.com>]

Daniel Borkmann wrote:
> 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 ?

Right good catch, will fix in v2.