Re: [Patch bpf v3] skmsg: check sk_rcvbuf limit before queuing to ingress_skb

From: John Fastabend <john.fastabend@gmail.com>
To: Cong Wang <xiyou.wangcong@gmail.com>,
	John Fastabend <john.fastabend@gmail.com>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>, Cong Wang <cong.wang@bytedance.com>,
	Jiang Wang <jiang.wang@bytedance.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Lorenz Bauer <lmb@cloudflare.com>,
	Jakub Sitnicki <jakub@cloudflare.com>
Subject: Re: [Patch bpf v3] skmsg: check sk_rcvbuf limit before queuing to ingress_skb
Date: Wed, 13 Oct 2021 09:53:32 -0700	[thread overview]
Message-ID: <61670f0cdb0dd_4d1c0208ee@john-XPS-13-9370.notmuch> (raw)
In-Reply-To: <CAM_iQpU11gHXS416D9KjTd5CoEGyhNT3kkejUMhymoOmnGUrcw@mail.gmail.com>

Cong Wang wrote:
> On Wed, Oct 13, 2021 at 7:07 AM John Fastabend <john.fastabend@gmail.com> wrote:
> >
> > Cong Wang wrote:
> > > From: Cong Wang <cong.wang@bytedance.com>
> > >
> > > Jiang observed OOM frequently when testing our AF_UNIX/UDP
> > > proxy. This is due to the fact that we do not actually limit
> > > the socket memory before queueing skb to ingress_skb. We
> > > charge the skb memory later when handling the psock backlog,
> > > and it is not limited either.

[...]

> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > index e8b48df73c85..8b243fcdbb8f 100644
> > > --- a/net/ipv4/tcp.c
> > > +++ b/net/ipv4/tcp.c
> > > @@ -1665,6 +1665,8 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> > >                       if (used <= 0) {
> > >                               if (!copied)
> > >                                       copied = used;
> > > +                             if (used == -EAGAIN)
> > > +                                     continue;
> >
> > This is not a good idea, looping through read_sock because we have
> > hit a memory limit is not going to work. If something is holding the
> > memlimit pinned this could loop indefinately.
> >
> > Also this will run the verdict multiple times on the same bytes. For
> > apply/cork logic this will break plus just basic parsers will be
> > confused when they see duplicate bytes.
> 
> Good point! I run out of ideas for dealing with this TCP case,
> dropping is not okay, retrying is hard, reworking TCP ACKing
> is even harder. :-/

I think it can be done with a retry queue in skmsg side. I'll give
it a try today/tomorrow.

> 
> >
> > We need to do a workqueue and then retry later.
> >
> > Final missing piece is that strparser logic would still not handle
> > this correctly.
> >
> > I don't mind spending some time on this today. I'll apply your
> > patch and then add a few fixes for above.
> 
> Ideally, we should move TCP ACK after ->sk_data_ready()
> so that dropping in ->sk_data_ready() would be fine, but this is
> certainly not easy even if it is doable.

iirc the original hook did this but there was concern from TCP
maintainers. So we decided to put hooks on top of TCP vs inside
TCP. Its also helpful for TLS hooks.

> 
> Thanks.