memory leak in vq_meta_prefetch

memory leak in vq_meta_prefetch

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    c6dd78fc Merge branch 'x86-urgent-for-linus' of git://git...
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=15fffef4600000
kernel config:  https://syzkaller.appspot.com/x/.config?x=8de7d700ea5ac607
dashboard link: https://syzkaller.appspot.com/bug?extid=a871c1e6ea00685e73d7
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=127b0334600000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=12609e94600000

The bug was bisected to:

commit 0e5f7d0b39e1f184dc25e3adb580c79e85332167
Author: Nicolas Ferre <nicolas.ferre@atmel.com>
Date:   Wed Mar 16 13:19:49 2016 +0000

     ARM: dts: at91: shdwc binding: add new shutdown controller documentation

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=16c6d53fa00000
final crash:    https://syzkaller.appspot.com/x/report.txt?x=15c6d53fa00000
console output: https://syzkaller.appspot.com/x/log.txt?x=11c6d53fa00000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+a871c1e6ea00685e73d7@syzkaller.appspotmail.com
Fixes: 0e5f7d0b39e1 ("ARM: dts: at91: shdwc binding: add new shutdown  
controller documentation")

executing program
executing program
executing program
executing program
executing program
BUG: memory leak
unreferenced object 0xffff88811b327cc0 (size 32):
   comm "vhost-7201", pid 7205, jiffies 4294952492 (age 19.700s)
   hex dump (first 32 bytes):
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<000000009e106308>] kmemleak_alloc_recursive  
/./include/linux/kmemleak.h:43 [inline]
     [<000000009e106308>] slab_post_alloc_hook /mm/slab.h:522 [inline]
     [<000000009e106308>] slab_alloc /mm/slab.c:3319 [inline]
     [<000000009e106308>] kmem_cache_alloc_trace+0x145/0x280 /mm/slab.c:3548
     [<00000000ed2eec2d>] kmalloc /./include/linux/slab.h:552 [inline]
     [<00000000ed2eec2d>] vhost_map_prefetch /drivers/vhost/vhost.c:877  
[inline]
     [<00000000ed2eec2d>] vhost_vq_map_prefetch /drivers/vhost/vhost.c:1838  
[inline]
     [<00000000ed2eec2d>] vq_meta_prefetch+0x18e/0x350  
/drivers/vhost/vhost.c:1849
     [<000000009d9c11b8>] handle_rx+0x9d/0xc00 /drivers/vhost/net.c:1128
     [<000000008f883d86>] handle_rx_net+0x19/0x20 /drivers/vhost/net.c:1270
     [<00000000577ffdd8>] vhost_worker+0xc6/0x120 /drivers/vhost/vhost.c:519
     [<000000001201f3db>] kthread+0x13e/0x160 /kernel/kthread.c:255
     [<00000000093cd85a>] ret_from_fork+0x1f/0x30  
/arch/x86/entry/entry_64.S:352

BUG: memory leak
unreferenced object 0xffff88811b327cc0 (size 32):
   comm "vhost-7201", pid 7205, jiffies 4294952492 (age 20.600s)
   hex dump (first 32 bytes):
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<000000009e106308>] kmemleak_alloc_recursive  
/./include/linux/kmemleak.h:43 [inline]
     [<000000009e106308>] slab_post_alloc_hook /mm/slab.h:522 [inline]
     [<000000009e106308>] slab_alloc /mm/slab.c:3319 [inline]
     [<000000009e106308>] kmem_cache_alloc_trace+0x145/0x280 /mm/slab.c:3548
     [<00000000ed2eec2d>] kmalloc /./include/linux/slab.h:552 [inline]
     [<00000000ed2eec2d>] vhost_map_prefetch /drivers/vhost/vhost.c:877  
[inline]
     [<00000000ed2eec2d>] vhost_vq_map_prefetch /drivers/vhost/vhost.c:1838  
[inline]
     [<00000000ed2eec2d>] vq_meta_prefetch+0x18e/0x350  
