netdev - RE: [Patch bpf v3] skmsg: check sk_rcvbuf limit before queuing to ingress

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 13 Oct 2021 07:07:02 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Cc:     bpf@...r.kernel.org, Cong Wang <cong.wang@...edance.com>,
        Jiang Wang <jiang.wang@...edance.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Jakub Sitnicki <jakub@...udflare.com>
Subject: RE: [Patch bpf v3] skmsg: check sk_rcvbuf limit before queuing to
 ingress_skb

Cong Wang wrote:
> From: Cong Wang <cong.wang@...edance.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.
> 
> This patch adds checks for sk->sk_rcvbuf right before queuing
> to ingress_skb and drops or retries the packets if this limit
> exceeds. This is very similar to UDP receive path. Ideally we
> should set the skb owner before this check too, but it is hard
> to make TCP happy with sk_forward_alloc.
> 
> For TCP, we can not just drop the packets on errors. TCP ACKs
> are already sent for those packet before reaching
> ->sk_data_ready(). Instead, we use best effort to retry, this
> works because TCP does not remove the skb from receive queue
> at that point and exceeding sk_rcvbuf limit is a temporary
> situation.
> 
> Reported-by: Jiang Wang <jiang.wang@...edance.com>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: Lorenz Bauer <lmb@...udflare.com>
> Cc: Jakub Sitnicki <jakub@...udflare.com>
> Signed-off-by: Cong Wang <cong.wang@...edance.com>

Makes sense to include a fixes tag here.

> ---
> v3: add retry logic for TCP
> v2: add READ_ONCE()

I agree this logic is needed, but I think the below is not
complete. I can get the couple extra fixes in front of this
today/tomorrow on my side and kick it through some testing here.
Then we should push it as a series. Your patch + additions.

> 
>  net/core/skmsg.c | 15 +++++++++------
>  net/ipv4/tcp.c   |  2 ++
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2d6249b28928..356c314cd60c 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c

All the skmsg changes are good.


> 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.

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.

Thanks,
John