netdev - Re: [Patch bpf] udp: fix a memory leak in udp_read

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: <CAM_iQpVCfGEA+TOfWvXYxJ1kk9z_thdbvRmZHxhWpuBMx9x2zg@mail.gmail.com>
Date:   Tue, 18 May 2021 09:54:33 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Cong Wang <cong.wang@...edance.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Lorenz Bauer <lmb@...udflare.com>
Subject: Re: [Patch bpf] udp: fix a memory leak in udp_read_sock()

On Mon, May 17, 2021 at 10:36 PM John Fastabend
<john.fastabend@...il.com> wrote:
>
> Cong Wang wrote:
> > From: Cong Wang <cong.wang@...edance.com>
> >
> > sk_psock_verdict_recv() clones the skb and uses the clone
> > afterward, so udp_read_sock() should free the original skb after
> > done using it.
>
> The clone only happens if sk_psock_verdict_recv() returns >0.

Sure, in case of error, no one uses the original skb either,
so still need to free it.

>
> >
> > Fixes: d7f571188ecf ("udp: Implement ->read_sock() for sockmap")
> > Cc: John Fastabend <john.fastabend@...il.com>
> > Cc: Daniel Borkmann <daniel@...earbox.net>
> > Cc: Jakub Sitnicki <jakub@...udflare.com>
> > Cc: Lorenz Bauer <lmb@...udflare.com>
> > Signed-off-by: Cong Wang <cong.wang@...edance.com>
> > ---
> >  net/ipv4/udp.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 15f5504adf5b..e31d67fd5183 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1798,11 +1798,13 @@ int udp_read_sock(struct sock *sk, read_descriptor_t *desc,
> >               if (used <= 0) {
> >                       if (!copied)
> >                               copied = used;
> > +                     kfree_skb(skb);
>
> This case is different from the TCP side, if there is an error
> the sockmap side will also call kfree_skb(). In TCP side we peek
> the skb because we don't want to drop it. On UDP side this will
> just drop data on the floor. Its not super friendly, but its
> UDP so we are making the assumption this is ok? We've tried
> to remove all the drop data cases from TCP it would be nice
> to not drop data on UDP side if we can help it. Could we
> requeue or peek the UDP skb to avoid this?

TCP is special because it supports splice() where we can
do a partial read, so it needs to peek the skb, right? UDP only
supports sockmap, where we always read a whole skb, so we
do not need to peek here?

Thanks.