/drivers/vhost/vhost.c:1849
     [<000000009d9c11b8>] handle_rx+0x9d/0xc00 /drivers/vhost/net.c:1128
     [<000000008f883d86>] handle_rx_net+0x19/0x20 /drivers/vhost/net.c:1270
     [<00000000577ffdd8>] vhost_worker+0xc6/0x120 /drivers/vhost/vhost.c:519
     [<000000001201f3db>] kthread+0x13e/0x160 /kernel/kthread.c:255
     [<00000000093cd85a>] ret_from_fork+0x1f/0x30  
/arch/x86/entry/entry_64.S:352



---
This bug 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 bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: memory leak in vq_meta_prefetch

[Catalin Marinas]

On Wed, Jul 24, 2019 at 12:18:07PM -0700, syzbot wrote:
> syzbot found the following crash on:
> 
> HEAD commit:    c6dd78fc Merge branch 'x86-urgent-for-linus' of git://git...
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=15fffef4600000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=8de7d700ea5ac607
> dashboard link: https://syzkaller.appspot.com/bug?extid=a871c1e6ea00685e73d7
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=127b0334600000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=12609e94600000
> 
> The bug was bisected to:
> 
> commit 0e5f7d0b39e1f184dc25e3adb580c79e85332167
> Author: Nicolas Ferre <nicolas.ferre@atmel.com>
> Date:   Wed Mar 16 13:19:49 2016 +0000
> 
>     ARM: dts: at91: shdwc binding: add new shutdown controller documentation

That's another wrong commit identification (a documentation patch should
not cause a memory leak).

I don't really think kmemleak, with its relatively high rate of false
positives, is suitable for automated testing like syzbot. You could
reduce the false positives if you add support for scanning in
stop_machine(). Otherwise, in order to avoid locking the kernel for long
periods, kmemleak runs concurrently with other threads (even on the
current CPU) and under high load, pointers are missed (e.g. they are in
CPU registers rather than stack).

-- 
Catalin

Re: memory leak in vq_meta_prefetch

[Dmitry Vyukov]

On Fri, Jul 26, 2019 at 3:00 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, Jul 24, 2019 at 12:18:07PM -0700, syzbot wrote:
> > syzbot found the following crash on:
> >
> > HEAD commit:    c6dd78fc Merge branch 'x86-urgent-for-linus' of git://git...
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=15fffef4600000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=8de7d700ea5ac607
> > dashboard link: https://syzkaller.appspot.com/bug?extid=a871c1e6ea00685e73d7
> > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=127b0334600000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=12609e94600000
> >
> > The bug was bisected to:
> >
> > commit 0e5f7d0b39e1f184dc25e3adb580c79e85332167
> > Author: Nicolas Ferre <nicolas.ferre@atmel.com>
> > Date:   Wed Mar 16 13:19:49 2016 +0000
> >
> >     ARM: dts: at91: shdwc binding: add new shutdown controller documentation
>
> That's another wrong commit identification (a documentation patch should
> not cause a memory leak).
>
> I don't really think kmemleak, with its relatively high rate of false
> positives, is suitable for automated testing like syzbot. You could

Hi Catalin,

Do you mean automated testing in general, or bisection only?
The wrong commit identification is related to bisection only, but you
generalized it to automated testing in general. So which exactly you
mean?


> reduce the false positives if you add support for scanning in
> stop_machine(). Otherwise, in order to avoid locking the kernel for long
> periods, kmemleak runs concurrently with other threads (even on the
> current CPU) and under high load, pointers are missed (e.g. they are in
> CPU registers rather than stack).

Re: memory leak in vq_meta_prefetch

[Catalin Marinas]

On Fri, Jul 26, 2019 at 05:20:55PM +0200, Dmitry Vyukov wrote:
> On Fri, Jul 26, 2019 at 3:00 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Wed, Jul 24, 2019 at 12:18:07PM -0700, syzbot wrote:
> > > syzbot found the following crash on:
> > >
> > > HEAD commit:    c6dd78fc Merge branch 'x86-urgent-for-linus' of git://git...
> > > git tree:       upstream
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=15fffef4600000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=8de7d700ea5ac607
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=a871c1e6ea00685e73d7
> > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=127b0334600000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=12609e94600000
> > >
> > > The bug was bisected to:
> > >
> > > commit 0e5f7d0b39e1f184dc25e3adb580c79e85332167
> > > Author: Nicolas Ferre <nicolas.ferre@atmel.com>
> > > Date:   Wed Mar 16 13:19:49 2016 +0000
> > >
> > >     ARM: dts: at91: shdwc binding: add new shutdown controller documentation
> >
> > That's another wrong commit identification (a documentation patch should
> > not cause a memory leak).
> >
> > I don't really think kmemleak, with its relatively high rate of false
> > positives, is suitable for automated testing like syzbot. You could
> 
> Do you mean automated testing in general, or bisection only?
> The wrong commit identification is related to bisection only, but you
> generalized it to automated testing in general. So which exactly you
> mean?

