mbox series

[v2,bpf,0/2] potential sockmap memleak and proc stats fix

Message ID 20210702001123.728035-1-john.fastabend@gmail.com
Headers show
Series potential sockmap memleak and proc stats fix | expand

Message

John Fastabend July 2, 2021, 12:11 a.m. UTC 
While investigating a memleak in sockmap I found these two issues. Patch
1 found doing code review, I wasn't able to get KASAN to trigger a
memleak here, but should be necessary. Patch 2 fixes proc stats so when
we use sockstats for debugging we get correct values.

The fix for observered memleak will come after these, but requires some
more discussion and potentially patch revert so I'll try to get the set
here going now.

John Fastabend (2):
  bpf, sockmap: fix potential memory leak on unlikely error case
  bpf, sockmap: sk_prot needs inuse_idx set for proc stats

 net/core/skmsg.c    | 4 +++-
 net/core/sock_map.c | 9 +++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Cong Wang July 2, 2021, 7:54 p.m. UTC | #1 
On Thu, Jul 1, 2021 at 5:12 PM John Fastabend <john.fastabend@gmail.com> wrote:
>

> If skb_linearize is needed and fails we could leak a msg on the error

> handling. To fix ensure we kfree the msg block before returning error.

> Found during code review.

>

> Fixes: 4363023d2668e ("bpf, sockmap: Avoid failures from skb_to_sgvec when skb has frag_list")

> Signed-off-by: John Fastabend <john.fastabend@gmail.com>

> ---

>  net/core/skmsg.c | 4 +++-

>  1 file changed, 3 insertions(+), 1 deletion(-)

>

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c

> index 9b6160a191f8..22603289c2b2 100644

> --- a/net/core/skmsg.c

> +++ b/net/core/skmsg.c

> @@ -505,8 +505,10 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,

>          * drop the skb. We need to linearize the skb so that the mapping

>          * in skb_to_sgvec can not error.

>          */

> -       if (skb_linearize(skb))

> +       if (skb_linearize(skb)) {

> +               kfree(msg);

>                 return -EAGAIN;

> +       }

>         num_sge = skb_to_sgvec(skb, msg->sg.data, 0, skb->len);

>         if (unlikely(num_sge < 0)) {

>                 kfree(msg);


I think it is better to let whoever allocates msg free it, IOW,
let sk_psock_skb_ingress_enqueue()'s callers handle its failure.

Thanks.
John Fastabend July 5, 2021, 4:27 p.m. UTC | #2 
Cong Wang wrote:
> On Thu, Jul 1, 2021 at 5:12 PM John Fastabend <john.fastabend@gmail.com> wrote:

> >

> > If skb_linearize is needed and fails we could leak a msg on the error

> > handling. To fix ensure we kfree the msg block before returning error.

> > Found during code review.

> >

> > Fixes: 4363023d2668e ("bpf, sockmap: Avoid failures from skb_to_sgvec when skb has frag_list")

> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>

> > ---

> >  net/core/skmsg.c | 4 +++-

> >  1 file changed, 3 insertions(+), 1 deletion(-)

> >

> > diff --git a/net/core/skmsg.c b/net/core/skmsg.c

> > index 9b6160a191f8..22603289c2b2 100644

> > --- a/net/core/skmsg.c

> > +++ b/net/core/skmsg.c

> > @@ -505,8 +505,10 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,

> >          * drop the skb. We need to linearize the skb so that the mapping

> >          * in skb_to_sgvec can not error.

> >          */

> > -       if (skb_linearize(skb))

> > +       if (skb_linearize(skb)) {

> > +               kfree(msg);

> >                 return -EAGAIN;

> > +       }

> >         num_sge = skb_to_sgvec(skb, msg->sg.data, 0, skb->len);

> >         if (unlikely(num_sge < 0)) {

> >                 kfree(msg);

> 

> I think it is better to let whoever allocates msg free it, IOW,

> let sk_psock_skb_ingress_enqueue()'s callers handle its failure.


Sure, although we already have the one kfree(msg) below. Anyways
I'll just move these back a bit. Agree it is slightly nicer.

Thanks.