netdev - Re: [Patch bpf v2] 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]

Message-ID: <60ddec9f5abd6_3fe24208c3@john-XPS-13-9370.notmuch>
Date:   Thu, 01 Jul 2021 09:26:07 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>,
        Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, 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>
Subject: Re: [Patch bpf v2] skmsg: check sk_rcvbuf limit before queuing to
 ingress_skb

Jakub Sitnicki wrote:
> On Thu, Jul 01, 2021 at 08:16 AM CEST, 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,
> > but it is not limited either.
> >
> > This patch adds checks for sk->sk_rcvbuf right before queuing
> > to ingress_skb and drops 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 about sk_forward_alloc.
> >
> > 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>
> > ---
> 
> Acked-by: Jakub Sitnicki <jakub@...udflare.com>
> 
> By saying that it is hard to make TCP happy about sk_forward_alloc, I'm
> guessing you're referring to problems described in 144748eb0c44 ("bpf,
> sockmap: Fix incorrect fwd_alloc accounting") [1]?

I have a couple fixes on my stack here I'm testing that clean up
the tear down logic. Once thats in place maybe its as simple
as adding the owner_r bits and calling the destructor to ensure
memory accounting happens earlier so these ingress_skb packets
are accounted for.

I'll flush those out today, maybe it will be clear then.

> 
> Thanks for the fix.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=144748eb0c445091466c9b741ebd0bfcc5914f3d
> 
> [...]