I probably meant both. In terms of automated testing and reporting, if
the false positives rate is high, people start ignoring the reports. So
it requires some human checking first (or make the tool more robust).

W.r.t. bisection, the false negatives (rather than positives) will cause
the tool to miss the problematic commit and misreport. I'm not sure you
can make the reporting deterministic on successive runs given that you
changed the kernel HEAD (for bisection). But it may get better if you
have a "stopscan" kmemleak option which freezes the machine during
scanning (it has been discussed in the past but I really struggle to
find time to work on it; any help appreciated ;)).

-- 
Catalin

Re: memory leak in vq_meta_prefetch

[Dmitry Vyukov]

On Fri, Jul 26, 2019 at 5:57 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Fri, Jul 26, 2019 at 05:20:55PM +0200, Dmitry Vyukov wrote:
> > On Fri, Jul 26, 2019 at 3:00 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > On Wed, Jul 24, 2019 at 12:18:07PM -0700, syzbot wrote:
> > > > syzbot found the following crash on:
> > > >
> > > > HEAD commit:    c6dd78fc Merge branch 'x86-urgent-for-linus' of git://git...
> > > > git tree:       upstream
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=15fffef4600000
> > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=8de7d700ea5ac607
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=a871c1e6ea00685e73d7
> > > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=127b0334600000
> > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=12609e94600000
> > > >
> > > > The bug was bisected to:
> > > >
> > > > commit 0e5f7d0b39e1f184dc25e3adb580c79e85332167
> > > > Author: Nicolas Ferre <nicolas.ferre@atmel.com>
> > > > Date:   Wed Mar 16 13:19:49 2016 +0000
> > > >
> > > >     ARM: dts: at91: shdwc binding: add new shutdown controller documentation
> > >
> > > That's another wrong commit identification (a documentation patch should
> > > not cause a memory leak).
> > >
> > > I don't really think kmemleak, with its relatively high rate of false
> > > positives, is suitable for automated testing like syzbot. You could
> >
> > Do you mean automated testing in general, or bisection only?
> > The wrong commit identification is related to bisection only, but you
> > generalized it to automated testing in general. So which exactly you
> > mean?
>
> I probably meant both. In terms of automated testing and reporting, if
> the false positives rate is high, people start ignoring the reports. So
> it requires some human checking first (or make the tool more robust).
>
> W.r.t. bisection, the false negatives (rather than positives) will cause
> the tool to miss the problematic commit and misreport. I'm not sure you
> can make the reporting deterministic on successive runs given that you
> changed the kernel HEAD (for bisection). But it may get better if you
> have a "stopscan" kmemleak option which freezes the machine during
> scanning (it has been discussed in the past but I really struggle to
> find time to work on it; any help appreciated ;)).


Do you have any data points wrt automated testing in general? This
disagrees with what I see.

For bisection, I agree. Need to look at the data we got over the past
days when it become enabled. But I suspect that, yes, false positives,
flakes, and other true leaks can make it infeasible.

Re: memory leak in vq_meta_prefetch

[Catalin Marinas]

