Message-ID: <CACT4Y+a-J5_t2xsHn6RGWoHPE-huJ2zJ0S01zR1kJew=c6SUsQ@mail.gmail.com>
Date:	Mon, 29 Feb 2016 11:22:58 +0100
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	Catalin Marinas <catalin.marinas@....com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Peter Hurley <peter@...leysoftware.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	J Freyensee <james_p_freyensee@...ux.intel.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Paul Bolle <pebolle@...cali.nl>,
	Alexander Potapenko <glider@...gle.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	syzkaller <syzkaller@...glegroups.com>
Subject: Re: tty: memory leak in tty_register_driver

On Mon, Feb 29, 2016 at 12:47 AM, Catalin Marinas
<catalin.marinas@....com> wrote:
> On Sun, Feb 28, 2016 at 05:42:24PM +0100, Dmitry Vyukov wrote:
>> On Mon, Feb 15, 2016 at 11:42 AM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
>> > When I am running the following program in a parallel loop, kmemleak
>> > starts reporting memory leaks of objects allocated in
>> > tty_register_driver during boot. These leaks start popping up
>> > chaotically and as you can see they originate in different drivers
>> > (synclinkmp_init, isdn_init, chr_dev_init, sysfs_init).
>> >
>> > On commit 388f7b1d6e8ca06762e2454d28d6c3c55ad0fe95 (4.5-rc3).
> [...]
>> > unreferenced object 0xffff88006708dc20 (size 8):
>> >   comm "swapper/0", pid 1, jiffies 4294672590 (age 930.839s)
>> >   hex dump (first 8 bytes):
>> >     74 74 79 53 4c 4d 38 00                          ttySLM8.
>> >   backtrace:
>> >     [<ffffffff81765d10>] __kmalloc_track_caller+0x1b0/0x320 mm/slub.c:4068
>> >     [<ffffffff816b37a9>] kstrdup+0x39/0x70 mm/util.c:53
>> >     [<ffffffff816b3826>] kstrdup_const+0x46/0x60 mm/util.c:74
>> >     [<ffffffff8194e5bb>] __kernfs_new_node+0x2b/0x2b0 fs/kernfs/dir.c:536
>> >     [<ffffffff81951c70>] kernfs_new_node+0x80/0xe0 fs/kernfs/dir.c:572
>> >     [<ffffffff81957223>] kernfs_create_link+0x33/0x150 fs/kernfs/symlink.c:32
>> >     [<ffffffff81959c4b>] sysfs_do_create_link_sd.isra.2+0x8b/0x120
> [...]
>> +Catalin (kmemleak maintainer)
>>
>> I am noticed a weird thing. I am not 100% sure but it seems that the
>> leaks are reported iff I run leak checking concurrently with the
>> programs running. And if I run the program several thousand times and
>> then run leak checking, then no leaks reported.
>>
>> Catalin, it is possible that it is a kmemleak false positive?
>
> Yes, it's possible. If you run kmemleak scanning continuously (or at
> very short intervals) and especially in parallel with some intensive
> tasks, it will miss pointers that may be stored in registers (on other
> CPUs) or moved between task stacks, other memory locations. Linked lists
> are especially prone to such false positives.
>
> Kmemleak tries to work around this by checksumming each object, so it
> will only be reported if it hasn't changed on two consecutive scans.
> Since the default scanning is 10min, it is very unlikely to trigger
> false positives in such scenarios. However, if you reduce the scanning
> time (or trigger it manually in a loop), you can hit this condition.
>
>> I see that kmemleak just scans thread stacks one-by-one. I would
>> expect that kmemleak should stop all threads, then scan all stacks and
>> all registers of all threads, and then restart threads. If it does not
>> scan registers or does not stop threads, then I think it should be
>> possible that a pointer value can sneak off kmemleak. Does it make
>> sense?
>
> Given how long it takes to scan the memory, stopping the threads is not
> really feasible. You could do something like stop_machine() only for
> scanning the current stack on all CPUs but it still wouldn't catch
> pointers being moved around in memory unless you stop the system
> completely for a full scan. The heuristic about periodic scanning and
> checksumming seems to work fine in normal usage scenarios.
>
> For your tests, I would recommend that you run the tests for a long(ish)
> time and only do two kmemleak scans at the end after they finished (and
> with a few seconds delay between them). Continuous scanning is less
> reliable.


Thanks for the explanation, Catalin!

Let me describe my usage scenario first. I am running automatic
testing 24x7. Currently a VM executes a dozen of small programs (a
dozen of syscalls each), then I run manual leak scanning. I can't run
significantly more programs between scans, because then I won't be
able to restore reproducers for bugs and they will be unactionable. I
could run leak checking after each program, but it will increase
overhead significantly. So a dozen of programs is a trade-off. And I
disable automatic scanning.

False positives are super unpleasant in automatic testing. If a tool
false positive rate if high, I just disable it, it is unusable. It is
not that bad for leak checking. But each false positive consumes human
(my) time.

So I need to run scanning twice, because the first one never reports leaks.

For the false positives due to registers/pointer jumping, will it help
if I run scanning one more time if leaks are detected? I mean: run
scanning twice, if leaks are found sleep for several seconds and run
scanning third time. Since leaks are usually not detected I can afford
to sleep more and do one or two additional scans. The question here:
will kmemleak _remove_ an object for leaked objects, if it discovered
reachable or contents change on subsequent scans?

Regarding stopping all threads and doing proper scan, why is not it
feasible? Will kernel break if we stall all CPUs for seconds? In
automatic testing scenarios a stalled for several seconds machine is
not a problem. But on the other hand, absence of false positives is a
must. And it would improve testing bandwidth, because we don't need
sleep and second scan.

Powered by blists - more mailing lists