Received-SPF: pass (google.com: domain of linux-kernel+bounces-5889-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 11:54:49 -0800
From: John Fastabend <john.fastabend@gmail.com>
To: 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: <6581f509a56ea_90e25208c7@john.notmuch>
In-Reply-To: <20231219155057.12716-1-kuniyu@amazon.com>
References: <CABOYnLwXyxPukiaL36NvGvSa6yW3y0rXgrU=ABOzE-1gDAc4-g@mail.gmail.com>
 <20231219155057.12716-1-kuniyu@amazon.com>
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

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 upload=
 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?

> =

> ---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, s=
truct 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_destor=
y
is because we need to wait a RCU grace period for any pending queued
BPF sends to also be flushed.

>  	}
>  =

> ---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=3DKernelConfig&=
x=3D8c4e4700f1727d30
> > =

> > in the lastest net tree, the crash like:
> > Linux syzkaller 6.7.0-rc5-00172-g979e90173af8 #4 SMP PREEMPT_DYNAMIC
> > 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:2366
> >    [<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 [in=
line]
> >    [<ffffffff84b70dcf>] do_syscall_64+0x3f/0x110 arch/x86/entry/commo=
n.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, /*s=
ize=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)

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, /*pr=
ot=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, /*pr=
ot=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, 0x=
4, 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?

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

Thanks,
John=