On Fri, Jul 26, 2019 at 06:05:32PM +0200, Dmitry Vyukov wrote:
> On Fri, Jul 26, 2019 at 5:57 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> >
> > On Fri, Jul 26, 2019 at 05:20:55PM +0200, Dmitry Vyukov wrote:
> > > On Fri, Jul 26, 2019 at 3:00 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > On Wed, Jul 24, 2019 at 12:18:07PM -0700, syzbot wrote:
> > > > > syzbot found the following crash on:
> > > > >
> > > > > HEAD commit:    c6dd78fc Merge branch 'x86-urgent-for-linus' of git://git...
> > > > > git tree:       upstream
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=15fffef4600000
> > > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=8de7d700ea5ac607
> > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=a871c1e6ea00685e73d7
> > > > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=127b0334600000
> > > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=12609e94600000
> > > > >
> > > > > The bug was bisected to:
> > > > >
> > > > > commit 0e5f7d0b39e1f184dc25e3adb580c79e85332167
> > > > > Author: Nicolas Ferre <nicolas.ferre@atmel.com>
> > > > > Date:   Wed Mar 16 13:19:49 2016 +0000
> > > > >
> > > > >     ARM: dts: at91: shdwc binding: add new shutdown controller documentation
> > > >
> > > > That's another wrong commit identification (a documentation patch should
> > > > not cause a memory leak).
> > > >
> > > > I don't really think kmemleak, with its relatively high rate of false
> > > > positives, is suitable for automated testing like syzbot. You could
> > >
> > > Do you mean automated testing in general, or bisection only?
> > > The wrong commit identification is related to bisection only, but you
> > > generalized it to automated testing in general. So which exactly you
> > > mean?
> >
> > I probably meant both. In terms of automated testing and reporting, if
> > the false positives rate is high, people start ignoring the reports. So
> > it requires some human checking first (or make the tool more robust).
[...]
> Do you have any data points wrt automated testing in general? This
> disagrees with what I see.

I'm fine with automated testing in general. Just that automated
reporting for kmemleak could be improved a bit to reduce the false
positives (e.g. run it a few times to confirm that it is a real leak).

Just to be clear, I'm not talking about syzbot in general, it's a great
tool, only about improving kmemleak reporting and bisecting.

-- 
Catalin

Re: memory leak in vq_meta_prefetch

[Dmitry Vyukov]

On Fri, Jul 26, 2019 at 6:15 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > > On Wed, Jul 24, 2019 at 12:18:07PM -0700, syzbot wrote:
> > > > > > syzbot found the following crash on:
> > > > > >
> > > > > > HEAD commit:    c6dd78fc Merge branch 'x86-urgent-for-linus' of git://git...
> > > > > > git tree:       upstream
> > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=15fffef4600000
> > > > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=8de7d700ea5ac607
> > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=a871c1e6ea00685e73d7
> > > > > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > > > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=127b0334600000
> > > > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=12609e94600000
> > > > > >
> > > > > > The bug was bisected to:
> > > > > >
> > > > > > commit 0e5f7d0b39e1f184dc25e3adb580c79e85332167
> > > > > > Author: Nicolas Ferre <nicolas.ferre@atmel.com>
> > > > > > Date:   Wed Mar 16 13:19:49 2016 +0000
> > > > > >
> > > > > >     ARM: dts: at91: shdwc binding: add new shutdown controller documentation
> > > > >
> > > > > That's another wrong commit identification (a documentation patch should
> > > > > not cause a memory leak).
> > > > >
> > > > > I don't really think kmemleak, with its relatively high rate of false
> > > > > positives, is suitable for automated testing like syzbot. You could
> > > >
> > > > Do you mean automated testing in general, or bisection only?
> > > > The wrong commit identification is related to bisection only, but you
> > > > generalized it to automated testing in general. So which exactly you
> > > > mean?
> > >
> > > I probably meant both. In terms of automated testing and reporting, if
> > > the false positives rate is high, people start ignoring the reports. So
> > > it requires some human checking first (or make the tool more robust).
> [...]
> > Do you have any data points wrt automated testing in general? This
> > disagrees with what I see.
>
> I'm fine with automated testing in general. Just that automated
> reporting for kmemleak could be improved a bit to reduce the false
> positives (e.g. run it a few times to confirm that it is a real leak).


I did a bunch of various external measures in syzkaller to improve
kmemleak quality. As far as I see the current rate is close to 100%
true positives. We already have 40 leaks (>50%) fixed.

Though, kmemleak can be improved too (stop-the-world, etc what we
discussed). That would make kmemleak directly usable e.g. during
unit-testing, something that's badly needed for kernel.


> Just to be clear, I'm not talking about syzbot in general, it's a great
> tool, only about improving kmemleak reporting and bisecting.