Received-SPF: pass (google.com: domain of linux-kernel+bounces-5915-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 19 Dec 2023 12:29:43 -0800
From: John Fastabend <john.fastabend@gmail.com>
To: John Fastabend <john.fastabend@gmail.com>, 
 Kuniyuki Iwashima <kuniyu@amazon.com>, 
 xrivendell7@gmail.com
Cc: alexander@mihalicyn.com, 
 bpf@vger.kernel.org, 
 daan.j.demeyer@gmail.com, 
 davem@davemloft.net, 
 dhowells@redhat.com, 
 edumazet@google.com, 
 john.fastabend@gmail.com, 
 kuba@kernel.org, 
 kuniyu@amazon.com, 
 linux-kernel@vger.kernel.org, 
 netdev@vger.kernel.org, 
 pabeni@redhat.com
Message-ID: <6581fd3754b79_95e63208f@john.notmuch>
In-Reply-To: <6581f509a56ea_90e25208c7@john.notmuch>
References: <CABOYnLwXyxPukiaL36NvGvSa6yW3y0rXgrU=ABOzE-1gDAc4-g@mail.gmail.com>
 <20231219155057.12716-1-kuniyu@amazon.com>
 <6581f509a56ea_90e25208c7@john.notmuch>
Subject: Re: memory leak in unix_create1/copy_process/security_prepare_creds
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable

John Fastabend wrote:
> Kuniyuki Iwashima wrote:
> > From: xingwei lee <xrivendell7@gmail.com>
> > Date: Tue, 19 Dec 2023 17:12:25 +0800
> > > Hello I found a bug in net/af_unix in the lastest upstream linux
> > > 6.7.rc5 and comfired in lastest net/net-next/bpf/bpf-next tree.
> > > Titled "TITLE: memory leak in unix_create1=E2=80=9D and I also uplo=
ad the
> > > repro.c and repro.txt.
> > > =

> > > If you fix this issue, please add the following tag to the commit:
> > > Reported-by: xingwei Lee <xrivendell7@gmail.com>
> > =

> > Thanks for reporting!
> > =

> > It seems 8866730aed510 forgot to add sock_put().
> > I've confirmed that the diff below silenced kmemleak but will check
> > more before posting a patch.
> =

> Did it really silence the memleak?

Yes reverting the patch fixed the issue for me.

> =

> > =

> > ---8<---
> > diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
> > index 7ea7c3a0d0d0..32daba9e7f8b 100644
> > --- a/net/unix/unix_bpf.c
> > +++ b/net/unix/unix_bpf.c
> > @@ -164,6 +164,7 @@ int unix_stream_bpf_update_proto(struct sock *sk,=
 struct sk_psock *psock, bool r
> >  	if (restore) {
> >  		sk->sk_write_space =3D psock->saved_write_space;
> >  		sock_replace_proto(sk, psock->sk_proto);
> > +		sock_put(psock->sk_pair);
> >  		return 0;
> =

> The reason the sock_put is not in this routine but in the sk_psock_dest=
ory
> is because we need to wait a RCU grace period for any pending queued
> BPF sends to also be flushed.

So we need a different fix here. I'll look into it.

> =

> >  	}
> >  =

> > ---8<---
> > =

> > Thanks!
> > =

> > =

> =

> I'm also trying to understand how this adds up to
> unix_stream_bpf_update_proto() issue. The reproduce has a map_create
> followed by two map_delete() calls. I can't see how the unix socket
> ever got added to the BPF map and the deletes should be empty?
> =

> > > =

> > > lastest net tree: 979e90173af8d2f52f671d988189aab98c6d1be6
> > > Kernel config: https://syzkaller.appspot.com/text?tag=3DKernelConfi=
g&x=3D8c4e4700f1727d30
> > > =

> > > in the lastest net tree, the crash like:
> > > Linux syzkaller 6.7.0-rc5-00172-g979e90173af8 #4 SMP PREEMPT_DYNAMI=
C
> > > Tue Dec 19 11:03:58 HKT 2023 x86_4
> > > =

> > > TITLE: memory leak in security_prepare_creds
> > >    [<ffffffff8129291a>] copy_process+0x6aa/0x25c0 kernel/fork.c:236=
6
> > >    [<ffffffff812949db>] kernel_clone+0x11b/0x690 kernel/fork.c:2907=

> > >    [<ffffffff81294fcc>] __do_sys_clone+0x7c/0xb0 kernel/fork.c:3050=

> > >    [<ffffffff84b70dcf>] do_syscall_x64 arch/x86/entry/common.c:52 [=
inline]
> > >    [<ffffffff84b70dcf>] do_syscall_64+0x3f/0x110 arch/x86/entry/com=
mon.c:83
> > >    [<ffffffff84c0008b>] entry_SYSCALL_64_after_hwframe+0x63/0x6b
> =

> ...
> =

> > > uint64_t r[1] =3D {0xffffffffffffffff};
> > > =

> > > void execute_one(void) {
> > >  intptr_t res =3D 0;
> > >  syscall(__NR_socketpair, /*domain=3D*/1ul, /*type=3D*/1ul, /*proto=
=3D*/0,
> > >          /*fds=3D*/0x20000000ul);
> > >  *(uint32_t*)0x200000c0 =3D 0x12;
> > >  *(uint32_t*)0x200000c4 =3D 2;
> > >  *(uint32_t*)0x200000c8 =3D 4;
> > >  *(uint32_t*)0x200000cc =3D 1;
> > >  *(uint32_t*)0x200000d0 =3D 0;
> > >  *(uint32_t*)0x200000d4 =3D -1;
> > >  *(uint32_t*)0x200000d8 =3D 0;
> =

> =

> > >  memset((void*)0x200000dc, 0, 16);
> > >  *(uint32_t*)0x200000ec =3D 0;
> > >  *(uint32_t*)0x200000f0 =3D -1;
> > >  *(uint32_t*)0x200000f4 =3D 0;
> > >  *(uint32_t*)0x200000f8 =3D 0;
> > >  *(uint32_t*)0x200000fc =3D 0;
> > >  *(uint64_t*)0x20000100 =3D 0;
> > >  res =3D syscall(__NR_bpf, /*cmd=3D*/0ul, /*arg=3D*/0x200000c0ul, /=
*size=3D*/0x48ul);
> =

> mapfd =3D map_create( bpf_attr { SOCKHASH, 1 entry, 0 flags, ...} )
> =

> > >  if (res !=3D -1) r[0] =3D res;
> > >  *(uint32_t*)0x200003c0 =3D r[0];
> > >  *(uint64_t*)0x200003c8 =3D 0x20000040;
> > >  *(uint64_t*)0x200003d0 =3D 0x20000000;
> > >  *(uint64_t*)0x200003d8 =3D 0;
> > >  syscall(__NR_bpf, /*cmd=3D*/2ul, /*arg=3D*/0x200003c0ul, /*size=3D=
*/0x20ul);
> =

> map_delete(mapfd, key=3D0x20000040, value=3D0x20000000, flags =3D 0)
> =

> > >  *(uint32_t*)0x200003c0 =3D r[0];
> > >  *(uint64_t*)0x200003c8 =3D 0x20000040;
> > >  *(uint64_t*)0x200003d0 =3D 0x20000000;
> > >  *(uint64_t*)0x200003d8 =3D 0;
> > >  syscall(__NR_bpf, /*cmd=3D*/2ul, /*arg=3D*/0x200003c0ul, /*size=3D=
*/0x20ul);
> =

> map_delete(mapfd, key=3D0x20000040, value=3D0x20000000, flags =3D 0)

cmd=3D2 is update so this splat makes more sense.

> =

> so same as repro.txt below makes sense. But, if the sockets are
> never added to the sockhash then we never touched the proto from
> BPF side. And both of these deletes should return errors.
> =

> > > }
> > > int main(void) {
> > >  syscall(__NR_mmap, /*addr=3D*/0x1ffff000ul, /*len=3D*/0x1000ul, /*=
prot=3D*/0ul,
> > >          /*flags=3D*/0x32ul, /*fd=3D*/-1, /*offset=3D*/0ul);
> > >  syscall(__NR_mmap, /*addr=3D*/0x20000000ul, /*len=3D*/0x1000000ul,=
 /*prot=3D*/7ul,
> > >          /*flags=3D*/0x32ul, /*fd=3D*/-1, /*offset=3D*/0ul);
> > >  syscall(__NR_mmap, /*addr=3D*/0x21000000ul, /*len=3D*/0x1000ul, /*=
prot=3D*/0ul,
> > >          /*flags=3D*/0x32ul, /*fd=3D*/-1, /*offset=3D*/0ul);
> > >  setup_leak();
> > >  loop();
> > >  return 0;
> > > }
> > > =

> > > =

> > > =

> > > =3D* repro.txt =3D*
> > > socketpair(0x1, 0x1, 0x0, &(0x7f0000000000))
> > > r0 =3D bpf$MAP_CREATE(0x0, &(0x7f00000000c0)=3D@base=3D{0x12, 0x2, =
0x4, 0x1}, 0x48)
> > > bpf$MAP_DELETE_ELEM(0x2, &(0x7f00000003c0)=3D{r0, &(0x7f0000000040)=
,
> > > 0x20000000}, 0x20)
> > > bpf$MAP_DELETE_ELEM(0x2, &(0x7f00000003c0)=3D{r0, &(0x7f0000000040)=
,
> > > 0x20000000}, 0x20)
> =

> So not making sense to me how we got to blaming the proto delete
> logic here. It doesn't look like we ever added the psock and
> configured the proto?

Not sure why these is delete cmd=3D0x2.

#define ___BPF_FUNC_MAPPER(FN, ctx...)                  \
        FN(unspec, 0, ##ctx)                            \
        FN(map_lookup_elem, 1, ##ctx)                   \            =

        FN(map_update_elem, 2, ##ctx)                   \


> =

> Did a bisect really blame the mentioned patch? I can likely try here
> as well.

Seems so.

> =

> Thanks,
> John