Re: [Patch bpf] udp: fix a memory leak in udp_read_sock()

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

Cong Wang wrote:
> On Thu, May 20, 2021 at 10:43 AM John Fastabend
> <john.fastabend@gmail.com> wrote:
> >
> > Cong Wang wrote:
> > > On Wed, May 19, 2021 at 2:54 PM John Fastabend <john.fastabend@gmail.com> wrote:
> > > >
> > > > Cong Wang wrote:
> > > > > On Wed, May 19, 2021 at 12:06 PM John Fastabend
> > > > > <john.fastabend@gmail.com> wrote:
> > > > > >
> > > > > > Cong Wang wrote:
> > > > > > > On Tue, May 18, 2021 at 12:56 PM John Fastabend
> > > > > > > <john.fastabend@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Cong Wang wrote:
> > > > > > > > > On Mon, May 17, 2021 at 10:36 PM John Fastabend
> > > > > > > > > <john.fastabend@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Cong Wang wrote:
> > > > > > > > > > > From: Cong Wang <cong.wang@bytedance.com>
> > > > > > > > > > >

[...]

> >
> > "We are not dropping the packet" you'll need to be more explicit on
> > how this path is not dropping the skb,
> 
> You know it is cloned, don't you? So if we clone an skb and deliver
> the clone then free the original, what is dropped here? Absolutely
> nothing.
> 
> By "drop", we clearly mean nothing is delivered. Or do you have
> any different definition of "drop"?

This is my meaning of 'drop'.

> 
> >
> >   udp_read_sock()
> >     skb = skb_recv_udp()
> >      __skb_recv_udp()
> >        __skb_try_recv_from_queue()
> >         __skb_unlink()              // skb is off the queue
> >     used = recv_actor()
> >       sk_psock_verdict_recv()
> 
> Why do you intentionally ignore the fact the skb is cloned
> and the clone is delivered??

I'm only concerned about the error case here.

> 
> >         return 0;
> >     if (used <= 0) {
> >       kfree(skb)         // skb is unlink'd and kfree'd
> 
> Why do you ignore the other kfree_skb() I added in this patch?
> Which is clearly on the non-error path. This is why I said the
> skb needs to be freed _regardless_ of error or not. You just
> keep ignoring it.

Agree it needs a kfree_skb in both cases I'm not ignoring it. My
issue with this fix is it is not complete. We need some counter
to increment the udp stats so we can learn about these drops.
And it looks like an extra two lines will get us that.

> 
> 
> >       break;
> >     return 0
> >
> > So if in the error case the skb is unlink'd from the queue and
> > kfree'd where is it still existing, how do we get it back? It
> > sure looks dropped to me. Yes, the kfree() is needed to not
> > leak it, but I'm saying we don't want to drop packets silently.
> 
> See above, you clearly ignored the other kfree_skb() which is
> on non-error path.

Ignored in this thread because its correct and looks good to me.
My only issue is with the error path.

> 
> > The convention in UDP space looks to be inc sk->sk_drop and inc
> > the MIB. When we have to debug this on deployed systems and
> > packets silently get dropped its going to cause lots of pain so
> > lets be sure we get the counters correct.
> 
> Sure, let me quote what I already said:
> " A separate patch is need to check desc->error, if really needed."

Check desc->error or even just used <= 0 is sufficient here. Yes it is
really needed I don't want to debug this in a prod environment
without it.

> 
> This patch, as its subject tells, aims to address a memory leak, not
> to address error counters. BTW, TCP does not increase error

OK either add the counters in this patch or as a series of two
patches so we get a complete fix in one series. Without it some
box out there will randomly drop UDP packets, probably DNS
packets for extra fun, and we will have no way of knowing other
than sporadic packet loss. Unless your arguing against having the
counters or that the counters don't make sense for some reason?

> counters either, yet another reason it deserves a separate patch
> to address both.

TCP case is different if we drop packets in a TCP error case
thats not a 'lets increment the counters' problem the drop needs
to be fixed we can't let data fall out of a TCP stream because
no one will retransmit it. We've learned this the hard way.

> 
> Thanks.