linux-kernel - Re: [PATCH v2 7/7] kunit, slub: add test_kfree_rcu() and test_leak

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <07d5a214-a6c2-4444-8122-0a7b1cdd711f@suse.cz>
Date: Sat, 21 Sep 2024 22:40:15 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Guenter Roeck <linux@...ck-us.net>,
 KUnit Development <kunit-dev@...glegroups.com>,
 Brendan Higgins <brendanhiggins@...gle.com>, David Gow <davidgow@...gle.com>
Cc: "Paul E. McKenney" <paulmck@...nel.org>,
 Joel Fernandes <joel@...lfernandes.org>,
 Josh Triplett <josh@...htriplett.org>, Boqun Feng <boqun.feng@...il.com>,
 Christoph Lameter <cl@...ux.com>, David Rientjes <rientjes@...gle.com>,
 Steven Rostedt <rostedt@...dmis.org>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Lai Jiangshan <jiangshanlai@...il.com>, Zqiang <qiang.zhang1211@...il.com>,
 Julia Lawall <Julia.Lawall@...ia.fr>, Jakub Kicinski <kuba@...nel.org>,
 "Jason A. Donenfeld" <Jason@...c4.com>,
 "Uladzislau Rezki (Sony)" <urezki@...il.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Roman Gushchin <roman.gushchin@...ux.dev>,
 Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, rcu@...r.kernel.org,
 Alexander Potapenko <glider@...gle.com>, Marco Elver <elver@...gle.com>,
 Dmitry Vyukov <dvyukov@...gle.com>, kasan-dev@...glegroups.com,
 Jann Horn <jannh@...gle.com>, Mateusz Guzik <mjguzik@...il.com>
Subject: Re: [PATCH v2 7/7] kunit, slub: add test_kfree_rcu() and
 test_leak_destroy()

+CC kunit folks

On 9/20/24 15:35, Guenter Roeck wrote:
> Hi,

Hi,

> On Wed, Aug 07, 2024 at 12:31:20PM +0200, Vlastimil Babka wrote:
>> Add a test that will create cache, allocate one object, kfree_rcu() it
>> and attempt to destroy it. As long as the usage of kvfree_rcu_barrier()
>> in kmem_cache_destroy() works correctly, there should be no warnings in
>> dmesg and the test should pass.
>> 
>> Additionally add a test_leak_destroy() test that leaks an object on
>> purpose and verifies that kmem_cache_destroy() catches it.
>> 
>> Signed-off-by: Vlastimil Babka <vbabka@...e.cz>
> 
> This test case, when run, triggers a warning traceback.
> 
> kmem_cache_destroy TestSlub_kfree_rcu: Slab cache still has objects when called from test_leak_destroy+0x70/0x11c
> WARNING: CPU: 0 PID: 715 at mm/slab_common.c:511 kmem_cache_destroy+0x1dc/0x1e4

Yes that should be suppressed like the other slub_kunit tests do. I have
assumed it's not that urgent because for example the KASAN kunit tests all
produce tons of warnings and thus assumed it's in some way acceptable for
kunit tests to do.

> That is, however, not the worst of it. It also causes boot stalls on
> several platforms and architectures (various arm platforms, arm64,
> loongarch, various ppc, and various x86_64). Reverting it fixes the
> problem. Bisect results are attached for reference.

OK, this part is unexpected. I assume you have the test built-in and not a
module, otherwise it can't affect boot? And by stall you mean a delay or a
complete lockup? I've tried to reproduce that with virtme, but it seemed
fine, maybe it's .config specific?

I do wonder about the placement of the call of kunit_run_all_tests() from
kernel_init_freeable() as that's before a bunch of initialization finishes.

For example, system_state = SYSTEM_RUNNING; and rcu_end_inkernel_boot() only
happens later in kernel_init(). I wouldn't be surprised if that means
calling kfree_rcu() or rcu_barrier() or kvfree_rcu_barrier() as part of the
slub tests is too early.

Does the diff below fix the problem? Any advice from kunit folks? I could
perhaps possibly make the slab test module-only instead of tristate or do
some ifdef builtin on the problematic tests, but maybe it wouldn't be
necessary with kunit_run_all_tests() happening a bit later.

----8<----
diff --git a/init/main.c b/init/main.c
index c4778edae797..7890ebb00e84 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1489,6 +1489,8 @@ static int __ref kernel_init(void *unused)

 	rcu_end_inkernel_boot();

+	kunit_run_all_tests();
+
 	do_sysctl_args();

 	if (ramdisk_execute_command) {
@@ -1579,8 +1581,6 @@ static noinline void __init kernel_init_freeable(void)

 	do_basic_setup();

-	kunit_run_all_tests();
-
 	wait_for_initramfs();
 	console_on_rootfs();