* memory leak in bpf
@ 2020-11-18 3:50 syzbot
2020-12-10 6:58 ` syzbot
0 siblings, 1 reply; 14+ messages in thread
From: syzbot @ 2020-11-18 3:50 UTC (permalink / raw)
To: andrii, ast, bpf, daniel, john.fastabend, kafai, kpsingh,
linux-kernel, netdev, songliubraving, syzkaller-bugs, yhs
Hello,
syzbot found the following issue on:
HEAD commit: f01c30de Merge tag 'vfs-5.10-fixes-2' of git://git.kernel...
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=15b9b181500000
kernel config: https://syzkaller.appspot.com/x/.config?x=a3f13716fa0212fd
dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
compiler: gcc (GCC) 10.1.0-syz 20200507
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14b78b81500000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
2020/11/14 15:01:05 executed programs: 33
2020/11/14 15:01:11 executed programs: 35
2020/11/14 15:01:17 executed programs: 37
2020/11/14 15:01:22 executed programs: 39
BUG: memory leak
unreferenced object 0xffff8881161c6440 (size 64):
comm "syz-executor.0", pid 8961, jiffies 4295107077 (age 12.370s)
hex dump (first 32 bytes):
80 62 58 04 00 ea ff ff c0 17 37 04 00 ea ff ff .bX.......7.....
80 6f 47 04 00 ea ff ff 40 64 58 04 00 ea ff ff .oG.....@dX.....
backtrace:
[<00000000189a27fd>] kmalloc_node include/linux/slab.h:575 [inline]
[<00000000189a27fd>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
[<00000000189a27fd>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
[<00000000189a27fd>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
[<00000000189a27fd>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
[<000000009e5cec3e>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
[<000000009e5cec3e>] map_create kernel/bpf/syscall.c:825 [inline]
[<000000009e5cec3e>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
[<00000000c513b5d1>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
[<0000000033006ec5>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2020-11-18 3:50 memory leak in bpf syzbot
@ 2020-12-10 6:58 ` syzbot
2021-03-01 16:21 ` Rustam Kovhaev
0 siblings, 1 reply; 14+ messages in thread
From: syzbot @ 2020-12-10 6:58 UTC (permalink / raw)
To: andrii, ast, bpf, daniel, john.fastabend, kafai, kpsingh,
linux-kernel, netdev, songliubraving, syzkaller-bugs, yhs
syzbot has found a reproducer for the following issue on:
HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
compiler: gcc (GCC) 10.1.0-syz 20200507
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
Debian GNU/Linux 9 syzkaller ttyS0
Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
executing program
executing program
executing program
BUG: memory leak
unreferenced object 0xffff88810efccc80 (size 64):
comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
hex dump (first 32 bytes):
c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
backtrace:
[<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
[<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
[<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
[<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
[<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
[<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
[<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
[<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
[<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
[<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2020-12-10 6:58 ` syzbot
@ 2021-03-01 16:21 ` Rustam Kovhaev
2021-03-01 19:05 ` Dmitry Vyukov
0 siblings, 1 reply; 14+ messages in thread
From: Rustam Kovhaev @ 2021-03-01 16:21 UTC (permalink / raw)
To: syzbot
Cc: andrii, ast, bpf, daniel, john.fastabend, kafai, kpsingh,
linux-kernel, netdev, songliubraving, syzkaller-bugs, yhs,
dvyukov, gregkh
On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> syzbot has found a reproducer for the following issue on:
>
> HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> compiler: gcc (GCC) 10.1.0-syz 20200507
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
>
> Debian GNU/Linux 9 syzkaller ttyS0
> Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> executing program
> executing program
> executing program
> BUG: memory leak
> unreferenced object 0xffff88810efccc80 (size 64):
> comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> hex dump (first 32 bytes):
> c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> backtrace:
> [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
>
i am pretty sure that this one is a false positive
the problem with reproducer is that it does not terminate all of the
child processes that it spawns
i confirmed that it is a false positive by tracing __fput() and
bpf_map_release(), i ran reproducer, got kmemleak report, then i
manually killed those running leftover processes from reproducer and
then both functions were executed and memory was freed
i am marking this one as:
#syz invalid
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-03-01 16:21 ` Rustam Kovhaev
@ 2021-03-01 19:05 ` Dmitry Vyukov
2021-03-01 20:39 ` Rustam Kovhaev
0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Vyukov @ 2021-03-01 19:05 UTC (permalink / raw)
To: Rustam Kovhaev
Cc: syzbot, andrii, Alexei Starovoitov, bpf, Daniel Borkmann,
John Fastabend, Martin KaFai Lau, KP Singh, LKML, netdev,
Song Liu, syzkaller-bugs, Yonghong Song, Greg Kroah-Hartman
On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
>
> On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > syzbot has found a reproducer for the following issue on:
> >
> > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > git tree: upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > compiler: gcc (GCC) 10.1.0-syz 20200507
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> >
> > Debian GNU/Linux 9 syzkaller ttyS0
> > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > executing program
> > executing program
> > executing program
> > BUG: memory leak
> > unreferenced object 0xffff88810efccc80 (size 64):
> > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > hex dump (first 32 bytes):
> > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > backtrace:
> > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >
> >
>
> i am pretty sure that this one is a false positive
> the problem with reproducer is that it does not terminate all of the
> child processes that it spawns
>
> i confirmed that it is a false positive by tracing __fput() and
> bpf_map_release(), i ran reproducer, got kmemleak report, then i
> manually killed those running leftover processes from reproducer and
> then both functions were executed and memory was freed
>
> i am marking this one as:
> #syz invalid
Hi Rustam,
Thanks for looking into this.
I wonder how/where are these objects referenced? If they are not
leaked and referenced somewhere, KMEMLEAK should not report them as
leaks.
So even if this is a false positive for BPF, this is a true positive
bug and something to fix for KMEMLEAK ;)
And syzbot will probably re-create this bug report soon as this still
happens and is not a one-off thing.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-03-01 19:05 ` Dmitry Vyukov
@ 2021-03-01 20:39 ` Rustam Kovhaev
2021-03-01 20:43 ` Dmitry Vyukov
0 siblings, 1 reply; 14+ messages in thread
From: Rustam Kovhaev @ 2021-03-01 20:39 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: syzbot, andrii, Alexei Starovoitov, bpf, Daniel Borkmann,
John Fastabend, Martin KaFai Lau, KP Singh, LKML, netdev,
Song Liu, syzkaller-bugs, Yonghong Song, Greg Kroah-Hartman
On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> >
> > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > syzbot has found a reproducer for the following issue on:
> > >
> > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > git tree: upstream
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > >
> > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > >
> > > Debian GNU/Linux 9 syzkaller ttyS0
> > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > executing program
> > > executing program
> > > executing program
> > > BUG: memory leak
> > > unreferenced object 0xffff88810efccc80 (size 64):
> > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > hex dump (first 32 bytes):
> > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > backtrace:
> > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > >
> > >
> >
> > i am pretty sure that this one is a false positive
> > the problem with reproducer is that it does not terminate all of the
> > child processes that it spawns
> >
> > i confirmed that it is a false positive by tracing __fput() and
> > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > manually killed those running leftover processes from reproducer and
> > then both functions were executed and memory was freed
> >
> > i am marking this one as:
> > #syz invalid
>
> Hi Rustam,
>
> Thanks for looking into this.
>
> I wonder how/where are these objects referenced? If they are not
> leaked and referenced somewhere, KMEMLEAK should not report them as
> leaks.
> So even if this is a false positive for BPF, this is a true positive
> bug and something to fix for KMEMLEAK ;)
> And syzbot will probably re-create this bug report soon as this still
> happens and is not a one-off thing.
hi Dmitry, i haven't thought of it this way, but i guess you are right,
it is a kmemleak bug, ideally kmemleak should be aware that there are
still running processes holding references to bpf fd/anonymous inodes
which in their turn hold references to allocated bpf maps
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-03-01 20:39 ` Rustam Kovhaev
@ 2021-03-01 20:43 ` Dmitry Vyukov
2021-04-07 23:24 ` Rustam Kovhaev
0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Vyukov @ 2021-03-01 20:43 UTC (permalink / raw)
To: Rustam Kovhaev
Cc: syzbot, andrii, Alexei Starovoitov, bpf, Daniel Borkmann,
John Fastabend, Martin KaFai Lau, KP Singh, LKML, netdev,
Song Liu, syzkaller-bugs, Yonghong Song, Greg Kroah-Hartman
On Mon, Mar 1, 2021 at 9:39 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
>
> On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> > On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > >
> > > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > > syzbot has found a reproducer for the following issue on:
> > > >
> > > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > > git tree: upstream
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > > >
> > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > > >
> > > > Debian GNU/Linux 9 syzkaller ttyS0
> > > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > > executing program
> > > > executing program
> > > > executing program
> > > > BUG: memory leak
> > > > unreferenced object 0xffff88810efccc80 (size 64):
> > > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > > hex dump (first 32 bytes):
> > > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > > backtrace:
> > > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > >
> > > >
> > >
> > > i am pretty sure that this one is a false positive
> > > the problem with reproducer is that it does not terminate all of the
> > > child processes that it spawns
> > >
> > > i confirmed that it is a false positive by tracing __fput() and
> > > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > > manually killed those running leftover processes from reproducer and
> > > then both functions were executed and memory was freed
> > >
> > > i am marking this one as:
> > > #syz invalid
> >
> > Hi Rustam,
> >
> > Thanks for looking into this.
> >
> > I wonder how/where are these objects referenced? If they are not
> > leaked and referenced somewhere, KMEMLEAK should not report them as
> > leaks.
> > So even if this is a false positive for BPF, this is a true positive
> > bug and something to fix for KMEMLEAK ;)
> > And syzbot will probably re-create this bug report soon as this still
> > happens and is not a one-off thing.
>
> hi Dmitry, i haven't thought of it this way, but i guess you are right,
> it is a kmemleak bug, ideally kmemleak should be aware that there are
> still running processes holding references to bpf fd/anonymous inodes
> which in their turn hold references to allocated bpf maps
KMEMLEAK scans whole memory, so if there are pointers to the object
anywhere in memory, KMEMLEAK should not report them as leaked. Running
processes have no direct effect on KMEMLEAK logic.
So the question is: where are these pointers to these objects? If we
answer this, we can check how/why KMEMLEAK misses them. Are they
mangled in some way?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-03-01 20:43 ` Dmitry Vyukov
@ 2021-04-07 23:24 ` Rustam Kovhaev
2021-04-07 23:35 ` Andrii Nakryiko
0 siblings, 1 reply; 14+ messages in thread
From: Rustam Kovhaev @ 2021-04-07 23:24 UTC (permalink / raw)
To: Dmitry Vyukov, andrii
Cc: syzbot, andrii, Alexei Starovoitov, bpf, Daniel Borkmann,
John Fastabend, Martin KaFai Lau, KP Singh, LKML, netdev,
Song Liu, syzkaller-bugs, Yonghong Song, Greg Kroah-Hartman
On Mon, Mar 01, 2021 at 09:43:00PM +0100, Dmitry Vyukov wrote:
> On Mon, Mar 1, 2021 at 9:39 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> >
> > On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> > > On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > >
> > > > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > > > syzbot has found a reproducer for the following issue on:
> > > > >
> > > > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > > > git tree: upstream
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > > > >
> > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > > > >
> > > > > Debian GNU/Linux 9 syzkaller ttyS0
> > > > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > > > executing program
> > > > > executing program
> > > > > executing program
> > > > > BUG: memory leak
> > > > > unreferenced object 0xffff88810efccc80 (size 64):
> > > > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > > > hex dump (first 32 bytes):
> > > > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > > > backtrace:
> > > > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > >
> > > > >
> > > >
> > > > i am pretty sure that this one is a false positive
> > > > the problem with reproducer is that it does not terminate all of the
> > > > child processes that it spawns
> > > >
> > > > i confirmed that it is a false positive by tracing __fput() and
> > > > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > > > manually killed those running leftover processes from reproducer and
> > > > then both functions were executed and memory was freed
> > > >
> > > > i am marking this one as:
> > > > #syz invalid
> > >
> > > Hi Rustam,
> > >
> > > Thanks for looking into this.
> > >
> > > I wonder how/where are these objects referenced? If they are not
> > > leaked and referenced somewhere, KMEMLEAK should not report them as
> > > leaks.
> > > So even if this is a false positive for BPF, this is a true positive
> > > bug and something to fix for KMEMLEAK ;)
> > > And syzbot will probably re-create this bug report soon as this still
> > > happens and is not a one-off thing.
> >
> > hi Dmitry, i haven't thought of it this way, but i guess you are right,
> > it is a kmemleak bug, ideally kmemleak should be aware that there are
> > still running processes holding references to bpf fd/anonymous inodes
> > which in their turn hold references to allocated bpf maps
>
> KMEMLEAK scans whole memory, so if there are pointers to the object
> anywhere in memory, KMEMLEAK should not report them as leaked. Running
> processes have no direct effect on KMEMLEAK logic.
> So the question is: where are these pointers to these objects? If we
> answer this, we can check how/why KMEMLEAK misses them. Are they
> mangled in some way?
thank you for your comments, they make sense, and indeed, the pointer
gets vmaped.
i should have looked into this sooner, becaused syzbot did trigger the
issue again, and Andrii had to look into the same bug, sorry about that.
if i am understanding this correctly here is what the fix should be:
---
kernel/bpf/ringbuf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
index f25b719ac786..30400e74abe2 100644
--- a/kernel/bpf/ringbuf.c
+++ b/kernel/bpf/ringbuf.c
@@ -8,6 +8,7 @@
#include <linux/vmalloc.h>
#include <linux/wait.h>
#include <linux/poll.h>
+#include <linux/kmemleak.h>
#include <uapi/linux/btf.h>
#define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
@@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
if (rb) {
+ kmemleak_not_leak((void *) pages);
rb->pages = pages;
rb->nr_pages = nr_pages;
return rb;
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-04-07 23:24 ` Rustam Kovhaev
@ 2021-04-07 23:35 ` Andrii Nakryiko
2021-04-08 18:56 ` Rustam Kovhaev
0 siblings, 1 reply; 14+ messages in thread
From: Andrii Nakryiko @ 2021-04-07 23:35 UTC (permalink / raw)
To: Rustam Kovhaev
Cc: Dmitry Vyukov, Andrii Nakryiko, syzbot, Alexei Starovoitov, bpf,
Daniel Borkmann, John Fastabend, Martin KaFai Lau, KP Singh,
LKML, netdev, Song Liu, syzkaller-bugs, Yonghong Song,
Greg Kroah-Hartman
On Wed, Apr 7, 2021 at 4:24 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
>
> On Mon, Mar 01, 2021 at 09:43:00PM +0100, Dmitry Vyukov wrote:
> > On Mon, Mar 1, 2021 at 9:39 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > >
> > > On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> > > > On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > >
> > > > > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > > > > syzbot has found a reproducer for the following issue on:
> > > > > >
> > > > > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > > > > git tree: upstream
> > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > > > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > > > > >
> > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > > > > >
> > > > > > Debian GNU/Linux 9 syzkaller ttyS0
> > > > > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > > > > executing program
> > > > > > executing program
> > > > > > executing program
> > > > > > BUG: memory leak
> > > > > > unreferenced object 0xffff88810efccc80 (size 64):
> > > > > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > > > > hex dump (first 32 bytes):
> > > > > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > > > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > > > > backtrace:
> > > > > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > > > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > > > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > > > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > > > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > > > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > > > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > > > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > > > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > > > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > > >
> > > > > >
> > > > >
> > > > > i am pretty sure that this one is a false positive
> > > > > the problem with reproducer is that it does not terminate all of the
> > > > > child processes that it spawns
> > > > >
> > > > > i confirmed that it is a false positive by tracing __fput() and
> > > > > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > > > > manually killed those running leftover processes from reproducer and
> > > > > then both functions were executed and memory was freed
> > > > >
> > > > > i am marking this one as:
> > > > > #syz invalid
> > > >
> > > > Hi Rustam,
> > > >
> > > > Thanks for looking into this.
> > > >
> > > > I wonder how/where are these objects referenced? If they are not
> > > > leaked and referenced somewhere, KMEMLEAK should not report them as
> > > > leaks.
> > > > So even if this is a false positive for BPF, this is a true positive
> > > > bug and something to fix for KMEMLEAK ;)
> > > > And syzbot will probably re-create this bug report soon as this still
> > > > happens and is not a one-off thing.
> > >
> > > hi Dmitry, i haven't thought of it this way, but i guess you are right,
> > > it is a kmemleak bug, ideally kmemleak should be aware that there are
> > > still running processes holding references to bpf fd/anonymous inodes
> > > which in their turn hold references to allocated bpf maps
> >
> > KMEMLEAK scans whole memory, so if there are pointers to the object
> > anywhere in memory, KMEMLEAK should not report them as leaked. Running
> > processes have no direct effect on KMEMLEAK logic.
> > So the question is: where are these pointers to these objects? If we
> > answer this, we can check how/why KMEMLEAK misses them. Are they
> > mangled in some way?
> thank you for your comments, they make sense, and indeed, the pointer
> gets vmaped.
> i should have looked into this sooner, becaused syzbot did trigger the
> issue again, and Andrii had to look into the same bug, sorry about that.
No worries! I actually forgot about this thread :) Let's leave the
link to my today's investigation ([0]) just for completeness.
[0] https://lore.kernel.org/bpf/CAEf4BzYk+dqs+jwu6VKXP-RttcTEGFe+ySTGWT9CRNkagDiJVA@mail.gmail.com/
> if i am understanding this correctly here is what the fix should be:
> ---
> kernel/bpf/ringbuf.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index f25b719ac786..30400e74abe2 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -8,6 +8,7 @@
> #include <linux/vmalloc.h>
> #include <linux/wait.h>
> #include <linux/poll.h>
> +#include <linux/kmemleak.h>
> #include <uapi/linux/btf.h>
>
> #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> if (rb) {
> + kmemleak_not_leak((void *) pages);
If that makes kmemleak happy, I have no problems with this. But maybe
leave some comment explaining why this is needed at all?
And for my understanding, how vmap changes anything? Those pages are
still referenced from rb, which is referenced from some struct file in
the system. Sorry if that's a naive question.
> rb->pages = pages;
> rb->nr_pages = nr_pages;
> return rb;
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-04-07 23:35 ` Andrii Nakryiko
@ 2021-04-08 18:56 ` Rustam Kovhaev
2021-06-13 15:17 ` Rustam Kovhaev
0 siblings, 1 reply; 14+ messages in thread
From: Rustam Kovhaev @ 2021-04-08 18:56 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Dmitry Vyukov, Andrii Nakryiko, syzbot, Alexei Starovoitov, bpf,
Daniel Borkmann, John Fastabend, Martin KaFai Lau, KP Singh,
LKML, netdev, Song Liu, syzkaller-bugs, Yonghong Song,
Greg Kroah-Hartman
On Wed, Apr 07, 2021 at 04:35:34PM -0700, Andrii Nakryiko wrote:
> On Wed, Apr 7, 2021 at 4:24 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> >
> > On Mon, Mar 01, 2021 at 09:43:00PM +0100, Dmitry Vyukov wrote:
> > > On Mon, Mar 1, 2021 at 9:39 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> > > > > On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > >
> > > > > > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > > > > > syzbot has found a reproducer for the following issue on:
> > > > > > >
> > > > > > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > > > > > git tree: upstream
> > > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > > > > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > > > > > >
> > > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > > > > > >
> > > > > > > Debian GNU/Linux 9 syzkaller ttyS0
> > > > > > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > > > > > executing program
> > > > > > > executing program
> > > > > > > executing program
> > > > > > > BUG: memory leak
> > > > > > > unreferenced object 0xffff88810efccc80 (size 64):
> > > > > > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > > > > > hex dump (first 32 bytes):
> > > > > > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > > > > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > > > > > backtrace:
> > > > > > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > > > > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > > > > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > > > > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > > > > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > > > > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > > > > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > > > > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > i am pretty sure that this one is a false positive
> > > > > > the problem with reproducer is that it does not terminate all of the
> > > > > > child processes that it spawns
> > > > > >
> > > > > > i confirmed that it is a false positive by tracing __fput() and
> > > > > > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > > > > > manually killed those running leftover processes from reproducer and
> > > > > > then both functions were executed and memory was freed
> > > > > >
> > > > > > i am marking this one as:
> > > > > > #syz invalid
> > > > >
> > > > > Hi Rustam,
> > > > >
> > > > > Thanks for looking into this.
> > > > >
> > > > > I wonder how/where are these objects referenced? If they are not
> > > > > leaked and referenced somewhere, KMEMLEAK should not report them as
> > > > > leaks.
> > > > > So even if this is a false positive for BPF, this is a true positive
> > > > > bug and something to fix for KMEMLEAK ;)
> > > > > And syzbot will probably re-create this bug report soon as this still
> > > > > happens and is not a one-off thing.
> > > >
> > > > hi Dmitry, i haven't thought of it this way, but i guess you are right,
> > > > it is a kmemleak bug, ideally kmemleak should be aware that there are
> > > > still running processes holding references to bpf fd/anonymous inodes
> > > > which in their turn hold references to allocated bpf maps
> > >
> > > KMEMLEAK scans whole memory, so if there are pointers to the object
> > > anywhere in memory, KMEMLEAK should not report them as leaked. Running
> > > processes have no direct effect on KMEMLEAK logic.
> > > So the question is: where are these pointers to these objects? If we
> > > answer this, we can check how/why KMEMLEAK misses them. Are they
> > > mangled in some way?
> > thank you for your comments, they make sense, and indeed, the pointer
> > gets vmaped.
> > i should have looked into this sooner, becaused syzbot did trigger the
> > issue again, and Andrii had to look into the same bug, sorry about that.
>
> No worries! I actually forgot about this thread :) Let's leave the
> link to my today's investigation ([0]) just for completeness.
>
> [0] https://lore.kernel.org/bpf/CAEf4BzYk+dqs+jwu6VKXP-RttcTEGFe+ySTGWT9CRNkagDiJVA@mail.gmail.com/
>
> > if i am understanding this correctly here is what the fix should be:
> > ---
> > kernel/bpf/ringbuf.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > index f25b719ac786..30400e74abe2 100644
> > --- a/kernel/bpf/ringbuf.c
> > +++ b/kernel/bpf/ringbuf.c
> > @@ -8,6 +8,7 @@
> > #include <linux/vmalloc.h>
> > #include <linux/wait.h>
> > #include <linux/poll.h>
> > +#include <linux/kmemleak.h>
> > #include <uapi/linux/btf.h>
> >
> > #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> > @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> > rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> > VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> > if (rb) {
> > + kmemleak_not_leak((void *) pages);
>
> If that makes kmemleak happy, I have no problems with this. But maybe
> leave some comment explaining why this is needed at all?
>
> And for my understanding, how vmap changes anything? Those pages are
> still referenced from rb, which is referenced from some struct file in
> the system. Sorry if that's a naive question.
>
valid question, it does look like kmemleak should be scanning
vmalloc()/vmap() memory, i will research this further
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-04-08 18:56 ` Rustam Kovhaev
@ 2021-06-13 15:17 ` Rustam Kovhaev
2021-06-24 17:28 ` Rustam Kovhaev
0 siblings, 1 reply; 14+ messages in thread
From: Rustam Kovhaev @ 2021-06-13 15:17 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Dmitry Vyukov, Andrii Nakryiko, syzbot, Alexei Starovoitov, bpf,
Daniel Borkmann, John Fastabend, Martin KaFai Lau, KP Singh,
LKML, netdev, Song Liu, syzkaller-bugs, Yonghong Song,
Greg Kroah-Hartman
On Thu, Apr 08, 2021 at 11:56:15AM -0700, Rustam Kovhaev wrote:
> On Wed, Apr 07, 2021 at 04:35:34PM -0700, Andrii Nakryiko wrote:
> > On Wed, Apr 7, 2021 at 4:24 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > >
> > > On Mon, Mar 01, 2021 at 09:43:00PM +0100, Dmitry Vyukov wrote:
> > > > On Mon, Mar 1, 2021 at 9:39 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > >
> > > > > On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> > > > > > On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > > >
> > > > > > > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > > > > > > syzbot has found a reproducer for the following issue on:
> > > > > > > >
> > > > > > > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > > > > > > git tree: upstream
> > > > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > > > > > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > > > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > > > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > > > > > > >
> > > > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > > > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > > > > > > >
> > > > > > > > Debian GNU/Linux 9 syzkaller ttyS0
> > > > > > > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > > > > > > executing program
> > > > > > > > executing program
> > > > > > > > executing program
> > > > > > > > BUG: memory leak
> > > > > > > > unreferenced object 0xffff88810efccc80 (size 64):
> > > > > > > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > > > > > > hex dump (first 32 bytes):
> > > > > > > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > > > > > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > > > > > > backtrace:
> > > > > > > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > > > > > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > > > > > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > > > > > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > > > > > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > > > > > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > i am pretty sure that this one is a false positive
> > > > > > > the problem with reproducer is that it does not terminate all of the
> > > > > > > child processes that it spawns
> > > > > > >
> > > > > > > i confirmed that it is a false positive by tracing __fput() and
> > > > > > > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > > > > > > manually killed those running leftover processes from reproducer and
> > > > > > > then both functions were executed and memory was freed
> > > > > > >
> > > > > > > i am marking this one as:
> > > > > > > #syz invalid
> > > > > >
> > > > > > Hi Rustam,
> > > > > >
> > > > > > Thanks for looking into this.
> > > > > >
> > > > > > I wonder how/where are these objects referenced? If they are not
> > > > > > leaked and referenced somewhere, KMEMLEAK should not report them as
> > > > > > leaks.
> > > > > > So even if this is a false positive for BPF, this is a true positive
> > > > > > bug and something to fix for KMEMLEAK ;)
> > > > > > And syzbot will probably re-create this bug report soon as this still
> > > > > > happens and is not a one-off thing.
> > > > >
> > > > > hi Dmitry, i haven't thought of it this way, but i guess you are right,
> > > > > it is a kmemleak bug, ideally kmemleak should be aware that there are
> > > > > still running processes holding references to bpf fd/anonymous inodes
> > > > > which in their turn hold references to allocated bpf maps
> > > >
> > > > KMEMLEAK scans whole memory, so if there are pointers to the object
> > > > anywhere in memory, KMEMLEAK should not report them as leaked. Running
> > > > processes have no direct effect on KMEMLEAK logic.
> > > > So the question is: where are these pointers to these objects? If we
> > > > answer this, we can check how/why KMEMLEAK misses them. Are they
> > > > mangled in some way?
> > > thank you for your comments, they make sense, and indeed, the pointer
> > > gets vmaped.
> > > i should have looked into this sooner, becaused syzbot did trigger the
> > > issue again, and Andrii had to look into the same bug, sorry about that.
> >
> > No worries! I actually forgot about this thread :) Let's leave the
> > link to my today's investigation ([0]) just for completeness.
> >
> > [0] https://lore.kernel.org/bpf/CAEf4BzYk+dqs+jwu6VKXP-RttcTEGFe+ySTGWT9CRNkagDiJVA@mail.gmail.com/
> >
> > > if i am understanding this correctly here is what the fix should be:
> > > ---
> > > kernel/bpf/ringbuf.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > > index f25b719ac786..30400e74abe2 100644
> > > --- a/kernel/bpf/ringbuf.c
> > > +++ b/kernel/bpf/ringbuf.c
> > > @@ -8,6 +8,7 @@
> > > #include <linux/vmalloc.h>
> > > #include <linux/wait.h>
> > > #include <linux/poll.h>
> > > +#include <linux/kmemleak.h>
> > > #include <uapi/linux/btf.h>
> > >
> > > #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> > > @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> > > rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> > > VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> > > if (rb) {
> > > + kmemleak_not_leak((void *) pages);
> >
> > If that makes kmemleak happy, I have no problems with this. But maybe
> > leave some comment explaining why this is needed at all?
> >
> > And for my understanding, how vmap changes anything? Those pages are
> > still referenced from rb, which is referenced from some struct file in
> > the system. Sorry if that's a naive question.
> >
> valid question, it does look like kmemleak should be scanning
> vmalloc()/vmap() memory, i will research this further
a quick update, i see a problem in kmemleak code, and i have simplified
the reproducer by getting rid of a vmap().
i will reach out to maintainer and mm and afterwards i will update this
bug, cheers!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-06-13 15:17 ` Rustam Kovhaev
@ 2021-06-24 17:28 ` Rustam Kovhaev
2021-06-25 14:28 ` Dmitry Vyukov
2021-07-07 17:06 ` Andrii Nakryiko
0 siblings, 2 replies; 14+ messages in thread
From: Rustam Kovhaev @ 2021-06-24 17:28 UTC (permalink / raw)
To: Andrii Nakryiko, Dmitry Vyukov
Cc: Andrii Nakryiko, syzbot, Alexei Starovoitov, bpf,
Daniel Borkmann, John Fastabend, Martin KaFai Lau, KP Singh,
LKML, netdev, Song Liu, syzkaller-bugs, Yonghong Song,
Greg Kroah-Hartman
On Sun, Jun 13, 2021 at 08:17:04AM -0700, Rustam Kovhaev wrote:
> On Thu, Apr 08, 2021 at 11:56:15AM -0700, Rustam Kovhaev wrote:
> > On Wed, Apr 07, 2021 at 04:35:34PM -0700, Andrii Nakryiko wrote:
> > > On Wed, Apr 7, 2021 at 4:24 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 01, 2021 at 09:43:00PM +0100, Dmitry Vyukov wrote:
> > > > > On Mon, Mar 1, 2021 at 9:39 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> > > > > > > On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > > > > > > > syzbot has found a reproducer for the following issue on:
> > > > > > > > >
> > > > > > > > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > > > > > > > git tree: upstream
> > > > > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > > > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > > > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > > > > > > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > > > > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > > > > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > > > > > > > >
> > > > > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > > > > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > > > > > > > >
> > > > > > > > > Debian GNU/Linux 9 syzkaller ttyS0
> > > > > > > > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > > > > > > > executing program
> > > > > > > > > executing program
> > > > > > > > > executing program
> > > > > > > > > BUG: memory leak
> > > > > > > > > unreferenced object 0xffff88810efccc80 (size 64):
> > > > > > > > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > > > > > > > hex dump (first 32 bytes):
> > > > > > > > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > > > > > > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > > > > > > > backtrace:
> > > > > > > > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > > > > > > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > > > > > > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > > > > > > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > > > > > > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > > > > > > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > > i am pretty sure that this one is a false positive
> > > > > > > > the problem with reproducer is that it does not terminate all of the
> > > > > > > > child processes that it spawns
> > > > > > > >
> > > > > > > > i confirmed that it is a false positive by tracing __fput() and
> > > > > > > > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > > > > > > > manually killed those running leftover processes from reproducer and
> > > > > > > > then both functions were executed and memory was freed
> > > > > > > >
> > > > > > > > i am marking this one as:
> > > > > > > > #syz invalid
> > > > > > >
> > > > > > > Hi Rustam,
> > > > > > >
> > > > > > > Thanks for looking into this.
> > > > > > >
> > > > > > > I wonder how/where are these objects referenced? If they are not
> > > > > > > leaked and referenced somewhere, KMEMLEAK should not report them as
> > > > > > > leaks.
> > > > > > > So even if this is a false positive for BPF, this is a true positive
> > > > > > > bug and something to fix for KMEMLEAK ;)
> > > > > > > And syzbot will probably re-create this bug report soon as this still
> > > > > > > happens and is not a one-off thing.
> > > > > >
> > > > > > hi Dmitry, i haven't thought of it this way, but i guess you are right,
> > > > > > it is a kmemleak bug, ideally kmemleak should be aware that there are
> > > > > > still running processes holding references to bpf fd/anonymous inodes
> > > > > > which in their turn hold references to allocated bpf maps
> > > > >
> > > > > KMEMLEAK scans whole memory, so if there are pointers to the object
> > > > > anywhere in memory, KMEMLEAK should not report them as leaked. Running
> > > > > processes have no direct effect on KMEMLEAK logic.
> > > > > So the question is: where are these pointers to these objects? If we
> > > > > answer this, we can check how/why KMEMLEAK misses them. Are they
> > > > > mangled in some way?
> > > > thank you for your comments, they make sense, and indeed, the pointer
> > > > gets vmaped.
> > > > i should have looked into this sooner, becaused syzbot did trigger the
> > > > issue again, and Andrii had to look into the same bug, sorry about that.
> > >
> > > No worries! I actually forgot about this thread :) Let's leave the
> > > link to my today's investigation ([0]) just for completeness.
> > >
> > > [0] https://lore.kernel.org/bpf/CAEf4BzYk+dqs+jwu6VKXP-RttcTEGFe+ySTGWT9CRNkagDiJVA@mail.gmail.com/
> > >
> > > > if i am understanding this correctly here is what the fix should be:
> > > > ---
> > > > kernel/bpf/ringbuf.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > > > index f25b719ac786..30400e74abe2 100644
> > > > --- a/kernel/bpf/ringbuf.c
> > > > +++ b/kernel/bpf/ringbuf.c
> > > > @@ -8,6 +8,7 @@
> > > > #include <linux/vmalloc.h>
> > > > #include <linux/wait.h>
> > > > #include <linux/poll.h>
> > > > +#include <linux/kmemleak.h>
> > > > #include <uapi/linux/btf.h>
> > > >
> > > > #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> > > > @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> > > > rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> > > > VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> > > > if (rb) {
> > > > + kmemleak_not_leak((void *) pages);
> > >
> > > If that makes kmemleak happy, I have no problems with this. But maybe
> > > leave some comment explaining why this is needed at all?
> > >
> > > And for my understanding, how vmap changes anything? Those pages are
> > > still referenced from rb, which is referenced from some struct file in
> > > the system. Sorry if that's a naive question.
> > >
> > valid question, it does look like kmemleak should be scanning
> > vmalloc()/vmap() memory, i will research this further
>
> a quick update, i see a problem in kmemleak code, and i have simplified
> the reproducer by getting rid of a vmap().
> i will reach out to maintainer and mm and afterwards i will update this
> bug, cheers!
>
Andrii, we have discovered that kmemleak scans struct page, but it does
not scan page contents and this is by design. if we allocate some memory
with kmalloc(), then allocate page with alloc_page(), and if we put
kmalloc pointer somewhere inside that page, kmemleak will report kmalloc
pointer as a false positive.
we can instruct kmemleak to scan the memory area by calling
kmemleak_alloc()/kmemleak_free() as shown below. if we don't need that
memory to be scanned then we can use kmemleak_not_leak().
if we use the former then i guess we need to be careful since we do not
want/need to scan the memory that is being used by user-space.
---
kernel/bpf/ringbuf.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
index 84b3b35fc0d0..cf7ce10b4fb1 100644
--- a/kernel/bpf/ringbuf.c
+++ b/kernel/bpf/ringbuf.c
@@ -8,6 +8,7 @@
#include <linux/vmalloc.h>
#include <linux/wait.h>
#include <linux/poll.h>
+#include <linux/kmemleak.h>
#include <uapi/linux/btf.h>
#define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
@@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
if (rb) {
+ kmemleak_alloc(rb, PAGE_SIZE, 1, flags);
rb->pages = pages;
rb->nr_pages = nr_pages;
return rb;
@@ -184,6 +186,7 @@ static void bpf_ringbuf_free(struct bpf_ringbuf *rb)
struct page **pages = rb->pages;
int i, nr_pages = rb->nr_pages;
+ kmemleak_free(rb);
vunmap(rb);
for (i = 0; i < nr_pages; i++)
__free_page(pages[i]);
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-06-24 17:28 ` Rustam Kovhaev
@ 2021-06-25 14:28 ` Dmitry Vyukov
2021-06-26 18:40 ` Rustam Kovhaev
2021-07-07 17:06 ` Andrii Nakryiko
1 sibling, 1 reply; 14+ messages in thread
From: Dmitry Vyukov @ 2021-06-25 14:28 UTC (permalink / raw)
To: Rustam Kovhaev
Cc: Andrii Nakryiko, Andrii Nakryiko, syzbot, Alexei Starovoitov,
bpf, Daniel Borkmann, John Fastabend, Martin KaFai Lau, KP Singh,
LKML, netdev, Song Liu, syzkaller-bugs, Yonghong Song,
Greg Kroah-Hartman
On Thu, Jun 24, 2021 at 7:28 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > On Wed, Apr 7, 2021 at 4:24 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > >
> > > > > On Mon, Mar 01, 2021 at 09:43:00PM +0100, Dmitry Vyukov wrote:
> > > > > > On Mon, Mar 1, 2021 at 9:39 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > > >
> > > > > > > On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> > > > > > > > On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > > > > > > > > syzbot has found a reproducer for the following issue on:
> > > > > > > > > >
> > > > > > > > > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > > > > > > > > git tree: upstream
> > > > > > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > > > > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > > > > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > > > > > > > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > > > > > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > > > > > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > > > > > > > > >
> > > > > > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > > > > > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > > > > > > > > >
> > > > > > > > > > Debian GNU/Linux 9 syzkaller ttyS0
> > > > > > > > > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > > > > > > > > executing program
> > > > > > > > > > executing program
> > > > > > > > > > executing program
> > > > > > > > > > BUG: memory leak
> > > > > > > > > > unreferenced object 0xffff88810efccc80 (size 64):
> > > > > > > > > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > > > > > > > > hex dump (first 32 bytes):
> > > > > > > > > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > > > > > > > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > > > > > > > > backtrace:
> > > > > > > > > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > > > > > > > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > > > > > > > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > > > > > > > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > > > > > > > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > > > > > > > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > i am pretty sure that this one is a false positive
> > > > > > > > > the problem with reproducer is that it does not terminate all of the
> > > > > > > > > child processes that it spawns
> > > > > > > > >
> > > > > > > > > i confirmed that it is a false positive by tracing __fput() and
> > > > > > > > > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > > > > > > > > manually killed those running leftover processes from reproducer and
> > > > > > > > > then both functions were executed and memory was freed
> > > > > > > > >
> > > > > > > > > i am marking this one as:
> > > > > > > > > #syz invalid
> > > > > > > >
> > > > > > > > Hi Rustam,
> > > > > > > >
> > > > > > > > Thanks for looking into this.
> > > > > > > >
> > > > > > > > I wonder how/where are these objects referenced? If they are not
> > > > > > > > leaked and referenced somewhere, KMEMLEAK should not report them as
> > > > > > > > leaks.
> > > > > > > > So even if this is a false positive for BPF, this is a true positive
> > > > > > > > bug and something to fix for KMEMLEAK ;)
> > > > > > > > And syzbot will probably re-create this bug report soon as this still
> > > > > > > > happens and is not a one-off thing.
> > > > > > >
> > > > > > > hi Dmitry, i haven't thought of it this way, but i guess you are right,
> > > > > > > it is a kmemleak bug, ideally kmemleak should be aware that there are
> > > > > > > still running processes holding references to bpf fd/anonymous inodes
> > > > > > > which in their turn hold references to allocated bpf maps
> > > > > >
> > > > > > KMEMLEAK scans whole memory, so if there are pointers to the object
> > > > > > anywhere in memory, KMEMLEAK should not report them as leaked. Running
> > > > > > processes have no direct effect on KMEMLEAK logic.
> > > > > > So the question is: where are these pointers to these objects? If we
> > > > > > answer this, we can check how/why KMEMLEAK misses them. Are they
> > > > > > mangled in some way?
> > > > > thank you for your comments, they make sense, and indeed, the pointer
> > > > > gets vmaped.
> > > > > i should have looked into this sooner, becaused syzbot did trigger the
> > > > > issue again, and Andrii had to look into the same bug, sorry about that.
> > > >
> > > > No worries! I actually forgot about this thread :) Let's leave the
> > > > link to my today's investigation ([0]) just for completeness.
> > > >
> > > > [0] https://lore.kernel.org/bpf/CAEf4BzYk+dqs+jwu6VKXP-RttcTEGFe+ySTGWT9CRNkagDiJVA@mail.gmail.com/
> > > >
> > > > > if i am understanding this correctly here is what the fix should be:
> > > > > ---
> > > > > kernel/bpf/ringbuf.c | 2 ++
> > > > > 1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > > > > index f25b719ac786..30400e74abe2 100644
> > > > > --- a/kernel/bpf/ringbuf.c
> > > > > +++ b/kernel/bpf/ringbuf.c
> > > > > @@ -8,6 +8,7 @@
> > > > > #include <linux/vmalloc.h>
> > > > > #include <linux/wait.h>
> > > > > #include <linux/poll.h>
> > > > > +#include <linux/kmemleak.h>
> > > > > #include <uapi/linux/btf.h>
> > > > >
> > > > > #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> > > > > @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> > > > > rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> > > > > VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> > > > > if (rb) {
> > > > > + kmemleak_not_leak((void *) pages);
> > > >
> > > > If that makes kmemleak happy, I have no problems with this. But maybe
> > > > leave some comment explaining why this is needed at all?
> > > >
> > > > And for my understanding, how vmap changes anything? Those pages are
> > > > still referenced from rb, which is referenced from some struct file in
> > > > the system. Sorry if that's a naive question.
> > > >
> > > valid question, it does look like kmemleak should be scanning
> > > vmalloc()/vmap() memory, i will research this further
> >
> > a quick update, i see a problem in kmemleak code, and i have simplified
> > the reproducer by getting rid of a vmap().
> > i will reach out to maintainer and mm and afterwards i will update this
> > bug, cheers!
> >
>
> Andrii, we have discovered that kmemleak scans struct page, but it does
> not scan page contents and this is by design. if we allocate some memory
> with kmalloc(), then allocate page with alloc_page(), and if we put
> kmalloc pointer somewhere inside that page, kmemleak will report kmalloc
> pointer as a false positive.
> we can instruct kmemleak to scan the memory area by calling
> kmemleak_alloc()/kmemleak_free() as shown below. if we don't need that
> memory to be scanned then we can use kmemleak_not_leak().
> if we use the former then i guess we need to be careful since we do not
> want/need to scan the memory that is being used by user-space.
Thanks for your heroic digging and persistence on this issue, Rustam!
> ---
> kernel/bpf/ringbuf.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 84b3b35fc0d0..cf7ce10b4fb1 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -8,6 +8,7 @@
> #include <linux/vmalloc.h>
> #include <linux/wait.h>
> #include <linux/poll.h>
> +#include <linux/kmemleak.h>
> #include <uapi/linux/btf.h>
>
> #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> if (rb) {
> + kmemleak_alloc(rb, PAGE_SIZE, 1, flags);
> rb->pages = pages;
> rb->nr_pages = nr_pages;
> return rb;
> @@ -184,6 +186,7 @@ static void bpf_ringbuf_free(struct bpf_ringbuf *rb)
> struct page **pages = rb->pages;
> int i, nr_pages = rb->nr_pages;
>
> + kmemleak_free(rb);
> vunmap(rb);
> for (i = 0; i < nr_pages; i++)
> __free_page(pages[i]);
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-06-25 14:28 ` Dmitry Vyukov
@ 2021-06-26 18:40 ` Rustam Kovhaev
0 siblings, 0 replies; 14+ messages in thread
From: Rustam Kovhaev @ 2021-06-26 18:40 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrii Nakryiko, Andrii Nakryiko, syzbot, Alexei Starovoitov,
bpf, Daniel Borkmann, John Fastabend, Martin KaFai Lau, KP Singh,
LKML, netdev, Song Liu, syzkaller-bugs, Yonghong Song,
Greg Kroah-Hartman
On Fri, Jun 25, 2021 at 04:28:00PM +0200, Dmitry Vyukov wrote:
> On Thu, Jun 24, 2021 at 7:28 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > On Wed, Apr 7, 2021 at 4:24 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Mar 01, 2021 at 09:43:00PM +0100, Dmitry Vyukov wrote:
> > > > > > > On Mon, Mar 1, 2021 at 9:39 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> > > > > > > > > On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > > > > > > > > > syzbot has found a reproducer for the following issue on:
> > > > > > > > > > >
> > > > > > > > > > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > > > > > > > > > git tree: upstream
> > > > > > > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > > > > > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > > > > > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > > > > > > > > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > > > > > > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > > > > > > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > > > > > > > > > >
> > > > > > > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > > > > > > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > > > > > > > > > >
> > > > > > > > > > > Debian GNU/Linux 9 syzkaller ttyS0
> > > > > > > > > > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > > > > > > > > > executing program
> > > > > > > > > > > executing program
> > > > > > > > > > > executing program
> > > > > > > > > > > BUG: memory leak
> > > > > > > > > > > unreferenced object 0xffff88810efccc80 (size 64):
> > > > > > > > > > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > > > > > > > > > hex dump (first 32 bytes):
> > > > > > > > > > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > > > > > > > > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > > > > > > > > > backtrace:
> > > > > > > > > > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > > > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > > > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > > > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > > > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > > > > > > > > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > > > > > > > > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > > > > > > > > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > > > > > > > > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > > > > > > > > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > i am pretty sure that this one is a false positive
> > > > > > > > > > the problem with reproducer is that it does not terminate all of the
> > > > > > > > > > child processes that it spawns
> > > > > > > > > >
> > > > > > > > > > i confirmed that it is a false positive by tracing __fput() and
> > > > > > > > > > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > > > > > > > > > manually killed those running leftover processes from reproducer and
> > > > > > > > > > then both functions were executed and memory was freed
> > > > > > > > > >
> > > > > > > > > > i am marking this one as:
> > > > > > > > > > #syz invalid
> > > > > > > > >
> > > > > > > > > Hi Rustam,
> > > > > > > > >
> > > > > > > > > Thanks for looking into this.
> > > > > > > > >
> > > > > > > > > I wonder how/where are these objects referenced? If they are not
> > > > > > > > > leaked and referenced somewhere, KMEMLEAK should not report them as
> > > > > > > > > leaks.
> > > > > > > > > So even if this is a false positive for BPF, this is a true positive
> > > > > > > > > bug and something to fix for KMEMLEAK ;)
> > > > > > > > > And syzbot will probably re-create this bug report soon as this still
> > > > > > > > > happens and is not a one-off thing.
> > > > > > > >
> > > > > > > > hi Dmitry, i haven't thought of it this way, but i guess you are right,
> > > > > > > > it is a kmemleak bug, ideally kmemleak should be aware that there are
> > > > > > > > still running processes holding references to bpf fd/anonymous inodes
> > > > > > > > which in their turn hold references to allocated bpf maps
> > > > > > >
> > > > > > > KMEMLEAK scans whole memory, so if there are pointers to the object
> > > > > > > anywhere in memory, KMEMLEAK should not report them as leaked. Running
> > > > > > > processes have no direct effect on KMEMLEAK logic.
> > > > > > > So the question is: where are these pointers to these objects? If we
> > > > > > > answer this, we can check how/why KMEMLEAK misses them. Are they
> > > > > > > mangled in some way?
> > > > > > thank you for your comments, they make sense, and indeed, the pointer
> > > > > > gets vmaped.
> > > > > > i should have looked into this sooner, becaused syzbot did trigger the
> > > > > > issue again, and Andrii had to look into the same bug, sorry about that.
> > > > >
> > > > > No worries! I actually forgot about this thread :) Let's leave the
> > > > > link to my today's investigation ([0]) just for completeness.
> > > > >
> > > > > [0] https://lore.kernel.org/bpf/CAEf4BzYk+dqs+jwu6VKXP-RttcTEGFe+ySTGWT9CRNkagDiJVA@mail.gmail.com/
> > > > >
> > > > > > if i am understanding this correctly here is what the fix should be:
> > > > > > ---
> > > > > > kernel/bpf/ringbuf.c | 2 ++
> > > > > > 1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > > > > > index f25b719ac786..30400e74abe2 100644
> > > > > > --- a/kernel/bpf/ringbuf.c
> > > > > > +++ b/kernel/bpf/ringbuf.c
> > > > > > @@ -8,6 +8,7 @@
> > > > > > #include <linux/vmalloc.h>
> > > > > > #include <linux/wait.h>
> > > > > > #include <linux/poll.h>
> > > > > > +#include <linux/kmemleak.h>
> > > > > > #include <uapi/linux/btf.h>
> > > > > >
> > > > > > #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> > > > > > @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> > > > > > rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> > > > > > VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> > > > > > if (rb) {
> > > > > > + kmemleak_not_leak((void *) pages);
> > > > >
> > > > > If that makes kmemleak happy, I have no problems with this. But maybe
> > > > > leave some comment explaining why this is needed at all?
> > > > >
> > > > > And for my understanding, how vmap changes anything? Those pages are
> > > > > still referenced from rb, which is referenced from some struct file in
> > > > > the system. Sorry if that's a naive question.
> > > > >
> > > > valid question, it does look like kmemleak should be scanning
> > > > vmalloc()/vmap() memory, i will research this further
> > >
> > > a quick update, i see a problem in kmemleak code, and i have simplified
> > > the reproducer by getting rid of a vmap().
> > > i will reach out to maintainer and mm and afterwards i will update this
> > > bug, cheers!
> > >
> >
> > Andrii, we have discovered that kmemleak scans struct page, but it does
> > not scan page contents and this is by design. if we allocate some memory
> > with kmalloc(), then allocate page with alloc_page(), and if we put
> > kmalloc pointer somewhere inside that page, kmemleak will report kmalloc
> > pointer as a false positive.
> > we can instruct kmemleak to scan the memory area by calling
> > kmemleak_alloc()/kmemleak_free() as shown below. if we don't need that
> > memory to be scanned then we can use kmemleak_not_leak().
> > if we use the former then i guess we need to be careful since we do not
> > want/need to scan the memory that is being used by user-space.
>
> Thanks for your heroic digging and persistence on this issue, Rustam!
>
thank you for the kind words, Dmitry, and thank you for asking the
questions, i learned quite a lot while trying to answer them.
i've just sent out a patch for this.
> > ---
> > kernel/bpf/ringbuf.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > index 84b3b35fc0d0..cf7ce10b4fb1 100644
> > --- a/kernel/bpf/ringbuf.c
> > +++ b/kernel/bpf/ringbuf.c
> > @@ -8,6 +8,7 @@
> > #include <linux/vmalloc.h>
> > #include <linux/wait.h>
> > #include <linux/poll.h>
> > +#include <linux/kmemleak.h>
> > #include <uapi/linux/btf.h>
> >
> > #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> > @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> > rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> > VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> > if (rb) {
> > + kmemleak_alloc(rb, PAGE_SIZE, 1, flags);
> > rb->pages = pages;
> > rb->nr_pages = nr_pages;
> > return rb;
> > @@ -184,6 +186,7 @@ static void bpf_ringbuf_free(struct bpf_ringbuf *rb)
> > struct page **pages = rb->pages;
> > int i, nr_pages = rb->nr_pages;
> >
> > + kmemleak_free(rb);
> > vunmap(rb);
> > for (i = 0; i < nr_pages; i++)
> > __free_page(pages[i]);
> > --
> > 2.30.2
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: memory leak in bpf
2021-06-24 17:28 ` Rustam Kovhaev
2021-06-25 14:28 ` Dmitry Vyukov
@ 2021-07-07 17:06 ` Andrii Nakryiko
1 sibling, 0 replies; 14+ messages in thread
From: Andrii Nakryiko @ 2021-07-07 17:06 UTC (permalink / raw)
To: Rustam Kovhaev
Cc: Dmitry Vyukov, Andrii Nakryiko, syzbot, Alexei Starovoitov, bpf,
Daniel Borkmann, John Fastabend, Martin KaFai Lau, KP Singh,
LKML, netdev, Song Liu, syzkaller-bugs, Yonghong Song,
Greg Kroah-Hartman
On Thu, Jun 24, 2021 at 10:28 AM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
>
> On Sun, Jun 13, 2021 at 08:17:04AM -0700, Rustam Kovhaev wrote:
> > On Thu, Apr 08, 2021 at 11:56:15AM -0700, Rustam Kovhaev wrote:
> > > On Wed, Apr 07, 2021 at 04:35:34PM -0700, Andrii Nakryiko wrote:
> > > > On Wed, Apr 7, 2021 at 4:24 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > >
> > > > > On Mon, Mar 01, 2021 at 09:43:00PM +0100, Dmitry Vyukov wrote:
> > > > > > On Mon, Mar 1, 2021 at 9:39 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > > >
> > > > > > > On Mon, Mar 01, 2021 at 08:05:42PM +0100, Dmitry Vyukov wrote:
> > > > > > > > On Mon, Mar 1, 2021 at 5:21 PM Rustam Kovhaev <rkovhaev@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Dec 09, 2020 at 10:58:10PM -0800, syzbot wrote:
> > > > > > > > > > syzbot has found a reproducer for the following issue on:
> > > > > > > > > >
> > > > > > > > > > HEAD commit: a68a0262 mm/madvise: remove racy mm ownership check
> > > > > > > > > > git tree: upstream
> > > > > > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=11facf17500000
> > > > > > > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4305fa9ea70c7a9f
> > > > > > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f3694595248708227d35
> > > > > > > > > > compiler: gcc (GCC) 10.1.0-syz 20200507
> > > > > > > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=159a9613500000
> > > > > > > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11bf7123500000
> > > > > > > > > >
> > > > > > > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > > > > > > > Reported-by: syzbot+f3694595248708227d35@syzkaller.appspotmail.com
> > > > > > > > > >
> > > > > > > > > > Debian GNU/Linux 9 syzkaller ttyS0
> > > > > > > > > > Warning: Permanently added '10.128.0.9' (ECDSA) to the list of known hosts.
> > > > > > > > > > executing program
> > > > > > > > > > executing program
> > > > > > > > > > executing program
> > > > > > > > > > BUG: memory leak
> > > > > > > > > > unreferenced object 0xffff88810efccc80 (size 64):
> > > > > > > > > > comm "syz-executor334", pid 8460, jiffies 4294945724 (age 13.850s)
> > > > > > > > > > hex dump (first 32 bytes):
> > > > > > > > > > c0 cb 14 04 00 ea ff ff c0 c2 11 04 00 ea ff ff ................
> > > > > > > > > > c0 56 3f 04 00 ea ff ff 40 18 38 04 00 ea ff ff .V?.....@.8.....
> > > > > > > > > > backtrace:
> > > > > > > > > > [<0000000036ae98a7>] kmalloc_node include/linux/slab.h:575 [inline]
> > > > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_area_alloc kernel/bpf/ringbuf.c:94 [inline]
> > > > > > > > > > [<0000000036ae98a7>] bpf_ringbuf_alloc kernel/bpf/ringbuf.c:135 [inline]
> > > > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc kernel/bpf/ringbuf.c:183 [inline]
> > > > > > > > > > [<0000000036ae98a7>] ringbuf_map_alloc+0x1be/0x410 kernel/bpf/ringbuf.c:150
> > > > > > > > > > [<00000000d2cb93ae>] find_and_alloc_map kernel/bpf/syscall.c:122 [inline]
> > > > > > > > > > [<00000000d2cb93ae>] map_create kernel/bpf/syscall.c:825 [inline]
> > > > > > > > > > [<00000000d2cb93ae>] __do_sys_bpf+0x7d0/0x30a0 kernel/bpf/syscall.c:4381
> > > > > > > > > > [<000000008feaf393>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > > > > > > > > > [<00000000e1f53cfd>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > i am pretty sure that this one is a false positive
> > > > > > > > > the problem with reproducer is that it does not terminate all of the
> > > > > > > > > child processes that it spawns
> > > > > > > > >
> > > > > > > > > i confirmed that it is a false positive by tracing __fput() and
> > > > > > > > > bpf_map_release(), i ran reproducer, got kmemleak report, then i
> > > > > > > > > manually killed those running leftover processes from reproducer and
> > > > > > > > > then both functions were executed and memory was freed
> > > > > > > > >
> > > > > > > > > i am marking this one as:
> > > > > > > > > #syz invalid
> > > > > > > >
> > > > > > > > Hi Rustam,
> > > > > > > >
> > > > > > > > Thanks for looking into this.
> > > > > > > >
> > > > > > > > I wonder how/where are these objects referenced? If they are not
> > > > > > > > leaked and referenced somewhere, KMEMLEAK should not report them as
> > > > > > > > leaks.
> > > > > > > > So even if this is a false positive for BPF, this is a true positive
> > > > > > > > bug and something to fix for KMEMLEAK ;)
> > > > > > > > And syzbot will probably re-create this bug report soon as this still
> > > > > > > > happens and is not a one-off thing.
> > > > > > >
> > > > > > > hi Dmitry, i haven't thought of it this way, but i guess you are right,
> > > > > > > it is a kmemleak bug, ideally kmemleak should be aware that there are
> > > > > > > still running processes holding references to bpf fd/anonymous inodes
> > > > > > > which in their turn hold references to allocated bpf maps
> > > > > >
> > > > > > KMEMLEAK scans whole memory, so if there are pointers to the object
> > > > > > anywhere in memory, KMEMLEAK should not report them as leaked. Running
> > > > > > processes have no direct effect on KMEMLEAK logic.
> > > > > > So the question is: where are these pointers to these objects? If we
> > > > > > answer this, we can check how/why KMEMLEAK misses them. Are they
> > > > > > mangled in some way?
> > > > > thank you for your comments, they make sense, and indeed, the pointer
> > > > > gets vmaped.
> > > > > i should have looked into this sooner, becaused syzbot did trigger the
> > > > > issue again, and Andrii had to look into the same bug, sorry about that.
> > > >
> > > > No worries! I actually forgot about this thread :) Let's leave the
> > > > link to my today's investigation ([0]) just for completeness.
> > > >
> > > > [0] https://lore.kernel.org/bpf/CAEf4BzYk+dqs+jwu6VKXP-RttcTEGFe+ySTGWT9CRNkagDiJVA@mail.gmail.com/
> > > >
> > > > > if i am understanding this correctly here is what the fix should be:
> > > > > ---
> > > > > kernel/bpf/ringbuf.c | 2 ++
> > > > > 1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > > > > index f25b719ac786..30400e74abe2 100644
> > > > > --- a/kernel/bpf/ringbuf.c
> > > > > +++ b/kernel/bpf/ringbuf.c
> > > > > @@ -8,6 +8,7 @@
> > > > > #include <linux/vmalloc.h>
> > > > > #include <linux/wait.h>
> > > > > #include <linux/poll.h>
> > > > > +#include <linux/kmemleak.h>
> > > > > #include <uapi/linux/btf.h>
> > > > >
> > > > > #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> > > > > @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> > > > > rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> > > > > VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> > > > > if (rb) {
> > > > > + kmemleak_not_leak((void *) pages);
> > > >
> > > > If that makes kmemleak happy, I have no problems with this. But maybe
> > > > leave some comment explaining why this is needed at all?
> > > >
> > > > And for my understanding, how vmap changes anything? Those pages are
> > > > still referenced from rb, which is referenced from some struct file in
> > > > the system. Sorry if that's a naive question.
> > > >
> > > valid question, it does look like kmemleak should be scanning
> > > vmalloc()/vmap() memory, i will research this further
> >
> > a quick update, i see a problem in kmemleak code, and i have simplified
> > the reproducer by getting rid of a vmap().
> > i will reach out to maintainer and mm and afterwards i will update this
> > bug, cheers!
> >
>
> Andrii, we have discovered that kmemleak scans struct page, but it does
> not scan page contents and this is by design. if we allocate some memory
> with kmalloc(), then allocate page with alloc_page(), and if we put
> kmalloc pointer somewhere inside that page, kmemleak will report kmalloc
> pointer as a false positive.
> we can instruct kmemleak to scan the memory area by calling
> kmemleak_alloc()/kmemleak_free() as shown below. if we don't need that
> memory to be scanned then we can use kmemleak_not_leak().
> if we use the former then i guess we need to be careful since we do not
> want/need to scan the memory that is being used by user-space.
>
Hi, sorry for late reply, I was out for two weeks. Thanks for
investigating and sending a patch!
> ---
> kernel/bpf/ringbuf.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 84b3b35fc0d0..cf7ce10b4fb1 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -8,6 +8,7 @@
> #include <linux/vmalloc.h>
> #include <linux/wait.h>
> #include <linux/poll.h>
> +#include <linux/kmemleak.h>
> #include <uapi/linux/btf.h>
>
> #define RINGBUF_CREATE_FLAG_MASK (BPF_F_NUMA_NODE)
> @@ -105,6 +106,7 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(size_t data_sz, int numa_node)
> rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
> VM_ALLOC | VM_USERMAP, PAGE_KERNEL);
> if (rb) {
> + kmemleak_alloc(rb, PAGE_SIZE, 1, flags);
> rb->pages = pages;
> rb->nr_pages = nr_pages;
> return rb;
> @@ -184,6 +186,7 @@ static void bpf_ringbuf_free(struct bpf_ringbuf *rb)
> struct page **pages = rb->pages;
> int i, nr_pages = rb->nr_pages;
>
> + kmemleak_free(rb);
> vunmap(rb);
> for (i = 0; i < nr_pages; i++)
> __free_page(pages[i]);
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-07-07 17:06 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18 3:50 memory leak in bpf syzbot
2020-12-10 6:58 ` syzbot
2021-03-01 16:21 ` Rustam Kovhaev
2021-03-01 19:05 ` Dmitry Vyukov
2021-03-01 20:39 ` Rustam Kovhaev
2021-03-01 20:43 ` Dmitry Vyukov
2021-04-07 23:24 ` Rustam Kovhaev
2021-04-07 23:35 ` Andrii Nakryiko
2021-04-08 18:56 ` Rustam Kovhaev
2021-06-13 15:17 ` Rustam Kovhaev
2021-06-24 17:28 ` Rustam Kovhaev
2021-06-25 14:28 ` Dmitry Vyukov
2021-06-26 18:40 ` Rustam Kovhaev
2021-07-07 17:06 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).